Ash gets shelf alignment async, leading to flake in tests and likely windows to dance around on devices |
|||
Issue descriptionAutofillInteractiveTest.BasicFormFill is an example of a test impacted by this. Shelf starts out with an alignment of SHELF_ALIGNMENT_BOTTOM_LOCKED. When in this mode Shelf does not update the work area. When preferences finish loading (async) the real alignment is set. For browser_tests the alignment ends up being SHELF_ALIGNMENT_BOTTOM. SHELF_ALIGNMENT_BOTTOM means the work area is reduced slightly from the bottom. This effectively means in browser_tests the work area changes asynchronously, which in some cases can result in changing the bounds of the browser window. This obviously can impact interactive_ui_tests which are sensitive to the bounds of the browser window (because such tests may generate mouse events). I'm not terribly familiar with the shelf code, but possible fixes seem to be: 1. update work area for SHELF_ALIGNMENT_BOTTOM_LOCKED. This would fix the bug as SHELF_ALIGNMENT_BOTTOM_LOCKED and SHELF_ALIGNMENT_BOTTOM should reduce the work area by the same amount. 2. force a default of SHELF_ALIGNMENT_BOTTOM for tests. This isn't particularly satisfying because it means windows may shift slightly on real devices when layout changes.
,
Apr 18 2018
A third workaround might be to have the test harness spin all message loops until they're all idle before turning over control to the test, which would effectively make this synchronous. This seems a bit like a bandaid, but might be correct anyway, and could address other similar issues beyond just the shelf.
,
Apr 18 2018
I like the first workaround, making SHELF_ALIGNMENT_BOTTOM_LOCKED inset the work area similar to SHELF_ALIGNMENT_BOTTOM. I'm not sure why this isn't already the case, I'll take a quick look.
,
Apr 18 2018
Hmm, BOTTOM_LOCKED doesn't alter the work area for Issue 622431 , also see this comment: https://cs.chromium.org/chromium/src/ash/shelf/shelf_layout_manager.cc?rcl=2b6f6ca9cddd05ce8f17a50072b52f92ccd19daf&l=683 I'm not sure what compromise could be made for the two conflicting expectations of BOTTOM_LOCKED behavior. Perhaps test environments should be either setting SHELF_ALIGNMENT_BOTTOM right away, or waiting for the prefs to load.
,
Apr 18 2018
Maybe BOTTOM_LOCKED should only change the work-area if a non-BOTTOM_LOCKED state hasn't been set yet? I think that would translate to assume BOTTOM, but once non-bottom then don't touch work-area because it may trigger windows being resized/moved.
,
Apr 18 2018
That might work. It's akin to saying "we should initialize the work area insets (assuming a bottom shelf)"
,
Apr 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e6a058bbedd2afe2a63d67159d00968eb845eecf commit e6a058bbedd2afe2a63d67159d00968eb845eecf Author: Mike Wasserman <msw@chromium.org> Date: Tue Apr 24 15:23:48 2018 Ash: Fix test flakiness from aync shelf alignment. The 1st patch set triggers the flaky failure reliably and adds a fix. The proposed fix inits display work areas with BOTTOM_LOCKED insets. (prevents test flakes on async shelf alignment init and bounds changes) The 2nd patch set removes the trigger and re-enables autofill tests. This may cause window movement on left/right alignment user login? (if windows from a prior session are restored before shelf alignment) Alternative fix ideas: 1) Always initialize the primary display's shelf to BOTTOM alignment? (matches login appearance, keep secondary display shelves hidden) 2) Have InProcessBrowserTest init the alignment synchronously? (limits the workaround's scope to tests, where symptoms are felt) 3) Make (tests) wait for shelf init before loading user windows? Bug: 834369 , 834768 Test: No AutofillInteractiveTests flakes on Chrome OS. Change-Id: Ibb8ea879a81ff198a1a29e6c8de73cb5e8acd3c4 Reviewed-on: https://chromium-review.googlesource.com/1020195 Reviewed-by: Roger McFarlane <rogerm@chromium.org> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> Commit-Queue: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#553126} [modify] https://crrev.com/e6a058bbedd2afe2a63d67159d00968eb845eecf/ash/display/display_manager_unittest.cc [modify] https://crrev.com/e6a058bbedd2afe2a63d67159d00968eb845eecf/ash/shelf/shelf_layout_manager.cc [modify] https://crrev.com/e6a058bbedd2afe2a63d67159d00968eb845eecf/chrome/browser/autofill/autofill_interactive_uitest.cc
,
Apr 24 2018
Hopefully this is fixed with my patch, please lmk if we see any further flakes. |
|||
►
Sign in to add a comment |
|||
Comment 1 by ftirelo@chromium.org
, Apr 18 2018