New issue
Advanced search Search tips

Issue 916177 link

Starred by 3 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 866708



Sign in to add a comment

Fix remaining tests blocking Mojo dispatch changes

Project Member Reported by rockot@google.com, Dec 18

Issue description

It's time once again to try landing the damn dispatch task granularity change (bug 866708).

There are now a few more tests failing, but they are consistent across all platforms. Wew. Gonna try to fix them all in one go.

Tests are:

content_browsertests:
* MediaSessionImplParamBrowserTest.AddingMojoObserverNotifiesCurrentInformation_WithInfo/1
* MediaSessionImplParamBrowserTest.AddingMojoObserverNotifiesCurrentInformation_WithInfo/0

content_unittests:
* MediaSessionImplServiceRoutingTest.NotifyMojoObserverMetadataWhenControllable
* MediaInternalsAudioFocusTest.AudioFocusStateIsUpdated
* MojoAsyncResourceHandlerWithAllocationSizeTest/MojoAsyncResourceHandlerWithAllocationSizeTest.OnWillStartThenOnWillReadThenOnResponseStartedThenOnReadCompletedThenOnResponseCompleted/0
* MojoAsyncResourceHandlerWithAllocationSizeTest/MojoAsyncResourceHandlerWithAllocationSizeTest.OnWillStartThenOnWillReadThenOnResponseStartedThenOnReadCompletedThenOnResponseCompleted/1
* MojoAsyncResourceHandlerWithAllocationSizeTest/MojoAsyncResourceHandlerWithAllocationSizeTest.OnWillStartThenOnResponseStartedThenOnWillReadThenOnReadCompletedThenOnResponseCompleted/0
* MojoAsyncResourceHandlerWithAllocationSizeTest/MojoAsyncResourceHandlerWithAllocationSizeTest.OnWillStartThenOnResponseStartedThenOnWillReadThenOnReadCompletedThenOnResponseCompleted/1


services_unittests:
* AudioFocusManagerTest.ObserverActiveSessionChanged/0
* AudioFocusManagerTest.ObserverActiveSessionChanged/1
* MediaControllerTest.ActiveController_Metadata_Observer_WithInfo


 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/720c2c183cdad4da22a5d2d195ea487ca9b7f9ba

commit 720c2c183cdad4da22a5d2d195ea487ca9b7f9ba
Author: Ken Rockot <rockot@google.com>
Date: Wed Dec 19 04:15:33 2018

Correct some Mojo loading test expectations

This fixes a few instances where the meaning of "response received" and
"body arrived" were probably unintentionally conflated.

