Bug 49938 – ApplyStyleCommand should take EditingStyle instead of CSSStyleDeclaration

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 74630 [details]
refactoring

[Ryosuke Niwa]

Since deploying EditingStyle in ApplyStyleCommand will be a considerable amount of work, I'll do in a followup patch.

[Tony Chang]

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?

[Darin Adler]

(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.

[Darin Adler]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 75224 [details]
fixed per Tony's & Darin's comments

[Ryosuke Niwa]

Hi Tony & Darin,

Could either one of you review this patch?

[Tony Chang]

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

[Ryosuke Niwa]

Committed r73416: <http://trac.webkit.org/changeset/73416>