RESOLVED FIXED 53911
Deploy EditingStyle in applyBlockStyle and applyInlineStyle 
https://bugs.webkit.org/show_bug.cgi?id=53911
Summary Deploy EditingStyle in applyBlockStyle and applyInlineStyle
Ryosuke Niwa
Reported 2011-02-07 01:33:04 PST
ApplyStyleCommand::applyBlockStyle and ApplyStyleCommand::applyInlineStyle should take EditingStyle* instead of CSSMutableStyleDeclaration* and they should also use EditingStyle's methods.
Attachments
Patch (19.87 KB, patch)
2011-02-07 01:47 PST, Ryosuke Niwa 		tony: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2011-02-07 01:47:56 PST
Created attachment 81464 [details] Patch
Ryosuke Niwa
Comment 2 2011-02-09 19:43:39 PST
Could someone review this patch?
Tony Chang
Comment 3 2011-02-10 09:59:01 PST
Comment on attachment 81464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81464&action=review > Source/WebCore/ChangeLog:21 > + (WebCore::ApplyStyleCommand::removeInlineStyleFromElement): Allows style to be null; exit early instead. Is this because of the changes to removeInlineStyle() at ApplyStyleCommand.cpp:917,921? > Source/WebCore/editing/ApplyStyleCommand.cpp:895 > + style->textDirection(textDirection); Nit: Do you need to check the return value here?
Ryosuke Niwa
Comment 4 2011-02-10 16:41:36 PST
Comment on attachment 81464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81464&action=review Thanks for the review, Tony. >> Source/WebCore/ChangeLog:21 >> + (WebCore::ApplyStyleCommand::removeInlineStyleFromElement): Allows style to be null; exit early instead. > > Is this because of the changes to removeInlineStyle() at ApplyStyleCommand.cpp:917,921? Not really. The reason is that EditingStyle::style() can return a null-pointer, and these functions previously assumed that the style is always not null. >> Source/WebCore/editing/ApplyStyleCommand.cpp:895 >> + style->textDirection(textDirection); > > Nit: Do you need to check the return value here? Oops, I must have done: ASSERT(style->textDirection(textDirection));
Tony Chang
Comment 5 2011-02-10 16:58:23 PST
(In reply to comment #4) > > Is this because of the changes to removeInlineStyle() at ApplyStyleCommand.cpp:917,921? > > Not really. The reason is that EditingStyle::style() can return a null-pointer, and these functions previously assumed that the style is always not null. I think the change is fine, but it would be nice if you had a test case that showed this possibility.
Ryosuke Niwa
Comment 6 2011-02-10 17:04:08 PST
(In reply to comment #5) > (In reply to comment #4) > > > Is this because of the changes to removeInlineStyle() at ApplyStyleCommand.cpp:917,921? > > > > Not really. The reason is that EditingStyle::style() can return a null-pointer, and these functions previously assumed that the style is always not null. > > I think the change is fine, but it would be nice if you had a test case that showed this possibility. Oh, indeed some existing tests crash without this change because any test that inserts anchor element has null style pointer.
Ryosuke Niwa
Comment 7 2011-02-12 04:38:26 PST
Committed r78417: <http://trac.webkit.org/changeset/78417>
Note You need to log in before you can comment on or make changes to this bug.