Changing Mojo bindings dispatch to use more granular tasks (see
https://chromium-review.googlesource.com/c/chromium/src/+/1145692)
reveals these bugs.

This corrects the expectations.

Bug: 916177
Change-Id: I393736977f711ddc44d63328a1959e240486e2af
Reviewed-on: https://chromium-review.googlesource.com/c/1383291
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#617725}
[modify] https://crrev.com/720c2c183cdad4da22a5d2d195ea487ca9b7f9ba/content/browser/loader/mojo_async_resource_handler_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Dec 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/453a706c8b4fb186a6474fe15fda54346e4ad63b

commit 453a706c8b4fb186a6474fe15fda54346e4ad63b
Author: Ken Rockot <rockot@google.com>
Date: Wed Dec 19 14:31:46 2018

Fix media session and audio focus test behavior

When changing Mojo bindings dispatch timing (namely when attempting to
land https://chromium-review.googlesource.com/c/chromium/src/+/1145692),
a few of these tests break due to incorrect expectations which are no
longer being met.

Namely:

- Empty MediaSessionMetadata updates happen (presumably on sesssion
  initialization?) and appear to have been previously unanticipated
  by test code. They happened to be obscured because a single Mojo
  dispatch task would dispatch the empty metadata message and the
  populated metadata message all before the test's RunLoop could quit.

- The MediaInternalsAudioFocusTest makes some dubious assumptions about
  the precise number of update callbacks that will be invoked by the
  system, and this seems to change with the Mojo change in-place.
  This most likely means that tests waiting on a specific number of
  callbacks (rather than, say, waiting for specific update contents) at
  any point is a questionable testing strategy.

- AudioFocusManagerTest.ObserverActiveSessionChanged has similar
  issues, where the system is expected to be in a certain state despite
  there being insufficient work done to guarantee it.

In the first case, I've added a WaitForNonEmptyMetadata() helper to
MockMediaSessionMojoObserver which waits more precisely for what the
test code wants. This seemed like the best of a few possible options.

In the latter two cases, I have cheesily employed RunUntilIdle() to
keep the tests enabled without getting stuck trying to sort out the
correct expectations, because they seem complicated. Someone should go
back and fix the tests later.

TBR=xhwang@chromium.org

Bug: 916177
Change-Id: I15643e436354f290646f5c66dc3a54b18e99eae0
Reviewed-on: https://chromium-review.googlesource.com/c/1382700
Reviewed-by: Becca Hughes <beccahughes@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#617823}
[modify] https://crrev.com/453a706c8b4fb186a6474fe15fda54346e4ad63b/content/browser/media/media_internals_unittest.cc
[modify] https://crrev.com/453a706c8b4fb186a6474fe15fda54346e4ad63b/content/browser/media/session/media_session_impl_browsertest.cc
[modify] https://crrev.com/453a706c8b4fb186a6474fe15fda54346e4ad63b/content/browser/media/session/media_session_impl_service_routing_unittest.cc
[modify] https://crrev.com/453a706c8b4fb186a6474fe15fda54346e4ad63b/services/media_session/audio_focus_manager_unittest.cc
[modify] https://crrev.com/453a706c8b4fb186a6474fe15fda54346e4ad63b/services/media_session/media_controller_unittest.cc
[modify] https://crrev.com/453a706c8b4fb186a6474fe15fda54346e4ad63b/services/media_session/public/cpp/test/mock_media_session.cc
[modify] https://crrev.com/453a706c8b4fb186a6474fe15fda54346e4ad63b/services/media_session/public/cpp/test/mock_media_session.h

Cc: sky@chromium.org
Aaaaand now there are more! All Chrome OS, some flaky.

These failures are a strong indication that the tests are wrong and relying on unspecified behavior in the system. I have unfortunately been trying to land the Mojo change[1] for several months now, and every time I try there are new test failures that have to be fixed. Probably close to 100 failures have been fixed so far, and so far every single case has been due to incorrect test expectations.

+sky@ - any chance you have anyone who can help look into some of these ash/mash failures? Note that they will only repro with [1] applied, but if fixed they should work with or without [1] applied.

ash_unittests:
HighlighterControllerTest.SelectionInsideScreen

single_process_mash_ash_unittests:
HighlighterControllerTest.HighlighterGesturesScaled
HighlighterControllerTest.SelectionInsideScreen
MagnificationControllerTest.TextfieldFocusedWithKeyboard

chromeos_unittests:
EnableDisableSendingWifiData/SimpleGeolocationWirelessTest.CellularExists/1
EnableDisableSendingWifiData/SimpleGeolocationWirelessTest.WiFiExists/1

simple_process_mash_interactive_ui_tests:
ChromeVisibilityObserverInteractiveTest.VisibilityTest
LocationIconViewTest.HideOnSecondClick
TabDragging/DetachToBrowserTabDragControllerTest.FastResizeDuringDragging/1
TabDragging/DetachToBrowserTabDragControllerTest.DragToSeparateWindow/1
TabDragging/DetachToBrowserTabDragControllerTest.DoNotAttachToOtherWindowTest/1
TabDragging/DetachToBrowserTabDragControllerTest.DragToOverviewNewWindowItem/1
TabDragging/DetachToBrowserTabDragControllerTest.DragToOverviewWindow/1
TabDragging/DetachToBrowserTabDragControllerTest.DragAllToSeparateWindow/1
TabDragging/DetachToBrowserTabDragControllerTest.DragToMinimizedOverviewWindow/1
TabDragging/DetachToBrowserTabDragControllerTest.DragToMinimizedOverviewWindow/0
TabDragging/DetachToBrowserTabDragControllerTest.DragToOverviewNewWindowItem0
TabDragging/DetachToBrowserTabDragControllerTestTouch.SecondFingerPressTest/0
TabDragging/DetachToBrowserTabDragControllerTest.DeferredTargetTabStripTest/1
TabDragging/DetachToBrowserTabDragControllerTest.DragAllToSeparateWindowAndCancel/1

browser_tests:
EnterpriseEnrollmentConfigurationTest.TestEnrollUsingToken
TopControlsSlideControllerTest.TestDropDowns

simple_process_mash_browser_tests:
HomeLauncherBrowserNonClientFrameViewAshTest.TabletModeAppCaptionButtonVisibility/1
HomeLauncherBrowserNonClientFrameViewAshTest.TabletModeAppCaptionButtonVisibility/0
BrowserNonClientFrameViewAshTest.HeaderVisibilityInOverviewAndSplitview/1
BrowserNonClientFrameViewAshTest.HeaderVisibilityInOverviewAndSplitview/0
ChromeNativeAppWindowViewsAuraAshBrowserTest.ImmersiveWorkFlow
EnterpriseEnrollmentConfigurationTest.TestEnrollUsingToken
KeyboardControllerAppWindowTest.DisableOverscrollForImeWindow
BrowserNonClientFrameViewAshTest.ToggleTabletModeRelayout/0
BrowserNonClientFrameViewAshTest.ToggleTabletModeRelayout/1


[1] https://chromium-review.googlesource.com/c/chromium/src/+/1145692
We are busy trying to fix remaining issues for SingleProcessMash. We will likely have time for this once we get through those issues. Basically, we likely won't have time for this until the new year.
Cc: xiy...@chromium.org mukai@chromium.org
Jun or Xiyuan may have some cycles for this soon.
Thanks! I have an attempt to "fix" some of them and was considering disabling others: https://chromium-review.googlesource.com/c/chromium/src/+/1388172

Still a few other issues to sort out with that but I think it will be better if the dispatch changes land sooner rather than later.
I suspect that the fix for these is to make use of WaitForAllChangesToComplete() in ui/aura/test/mus.
I'll take a look at them.
(I'm not objecting to disable them for now, but I will see what was missing on those chromeos-mash-specific errors/flakes)
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c44c414dbf96ac6595b1500c2d0f0de8660ed828

commit c44c414dbf96ac6595b1500c2d0f0de8660ed828
Author: Jun Mukai <mukai@chromium.org>
Date: Wed Dec 26 18:44:43 2018

Make a wait for Mash on SetAndWaitForTabletMode

This fixes the following tests for issue 916177 along with others.
TabDragging/DetachToBrowserTabDragControllerTest.DragToMinimizedOverviewWindow/0
TabDragging/DetachToBrowserTabDragControllerTest.DragToMinimizedOverviewWindow/1

Bug: 916177
Test: interactive_ui_tests
Change-Id: I9fbc8a81932d1a775bda30cbfb9ac61e901b8b40
Reviewed-on: https://chromium-review.googlesource.com/c/1389439
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Jun Mukai <mukai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618944}
[modify] https://crrev.com/c44c414dbf96ac6595b1500c2d0f0de8660ed828/chrome/browser/ui/ash/tablet_mode_client_test_util.cc

Cc: shend@chromium.org
Re: KeyboardControllerAppWindowTest.DisableOverscrollForImeWindow

It's very unclear the expectation here; https://chromium-review.googlesource.com/c/chromium/src/+/1388172 changes some order of mojo calls, which causes the actual errors:
- without this patch: OnWindowOcclusionChanged happens first, which changes non_ime_app_window's bounds
- with this patch: EnsureCaretNotInRect mojo calls happens earlier, which moves non_ime_app_window away so that it won't overlap with the virtual keyboard. Thus the bounds do not change unexpectedly

This avoiding of virtual keyboard on appearing does sound reasonable, so the both behavior could be correct. +shend@
Cc: yhanada@chromium.org
Hmm unfortunately I'm not the author of the test. From my understanding:

- We should first try to move the window away to prevent overlap.
- If that's not possible (e.g. window is maximized, or the window size is too big and still overlaps with VK), then we have to also adjust the visual viewport size.

If I understand the test correctly, it sets the VK height to be (screen_height - ime_window_visible_height + 1). The "+ 1" means that there should always be an overlap between the app window and the keyboard, even if the window tries to move away.

AFAIK, EnsureCaretNotInRect should still update the visual viewport size even after moving the window? e.g. https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_aura.cc?sq=package:chromium&g=0&l=1508

+yhanada who probably knows more about this
Cc: steve...@chromium.org
Thanks! I've realized that I was wrong a bit -- I was seeing the window avoidance for showing VK initially.

So, the test scenario is 1. show VK, 2. wait for visible/loaded, 3. change the keyboard bounds, and the test scenario doesn't wait for the bounds changed properly when Mash, but previously it happened to succeed since the window was resized for 1. However, waiting explicitly for KeyboardOcclusionBoundsChanged() isn't yet enough, it seems ash_keyboard_ui.cc code doesn't work properly on the bounds changed (or at least doesn't work well with the test scenario). FYI: stevenjb@
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a915f824bd52357dccf146642048a0fb8bbd3b2

commit 2a915f824bd52357dccf146642048a0fb8bbd3b2
Author: Jun Mukai <mukai@chromium.org>
Date: Fri Dec 28 00:24:11 2018

Fix EnterpriseEnrollmentConfigurationTest.TestEnrollUsingToken

The test case is broken with https://crrev.com/c/1388172. Turns out
that it does not wait for the state changes properly -- calling
ExecutePendingJavascript() properly waits for the state changes
in the oobe web content.

Bug: 916177
Test: browser_tests
Change-Id: I58b5730938d882afaebf713a222c01d4c92e2d46
Reviewed-on: https://chromium-review.googlesource.com/c/1391739
Commit-Queue: Jun Mukai <mukai@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619099}
[modify] https://crrev.com/2a915f824bd52357dccf146642048a0fb8bbd3b2/chrome/browser/chromeos/login/enterprise_enrollment_browsertest.cc

