Issue metadata
Sign in to add a comment
|
Regression: Weird overlapping of cookies is seen in 'cookies set by this page' overlay.
Reported by
vku...@etouch.net,
Aug 4 2016
|
||||||||||||||||||||||
Issue descriptionChrome Version: 54.0.2817.0 (Official Build) cd4d5eb8d957a3ce7ccf0fa60243eccd8ba47691-refs/heads/master@{#409416}-32/64 bit OS: Windows (7,8,8.1,10) What steps will reproduce the problem? (1)Launch chrome and navigate to http://www.shockwave.com/online/all-games.jsp (2)Click on "view site information" click on cookies link button and observe the button. Actual: Weird overlapping of cookies is seen in 'cookies set by this page' overlay. Expected: Overlapping of cookies should not be seen in 'cookies set by this page' overlay. This is a regression issue broken in 'M54' and will soon update other info.
,
Aug 4 2016
Adding release block label, please undo if not the case.
,
Aug 4 2016
This looks due to my change http://crrev.com/1484163002, I can repro synced to my commit and not with the preceding commit. I will look into it shortly.
,
Aug 4 2016
,
Aug 4 2016
Suspect some issue with ClipRecorder bounds as it pertains to ui::views::TreeView. Notes: [19499:19499:0804/133115:ERROR:tree_view.cc(604)] Got canvas clip bounds [clip_rect=-1.000000,-1.000000 386.000000x282.000000]. [19499:19499:0804/133115:ERROR:tree_view.cc(619)] view_content_bounds=0,0 384x280 but dimensions of the tree view scrollable region in actual screenshot are actually ~ 386x124 (excluding scrollbar) or 400x124 (including scrollbar). Note also that TreeView::OnPaint() overrides View::OnPaint() and does its own thing though this may or may not be related: https://cs.chromium.org/chromium/src/ui/views/controls/tree/tree_view.cc?sq=package:chromium&dr&rcl=1470324312&l=595 Believe TreeView is used for both bookmarks and the cookie list, and so would explain dupe'd bug as well.
,
Aug 4 2016
Actually, ScrollView contains the TreeView and probably the TreeView is not-incorrectly painting more than we need to allow for scrolling. The bounds of the scroll view look ok but perhaps there is an issue with nested clips or a prior assumption, now invalid, re: containing layer and its bounds producing an implicit clip. [20014:20014:0804/141434:ERROR:scroll_view.cc(339)] Scheduling paint [is_bounded=0, viewport bounds=1,1 384x123, localBounds=0,0 401x125]. [20014:20014:0804/141434:ERROR:scroll_view.cc(339)] Scheduling paint [is_bounded=0, viewport bounds=1,1 384x123, localBounds=0,0 401x125]. [20014:20014:0804/141434:ERROR:scroll_view.cc(339)] Scheduling paint [is_bounded=0, viewport bounds=1,1 399x123, localBounds=0,0 401x125]. [20014:20014:0804/141434:ERROR:clip_recorder.cc(54)] Begin clip in layer space=24,127 401x125 [20014:20014:0804/141434:ERROR:clip_recorder.cc(54)] Begin clip in layer space=25,128 384x123 [20014:20014:0804/141434:ERROR:clip_recorder.cc(54)] Begin clip in layer space=25,128 384x349 [20014:20014:0804/141434:ERROR:tree_view.cc(595)] Painting TreeView. [20014:20014:0804/141434:ERROR:tree_view.cc(604)] Got canvas clip bounds [clip_rect=-1.000000,-1.000000 386.000000x351.000000]. [20014:20014:0804/141434:ERROR:tree_view.cc(619)] view_content_bounds=0,0 384x349 [20014:20014:0804/141434:ERROR:clip_recorder.cc(23)] ClipRecorder destructor [num_closers=1]. [20014:20014:0804/141434:ERROR:clip_recorder.cc(28)] End clip bounds_in_layer=25,128 384x349 [20014:20014:0804/141434:ERROR:clip_recorder.cc(23)] ClipRecorder destructor [num_closers=1]. [20014:20014:0804/141434:ERROR:clip_recorder.cc(28)] End clip bounds_in_layer=25,128 384x123 [20014:20014:0804/141434:ERROR:clip_recorder.cc(54)] Begin clip in layer space=409,128 15x123 [20014:20014:0804/141434:ERROR:clip_recorder.cc(54)] Begin clip in layer space=409,128 15x123 [20014:20014:0804/141434:ERROR:clip_recorder.cc(54)] Begin clip in layer space=409,142 15x33 [20014:20014:0804/141434:ERROR:clip_recorder.cc(23)] ClipRecorder destructor [num_closers=1]. [20014:20014:0804/141434:ERROR:clip_recorder.cc(28)] End clip bounds_in_layer=409,142 15x33 [20014:20014:0804/141434:ERROR:clip_recorder.cc(54)] Begin clip in layer space=409,128 15x14 [20014:20014:0804/141434:ERROR:clip_recorder.cc(23)] ClipRecorder destructor [num_closers=1]. [20014:20014:0804/141434:ERROR:clip_recorder.cc(28)] End clip bounds_in_layer=409,128 15x14 [20014:20014:0804/141434:ERROR:clip_recorder.cc(54)] Begin clip in layer space=409,237 15x14 [20014:20014:0804/141434:ERROR:clip_recorder.cc(23)] ClipRecorder destructor [num_closers=1]. [20014:20014:0804/141434:ERROR:clip_recorder.cc(28)] End clip bounds_in_layer=409,237 15x14 [20014:20014:0804/141434:ERROR:clip_recorder.cc(23)] ClipRecorder destructor [num_closers=1]. [20014:20014:0804/141434:ERROR:clip_recorder.cc(28)] End clip bounds_in_layer=409,128 15x123 [20014:20014:0804/141434:ERROR:clip_recorder.cc(23)] ClipRecorder destructor [num_closers=1]. [20014:20014:0804/141434:ERROR:clip_recorder.cc(28)] End clip bounds_in_layer=409,128 15x123 [20014:20014:0804/141434:ERROR:clip_recorder.cc(23)] ClipRecorder destructor [num_closers=1]. [20014:20014:0804/141434:ERROR:clip_recorder.cc(28)] End clip bounds_in_layer=24,127 401x125
,
Aug 4 2016
So.. I'm adding a feature which seems to fix this, which is to scroll with Layers in views::ScrollView - https://codereview.chromium.org/2188133002/ . But I wasn't planning on turning it on by default anywhere except Mac just yet. Also, it doesn't address the underlying problem, but if there's no other symptoms, and addressing the underlying problem has bad performance implications, then it may be an option.
,
Aug 5 2016
@tapted that would be very exciting, to consider further though I see your change is non-trivial and bake time likely warranted. I've spent the afternoon debugging, but it's a pain as I've not got a reduced test case yet and haven't spent a lot of time in ui/views code to date. Considering either building a fake View > ScrollView > TreeView that demonstrates the issue, or hacking fake data into the cookie list dialog, to make debugging with an empty html doc easier. I'm not yet sure that there's a fundamental bug outside of ScrollView/TreeView. Work will continue on the morrow.
,
Aug 5 2016
I am preparing a revert due to http://crbug.com/634823 to land today and will continue looking into this to fix for re-land.
,
Aug 5 2016
,
Aug 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/490867260447e9360b52d5c03417b000490332e0 commit 490867260447e9360b52d5c03417b000490332e0 Author: wkorman <wkorman@chromium.org> Date: Fri Aug 05 22:24:57 2016 Revert "Raster display item lists via a visual rect RTree." Meta-revert for http://crrev.com/1484163002 due to bugs found in ui/views and DevTools. Original change description: Raster display item lists via a visual rect RTree. Rather than caching and playing back an entire SkPicture when rastering a display item list for a particular playback rect, instead retain display items and query them via an RTree of their visual rects to find and raster only what's needed. Display item lists no longer support the notion of a bounding "layer rect" with mutable origin. DisplayItemListSettings proto is obsolete after this change as it's comprised solely of one field to allow switching whether to use the aforementioned now-deleted cached SkPicture code path. It will be deleted in a subsequent patch. Revert "Raster display item lists via a visual rect RTree." This reverts commit ccb9e13712b1632b889960d1d85d556c0139fd51. Revert "Don't clear visual rects when finalizing display item lists for now." This reverts commit 1adf72a0a0a3e04151cc740d15ab19655b1e7e5e. Revert "Delete obsolete DisplayItemList::ProcessAppendedItem method definition." This reverts commit f652746f56c59523b0440cf18b769f8ba779d15d. BUG= 529938 , 633750 , 633869 , 634239 , 634823 , 634959 TBR=chrishtr@chromium.org,vmpstr@chromium.org,lushnikov@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Review-Url: https://codereview.chromium.org/2217263003 Cr-Commit-Position: refs/heads/master@{#410190} [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/blink/web_content_layer_impl.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/blink/web_display_item_list_impl.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/debug/rasterize_and_record_benchmark.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/debug/rasterize_and_record_benchmark.h [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/layers/empty_content_layer_client.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/layers/picture_image_layer.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/layers/picture_layer_impl_unittest.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/playback/discardable_image_map_unittest.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/playback/display_item_list.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/playback/display_item_list.h [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/playback/display_item_list_unittest.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/playback/recording_source.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/proto/display_item.proto [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/test/fake_content_layer_client.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/test/skia_common.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/test/solid_color_content_layer_client.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/trees/layer_tree_host_pixeltest_masks.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/cc/trees/layer_tree_host_pixeltest_tiles.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/ui/compositor/canvas_painter.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/ui/compositor/layer.cc [modify] https://crrev.com/490867260447e9360b52d5c03417b000490332e0/ui/views/view_unittest.cc
,
Aug 9 2016
Verified the issue on Latest Dev# 54.0.2824.0 on Windows and Linux and has been fixed and is working as intended. Hence adding TE-Verified Labels. Also adding screenshot for reference. Thank You.
,
Aug 9 2016
Just to update, verified the issue on Latest Dev# 54.0.2823.0 for Linux. So adding TE-Verified-54.0.2823.0 for Linux. Thank You.
,
Aug 9 2016
The problem is that the clip created by ui::ClipRecorder does not have an accurate bound, so the compositor rtree is culling it out. This is why scrolled elements are not getting clipped properly when outside the scroller. Note that this dialog is implemented in native UI view components, in ui/views/collected_cookies_views.cc.
,
Aug 9 2016
FYI I'm currently digging into this.
,
Aug 9 2016
Update: the bug is that paired display items need to expand to the bounds of the items they contain.
,
Aug 9 2016
Proposed fix is an adaptation of http://crrev.com/1814013003 to move visual rect unioning for paired display items into cc:DisplayItemList.
,
Aug 10 2016
Confirmed that patch in flight http://crrev.com/2230513005 merged with the rtree patch http://crrev.com/2225563002 fixes the broken clipping in shockwave cookie list view. I'm porting unit tests for the logic move from Blink to cc and will send change for review when complete.
,
Aug 11 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/923cfdd728ac4e0f7f35333c994c79e6f9403625 commit 923cfdd728ac4e0f7f35333c994c79e6f9403625 Author: wkorman <wkorman@chromium.org> Date: Thu Aug 11 00:04:33 2016 Move visual rect unioning between paired items to cc::DisplayItemList. http://crrev.com/1814013003 added this for Blink-driven visual rects, and it turns out we need it for ui/views as well, so we centralize in cc directly. Visual rects now only have to be explicitly passed for drawing display items. BUG= 634239 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Review-Url: https://codereview.chromium.org/2230513005 Cr-Commit-Position: refs/heads/master@{#411195} [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/cc/blink/web_display_item_list_impl.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/cc/blink/web_display_item_list_impl.h [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/cc/layers/picture_image_layer.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/cc/playback/display_item_list.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/cc/playback/display_item_list.h [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/cc/playback/display_item_list_unittest.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/cc/playback/display_item_proto_factory.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/cc/test/fake_content_layer_client.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/cc/test/geometry_test_utils.h [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/cc/test/solid_color_content_layer_client.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/cc/trees/layer_tree_host_pixeltest_masks.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/cc/trees/layer_tree_host_pixeltest_tiles.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/chrome/browser/ui/views/tabs/tab_strip.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/Source/platform/graphics/paint/ClipDisplayItem.cpp [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/Source/platform/graphics/paint/ClipPathDisplayItem.cpp [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/Source/platform/graphics/paint/CompositingDisplayItem.cpp [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.cpp [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/Source/platform/graphics/paint/DisplayItemListTest.cpp [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/Source/platform/graphics/paint/FilterDisplayItem.cpp [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/Source/platform/graphics/paint/FloatClipDisplayItem.cpp [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/Source/platform/graphics/paint/ScrollDisplayItem.cpp [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/Source/platform/graphics/paint/Transform3DDisplayItem.cpp [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/Source/platform/graphics/paint/TransformDisplayItem.cpp [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/third_party/WebKit/public/platform/WebDisplayItemList.h [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/ui/compositor/clip_recorder.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/ui/compositor/clip_recorder.h [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/ui/compositor/compositing_recorder.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/ui/compositor/compositing_recorder.h [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/ui/compositor/paint_cache.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/ui/compositor/paint_recorder.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/ui/compositor/transform_recorder.cc [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/ui/compositor/transform_recorder.h [modify] https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625/ui/views/view.cc
,
Aug 11 2016
When we re-land the patch for crbug.com/529938 shortly, potentially tomorrow, it should now no longer introduce the issue noted in this bug.
,
Aug 16 2016
Tested the issue on Windows 7, Ubuntu 14.04 using 54.0.2830.0.Overlapping of cookies is not seen in 'cookies set by this page' overlay. Please find attached screenshot. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by vku...@etouch.net
, Aug 4 2016Components: Blink>Layout
Labels: hasbisect OS-Linux
Owner: wkorman@chromium.org
Status: Assigned (was: Unconfirmed)
142 KB
142 KB View Download