Created attachment 74630 [details] refactoring

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

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?

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

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

Hi Tony & Darin, Could either one of you review this patch?

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

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