Okay, I think I got all of them, except for KeyboardControllerAppWindowTest.DisableOverscrollForImeWindow. Others are relatively simple and will be addressed by upcoming CLs.

KeyboardControllerAppWindowTest is hard for me since it's not clear for me how *actual* VK resize happens. I'll sync up with yhanada@ when he's back.
Maybe I can help in the meantime. What do you mean by "actual VK resize"?

When the VK shows for the first time, it starts off with 0 height. Then JavaScript will call window.resizeTo and resize the window with the correct VK height. That should cause any windows to move upwards and update the visual viewport sizes.
Project Member

Comment 17 by bugdroid1@chromium.org, Dec 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3fa5bb24df8a4eb8470d2562e88ee0a347f2d776

commit 3fa5bb24df8a4eb8470d2562e88ee0a347f2d776
Author: Jun Mukai <mukai@chromium.org>
Date: Fri Dec 28 17:49:44 2018

Fix HighlighterControllerTest flakiness

For crrev.com/c/1388172, some timing of methods calling can change.
More specifically, ReclaimResources() can be called after
DidLoseLayerTreeFrameSink() is called but it is not deleted.
This is revealed as a DCHECK error in ReclaimResources().

See crbug.com/916177 for the details.

Bug: 916177
Test: ash_unittests with --gtest_repeat=10
Change-Id: Icf0d16bf94e796c66bd660ad536a7e005bcef042
Reviewed-on: https://chromium-review.googlesource.com/c/1391787
Reviewed-by: Vladislav Kaznacheev <kaznacheev@chromium.org>
Commit-Queue: Jun Mukai <mukai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619170}
[modify] https://crrev.com/3fa5bb24df8a4eb8470d2562e88ee0a347f2d776/ash/components/fast_ink/fast_ink_view.cc

