Bug 49938 - ApplyStyleCommand should take EditingStyle instead of CSSStyleDeclaration
Summary: ApplyStyleCommand should take EditingStyle instead of CSSStyleDeclaration
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: Ryosuke Niwa
URL:
Keywords:
Depends on: 49813
Blocks: 27818 49956
  Show dependency treegraph
 
Reported: 2010-11-22 14:21 PST by Ryosuke Niwa
Modified: 2010-12-06 17:00 PST (History)
5 users (show)

See Also:


Attachments
refactoring (28.09 KB, patch)
2010-11-22 20:26 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed per Tony's & Darin's comments (31.08 KB, patch)
2010-11-30 16:44 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 2010-11-22 14:21:00 PST
ApplyStyleCommand::ApplyStyleCommand and ApplyStyleCommand::create should take EditingStyle instead of CSSStyleDeclaration so that we can move / integrate the StyleChange in ApplyStyleCommand into EditingStyle.  This change is required to fix the bug 27818.
Comment 1 Ryosuke Niwa 2010-11-22 20:26:44 PST
Created attachment 74630 [details]
refactoring
Comment 2 Ryosuke Niwa 2010-11-22 20:27:28 PST
Since deploying EditingStyle in ApplyStyleCommand will be a considerable amount of work, I'll do in a followup patch.
Comment 3 Tony Chang 2010-11-30 13:39:20 PST
Comment on attachment 74630 [details]
refactoring

View in context: https://bugs.webkit.org/attachment.cgi?id=74630&action=review

> WebCore/editing/ApplyStyleCommand.cpp:521
> +        // apply the block-centric properties of the style

Nit: Convert to a sentence (capitalize first letter and add a period) please.

> WebCore/editing/ApplyStyleCommand.cpp:525
> +        // apply any remaining styles to the inline elements

Nit: Convert to a sentence (capitalize first letter and add a period) please.

> WebCore/editing/Editor.cpp:858
> +            applyCommand(ApplyStyleCommand::create(m_frame->document(), EditingStyle::create(style).get(), editingAction));

If there's no refptr to hold the EditingStyle returned by ::create, will it get deleted properly?
Comment 4 Darin Adler 2010-11-30 13:40:25 PST
(In reply to comment #3)
> > WebCore/editing/Editor.cpp:858
> > +            applyCommand(ApplyStyleCommand::create(m_frame->document(), EditingStyle::create(style).get(), editingAction));
> 
> If there's no refptr to hold the EditingStyle returned by ::create, will it get deleted properly?

Yes. The PassRefPtr that is the result of create takes care of this.
Comment 5 Darin Adler 2010-11-30 13:48:33 PST
Comment on attachment 74630 [details]
refactoring

View in context: https://bugs.webkit.org/attachment.cgi?id=74630&action=review

> WebCore/editing/EditingStyle.h:74
> +    void overrideWith(const CSSMutableStyleDeclaration* style);

Seems that the argument name “style” isn’t so helpful here.

I’m also not entirely sure that overrideWith is a great name for this function. Even overrideWithStyle would be better, but maybe we can come up with an even better name. I’m worried about the different verbs we’re using, “merge”, “apply”, “override”. It would be best if we chose more-consistent usage.

Is there some reason this argument needs to be CSSMutableStyleDeclaration and not just CSSStyleDeclaration?

>> WebCore/editing/Editor.cpp:858
>> +            applyCommand(ApplyStyleCommand::create(m_frame->document(), EditingStyle::create(style).get(), editingAction));
> 
> If there's no refptr to hold the EditingStyle returned by ::create, will it get deleted properly?

Yes, the PassRefPtr that is the result of create takes care of that.

> WebCore/editing/SelectionController.h:29
> +#include "CSSMutableStyleDeclaration.h"

Maybe instead of adding this include we could make copyTypingStyle non-inline. Seems unlikely we get a lot of benefit from inlining it.
Comment 6 Ryosuke Niwa 2010-11-30 14:22:51 PST
Comment on attachment 74630 [details]
refactoring

View in context: https://bugs.webkit.org/attachment.cgi?id=74630&action=review

I'll post a new patch for another review since quite few changes are requested.

>> WebCore/editing/ApplyStyleCommand.cpp:521
>> +        // apply the block-centric properties of the style
> 
> Nit: Convert to a sentence (capitalize first letter and add a period) please.

Will do.

>> WebCore/editing/ApplyStyleCommand.cpp:525
>> +        // apply any remaining styles to the inline elements
> 
> Nit: Convert to a sentence (capitalize first letter and add a period) please.

Will do.

>> WebCore/editing/EditingStyle.h:74
>> +    void overrideWith(const CSSMutableStyleDeclaration* style);
> 
> Seems that the argument name “style” isn’t so helpful here.
> 
> I’m also not entirely sure that overrideWith is a great name for this function. Even overrideWithStyle would be better, but maybe we can come up with an even better name. I’m worried about the different verbs we’re using, “merge”, “apply”, “override”. It would be best if we chose more-consistent usage.
> 
> Is there some reason this argument needs to be CSSMutableStyleDeclaration and not just CSSStyleDeclaration?

Will remove the argument name.  How about mergeAndOverride, mergeOverridingConflictingStyles, or mergeStyleOverridingConflicts?

It needs to take CSSMutableStyleDeclaration because CSSMutableStyleDeclaration::merge takes CSSMutableStyleDeclaration and accesses m_properties.  I'm not happy about it either.

>> WebCore/editing/SelectionController.h:29
>> +#include "CSSMutableStyleDeclaration.h"
> 
> Maybe instead of adding this include we could make copyTypingStyle non-inline. Seems unlikely we get a lot of benefit from inlining it.

Doing so will require adding ~EditingStyle but I guess the cost of adding it is negligible.
Comment 7 Ryosuke Niwa 2010-11-30 16:44:21 PST
Created attachment 75224 [details]
fixed per Tony's & Darin's comments
Comment 8 Ryosuke Niwa 2010-12-01 18:46:45 PST
Hi Tony & Darin,

Could either one of you review this patch?
Comment 9 Tony Chang 2010-12-06 14:25:54 PST
Comment on attachment 75224 [details]
fixed per Tony's & Darin's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=75224&action=review

Darin may want to comment on the function name overrideWithStyle.

> WebCore/ChangeLog:10
> +        by calls to EditingStyle's member functions and extracted EditingStyle::overrideWith and

Nit: overrideWithStyle
Comment 10 Ryosuke Niwa 2010-12-06 17:00:39 PST
Committed r73416: <http://trac.webkit.org/changeset/73416>