Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2141743002:
Use node() when generating element id's (Closed)
|
Created:
4 years, 5 months ago by Ian Vollick Modified:
4 years, 5 months ago Reviewers:
jbroman CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, Eric Willigers, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rjwright, shans, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse node() when generating element id's
The current code followed the lead of crrev.com/279553002 and
incorrectly used generatingNode, which lead to discrepancies
when animating pseudo elements. It seems that node() is the
correct choice, both here and in the CL I referenced.
BUG=626571
Committed: https://crrev.com/10b3fec07dfda5c362f1fc27a314468541c72ae0
Cr-Commit-Position: refs/heads/master@{#404921}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address reviewer feedback. #
Total comments: 1
Patch Set 3 : Address nit. #
Messages
Total messages: 27 (12 generated)
vollick@chromium.org changed reviewers: + jbroman@chromium.org
https://codereview.chromium.org/2141743002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-pseudo-element-animation.html (right): https://codereview.chromium.org/2141743002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-pseudo-element-animation.html:8: div:before { Please use ::before, here and below. The single-colon syntax is deprecated for pseudo-elements (and is reserved for pseudo-classes, with the exception of backwards compatibility). https://codereview.chromium.org/2141743002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-pseudo-element-animation.html:10: transform: translateZ(0); nit: Is this the right compositing hint here? Would it be clearer to write 'will-change: transform'? https://codereview.chromium.org/2141743002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-pseudo-element-animation.html:13: position: absolute; Is absolute positioning significant in this test? https://codereview.chromium.org/2141743002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-pseudo-element-animation.html:21: onload = function() { Is it necessary to block on the load event? https://codereview.chromium.org/2141743002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2141743002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:233: graphicsLayer->setOwnerNodeId(DOMNodeIds::idForNode(owningNode)); Have you double-checked to see what consequences this has for DevTools (i.e., is it prepared to handle pseudo-elements here)?
https://codereview.chromium.org/2141743002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-pseudo-element-animation.html (right): https://codereview.chromium.org/2141743002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-pseudo-element-animation.html:8: div:before { On 2016/07/11 at 21:04:38, jbroman wrote: > Please use ::before, here and below. The single-colon syntax is deprecated for pseudo-elements (and is reserved for pseudo-classes, with the exception of backwards compatibility). Done. https://codereview.chromium.org/2141743002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-pseudo-element-animation.html:10: transform: translateZ(0); On 2016/07/11 at 21:04:38, jbroman wrote: > nit: Is this the right compositing hint here? Would it be clearer to write 'will-change: transform'? Not needed at all, actually. https://codereview.chromium.org/2141743002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-pseudo-element-animation.html:13: position: absolute; On 2016/07/11 at 21:04:38, jbroman wrote: > Is absolute positioning significant in this test? Turns out it just needs to be a "transformable" element. I've changed this to display:block and linked to the spec prose to explain why it's there. https://codereview.chromium.org/2141743002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-pseudo-element-animation.html:21: onload = function() { On 2016/07/11 at 21:04:38, jbroman wrote: > Is it necessary to block on the load event? Nope. Removed. https://codereview.chromium.org/2141743002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2141743002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:233: graphicsLayer->setOwnerNodeId(DOMNodeIds::idForNode(owningNode)); On 2016/07/11 at 21:04:38, jbroman wrote: > Have you double-checked to see what consequences this has for DevTools (i.e., is it prepared to handle pseudo-elements here)? I've done a cursory smoke-test and the experimental dev tools layers UI seems unaffected.
lgtm https://codereview.chromium.org/2141743002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-pseudo-element-animation.html (right): https://codereview.chromium.org/2141743002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-pseudo-element-animation.html:24: requestAnimationFrame(function(ts) { nit: indentation
The CQ bit was checked by vollick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/2141743002/#ps40001 (title: "Address nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vollick@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vollick@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vollick@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vollick@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Use node() when generating element id's The current code followed the lead of crrev.com/279553002 and incorrectly used generatingNode, which lead to discrepancies when animating pseudo elements. It seems that node() is the correct choice, both here and in the CL I referenced. BUG=626571 ========== to ========== Use node() when generating element id's The current code followed the lead of crrev.com/279553002 and incorrectly used generatingNode, which lead to discrepancies when animating pseudo elements. It seems that node() is the correct choice, both here and in the CL I referenced. BUG=626571 Committed: https://crrev.com/10b3fec07dfda5c362f1fc27a314468541c72ae0 Cr-Commit-Position: refs/heads/master@{#404921} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/10b3fec07dfda5c362f1fc27a314468541c72ae0 Cr-Commit-Position: refs/heads/master@{#404921} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