#c16; thanks!

As far as I see the vlog, indeed the VK window has 0 height and then gets resized to a height, and that seems properly handled with Mash too. That means I guess JS window.resizeTo works.

The test case, DisableOverscrollForImeWindow, does a slightly different thing; after the VK appears (and resized to some default height), the VK window is resized through GetKeyboardWindow()->SetBounds() (in C++). And this SetBounds() seems not properly handled with Mash. Maybe I need to find a better way to mock window.resizeTo which works well with Mash.
mukai@ - The SetBounds() timing is tricky with Mash. It requires multiple round trips:

JS -> Chrome -> [ash.mojom.KeyboardController.SetOccludedBounds] -> Ash -> [ash.mojom.KeyboardControllerObserver.OnKeyboardOccludedBoundsChanged] -> Chrome -> JS

There is a RunUntilIdle() that should probably be a controller->FlushForTesting():

https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/keyboard/keyboard_controller_browsertest.cc?type=cs&q=DisableOverscrollForImeWindow&sq=package:chromium&g=0&l=283

But that might not be sufficient, you might also requrie a JS flush somewhere.

stevenjb@ -- I'm afraid that wouldn't work. I actually made a change locally to replace the RunUntilIdle() by waiting OnKeyboardOccludedBoundsChanged(), then realized that the bounds change does not occur occluded bounds change and the test won't finish (uploaded this WIP as https://chromium-review.googlesource.com/c/chromium/src/+/1394595).

