[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


Darin Adler <darin at apple.com> changed:

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




--- Comment #5 from Darin Adler <darin at apple.com>  2010-11-30 13:48:33 PST ---
(From update of attachment 74630)
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.

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