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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for 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 Darin Adler <darin at apple.com>
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.