As analyzed a bit more, I've come to suspect that the keyboard window's resize isn't properly observed in Mash.

In classic environment, the window hierarchy is:
Root
- keyboard container
   - WebContentsViewAura (= KeyboardWindow)

The test code (SetBounds) and window.resizeTo() can update the keyboard window's resize eventually, and so KeyboardOccludedBoundsChange should happen.

In Mash:
Root
- keyboard container
   - AshKeyboardUI (= KeyboardWindow in Ash)
      - NativeViewHostAuraClip (in ServerRemoteViewHost)
         - a native view  <--> WebContentsViewAura in the browser (KeyboardWindow in ChromeKeyboardController)


window.resizeTo() comes to WebContentsViewAura, which should be propagated to the native view through WS. But I guess that the new bounds isn't propagated to AshKeyboardUI and therefore occluded bounds change never occurs.
It is probably because RemoteViewProvider/(Server)RemoteViewHost does not support bounds change from the embedded window. Currently, we make the embedded window change size with its host but not the other way around.
IIRC, the code relies on ChromeVirtualKeyboardDelegate::SetOccludedBounds() to notify Ash that the occluded bounds changed, not a window resize. i.e. the extension should be handing the window resize event and calling virtual_keyboard_private.setOclludedBounds().

It's possible that the way that test is implemented is not suited for Mash and needs to be re-implemented. If you want to file a bug and assign it to me, I can take a look (but I appreciate your investigating this, it would be good to have someone else familiar with how the VK works in Mash).

Hmm, ChromeVirtualKeyboardDelegate::SetOccludedBounds seems tied with a private virtual keyboard API setOccludedBounds, and I am afraid that this is not the case -- this case resizes the window for VK WebContents, which would end up with occluded bounds change.

So I think we need to change ServerRemoteViewHost to support such case as #c21 suggested. Updated my CL.
Project Member

Comment 24 by bugdroid1@chromium.org, Jan 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/107457cfbd0c28e71ff6e8af40580fbcaae0357c

commit 107457cfbd0c28e71ff6e8af40580fbcaae0357c
Author: Jun Mukai <mukai@chromium.org>
Date: Mon Jan 07 23:46:00 2019

Flush WindowTreeClient before the EventInjector's done callback

As with the changes on mojo layer, some tasks can be in window
tree client -- and to make the operation order more natural,
those tasks need to be finished when the control flow switches
to the next task.

Note that this shouldn't be WaitForAllChangesToComplete() unlike
other changes, since it may not stop at all at this point. Simply
flushing is fine.

This CL fixes the following tests in issue 916177
LocationIconViewTest.HideOnSecondClick
all other TabDragging tests

