Bug 72660 - Remove notifyChange from the public interface of CSSMutableStyleDeclaration
Summary: Remove notifyChange from the public interface of CSSMutableStyleDeclaration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rafael Weinstein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-17 14:37 PST by Rafael Weinstein
Modified: 2011-11-23 04:19 PST (History)
10 users (show)

See Also:


Attachments
Patch (13.75 KB, patch)
2011-11-17 17:33 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (13.75 KB, patch)
2011-11-18 15:01 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (6.75 KB, patch)
2011-11-21 15:33 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (9.23 KB, patch)
2011-11-22 16:08 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Weinstein 2011-11-17 14:37:05 PST
This is an implementation detail that is leaking out and should not be.
Comment 1 Rafael Weinstein 2011-11-17 17:33:11 PST
Created attachment 115713 [details]
Patch
Comment 2 Rafael Weinstein 2011-11-17 17:38:26 PST
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.
Comment 3 WebKit Review Bot 2011-11-17 20:14:06 PST
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
Comment 4 Rafael Weinstein 2011-11-18 15:01:24 PST
Created attachment 115879 [details]
Patch
Comment 5 Rafael Weinstein 2011-11-18 15:02:29 PST
2nd time's a charm.
Comment 6 Ryosuke Niwa 2011-11-21 11:32:50 PST
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.
Comment 7 Rafael Weinstein 2011-11-21 15:33:30 PST
Created attachment 116150 [details]
Patch
Comment 8 WebKit Review Bot 2011-11-21 20:45:52 PST
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
Comment 9 WebKit Review Bot 2011-11-21 21:54:43 PST
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
Comment 10 Rafael Weinstein 2011-11-22 16:08:15 PST
Created attachment 116288 [details]
Patch
Comment 11 Luke Macpherson 2011-11-22 16:17:39 PST
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.
Comment 12 Ryosuke Niwa 2011-11-22 16:32:11 PST
On my second thought, I'm no longer certain this change is correct. Someone more familiar with css, editing, etc... should review this patch.
Comment 13 Rafael Weinstein 2011-11-22 16:36:36 PST
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.
Comment 14 Ojan Vafai 2011-11-22 17:23:49 PST
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.
Comment 15 Ryosuke Niwa 2011-11-23 00:01:37 PST
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?
Comment 16 Ryosuke Niwa 2011-11-23 00:12:04 PST
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.
Comment 17 Ryosuke Niwa 2011-11-23 00:17:52 PST
Hm... in fact, this may have been a regression from http://trac.webkit.org/changeset/12984.
Comment 18 WebKit Review Bot 2011-11-23 04:19:51 PST
Comment on attachment 116288 [details]
Patch

Clearing flags on attachment: 116288

Committed r101066: <http://trac.webkit.org/changeset/101066>
Comment 19 WebKit Review Bot 2011-11-23 04:19:57 PST
All reviewed patches have been landed.  Closing bug.