This is an implementation detail that is leaking out and should not be.
Created attachment 115713 [details] Patch
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 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
Created attachment 115879 [details] Patch
2nd time's a charm.
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.
Created attachment 116150 [details] Patch
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 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
Created attachment 116288 [details] Patch
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.
On my second thought, I'm no longer certain this change is correct. Someone more familiar with css, editing, etc... should review this patch.
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 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 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?
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.
Hm... in fact, this may have been a regression from http://trac.webkit.org/changeset/12984.
Comment on attachment 116288 [details] Patch Clearing flags on attachment: 116288 Committed r101066: <http://trac.webkit.org/changeset/101066>
All reviewed patches have been landed. Closing bug.