[Webkit-unassigned] [Bug 49938] ApplyStyleCommand should take EditingStyle instead of CSSStyleDeclaration

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49938


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #74630|review+                     |
               Flag|                            |




--- Comment #6 from Ryosuke Niwa <rniwa at webkit.org>  2010-11-30 14:22:51 PST ---
(From update of attachment 74630)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.