Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1473453002:
Fix clip/transform pairing to follow LIFO order. (Closed)
|
Created:
5 years, 1 month ago by wkorman Modified:
5 years ago CC:
chromium-reviews, danakj+watch_chromium.org, jbauman+watch_chromium.org, kalyank, piman+watch_chromium.org, sievers+watch_chromium.org, Ian Vollick Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix clip/transform pairing to follow LIFO order.
BUG=529938
Committed: https://crrev.com/4485bf60d087482aa544953bd45c2dce3dd4d380
Cr-Commit-Position: refs/heads/master@{#361261}
Patch Set 1 #
Messages
Total messages: 15 (6 generated)
wkorman@chromium.org changed reviewers: + danakj@chromium.org, enne@chromium.org, vmpstr@chromium.org
Before we outputted a list as: Clip Transform EndClip EndTransform Now we'll output a list as: Clip Transform EndTransform EndClip which from discussion with danakj@ is what was always intended (and is what Blink also does). I started on a unit test though it seems a bit like overkill, but since we had a bug maybe it's not overkill. However, testing requires: (1) adding some mix of accessors, friends, or a test api to retrieve things like individual display items from cc:DisplayItemList, and the clip rect from ClipDisplayItem (2) having some way to validate the items retrieved from the cc::DisplayItemList. Ideally we'd just create another cc::DisplayItemList (or std::vector<DisplayItem*>) and add what we expect to see and then see if the lists are equal but I don't think currently equality is correctly supported, so we'd need to discuss how to fix that. Here is what Blink does, egads: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... In any case, I'm happy to add a test and whatever the test needs with input/buy-in on direction. We have more upcoming changes where we split TransformRecorder into its own class, and add tracking of the visual rect for the Clip/Transform end items, so these issues are not limited to this change. I'm planning to add at least some initial test api for cc::DisplayItemList so that we can test the view logic in http://crrev.com/1423653005
LGTM
The CQ bit was checked by wkorman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473453002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473453002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wkorman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473453002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473453002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wkorman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473453002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473453002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/4485bf60d087482aa544953bd45c2dce3dd4d380 Cr-Commit-Position: refs/heads/master@{#361261} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
