Bug 55720

Created attachment 84643 [details] Patch

(the CC list is care of webkit-patch --suggest-reviewers, feel free to remove yourself if this change to Node.cpp does not interest you)

Created attachment 84644 [details] Fix ChangeLog typo

Comment on attachment 84644 [details] Fix ChangeLog typo great!

Comment on attachment 84644 [details] Fix ChangeLog typo View in context: https://bugs.webkit.org/attachment.cgi?id=84644&action=review > Source/WebCore/dom/Node.cpp:1394 > + newRenderer->setAnimatableStyle(style.release()); // setAnimatableStyle() can depend on renderer() already being set. I don’t understand this comment.

(In reply to comment #5) > (From update of attachment 84644 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84644&action=review > > > Source/WebCore/dom/Node.cpp:1394 > > + newRenderer->setAnimatableStyle(style.release()); // setAnimatableStyle() can depend on renderer() already being set. > > I don’t understand this comment. I can dig up the callstack if you like. But deep inside setAnimatable style it calls back through RenderLayer which grabs renderer() w/o checking for null. So it's required to call setRenderer before setting the animatable style on the renderer it seems.

(In reply to comment #6) > (In reply to comment #5) > > > Source/WebCore/dom/Node.cpp:1394 > > > + newRenderer->setAnimatableStyle(style.release()); // setAnimatableStyle() can depend on renderer() already being set. > > > > I don’t understand this comment. > > I can dig up the callstack if you like. But deep inside setAnimatable style it calls back through RenderLayer which grabs renderer() w/o checking for null. So it's required to call setRenderer before setting the animatable style on the renderer it seems. OK, but I am not sure this comment is helpful. The code was setting the renderer and then setting the animatable style, and you added a comment that basically says “this has to be in this order”, but I don’t think that adds value. Also the wording “can depend” gives me a feeling of fear, and since the comment doesn’t say why there is a dependency, I’m not sure it adds anything beyond what the test coverage already provides.

Comment on attachment 84644 [details] Fix ChangeLog typo Clearing flags on attachment: 84644 Committed r80330: <http://trac.webkit.org/changeset/80330>

All reviewed patches have been landed. Closing bug.

(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > > Source/WebCore/dom/Node.cpp:1394 > > > > + newRenderer->setAnimatableStyle(style.release()); // setAnimatableStyle() can depend on renderer() already being set. > > > > > > I don’t understand this comment. > > > > I can dig up the callstack if you like. But deep inside setAnimatable style it calls back through RenderLayer which grabs renderer() w/o checking for null. So it's required to call setRenderer before setting the animatable style on the renderer it seems. > > OK, but I am not sure this comment is helpful. > > The code was setting the renderer and then setting the animatable style, and you added a comment that basically says “this has to be in this order”, but I don’t think that adds value. > > Also the wording “can depend” gives me a feeling of fear, and since the comment doesn’t say why there is a dependency, I’m not sure it adds anything beyond what the test coverage already provides. I'll remove it in a follow-up.