Bug: 916177
Test: interactive_ui_tests
Change-Id: I238b3e4f470b64229614a708f337e3548cb130b9
Reviewed-on: https://chromium-review.googlesource.com/c/1389499
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Jun Mukai <mukai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620530}
[modify] https://crrev.com/107457cfbd0c28e71ff6e8af40580fbcaae0357c/ui/aura/test/mus/window_tree_client_test_api.h
[modify] https://crrev.com/107457cfbd0c28e71ff6e8af40580fbcaae0357c/ui/aura/test/ui_controls_factory_aura.h
[modify] https://crrev.com/107457cfbd0c28e71ff6e8af40580fbcaae0357c/ui/aura/test/ui_controls_factory_ozone.cc

Project Member

Comment 25 by bugdroid1@chromium.org, Jan 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/389ff295112cbdcf60059f0b182ea0a1a96e8f16

commit 389ff295112cbdcf60059f0b182ea0a1a96e8f16
Author: Jun Mukai <mukai@chromium.org>
Date: Mon Jan 07 23:46:35 2019

Add WaitForChangesToComplete() in TabDragController tests

This fixes many (but not all) of the tab-dragging test failures
reported in the issue 916177. Some others are still broken and
need to be fixed separately.

More specifically, the folloing tests are still failing:
TabDragging/DetachToBrowserTabDragControllerTest.DeferredTargetTabStripTest/1
TabDragging/DetachToBrowserTabDragControllerTest.DragToMinimizedOverviewWindow/0
TabDragging/DetachToBrowserTabDragControllerTest.DragToMinimizedOverviewWindow/1
TabDragging/DetachToBrowserTabDragControllerTest.DragToOverviewNewWindowItem/0
TabDragging/DetachToBrowserTabDragControllerTest.DragToOverviewNewWindowItem/1
TabDragging/DetachToBrowserTabDragControllerTest.FastResizeDuringDragging/1

Bug: 916177
Test: interactive_ui_tests with patches
Change-Id: I92cdad63dbea6245387d0dceba7120fd7f992cbb
Reviewed-on: https://chromium-review.googlesource.com/c/1389322
Commit-Queue: Jun Mukai <mukai@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620531}
[modify] https://crrev.com/389ff295112cbdcf60059f0b182ea0a1a96e8f16/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc

Project Member

Comment 26 by bugdroid1@chromium.org, Jan 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df9e5f7d297bab04d2dc4ab2d2ee21e1cabc4234

commit df9e5f7d297bab04d2dc4ab2d2ee21e1cabc4234
Author: Jun Mukai <mukai@chromium.org>
Date: Mon Jan 07 23:47:06 2019

Use the owned GeolocationHandler instance for tests

crrev.com/c/1388172 changes the timing of the callback is called,
that leads failures on two SimpleGeolocationWirelessTest test cases.

By looking the details, SimpleGeolocationProvider always uses
NetworkHandler::Get()->geolocation_handler() while the test case
makes sure the status of its own geolocation_handler. This CL
allows to use its own geolocaion_handler for SimpleGeolocationProvider.

Bug: 916177
Test: chromeos_unittests
Change-Id: I56060bdd440591bfa4c2f9bf0b17c114780cee7b
Reviewed-on: https://chromium-review.googlesource.com/c/1391869
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Commit-Queue: Jun Mukai <mukai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620532}
[modify] https://crrev.com/df9e5f7d297bab04d2dc4ab2d2ee21e1cabc4234/chromeos/geolocation/simple_geolocation_provider.cc
[modify] https://crrev.com/df9e5f7d297bab04d2dc4ab2d2ee21e1cabc4234/chromeos/geolocation/simple_geolocation_provider.h
[modify] https://crrev.com/df9e5f7d297bab04d2dc4ab2d2ee21e1cabc4234/chromeos/geolocation/simple_geolocation_unittest.cc

Project Member

Comment 27 by bugdroid1@chromium.org, Jan 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6a1e8aef3b706eff4071b55ce0a851922c369bfa

commit 6a1e8aef3b706eff4071b55ce0a851922c369bfa
Author: Jun Mukai <mukai@chromium.org>
Date: Tue Jan 08 00:57:28 2019

