[webkit-reviews] review canceled: [Bug 49938] ApplyStyleCommand should take EditingStyle instead of CSSStyleDeclaration : [Attachment 74630] refactoring

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Nov 30 14:22:50 PST 2010


Ryosuke Niwa <rniwa at webkit.org> has canceled  review:
Bug 49938: ApplyStyleCommand should take EditingStyle instead of
CSSStyleDeclaration
https://bugs.webkit.org/show_bug.cgi?id=49938

Attachment 74630: refactoring
https://bugs.webkit.org/attachment.cgi?id=74630&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.


More information about the webkit-reviews mailing list