Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2556803004:
[SPInvalidation] Invalidate paint property when transform etc. change (Closed)
Description[SPInvalidation] Invalidate paint property when transform etc. change
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=671695
TEST=virtual/spinvalidation/paint/invalidation/animated-gif-transformed-offscreen.html (repeat many times)
Committed: https://crrev.com/d76ff3eba94442beb49b45c1bdc7abca39117a91
Cr-Commit-Position: refs/heads/master@{#436822}
Patch Set 1 #Patch Set 2 : Fix #
Total comments: 4
Patch Set 3 : Rebase on origin #
Dependent Patchsets: Messages
Total messages: 25 (15 generated)
Description was changed from ========== Try crash ========== to ========== Try crash CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Try crash CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPInvalidation] Invalidate paint property when transform etc. change CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=671695 TEST=virtual/spinvalidation/paint/invalidation/animated-gif-transformed-offscreen.html (repeat many times) ==========
Description was changed from ========== [SPInvalidation] Invalidate paint property when transform etc. change CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=671695 TEST=virtual/spinvalidation/paint/invalidation/animated-gif-transformed-offscreen.html (repeat many times) ========== to ========== [SPInvalidation] Invalidate paint property when transform etc. change CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=671695 TEST=virtual/spinvalidation/paint/invalidation/animated-gif-transformed-offscreen.html (repeat many times) ==========
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/2556803004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2556803004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1476: if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) Should this be: "if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled())"?
https://codereview.chromium.org/2556803004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2556803004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1476: if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) On 2016/12/06 at 22:03:55, pdr. wrote: > Should this be: > "if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled())"? Nevermind. LGTM
About the reason of the crash, I got the following hypothesis: The image changed transform (to be offscreen) and image in the same cycle, and the latter somehow overrode the paint invalidation flag and the paint invalidation is delayed. As previously we didn't set property update flag on transform change, we depended on paint invalidation flags and in the case we missed property update. If the hypothesis is true, this seems a bug of the original code about off screen animated gif. We should always invalidate the image on transform change, no matter if it's off screen to update the visual rect and in case that the previous location is on screen. Looking into why the paint invalidation flag is not set or is overridden. This will be independent with this patch. https://codereview.chromium.org/2556803004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2556803004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1476: if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) On 2016/12/06 22:03:55, pdr. wrote: > Should this be: > "if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled())"? This is needed by spv2. For SlimmingPaintInvalidation we'll still execute the old paint invalidation path so doesn't need this.
https://codereview.chromium.org/2556803004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2556803004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1476: if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) On 2016/12/06 22:12:00, Xianzhu wrote: > On 2016/12/06 22:03:55, pdr. wrote: > > Should this be: > > "if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled())"? > > This is needed by spv2. For SlimmingPaintInvalidation we'll still execute the > old paint invalidation path so doesn't need this. Also never mind my reply :)
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2556803004/#ps40001 (title: "Rebase on origin")
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_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481070069747780,
"parent_rev": "be70c4a3a9080be7b86dd6acb35e5c4754dded90", "commit_rev":
"ddfa1308c8f539fcb23f9277606b58bca1adacf5"}
Message was sent while issue was closed.
Description was changed from ========== [SPInvalidation] Invalidate paint property when transform etc. change CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=671695 TEST=virtual/spinvalidation/paint/invalidation/animated-gif-transformed-offscreen.html (repeat many times) ========== to ========== [SPInvalidation] Invalidate paint property when transform etc. change CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=671695 TEST=virtual/spinvalidation/paint/invalidation/animated-gif-transformed-offscreen.html (repeat many times) ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [SPInvalidation] Invalidate paint property when transform etc. change CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=671695 TEST=virtual/spinvalidation/paint/invalidation/animated-gif-transformed-offscreen.html (repeat many times) ========== to ========== [SPInvalidation] Invalidate paint property when transform etc. change CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=671695 TEST=virtual/spinvalidation/paint/invalidation/animated-gif-transformed-offscreen.html (repeat many times) Committed: https://crrev.com/d76ff3eba94442beb49b45c1bdc7abca39117a91 Cr-Commit-Position: refs/heads/master@{#436822} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d76ff3eba94442beb49b45c1bdc7abca39117a91 Cr-Commit-Position: refs/heads/master@{#436822} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
