Bug 72660 – Remove notifyChange from the public interface of CSSMutableStyleDeclaration

• Component: CSS
• Status: RESOLVED FIXED

[Rafael Weinstein]

This is an implementation detail that is leaking out and should not be.

[Rafael Weinstein]

Created attachment 115713 [details]
Patch

[Rafael Weinstein]

Note that the this patch is in anticipation of factoring CSSInlineStyleDeclaration out of CSSMutableStyleDeclaration.

Also note that the two public uses of notifyChanged that were passing false were:

1)  ApplyStyleCommand -- which rniwa assures me is safe to change
2) CSSStyleSelector -- which is safe because the declaration has neither a node nor a parentSelector, so notification would have no side effects.

[WebKit Review Bot]

Comment on attachment 115713 [details]
Patch

Attachment 115713 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10340483

New failing tests:
animations/animation-drt-api.html
animations/3d/transform-origin-vs-functions.html
accessibility/hang-in-isignored.html
accessibility/contenteditable-hidden-div.html
accessibility/removed-continuation-element-causes-crash.html
animations/animation-direction-normal.html
animations/animation-end-event-destroy-renderer.html
animations/animation-drt-api-multiple-keyframes.html
accessibility/crashing-a-tag-in-map.html
animations/animation-controller-drt-api.html
canvas/philip/tests/2d.canvas.readonly.html
animations/animation-add-events-in-handler.html
accessibility/input-image-alt.html
accessibility/inline-continuations.html
accessibility/image-map1.html
animations/3d/replace-filling-transform.html
animations/3d/change-transform-in-end-event.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Rafael Weinstein]

Created attachment 115879 [details]
Patch

[Rafael Weinstein]

2nd time's a charm.

[Ryosuke Niwa]

Comment on attachment 115879 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115879&action=review

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:64
> +        CSSMutableStyleDeclaration* decl= s_needsNotify ? s_currentDecl : 0;

Nit: need a space before =

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:909
> +    NotifyChangedScope changedScope(this);

Is this the only time scope can be nested? I'm not sure if it's a net-win to introduce a counter just for this case. As far as I know, only editing code calls this function directly or indirectly.

[Rafael Weinstein]

Created attachment 116150 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116150 [details]
Patch

Rejecting attachment 116150 [details] from commit-queue.

New failing tests:
inspector/debugger/dom-breakpoints.html
Full output: http://queues.webkit.org/results/10568075

[WebKit Review Bot]

Comment on attachment 116150 [details]
Patch

Attachment 116150 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10552084

New failing tests:
inspector/debugger/dom-breakpoints.html

[Rafael Weinstein]

Created attachment 116288 [details]
Patch

[Luke Macpherson]

Comment in passing - at some point (not this patch) it would be nice to make this code conform with style guide rule 10:

"Prefer enums to bools on function parameters if callers are likely to be passing constants, since named constants are easier to read at the call site."

I note that you're removing one of those bools here, so this patch makes it slightly better.

[Ryosuke Niwa]

On my second thought, I'm no longer certain this change is correct. Someone more familiar with css, editing, etc... should review this patch.

[Rafael Weinstein]

After some discussion with Ryosuke, I now think that I don't need this patch in order to make progress on 70137, so I'm tempted to abandon it.

However, it seems to me like it'd be nice, for code hygiene, to land it.

Darin, Ryosuke feels like you're the only one qualified at this point to r+ it. If you think it's worth removing notifyChanged from the public interface, let me know and I can push forward. Otherwise, I'll drop the effort.

[Ojan Vafai]

Comment on attachment 116288 [details]
Patch

This patch looks safe to me. The only place it changes behavior is in HTMLElement.cpp when setting contentEditable to false, we now notifyChanged where we didn't used to. I don't see how this could cause a bug and I think it was probably wrong to pass false here. Looks like it's been this way since it was first checked in http://trac.webkit.org/changeset/12984.

[Ryosuke Niwa]

Comment on attachment 116288 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116288&action=review

> Source/WebCore/html/HTMLElement.cpp:728
> -        attr->decl()->removeProperty(CSSPropertyWordWrap, false);
> -        attr->decl()->removeProperty(CSSPropertyWebkitNbspMode, false);
> -        attr->decl()->removeProperty(CSSPropertyWebkitLineBreak, false);
> +        attr->decl()->removeProperty(CSSPropertyWordWrap);
> +        attr->decl()->removeProperty(CSSPropertyWebkitNbspMode);
> +        attr->decl()->removeProperty(CSSPropertyWebkitLineBreak);

Why is it okay to make this change?

[Ryosuke Niwa]

Digging up further, this was an optimization from http://trac.webkit.org/changeset/8041 (by darin reviewed by Ken):

(KHTMLPart::removeEditingStyleFromElement): Change code to call setChanged only if removing
the style attributes really was a change, although it's not an important optimization it's
good to do it right.

It's probably okay to remove this optimization.

[Ryosuke Niwa]

Hm... in fact, this may have been a regression from http://trac.webkit.org/changeset/12984.

[WebKit Review Bot]

Comment on attachment 116288 [details]
Patch

Clearing flags on attachment: 116288

Committed r101066: <http://trac.webkit.org/changeset/101066>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.