Bug 53911 - Deploy EditingStyle in applyBlockStyle and applyInlineStyle
Summary: Deploy EditingStyle in applyBlockStyle and applyInlineStyle
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 49956
  Show dependency treegraph
 
Reported: 2011-02-07 01:33 PST by Ryosuke Niwa
Modified: 2011-02-12 04:38 PST (History)
5 users (show)

See Also:


Attachments
Patch (19.87 KB, patch)
2011-02-07 01:47 PST, Ryosuke Niwa
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 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.
Comment 1 Ryosuke Niwa 2011-02-07 01:47:56 PST
Created attachment 81464 [details]
Patch
Comment 2 Ryosuke Niwa 2011-02-09 19:43:39 PST
Could someone review this patch?
Comment 3 Tony Chang 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?
Comment 4 Ryosuke Niwa 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));
Comment 5 Tony Chang 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.
Comment 6 Ryosuke Niwa 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.
Comment 7 Ryosuke Niwa 2011-02-12 04:38:26 PST
Committed r78417: <http://trac.webkit.org/changeset/78417>