Wait pending changes on closing browser window

This fixes ChromeVisibilityObserverInteractiveTest.VisibilityTest

Bug: 916177
Test: interactive_ui_tests with the patch
Change-Id: Id4a7865c5afd2a5354425d89bfdf779cce925aca
Reviewed-on: https://chromium-review.googlesource.com/c/1391352
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Jun Mukai <mukai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620559}
[modify] https://crrev.com/6a1e8aef3b706eff4071b55ce0a851922c369bfa/chrome/test/base/in_process_browser_test.cc

Project Member

Comment 28 by bugdroid1@chromium.org, Jan 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/052fbb5f89174404f604f44929e7f0e963eb6fe0

commit 052fbb5f89174404f604f44929e7f0e963eb6fe0
Author: Jun Mukai <mukai@chromium.org>
Date: Tue Jan 08 01:04:54 2019

Fix BrowserNonClientFrameViewAsh tests

This CL waits properly for an asynchronous change. Confirmed
the following tests are fixed, reported at crbug.com/916177
HomeLauncherBrowserNonClientFrameViewAshTest.TabletModeAppCaptionButtonVisibility/*
BrowserNonClientFrameViewAshTest.HeaderVisibilityInOverviewAndSplitview/*

Bug: 916177
Test: browser_tests
Change-Id: Ic9f98dbb5e4644470ec3b97b0a7119f2ef9aead8
Reviewed-on: https://chromium-review.googlesource.com/c/1391359
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Jun Mukai <mukai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620560}
[modify] https://crrev.com/052fbb5f89174404f604f44929e7f0e963eb6fe0/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc

Project Member

Comment 29 by bugdroid1@chromium.org, Jan 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f7cf25475567a68b1f68017efe4269ed2608232d

commit f7cf25475567a68b1f68017efe4269ed2608232d
Author: Jun Mukai <mukai@chromium.org>
Date: Tue Jan 08 01:22:27 2019

Fix TopControlsSlideControllerTest.TestDropDowns

This is similar to crrev.com/c/1391739; executing an empty
sentence in the web content can wait for the content state updates.

Bug: 916177
Test: browser_tests
Change-Id: Ia2047a7f7d0179541bfa3c2ed46eea48aca0f5ef
Reviewed-on: https://chromium-review.googlesource.com/c/1399518
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Jun Mukai <mukai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620563}
[modify] https://crrev.com/f7cf25475567a68b1f68017efe4269ed2608232d/chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos_browsertest.cc

Project Member

Comment 30 by bugdroid1@chromium.org, Jan 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6a6bcece1c12cce174653ad631b8e0bf2a0058d8

commit 6a6bcece1c12cce174653ad631b8e0bf2a0058d8
Author: Jun Mukai <mukai@chromium.org>
Date: Wed Jan 09 00:32:50 2019

wait properly for the keyboard bounds change

Instead of RunUntilIdle, it should wait for OnKeyboardOccludedBoundsChange.
This CL also allows to resize the window which remotely hosts the VK
web-contents based on window.resizeTo().

BUG=916177
TEST=none

Change-Id: I5513e8a614c9fef26f92a179adfb25ac86207676
Reviewed-on: https://chromium-review.googlesource.com/c/1394595
Commit-Queue: Jun Mukai <mukai@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620961}
[modify] https://crrev.com/6a6bcece1c12cce174653ad631b8e0bf2a0058d8/ash/keyboard/ash_keyboard_ui.cc
[modify] https://crrev.com/6a6bcece1c12cce174653ad631b8e0bf2a0058d8/chrome/browser/ui/ash/keyboard/keyboard_controller_browsertest.cc

Project Member

Comment 31 by bugdroid1@chromium.org, Jan 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df0b6c576a294ad192adcf2e087e57672ecbf5b7

commit df0b6c576a294ad192adcf2e087e57672ecbf5b7
Author: Hajime Hoshi <hajimehoshi@chromium.org>
Date: Wed Jan 09 04:45:56 2019

Revert "wait properly for the keyboard bounds change"

This reverts commit 6a6bcece1c12cce174653ad631b8e0bf2a0058d8.

Reason for revert: Some test failures on ChromeOS build bots:  crbug.com/920073 

Original change's description:
> wait properly for the keyboard bounds change
> 
> Instead of RunUntilIdle, it should wait for OnKeyboardOccludedBoundsChange.
> This CL also allows to resize the window which remotely hosts the VK
> web-contents based on window.resizeTo().
> 
> BUG=916177
> TEST=none
> 
> Change-Id: I5513e8a614c9fef26f92a179adfb25ac86207676
> Reviewed-on: https://chromium-review.googlesource.com/c/1394595
> Commit-Queue: Jun Mukai <mukai@chromium.org>
> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#620961}

TBR=xiyuan@chromium.org,stevenjb@chromium.org,mukai@chromium.org

Change-Id: I6220281053f251efa9b9db6e7e384638422033b5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 916177
Reviewed-on: https://chromium-review.googlesource.com/c/1401926
Reviewed-by: Hajime Hoshi <hajimehoshi@chromium.org>
Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621045}
[modify] https://crrev.com/df0b6c576a294ad192adcf2e087e57672ecbf5b7/ash/keyboard/ash_keyboard_ui.cc
[modify] https://crrev.com/df0b6c576a294ad192adcf2e087e57672ecbf5b7/chrome/browser/ui/ash/keyboard/keyboard_controller_browsertest.cc

Project Member

Comment 32 by bugdroid1@chromium.org, Jan 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/59ec9ad1528347e6b2d7146966437bb89c5e9169

commit 59ec9ad1528347e6b2d7146966437bb89c5e9169
Author: Jun Mukai <mukai@chromium.org>
Date: Wed Jan 09 22:42:01 2019

Reland "wait properly for the keyboard bounds change"

Instead of RunUntilIdle, it should wait for OnKeyboardOccludedBoundsChange.
This CL also allows to resize the window which remotely hosts the VK
web-contents based on window.resizeTo().

This is a reland of crrev.com/c/1394595 with an addition of waiting
the processing of SetBounds in MockEnableIMEInDifferentExtension().

TBR=stevenjb@chromium.org, xiyuan@chromium.org

Bug: 916177,  920073 
Test: browser_tests
Change-Id: Ib353f5193ab4f7bb907a6c02ece6429b42408919
Reviewed-on: https://chromium-review.googlesource.com/c/1403923
Reviewed-by: Jun Mukai <mukai@chromium.org>
Commit-Queue: Jun Mukai <mukai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621343}
[modify] https://crrev.com/59ec9ad1528347e6b2d7146966437bb89c5e9169/ash/keyboard/ash_keyboard_ui.cc
[modify] https://crrev.com/59ec9ad1528347e6b2d7146966437bb89c5e9169/chrome/browser/ui/ash/keyboard/keyboard_controller_browsertest.cc

rockot@ -- I believe all of the Mash-related failures/flakiness are now addressed.
Please let me know if I'm missing something.
Project Member

Comment 34 by bugdroid1@chromium.org, Jan 16 (6 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/95800c5106678d024941ebc9e6f4e36a70be1ffe

commit 95800c5106678d024941ebc9e6f4e36a70be1ffe
Author: Ken Rockot <rockot@google.com>
Date: Wed Jan 16 20:57:06 2019

"Fix" Audio focus tests for Mojo dispatch change

Changing Mojo dispatch timing breaks these tests:

AudioFocusManagerTest.AudioFocusGrouping_TransientResume/2
AudioFocusManagerTest.AudioFocusGrouping_TransientResume/3

This adds some extra synchronization to the tests so that they end up
in the expected state.

Bug: 916177
Change-Id: If0915a50e43f66240e4b09a5ac0517de219a8bbc
Reviewed-on: https://chromium-review.googlesource.com/c/1409813
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Becca Hughes <beccahughes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623350}
[modify] https://crrev.com/95800c5106678d024941ebc9e6f4e36a70be1ffe/services/media_session/audio_focus_manager_unittest.cc

Sign in to add a comment