Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2204053003:
Don't clear visual rects when finalizing display item lists for now. (Closed)
DescriptionDon't clear visual rects when finalizing display item lists for now.
Breaks Blimp, will explore putting it back behind a switch separately.
BUG=633750
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
TBR=vmpstr@chromium.org
Committed: https://crrev.com/1adf72a0a0a3e04151cc740d15ab19655b1e7e5e
Cr-Commit-Position: refs/heads/master@{#409421}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 14 (6 generated)
Description was changed from ========== Don't clear visual rects when finalizing display item lists for now. Breaks Blimp, will explore putting it back behind a switch separately. BUG=633750 ========== to ========== Don't clear visual rects when finalizing display item lists for now. Breaks Blimp, will explore putting it back behind a switch separately. BUG=633750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
wkorman@chromium.org changed reviewers: + khushalsagar@chromium.org, lethalantidote@chromium.org, vmpstr@chromium.org
khushalsagar@ suggested this as culprit, and I expect it is indeed likely cause. Have not yet confirmed myself, attempting to repro issue now.
lgtm
https://codereview.chromium.org/2204053003/diff/1/cc/playback/display_item_li... File cc/playback/display_item_list.cc (right): https://codereview.chromium.org/2204053003/diff/1/cc/playback/display_item_li... cc/playback/display_item_list.cc:161: // if (!retain_visual_rects_) We don't really need to do any post processing on the list, on the engine side. Probably you can pass that in as a setting when building the DisplayList, so the finalize method wouldn't do anything. You can use switches::kUseRemoteCompositing to enable it.
https://codereview.chromium.org/2204053003/diff/1/cc/playback/display_item_li... File cc/playback/display_item_list.cc (right): https://codereview.chromium.org/2204053003/diff/1/cc/playback/display_item_li... cc/playback/display_item_list.cc:161: // if (!retain_visual_rects_) On 2016/08/03 at 00:29:05, Khushal wrote: > We don't really need to do any post processing on the list, on the engine side. > > Probably you can pass that in as a setting when building the DisplayList, so the finalize method wouldn't do anything. You can use switches::kUseRemoteCompositing to enable it. Yes, I wanted to do things more cleanly in a separate patch and land this just to un-block folks. Do you want to wait or go with this today? vmpstr@ and other cc folks I would normally have as reviewers are OOO all week. Also, this change is needed to prevent engine from crashing, but your comment reads as if it's not needed for engine, so I must be misinterpreting.
lgtm. https://codereview.chromium.org/2204053003/diff/1/cc/playback/display_item_li... File cc/playback/display_item_list.cc (right): https://codereview.chromium.org/2204053003/diff/1/cc/playback/display_item_li... cc/playback/display_item_list.cc:161: // if (!retain_visual_rects_) On 2016/08/03 00:37:00, wkorman wrote: > On 2016/08/03 at 00:29:05, Khushal wrote: > > We don't really need to do any post processing on the list, on the engine > side. > > > > Probably you can pass that in as a setting when building the DisplayList, so > the finalize method wouldn't do anything. You can use > switches::kUseRemoteCompositing to enable it. > > Yes, I wanted to do things more cleanly in a separate patch and land this just > to un-block folks. Do you want to wait or go with this today? > > vmpstr@ and other cc folks I would normally have as reviewers are OOO all week. > > Also, this change is needed to prevent engine from crashing, but your comment > reads as if it's not needed for engine, so I must be misinterpreting. Sorry for being unclear. I was suggesting that you could use the flag above to have a setting to side-step the entire Finalize step altogether, taking that up in a follow up patch is better.
Description was changed from ========== Don't clear visual rects when finalizing display item lists for now. Breaks Blimp, will explore putting it back behind a switch separately. BUG=633750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Don't clear visual rects when finalizing display item lists for now. Breaks Blimp, will explore putting it back behind a switch separately. BUG=633750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel TBR=vmpstr ==========
Description was changed from ========== Don't clear visual rects when finalizing display item lists for now. Breaks Blimp, will explore putting it back behind a switch separately. BUG=633750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel TBR=vmpstr ========== to ========== Don't clear visual rects when finalizing display item lists for now. Breaks Blimp, will explore putting it back behind a switch separately. BUG=633750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel TBR=vmpstr@chromium.org ==========
The CQ bit was checked by wkorman@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 #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Don't clear visual rects when finalizing display item lists for now. Breaks Blimp, will explore putting it back behind a switch separately. BUG=633750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel TBR=vmpstr@chromium.org ========== to ========== Don't clear visual rects when finalizing display item lists for now. Breaks Blimp, will explore putting it back behind a switch separately. BUG=633750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel TBR=vmpstr@chromium.org Committed: https://crrev.com/1adf72a0a0a3e04151cc740d15ab19655b1e7e5e Cr-Commit-Position: refs/heads/master@{#409421} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/1adf72a0a0a3e04151cc740d15ab19655b1e7e5e Cr-Commit-Position: refs/heads/master@{#409421} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
