|
|
Created:
5 years, 2 months ago by alogvinov Modified:
5 years, 1 month ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland of Wake Lock API implementation (Chromium part)
Original issue: https://codereview.chromium.org/1107333002/
Original issue description:
Wake Lock API implementation (Chromium part)
This is Chromium part of Wake Lock API implementation as per specification:
http://www.w3.org/TR/wake-lock/
The corresponding Blink part is submitted in issue 1084923002
Design document: https://docs.google.com/document/d/1KbIENP0wgxtSXDQFn9PbHZ_tAKZfR1Y8u4Hst8LpeaA/edit?usp=sharing
R=tsepez@chromium.org
R=nasko@chromium.org
R=mlamouri@chromium.org
R=jochen@chromium.org
BUG=257511
Committed: https://crrev.com/f50445a583e4b7f5f4c865a061c83a4d7b57e5c5
Cr-Commit-Position: refs/heads/master@{#357087}
Patch Set 1 : Original changes #Patch Set 2 : Rebased, fixed patch conflicts #Patch Set 3 : Removed unneeded visibility check form WakeLockApiIsPresent test #Patch Set 4 : Added browser test for visibility changes #Patch Set 5 : Add visibility simulation #Patch Set 6 : Disabled visibility-dependent browser tests on Mac #Patch Set 7 : Enable only WakeLock instead of all experimental features in browser tests #Patch Set 8 : Added visibility override, enabled tests on mac #Patch Set 9 : Try another approach to visibility mocking #Patch Set 10 : Try to use IncrementCaptureCount to prevent occlusion events from changing visibility #Patch Set 11 : Fix presubmit errors #Messages
Total messages: 44 (8 generated)
still lgtm
On 2015/10/13 10:23:38, Mounir Lamouri wrote: > still lgtm This CL still has the same problem with flaky content_browsertests on mac build bot (not reproducible on a local copy of OSX 10.9) which don't arise on other platforms. An example of a build which failed due to WakeLockTest flakiness: http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_dbg_... All of the problems appear to be related to incorrectly reported visibility, and explicit visibility checks in the WakeLockTest also fail in a flaky way. I did some research and found that the following two tests were also disabled on mac because of flaky visibility: RenderFrameHostImplBrowserTest.GetVisibilityState_Basic (comment refers to http://crbug.com/467670) RenderFrameHostImplBrowserTest.GetVisibilityState_Override (comment refers to http://crbug.com/525592) The bugs mentioned above are not resolved at this moment. I don't know what exactly could cause the problem with visibility as I was not able reproduce it on a local OSX 10.9 installation but suspect that they might be related to the fact that on mac there are additional circumstances when a page can become invisible, such as when it is occluded by other windows (http://code.google.com/p/chromium/issues/detail?id=310374). Maybe this is what happens on the mac build bot, but I have no way to confirm it. If I try to commit this CL as is again, it will likely be reverted again, which is counterproductive.
Using visibility in content_browsertests is flaky because more than one tests are run in parallel. Like focus, visibility tests should be moved to interactive_ui_tests but they are chrome/ tests, not content/. Is it possible for you to mock visibility?
On 2015/10/14 10:55:18, Mounir Lamouri wrote: > Using visibility in content_browsertests is flaky because more than one tests > are run in parallel. Like focus, visibility tests should be moved to > interactive_ui_tests but they are chrome/ tests, not content/. > > Is it possible for you to mock visibility? I wish it were, but how do I approach it? I can't find any examples of overriding page visibility in the tests, or any test utils which could do that. ScreenWakeLock uses Page::visibilityState() to obtain visibility information and PageLifecycleObserver::pageVisibilityChanged() to know when it changes.
mojom LGTM
On 2015/10/14 at 16:23:13, alogvinov wrote: > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > Using visibility in content_browsertests is flaky because more than one tests > > are run in parallel. Like focus, visibility tests should be moved to > > interactive_ui_tests but they are chrome/ tests, not content/. > > > > Is it possible for you to mock visibility? > > I wish it were, but how do I approach it? I can't find any examples of overriding page visibility in the tests, or any test utils which could do that. > > ScreenWakeLock uses Page::visibilityState() to obtain visibility information and PageLifecycleObserver::pageVisibilityChanged() to know when it changes. Can you call WasShown() and WasHidden() for the purpose of the test. For example: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
On 2015/10/15 12:23:21, Mounir Lamouri wrote: > On 2015/10/14 at 16:23:13, alogvinov wrote: > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > Using visibility in content_browsertests is flaky because more than one > tests > > > are run in parallel. Like focus, visibility tests should be moved to > > > interactive_ui_tests but they are chrome/ tests, not content/. > > > > > > Is it possible for you to mock visibility? > > > > I wish it were, but how do I approach it? I can't find any examples of > overriding page visibility in the tests, or any test utils which could do that. > > > > ScreenWakeLock uses Page::visibilityState() to obtain visibility information > and PageLifecycleObserver::pageVisibilityChanged() to know when it changes. > > Can you call WasShown() and WasHidden() for the purpose of the test. > For example: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... I tried doing that but it didn't completely remove the flakiness. I think this is because while we can force visibility state changes by calling WasShown() and WasHidden() on RenderWidgetHostImpl, those functions also get called from within the browser in response to actual visibility changes, so those "real" calls overwrite the results of our "test" calls while the test is in progress.
On 2015/10/16 at 08:47:48, alogvinov wrote: > On 2015/10/15 12:23:21, Mounir Lamouri wrote: > > On 2015/10/14 at 16:23:13, alogvinov wrote: > > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > > Using visibility in content_browsertests is flaky because more than one > > tests > > > > are run in parallel. Like focus, visibility tests should be moved to > > > > interactive_ui_tests but they are chrome/ tests, not content/. > > > > > > > > Is it possible for you to mock visibility? > > > > > > I wish it were, but how do I approach it? I can't find any examples of > > overriding page visibility in the tests, or any test utils which could do that. > > > > > > ScreenWakeLock uses Page::visibilityState() to obtain visibility information > > and PageLifecycleObserver::pageVisibilityChanged() to know when it changes. > > > > Can you call WasShown() and WasHidden() for the purpose of the test. > > For example: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > I tried doing that but it didn't completely remove the flakiness. I think this is because while we can force visibility state changes by calling WasShown() and WasHidden() on RenderWidgetHostImpl, those functions also get called from within the browser in response to actual visibility changes, so those "real" calls overwrite the results of our "test" calls while the test is in progress. What if you call WasShown() after calls to WaitForPossibleUpdate() and any call that might keep the test idle so an event such a visibility change might get handled by the window.
On 2015/10/19 10:55:55, Mounir Lamouri wrote: > On 2015/10/16 at 08:47:48, alogvinov wrote: > > On 2015/10/15 12:23:21, Mounir Lamouri wrote: > > > On 2015/10/14 at 16:23:13, alogvinov wrote: > > > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > > > Using visibility in content_browsertests is flaky because more than one > > > tests > > > > > are run in parallel. Like focus, visibility tests should be moved to > > > > > interactive_ui_tests but they are chrome/ tests, not content/. > > > > > > > > > > Is it possible for you to mock visibility? > > > > > > > > I wish it were, but how do I approach it? I can't find any examples of > > > overriding page visibility in the tests, or any test utils which could do > that. > > > > > > > > ScreenWakeLock uses Page::visibilityState() to obtain visibility > information > > > and PageLifecycleObserver::pageVisibilityChanged() to know when it changes. > > > > > > Can you call WasShown() and WasHidden() for the purpose of the test. > > > For example: > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > I tried doing that but it didn't completely remove the flakiness. I think this > is because while we can force visibility state changes by calling WasShown() and > WasHidden() on RenderWidgetHostImpl, those functions also get called from within > the browser in response to actual visibility changes, so those "real" calls > overwrite the results of our "test" calls while the test is in progress. > > What if you call WasShown() after calls to WaitForPossibleUpdate() and any call > that might keep the test idle so an event such a visibility change might get > handled by the window. If I called WasShown() after WaitForPossibleUpdate(), I would have to call WaitForPossibleUpdate() again, because visibility for the purpose of wake lock is handled in Blink by ScreenWakeLock and the new wake lock status is communicated back to the browser process via mojo channel which is not synchronized with any IPC (that's the original reason for WaitForPossibleUpdate()). I looked into hooking into RenderWidgetHostImpl / RenderProcessHost (for the purpose of blocking ViewMsg_WasShown / ViewMsg_WasHidden messages) but couldn't find a way to do it without modifying any of the existing code.
On 2015/10/19 at 12:50:55, alogvinov wrote: > On 2015/10/19 10:55:55, Mounir Lamouri wrote: > > On 2015/10/16 at 08:47:48, alogvinov wrote: > > > On 2015/10/15 12:23:21, Mounir Lamouri wrote: > > > > On 2015/10/14 at 16:23:13, alogvinov wrote: > > > > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > > > > Using visibility in content_browsertests is flaky because more than one > > > > tests > > > > > > are run in parallel. Like focus, visibility tests should be moved to > > > > > > interactive_ui_tests but they are chrome/ tests, not content/. > > > > > > > > > > > > Is it possible for you to mock visibility? > > > > > > > > > > I wish it were, but how do I approach it? I can't find any examples of > > > > overriding page visibility in the tests, or any test utils which could do > > that. > > > > > > > > > > ScreenWakeLock uses Page::visibilityState() to obtain visibility > > information > > > > and PageLifecycleObserver::pageVisibilityChanged() to know when it changes. > > > > > > > > Can you call WasShown() and WasHidden() for the purpose of the test. > > > > For example: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > > I tried doing that but it didn't completely remove the flakiness. I think this > > is because while we can force visibility state changes by calling WasShown() and > > WasHidden() on RenderWidgetHostImpl, those functions also get called from within > > the browser in response to actual visibility changes, so those "real" calls > > overwrite the results of our "test" calls while the test is in progress. > > > > What if you call WasShown() after calls to WaitForPossibleUpdate() and any call > > that might keep the test idle so an event such a visibility change might get > > handled by the window. > > If I called WasShown() after WaitForPossibleUpdate(), I would have to call WaitForPossibleUpdate() again, because visibility for the purpose of wake lock is handled in Blink by ScreenWakeLock and the new wake lock status is communicated back to the browser process via mojo channel which is not synchronized with any IPC (that's the original reason for WaitForPossibleUpdate()). > > I looked into hooking into RenderWidgetHostImpl / RenderProcessHost (for the purpose of blocking ViewMsg_WasShown / ViewMsg_WasHidden messages) but couldn't find a way to do it without modifying any of the existing code. Then maybe your last bet might be to add something like ::SetVisibilityForTest(bool visibility) that will override visibility. Though, I would do that in a different CL and maybe disable the tests in the time being while a solution is found so you can land.
jochen@chromium.org changed reviewers: + jam@chromium.org
+jam for advice on avoiding test flakiness. ordinarily, I'd assume we want an interactive ui test here, however, since this is in content, we can't easily do that?
On 2015/10/20 12:13:31, Mounir Lamouri wrote: > On 2015/10/19 at 12:50:55, alogvinov wrote: > > On 2015/10/19 10:55:55, Mounir Lamouri wrote: > > > On 2015/10/16 at 08:47:48, alogvinov wrote: > > > > On 2015/10/15 12:23:21, Mounir Lamouri wrote: > > > > > On 2015/10/14 at 16:23:13, alogvinov wrote: > > > > > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > > > > > Using visibility in content_browsertests is flaky because more than > one > > > > > tests > > > > > > > are run in parallel. Like focus, visibility tests should be moved to > > > > > > > interactive_ui_tests but they are chrome/ tests, not content/. > > > > > > > > > > > > > > Is it possible for you to mock visibility? > > > > > > > > > > > > I wish it were, but how do I approach it? I can't find any examples of > > > > > overriding page visibility in the tests, or any test utils which could > do > > > that. > > > > > > > > > > > > ScreenWakeLock uses Page::visibilityState() to obtain visibility > > > information > > > > > and PageLifecycleObserver::pageVisibilityChanged() to know when it > changes. > > > > > > > > > > Can you call WasShown() and WasHidden() for the purpose of the test. > > > > > For example: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > > > > I tried doing that but it didn't completely remove the flakiness. I think > this > > > is because while we can force visibility state changes by calling WasShown() > and > > > WasHidden() on RenderWidgetHostImpl, those functions also get called from > within > > > the browser in response to actual visibility changes, so those "real" calls > > > overwrite the results of our "test" calls while the test is in progress. > > > > > > What if you call WasShown() after calls to WaitForPossibleUpdate() and any > call > > > that might keep the test idle so an event such a visibility change might get > > > handled by the window. > > > > If I called WasShown() after WaitForPossibleUpdate(), I would have to call > WaitForPossibleUpdate() again, because visibility for the purpose of wake lock > is handled in Blink by ScreenWakeLock and the new wake lock status is > communicated back to the browser process via mojo channel which is not > synchronized with any IPC (that's the original reason for > WaitForPossibleUpdate()). > > > > I looked into hooking into RenderWidgetHostImpl / RenderProcessHost (for the > purpose of blocking ViewMsg_WasShown / ViewMsg_WasHidden messages) but couldn't > find a way to do it without modifying any of the existing code. > > Then maybe your last bet might be to add something like > ::SetVisibilityForTest(bool visibility) that will override visibility. Though, I > would do that in a different CL and maybe disable the tests in the time being > while a solution is found so you can land. I disabled visibility-dependent tests on Mac as you suggested, also rolled back previous attempt on forcing visibility. I think it would be nice to have some general visibility mocking mechanism for browser tests (including those in residing in content) because visibility information is exposed to Web APIs in Blink and to page scripts so it can be a factor in behavior correctness, hence the need to be able to mock it.
jochen@, PTAL
On 2015/10/21 at 08:38:19, Mounir Lamouri wrote: > jochen@, PTAL Scratch that, I missed your comment above.
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
See https://codereview.chromium.org/1411463004/ for a potential fix for the test flake.
On 2015/10/20 16:14:59, alogvinov wrote: > On 2015/10/20 12:13:31, Mounir Lamouri wrote: > > On 2015/10/19 at 12:50:55, alogvinov wrote: > > > On 2015/10/19 10:55:55, Mounir Lamouri wrote: > > > > On 2015/10/16 at 08:47:48, alogvinov wrote: > > > > > On 2015/10/15 12:23:21, Mounir Lamouri wrote: > > > > > > On 2015/10/14 at 16:23:13, alogvinov wrote: > > > > > > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > > > > > > Using visibility in content_browsertests is flaky because more > than > > one > > > > > > tests > > > > > > > > are run in parallel. Like focus, visibility tests should be moved > to > > > > > > > > interactive_ui_tests but they are chrome/ tests, not content/. > > > > > > > > > > > > > > > > Is it possible for you to mock visibility? > > > > > > > > > > > > > > I wish it were, but how do I approach it? I can't find any examples > of > > > > > > overriding page visibility in the tests, or any test utils which could > > do > > > > that. > > > > > > > > > > > > > > ScreenWakeLock uses Page::visibilityState() to obtain visibility > > > > information > > > > > > and PageLifecycleObserver::pageVisibilityChanged() to know when it > > changes. > > > > > > > > > > > > Can you call WasShown() and WasHidden() for the purpose of the test. > > > > > > For example: > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > > > > > > I tried doing that but it didn't completely remove the flakiness. I > think > > this > > > > is because while we can force visibility state changes by calling > WasShown() > > and > > > > WasHidden() on RenderWidgetHostImpl, those functions also get called from > > within > > > > the browser in response to actual visibility changes, so those "real" > calls > > > > overwrite the results of our "test" calls while the test is in progress. > > > > > > > > What if you call WasShown() after calls to WaitForPossibleUpdate() and any > > call > > > > that might keep the test idle so an event such a visibility change might > get > > > > handled by the window. > > > > > > If I called WasShown() after WaitForPossibleUpdate(), I would have to call > > WaitForPossibleUpdate() again, because visibility for the purpose of wake lock > > is handled in Blink by ScreenWakeLock and the new wake lock status is > > communicated back to the browser process via mojo channel which is not > > synchronized with any IPC (that's the original reason for > > WaitForPossibleUpdate()). > > > > > > I looked into hooking into RenderWidgetHostImpl / RenderProcessHost (for the > > purpose of blocking ViewMsg_WasShown / ViewMsg_WasHidden messages) but > couldn't > > find a way to do it without modifying any of the existing code. > > > > Then maybe your last bet might be to add something like > > ::SetVisibilityForTest(bool visibility) that will override visibility. Though, > I > > would do that in a different CL and maybe disable the tests in the time being > > while a solution is found so you can land. > > I disabled visibility-dependent tests on Mac as you suggested, also rolled back > previous attempt on forcing visibility. > > I think it would be nice to have some general visibility mocking mechanism for > browser tests (including those in residing in content) because visibility > information is exposed to Web APIs in Blink and to page scripts so it can be a > factor in behavior correctness, hence the need to be able to mock it. yeah we have avoided adding an interactive target in content. I didn't look at the test in details, but is it possible to put it inside src/chrome? i.e. if it doesn't need to muck around with any non-public content api.
On 2015/10/21 at 15:40:05, jam wrote: > On 2015/10/20 16:14:59, alogvinov wrote: > > On 2015/10/20 12:13:31, Mounir Lamouri wrote: > > > On 2015/10/19 at 12:50:55, alogvinov wrote: > > > > On 2015/10/19 10:55:55, Mounir Lamouri wrote: > > > > > On 2015/10/16 at 08:47:48, alogvinov wrote: > > > > > > On 2015/10/15 12:23:21, Mounir Lamouri wrote: > > > > > > > On 2015/10/14 at 16:23:13, alogvinov wrote: > > > > > > > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > > > > > > > Using visibility in content_browsertests is flaky because more > > than > > > one > > > > > > > tests > > > > > > > > > are run in parallel. Like focus, visibility tests should be moved > > to > > > > > > > > > interactive_ui_tests but they are chrome/ tests, not content/. > > > > > > > > > > > > > > > > > > Is it possible for you to mock visibility? > > > > > > > > > > > > > > > > I wish it were, but how do I approach it? I can't find any examples > > of > > > > > > > overriding page visibility in the tests, or any test utils which could > > > do > > > > > that. > > > > > > > > > > > > > > > > ScreenWakeLock uses Page::visibilityState() to obtain visibility > > > > > information > > > > > > > and PageLifecycleObserver::pageVisibilityChanged() to know when it > > > changes. > > > > > > > > > > > > > > Can you call WasShown() and WasHidden() for the purpose of the test. > > > > > > > For example: > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > > > > > > > > I tried doing that but it didn't completely remove the flakiness. I > > think > > > this > > > > > is because while we can force visibility state changes by calling > > WasShown() > > > and > > > > > WasHidden() on RenderWidgetHostImpl, those functions also get called from > > > within > > > > > the browser in response to actual visibility changes, so those "real" > > calls > > > > > overwrite the results of our "test" calls while the test is in progress. > > > > > > > > > > What if you call WasShown() after calls to WaitForPossibleUpdate() and any > > > call > > > > > that might keep the test idle so an event such a visibility change might > > get > > > > > handled by the window. > > > > > > > > If I called WasShown() after WaitForPossibleUpdate(), I would have to call > > > WaitForPossibleUpdate() again, because visibility for the purpose of wake lock > > > is handled in Blink by ScreenWakeLock and the new wake lock status is > > > communicated back to the browser process via mojo channel which is not > > > synchronized with any IPC (that's the original reason for > > > WaitForPossibleUpdate()). > > > > > > > > I looked into hooking into RenderWidgetHostImpl / RenderProcessHost (for the > > > purpose of blocking ViewMsg_WasShown / ViewMsg_WasHidden messages) but > > couldn't > > > find a way to do it without modifying any of the existing code. > > > > > > Then maybe your last bet might be to add something like > > > ::SetVisibilityForTest(bool visibility) that will override visibility. Though, > > I > > > would do that in a different CL and maybe disable the tests in the time being > > > while a solution is found so you can land. > > > > I disabled visibility-dependent tests on Mac as you suggested, also rolled back > > previous attempt on forcing visibility. > > > > I think it would be nice to have some general visibility mocking mechanism for > > browser tests (including those in residing in content) because visibility > > information is exposed to Web APIs in Blink and to page scripts so it can be a > > factor in behavior correctness, hence the need to be able to mock it. > > yeah we have avoided adding an interactive target in content. > > I didn't look at the test in details, but is it possible to put it inside src/chrome? i.e. if it doesn't need to muck around with any non-public content api. I don't see why we should run interactive tests when what we really want is simulate visibility. Interactive tests will make the test suite significantly slower and more flaky. Is there a reason why we should avoid mocking visibility in browser tests?
On 2015/10/22 at 10:56:03, mlamouri wrote: > On 2015/10/21 at 15:40:05, jam wrote: > > On 2015/10/20 16:14:59, alogvinov wrote: > > > On 2015/10/20 12:13:31, Mounir Lamouri wrote: > > > > On 2015/10/19 at 12:50:55, alogvinov wrote: > > > > > On 2015/10/19 10:55:55, Mounir Lamouri wrote: > > > > > > On 2015/10/16 at 08:47:48, alogvinov wrote: > > > > > > > On 2015/10/15 12:23:21, Mounir Lamouri wrote: > > > > > > > > On 2015/10/14 at 16:23:13, alogvinov wrote: > > > > > > > > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > > > > > > > > Using visibility in content_browsertests is flaky because more > > > than > > > > one > > > > > > > > tests > > > > > > > > > > are run in parallel. Like focus, visibility tests should be moved > > > to > > > > > > > > > > interactive_ui_tests but they are chrome/ tests, not content/. > > > > > > > > > > > > > > > > > > > > Is it possible for you to mock visibility? > > > > > > > > > > > > > > > > > > I wish it were, but how do I approach it? I can't find any examples > > > of > > > > > > > > overriding page visibility in the tests, or any test utils which could > > > > do > > > > > > that. > > > > > > > > > > > > > > > > > > ScreenWakeLock uses Page::visibilityState() to obtain visibility > > > > > > information > > > > > > > > and PageLifecycleObserver::pageVisibilityChanged() to know when it > > > > changes. > > > > > > > > > > > > > > > > Can you call WasShown() and WasHidden() for the purpose of the test. > > > > > > > > For example: > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > > > > > > > > > > I tried doing that but it didn't completely remove the flakiness. I > > > think > > > > this > > > > > > is because while we can force visibility state changes by calling > > > WasShown() > > > > and > > > > > > WasHidden() on RenderWidgetHostImpl, those functions also get called from > > > > within > > > > > > the browser in response to actual visibility changes, so those "real" > > > calls > > > > > > overwrite the results of our "test" calls while the test is in progress. > > > > > > > > > > > > What if you call WasShown() after calls to WaitForPossibleUpdate() and any > > > > call > > > > > > that might keep the test idle so an event such a visibility change might > > > get > > > > > > handled by the window. > > > > > > > > > > If I called WasShown() after WaitForPossibleUpdate(), I would have to call > > > > WaitForPossibleUpdate() again, because visibility for the purpose of wake lock > > > > is handled in Blink by ScreenWakeLock and the new wake lock status is > > > > communicated back to the browser process via mojo channel which is not > > > > synchronized with any IPC (that's the original reason for > > > > WaitForPossibleUpdate()). > > > > > > > > > > I looked into hooking into RenderWidgetHostImpl / RenderProcessHost (for the > > > > purpose of blocking ViewMsg_WasShown / ViewMsg_WasHidden messages) but > > > couldn't > > > > find a way to do it without modifying any of the existing code. > > > > > > > > Then maybe your last bet might be to add something like > > > > ::SetVisibilityForTest(bool visibility) that will override visibility. Though, > > > I > > > > would do that in a different CL and maybe disable the tests in the time being > > > > while a solution is found so you can land. > > > > > > I disabled visibility-dependent tests on Mac as you suggested, also rolled back > > > previous attempt on forcing visibility. > > > > > > I think it would be nice to have some general visibility mocking mechanism for > > > browser tests (including those in residing in content) because visibility > > > information is exposed to Web APIs in Blink and to page scripts so it can be a > > > factor in behavior correctness, hence the need to be able to mock it. > > > > yeah we have avoided adding an interactive target in content. > > > > I didn't look at the test in details, but is it possible to put it inside src/chrome? i.e. if it doesn't need to muck around with any non-public content api. > > I don't see why we should run interactive tests when what we really want is simulate visibility. Interactive tests will make the test suite significantly slower and more flaky. Is there a reason why we should avoid mocking visibility in browser tests? i guess mocking is ok as well, just disabling on mac seems wrong
On 2015/10/22 11:56:33, jochen wrote: > On 2015/10/22 at 10:56:03, mlamouri wrote: > > On 2015/10/21 at 15:40:05, jam wrote: > > > On 2015/10/20 16:14:59, alogvinov wrote: > > > > On 2015/10/20 12:13:31, Mounir Lamouri wrote: > > > > > On 2015/10/19 at 12:50:55, alogvinov wrote: > > > > > > On 2015/10/19 10:55:55, Mounir Lamouri wrote: > > > > > > > On 2015/10/16 at 08:47:48, alogvinov wrote: > > > > > > > > On 2015/10/15 12:23:21, Mounir Lamouri wrote: > > > > > > > > > On 2015/10/14 at 16:23:13, alogvinov wrote: > > > > > > > > > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > > > > > > > > > Using visibility in content_browsertests is flaky because > more > > > > than > > > > > one > > > > > > > > > tests > > > > > > > > > > > are run in parallel. Like focus, visibility tests should be > moved > > > > to > > > > > > > > > > > interactive_ui_tests but they are chrome/ tests, not > content/. > > > > > > > > > > > > > > > > > > > > > > Is it possible for you to mock visibility? > > > > > > > > > > > > > > > > > > > > I wish it were, but how do I approach it? I can't find any > examples > > > > of > > > > > > > > > overriding page visibility in the tests, or any test utils which > could > > > > > do > > > > > > > that. > > > > > > > > > > > > > > > > > > > > ScreenWakeLock uses Page::visibilityState() to obtain > visibility > > > > > > > information > > > > > > > > > and PageLifecycleObserver::pageVisibilityChanged() to know when > it > > > > > changes. > > > > > > > > > > > > > > > > > > Can you call WasShown() and WasHidden() for the purpose of the > test. > > > > > > > > > For example: > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > > > > > > > > > > > > I tried doing that but it didn't completely remove the flakiness. > I > > > > think > > > > > this > > > > > > > is because while we can force visibility state changes by calling > > > > WasShown() > > > > > and > > > > > > > WasHidden() on RenderWidgetHostImpl, those functions also get called > from > > > > > within > > > > > > > the browser in response to actual visibility changes, so those > "real" > > > > calls > > > > > > > overwrite the results of our "test" calls while the test is in > progress. > > > > > > > > > > > > > > What if you call WasShown() after calls to WaitForPossibleUpdate() > and any > > > > > call > > > > > > > that might keep the test idle so an event such a visibility change > might > > > > get > > > > > > > handled by the window. > > > > > > > > > > > > If I called WasShown() after WaitForPossibleUpdate(), I would have to > call > > > > > WaitForPossibleUpdate() again, because visibility for the purpose of > wake lock > > > > > is handled in Blink by ScreenWakeLock and the new wake lock status is > > > > > communicated back to the browser process via mojo channel which is not > > > > > synchronized with any IPC (that's the original reason for > > > > > WaitForPossibleUpdate()). > > > > > > > > > > > > I looked into hooking into RenderWidgetHostImpl / RenderProcessHost > (for the > > > > > purpose of blocking ViewMsg_WasShown / ViewMsg_WasHidden messages) but > > > > couldn't > > > > > find a way to do it without modifying any of the existing code. > > > > > > > > > > Then maybe your last bet might be to add something like > > > > > ::SetVisibilityForTest(bool visibility) that will override visibility. > Though, > > > > I > > > > > would do that in a different CL and maybe disable the tests in the time > being > > > > > while a solution is found so you can land. > > > > > > > > I disabled visibility-dependent tests on Mac as you suggested, also rolled > back > > > > previous attempt on forcing visibility. > > > > > > > > I think it would be nice to have some general visibility mocking mechanism > for > > > > browser tests (including those in residing in content) because visibility > > > > information is exposed to Web APIs in Blink and to page scripts so it can > be a > > > > factor in behavior correctness, hence the need to be able to mock it. > > > > > > yeah we have avoided adding an interactive target in content. > > > > > > I didn't look at the test in details, but is it possible to put it inside > src/chrome? i.e. if it doesn't need to muck around with any non-public content > api. > > > > I don't see why we should run interactive tests when what we really want is > simulate visibility. Interactive tests will make the test suite significantly > slower and more flaky. Is there a reason why we should avoid mocking visibility > in browser tests? > > i guess mocking is ok as well, just disabling on mac seems wrong The problem with mocking is that it cannot be achieved without modifying core classes such as RenderWidgetHostImpl etc. It is possible to send fake visibility changes in tests e.g by calling WasShown()/WasHidden() on RenderWidgetHostImpl, but there is no way to prevent actual visibility changes from also coming through while the test is in progress. This causes problems on Mac in particular because Chromium on Mac uses a more sophisticated visibility calculation method than on other platforms, i.e. it factors in such things as window occlusion etc. As multiple content browser tests are run at the same time on a single machine with unpredictable window arrangement, flaky visibility occurs even when the test is sending its own fake visibility changes.
On 2015/10/21 15:40:05, jam wrote: > On 2015/10/20 16:14:59, alogvinov wrote: > > On 2015/10/20 12:13:31, Mounir Lamouri wrote: > > > On 2015/10/19 at 12:50:55, alogvinov wrote: > > > > On 2015/10/19 10:55:55, Mounir Lamouri wrote: > > > > > On 2015/10/16 at 08:47:48, alogvinov wrote: > > > > > > On 2015/10/15 12:23:21, Mounir Lamouri wrote: > > > > > > > On 2015/10/14 at 16:23:13, alogvinov wrote: > > > > > > > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > > > > > > > Using visibility in content_browsertests is flaky because more > > than > > > one > > > > > > > tests > > > > > > > > > are run in parallel. Like focus, visibility tests should be > moved > > to > > > > > > > > > interactive_ui_tests but they are chrome/ tests, not content/. > > > > > > > > > > > > > > > > > > Is it possible for you to mock visibility? > > > > > > > > > > > > > > > > I wish it were, but how do I approach it? I can't find any > examples > > of > > > > > > > overriding page visibility in the tests, or any test utils which > could > > > do > > > > > that. > > > > > > > > > > > > > > > > ScreenWakeLock uses Page::visibilityState() to obtain visibility > > > > > information > > > > > > > and PageLifecycleObserver::pageVisibilityChanged() to know when it > > > changes. > > > > > > > > > > > > > > Can you call WasShown() and WasHidden() for the purpose of the test. > > > > > > > For example: > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > > > > > > > > I tried doing that but it didn't completely remove the flakiness. I > > think > > > this > > > > > is because while we can force visibility state changes by calling > > WasShown() > > > and > > > > > WasHidden() on RenderWidgetHostImpl, those functions also get called > from > > > within > > > > > the browser in response to actual visibility changes, so those "real" > > calls > > > > > overwrite the results of our "test" calls while the test is in progress. > > > > > > > > > > What if you call WasShown() after calls to WaitForPossibleUpdate() and > any > > > call > > > > > that might keep the test idle so an event such a visibility change might > > get > > > > > handled by the window. > > > > > > > > If I called WasShown() after WaitForPossibleUpdate(), I would have to call > > > WaitForPossibleUpdate() again, because visibility for the purpose of wake > lock > > > is handled in Blink by ScreenWakeLock and the new wake lock status is > > > communicated back to the browser process via mojo channel which is not > > > synchronized with any IPC (that's the original reason for > > > WaitForPossibleUpdate()). > > > > > > > > I looked into hooking into RenderWidgetHostImpl / RenderProcessHost (for > the > > > purpose of blocking ViewMsg_WasShown / ViewMsg_WasHidden messages) but > > couldn't > > > find a way to do it without modifying any of the existing code. > > > > > > Then maybe your last bet might be to add something like > > > ::SetVisibilityForTest(bool visibility) that will override visibility. > Though, > > I > > > would do that in a different CL and maybe disable the tests in the time > being > > > while a solution is found so you can land. > > > > I disabled visibility-dependent tests on Mac as you suggested, also rolled > back > > previous attempt on forcing visibility. > > > > I think it would be nice to have some general visibility mocking mechanism for > > browser tests (including those in residing in content) because visibility > > information is exposed to Web APIs in Blink and to page scripts so it can be a > > factor in behavior correctness, hence the need to be able to mock it. > > yeah we have avoided adding an interactive target in content. > > I didn't look at the test in details, but is it possible to put it inside > src/chrome? i.e. if it doesn't need to muck around with any non-public content > api. I tried that, but it seems that I can't use non-public content stuff such as WebContentsImpl and WakeLockServiceContext in interactive ui tests? I cannot check actual screen blocker status without having access to those classes.
On 2015/10/22 at 13:13:35, alogvinov wrote: > On 2015/10/21 15:40:05, jam wrote: > > On 2015/10/20 16:14:59, alogvinov wrote: > > > On 2015/10/20 12:13:31, Mounir Lamouri wrote: > > > > On 2015/10/19 at 12:50:55, alogvinov wrote: > > > > > On 2015/10/19 10:55:55, Mounir Lamouri wrote: > > > > > > On 2015/10/16 at 08:47:48, alogvinov wrote: > > > > > > > On 2015/10/15 12:23:21, Mounir Lamouri wrote: > > > > > > > > On 2015/10/14 at 16:23:13, alogvinov wrote: > > > > > > > > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > > > > > > > > Using visibility in content_browsertests is flaky because more > > > than > > > > one > > > > > > > > tests > > > > > > > > > > are run in parallel. Like focus, visibility tests should be > > moved > > > to > > > > > > > > > > interactive_ui_tests but they are chrome/ tests, not content/. > > > > > > > > > > > > > > > > > > > > Is it possible for you to mock visibility? > > > > > > > > > > > > > > > > > > I wish it were, but how do I approach it? I can't find any > > examples > > > of > > > > > > > > overriding page visibility in the tests, or any test utils which > > could > > > > do > > > > > > that. > > > > > > > > > > > > > > > > > > ScreenWakeLock uses Page::visibilityState() to obtain visibility > > > > > > information > > > > > > > > and PageLifecycleObserver::pageVisibilityChanged() to know when it > > > > changes. > > > > > > > > > > > > > > > > Can you call WasShown() and WasHidden() for the purpose of the test. > > > > > > > > For example: > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > > > > > > > > > > I tried doing that but it didn't completely remove the flakiness. I > > > think > > > > this > > > > > > is because while we can force visibility state changes by calling > > > WasShown() > > > > and > > > > > > WasHidden() on RenderWidgetHostImpl, those functions also get called > > from > > > > within > > > > > > the browser in response to actual visibility changes, so those "real" > > > calls > > > > > > overwrite the results of our "test" calls while the test is in progress. > > > > > > > > > > > > What if you call WasShown() after calls to WaitForPossibleUpdate() and > > any > > > > call > > > > > > that might keep the test idle so an event such a visibility change might > > > get > > > > > > handled by the window. > > > > > > > > > > If I called WasShown() after WaitForPossibleUpdate(), I would have to call > > > > WaitForPossibleUpdate() again, because visibility for the purpose of wake > > lock > > > > is handled in Blink by ScreenWakeLock and the new wake lock status is > > > > communicated back to the browser process via mojo channel which is not > > > > synchronized with any IPC (that's the original reason for > > > > WaitForPossibleUpdate()). > > > > > > > > > > I looked into hooking into RenderWidgetHostImpl / RenderProcessHost (for > > the > > > > purpose of blocking ViewMsg_WasShown / ViewMsg_WasHidden messages) but > > > couldn't > > > > find a way to do it without modifying any of the existing code. > > > > > > > > Then maybe your last bet might be to add something like > > > > ::SetVisibilityForTest(bool visibility) that will override visibility. > > Though, > > > I > > > > would do that in a different CL and maybe disable the tests in the time > > being > > > > while a solution is found so you can land. > > > > > > I disabled visibility-dependent tests on Mac as you suggested, also rolled > > back > > > previous attempt on forcing visibility. > > > > > > I think it would be nice to have some general visibility mocking mechanism for > > > browser tests (including those in residing in content) because visibility > > > information is exposed to Web APIs in Blink and to page scripts so it can be a > > > factor in behavior correctness, hence the need to be able to mock it. > > > > yeah we have avoided adding an interactive target in content. > > > > I didn't look at the test in details, but is it possible to put it inside > > src/chrome? i.e. if it doesn't need to muck around with any non-public content > > api. > > I tried that, but it seems that I can't use non-public content stuff such as WebContentsImpl and WakeLockServiceContext in interactive ui tests? I cannot check actual screen blocker status without having access to those classes. maybe a layout test was better for this? I think we just never create an window manager window, so we can control whether it feels visible or not (see headless_ in shell_mac.mm)
On 2015/10/23 at 12:26:21, jochen wrote: > On 2015/10/22 at 13:13:35, alogvinov wrote: > > On 2015/10/21 15:40:05, jam wrote: > > > On 2015/10/20 16:14:59, alogvinov wrote: > > > > On 2015/10/20 12:13:31, Mounir Lamouri wrote: > > > > > On 2015/10/19 at 12:50:55, alogvinov wrote: > > > > > > On 2015/10/19 10:55:55, Mounir Lamouri wrote: > > > > > > > On 2015/10/16 at 08:47:48, alogvinov wrote: > > > > > > > > On 2015/10/15 12:23:21, Mounir Lamouri wrote: > > > > > > > > > On 2015/10/14 at 16:23:13, alogvinov wrote: > > > > > > > > > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > > > > > > > > > Using visibility in content_browsertests is flaky because more > > > > than > > > > > one > > > > > > > > > tests > > > > > > > > > > > are run in parallel. Like focus, visibility tests should be > > > moved > > > > to > > > > > > > > > > > interactive_ui_tests but they are chrome/ tests, not content/. > > > > > > > > > > > > > > > > > > > > > > Is it possible for you to mock visibility? > > > > > > > > > > > > > > > > > > > > I wish it were, but how do I approach it? I can't find any > > > examples > > > > of > > > > > > > > > overriding page visibility in the tests, or any test utils which > > > could > > > > > do > > > > > > > that. > > > > > > > > > > > > > > > > > > > > ScreenWakeLock uses Page::visibilityState() to obtain visibility > > > > > > > information > > > > > > > > > and PageLifecycleObserver::pageVisibilityChanged() to know when it > > > > > changes. > > > > > > > > > > > > > > > > > > Can you call WasShown() and WasHidden() for the purpose of the test. > > > > > > > > > For example: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > > > > > > > > > > > > I tried doing that but it didn't completely remove the flakiness. I > > > > think > > > > > this > > > > > > > is because while we can force visibility state changes by calling > > > > WasShown() > > > > > and > > > > > > > WasHidden() on RenderWidgetHostImpl, those functions also get called > > > from > > > > > within > > > > > > > the browser in response to actual visibility changes, so those "real" > > > > calls > > > > > > > overwrite the results of our "test" calls while the test is in progress. > > > > > > > > > > > > > > What if you call WasShown() after calls to WaitForPossibleUpdate() and > > > any > > > > > call > > > > > > > that might keep the test idle so an event such a visibility change might > > > > get > > > > > > > handled by the window. > > > > > > > > > > > > If I called WasShown() after WaitForPossibleUpdate(), I would have to call > > > > > WaitForPossibleUpdate() again, because visibility for the purpose of wake > > > lock > > > > > is handled in Blink by ScreenWakeLock and the new wake lock status is > > > > > communicated back to the browser process via mojo channel which is not > > > > > synchronized with any IPC (that's the original reason for > > > > > WaitForPossibleUpdate()). > > > > > > > > > > > > I looked into hooking into RenderWidgetHostImpl / RenderProcessHost (for > > > the > > > > > purpose of blocking ViewMsg_WasShown / ViewMsg_WasHidden messages) but > > > > couldn't > > > > > find a way to do it without modifying any of the existing code. > > > > > > > > > > Then maybe your last bet might be to add something like > > > > > ::SetVisibilityForTest(bool visibility) that will override visibility. > > > Though, > > > > I > > > > > would do that in a different CL and maybe disable the tests in the time > > > being > > > > > while a solution is found so you can land. > > > > > > > > I disabled visibility-dependent tests on Mac as you suggested, also rolled > > > back > > > > previous attempt on forcing visibility. > > > > > > > > I think it would be nice to have some general visibility mocking mechanism for > > > > browser tests (including those in residing in content) because visibility > > > > information is exposed to Web APIs in Blink and to page scripts so it can be a > > > > factor in behavior correctness, hence the need to be able to mock it. > > > > > > yeah we have avoided adding an interactive target in content. > > > > > > I didn't look at the test in details, but is it possible to put it inside > > > src/chrome? i.e. if it doesn't need to muck around with any non-public content > > > api. > > > > I tried that, but it seems that I can't use non-public content stuff such as WebContentsImpl and WakeLockServiceContext in interactive ui tests? I cannot check actual screen blocker status without having access to those classes. > > maybe a layout test was better for this? I think we just never create an window manager window, so we can control whether it feels visible or not (see headless_ in shell_mac.mm) What about using ContentBrowserClient::OverridePageVisibilityState()? You could use this to make a frame always visible or always hidden I believe. WDYT?
On 2015/10/23 13:56:01, Mounir Lamouri wrote: > On 2015/10/23 at 12:26:21, jochen wrote: > > On 2015/10/22 at 13:13:35, alogvinov wrote: > > > On 2015/10/21 15:40:05, jam wrote: > > > > On 2015/10/20 16:14:59, alogvinov wrote: > > > > > On 2015/10/20 12:13:31, Mounir Lamouri wrote: > > > > > > On 2015/10/19 at 12:50:55, alogvinov wrote: > > > > > > > On 2015/10/19 10:55:55, Mounir Lamouri wrote: > > > > > > > > On 2015/10/16 at 08:47:48, alogvinov wrote: > > > > > > > > > On 2015/10/15 12:23:21, Mounir Lamouri wrote: > > > > > > > > > > On 2015/10/14 at 16:23:13, alogvinov wrote: > > > > > > > > > > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > > > > > > > > > > Using visibility in content_browsertests is flaky because > more > > > > > than > > > > > > one > > > > > > > > > > tests > > > > > > > > > > > > are run in parallel. Like focus, visibility tests should > be > > > > moved > > > > > to > > > > > > > > > > > > interactive_ui_tests but they are chrome/ tests, not > content/. > > > > > > > > > > > > > > > > > > > > > > > > Is it possible for you to mock visibility? > > > > > > > > > > > > > > > > > > > > > > I wish it were, but how do I approach it? I can't find any > > > > examples > > > > > of > > > > > > > > > > overriding page visibility in the tests, or any test utils > which > > > > could > > > > > > do > > > > > > > > that. > > > > > > > > > > > > > > > > > > > > > > ScreenWakeLock uses Page::visibilityState() to obtain > visibility > > > > > > > > information > > > > > > > > > > and PageLifecycleObserver::pageVisibilityChanged() to know > when it > > > > > > changes. > > > > > > > > > > > > > > > > > > > > Can you call WasShown() and WasHidden() for the purpose of the > test. > > > > > > > > > > For example: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > > > > > > > > > > > > > > I tried doing that but it didn't completely remove the > flakiness. I > > > > > think > > > > > > this > > > > > > > > is because while we can force visibility state changes by calling > > > > > WasShown() > > > > > > and > > > > > > > > WasHidden() on RenderWidgetHostImpl, those functions also get > called > > > > from > > > > > > within > > > > > > > > the browser in response to actual visibility changes, so those > "real" > > > > > calls > > > > > > > > overwrite the results of our "test" calls while the test is in > progress. > > > > > > > > > > > > > > > > What if you call WasShown() after calls to WaitForPossibleUpdate() > and > > > > any > > > > > > call > > > > > > > > that might keep the test idle so an event such a visibility change > might > > > > > get > > > > > > > > handled by the window. > > > > > > > > > > > > > > If I called WasShown() after WaitForPossibleUpdate(), I would have > to call > > > > > > WaitForPossibleUpdate() again, because visibility for the purpose of > wake > > > > lock > > > > > > is handled in Blink by ScreenWakeLock and the new wake lock status is > > > > > > communicated back to the browser process via mojo channel which is not > > > > > > synchronized with any IPC (that's the original reason for > > > > > > WaitForPossibleUpdate()). > > > > > > > > > > > > > > I looked into hooking into RenderWidgetHostImpl / RenderProcessHost > (for > > > > the > > > > > > purpose of blocking ViewMsg_WasShown / ViewMsg_WasHidden messages) but > > > > > couldn't > > > > > > find a way to do it without modifying any of the existing code. > > > > > > > > > > > > Then maybe your last bet might be to add something like > > > > > > ::SetVisibilityForTest(bool visibility) that will override visibility. > > > > Though, > > > > > I > > > > > > would do that in a different CL and maybe disable the tests in the > time > > > > being > > > > > > while a solution is found so you can land. > > > > > > > > > > I disabled visibility-dependent tests on Mac as you suggested, also > rolled > > > > back > > > > > previous attempt on forcing visibility. > > > > > > > > > > I think it would be nice to have some general visibility mocking > mechanism for > > > > > browser tests (including those in residing in content) because > visibility > > > > > information is exposed to Web APIs in Blink and to page scripts so it > can be a > > > > > factor in behavior correctness, hence the need to be able to mock it. > > > > > > > > yeah we have avoided adding an interactive target in content. > > > > > > > > I didn't look at the test in details, but is it possible to put it inside > > > > src/chrome? i.e. if it doesn't need to muck around with any non-public > content > > > > api. > > > > > > I tried that, but it seems that I can't use non-public content stuff such as > WebContentsImpl and WakeLockServiceContext in interactive ui tests? I cannot > check actual screen blocker status without having access to those classes. > > > > maybe a layout test was better for this? I think we just never create an > window manager window, so we can control whether it feels visible or not (see > headless_ in shell_mac.mm) > > What about using ContentBrowserClient::OverridePageVisibilityState()? You could > use this to make a frame always visible or always hidden I believe. WDYT? This is flaky on Mac as well (see https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...), I've mentioned it in https://codereview.chromium.org/1393203004/#msg2
On 2015/10/23 at 14:42:58, alogvinov wrote: > On 2015/10/23 13:56:01, Mounir Lamouri wrote: > > On 2015/10/23 at 12:26:21, jochen wrote: > > > On 2015/10/22 at 13:13:35, alogvinov wrote: > > > > On 2015/10/21 15:40:05, jam wrote: > > > > > On 2015/10/20 16:14:59, alogvinov wrote: > > > > > > On 2015/10/20 12:13:31, Mounir Lamouri wrote: > > > > > > > On 2015/10/19 at 12:50:55, alogvinov wrote: > > > > > > > > On 2015/10/19 10:55:55, Mounir Lamouri wrote: > > > > > > > > > On 2015/10/16 at 08:47:48, alogvinov wrote: > > > > > > > > > > On 2015/10/15 12:23:21, Mounir Lamouri wrote: > > > > > > > > > > > On 2015/10/14 at 16:23:13, alogvinov wrote: > > > > > > > > > > > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > > > > > > > > > > > Using visibility in content_browsertests is flaky because > > more > > > > > > than > > > > > > > one > > > > > > > > > > > tests > > > > > > > > > > > > > are run in parallel. Like focus, visibility tests should > > be > > > > > moved > > > > > > to > > > > > > > > > > > > > interactive_ui_tests but they are chrome/ tests, not > > content/. > > > > > > > > > > > > > > > > > > > > > > > > > > Is it possible for you to mock visibility? > > > > > > > > > > > > > > > > > > > > > > > > I wish it were, but how do I approach it? I can't find any > > > > > examples > > > > > > of > > > > > > > > > > > overriding page visibility in the tests, or any test utils > > which > > > > > could > > > > > > > do > > > > > > > > > that. > > > > > > > > > > > > > > > > > > > > > > > > ScreenWakeLock uses Page::visibilityState() to obtain > > visibility > > > > > > > > > information > > > > > > > > > > > and PageLifecycleObserver::pageVisibilityChanged() to know > > when it > > > > > > > changes. > > > > > > > > > > > > > > > > > > > > > > Can you call WasShown() and WasHidden() for the purpose of the > > test. > > > > > > > > > > > For example: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > > > > > > > > > > > > > > > > I tried doing that but it didn't completely remove the > > flakiness. I > > > > > > think > > > > > > > this > > > > > > > > > is because while we can force visibility state changes by calling > > > > > > WasShown() > > > > > > > and > > > > > > > > > WasHidden() on RenderWidgetHostImpl, those functions also get > > called > > > > > from > > > > > > > within > > > > > > > > > the browser in response to actual visibility changes, so those > > "real" > > > > > > calls > > > > > > > > > overwrite the results of our "test" calls while the test is in > > progress. > > > > > > > > > > > > > > > > > > What if you call WasShown() after calls to WaitForPossibleUpdate() > > and > > > > > any > > > > > > > call > > > > > > > > > that might keep the test idle so an event such a visibility change > > might > > > > > > get > > > > > > > > > handled by the window. > > > > > > > > > > > > > > > > If I called WasShown() after WaitForPossibleUpdate(), I would have > > to call > > > > > > > WaitForPossibleUpdate() again, because visibility for the purpose of > > wake > > > > > lock > > > > > > > is handled in Blink by ScreenWakeLock and the new wake lock status is > > > > > > > communicated back to the browser process via mojo channel which is not > > > > > > > synchronized with any IPC (that's the original reason for > > > > > > > WaitForPossibleUpdate()). > > > > > > > > > > > > > > > > I looked into hooking into RenderWidgetHostImpl / RenderProcessHost > > (for > > > > > the > > > > > > > purpose of blocking ViewMsg_WasShown / ViewMsg_WasHidden messages) but > > > > > > couldn't > > > > > > > find a way to do it without modifying any of the existing code. > > > > > > > > > > > > > > Then maybe your last bet might be to add something like > > > > > > > ::SetVisibilityForTest(bool visibility) that will override visibility. > > > > > Though, > > > > > > I > > > > > > > would do that in a different CL and maybe disable the tests in the > > time > > > > > being > > > > > > > while a solution is found so you can land. > > > > > > > > > > > > I disabled visibility-dependent tests on Mac as you suggested, also > > rolled > > > > > back > > > > > > previous attempt on forcing visibility. > > > > > > > > > > > > I think it would be nice to have some general visibility mocking > > mechanism for > > > > > > browser tests (including those in residing in content) because > > visibility > > > > > > information is exposed to Web APIs in Blink and to page scripts so it > > can be a > > > > > > factor in behavior correctness, hence the need to be able to mock it. > > > > > > > > > > yeah we have avoided adding an interactive target in content. > > > > > > > > > > I didn't look at the test in details, but is it possible to put it inside > > > > > src/chrome? i.e. if it doesn't need to muck around with any non-public > > content > > > > > api. > > > > > > > > I tried that, but it seems that I can't use non-public content stuff such as > > WebContentsImpl and WakeLockServiceContext in interactive ui tests? I cannot > > check actual screen blocker status without having access to those classes. > > > > > > maybe a layout test was better for this? I think we just never create an > > window manager window, so we can control whether it feels visible or not (see > > headless_ in shell_mac.mm) > > > > What about using ContentBrowserClient::OverridePageVisibilityState()? You could > > use this to make a frame always visible or always hidden I believe. WDYT? > > This is flaky on Mac as well (see https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...), I've mentioned it in https://codereview.chromium.org/1393203004/#msg2 The flakyness of that test isn't the usage of ::OverridePageVisibilityState() but the very first call to GetVisibilityState(). At that point, ::WasShown() needs to be called to guarantee that the WebContents is actually visible. This is a test I wrote and I have a fix pending reviews actually.
On 2015/10/24 07:06:24, Mounir Lamouri wrote: > On 2015/10/23 at 14:42:58, alogvinov wrote: > > On 2015/10/23 13:56:01, Mounir Lamouri wrote: > > > On 2015/10/23 at 12:26:21, jochen wrote: > > > > On 2015/10/22 at 13:13:35, alogvinov wrote: > > > > > On 2015/10/21 15:40:05, jam wrote: > > > > > > On 2015/10/20 16:14:59, alogvinov wrote: > > > > > > > On 2015/10/20 12:13:31, Mounir Lamouri wrote: > > > > > > > > On 2015/10/19 at 12:50:55, alogvinov wrote: > > > > > > > > > On 2015/10/19 10:55:55, Mounir Lamouri wrote: > > > > > > > > > > On 2015/10/16 at 08:47:48, alogvinov wrote: > > > > > > > > > > > On 2015/10/15 12:23:21, Mounir Lamouri wrote: > > > > > > > > > > > > On 2015/10/14 at 16:23:13, alogvinov wrote: > > > > > > > > > > > > > On 2015/10/14 10:55:18, Mounir Lamouri wrote: > > > > > > > > > > > > > > Using visibility in content_browsertests is flaky > because > > > more > > > > > > > than > > > > > > > > one > > > > > > > > > > > > tests > > > > > > > > > > > > > > are run in parallel. Like focus, visibility tests > should > > > be > > > > > > moved > > > > > > > to > > > > > > > > > > > > > > interactive_ui_tests but they are chrome/ tests, not > > > content/. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Is it possible for you to mock visibility? > > > > > > > > > > > > > > > > > > > > > > > > > > I wish it were, but how do I approach it? I can't find > any > > > > > > examples > > > > > > > of > > > > > > > > > > > > overriding page visibility in the tests, or any test utils > > > which > > > > > > could > > > > > > > > do > > > > > > > > > > that. > > > > > > > > > > > > > > > > > > > > > > > > > > ScreenWakeLock uses Page::visibilityState() to obtain > > > visibility > > > > > > > > > > information > > > > > > > > > > > > and PageLifecycleObserver::pageVisibilityChanged() to know > > > when it > > > > > > > > changes. > > > > > > > > > > > > > > > > > > > > > > > > Can you call WasShown() and WasHidden() for the purpose of > the > > > test. > > > > > > > > > > > > For example: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > > > > > > > > > > > > > > > > > > I tried doing that but it didn't completely remove the > > > flakiness. I > > > > > > > think > > > > > > > > this > > > > > > > > > > is because while we can force visibility state changes by > calling > > > > > > > WasShown() > > > > > > > > and > > > > > > > > > > WasHidden() on RenderWidgetHostImpl, those functions also get > > > called > > > > > > from > > > > > > > > within > > > > > > > > > > the browser in response to actual visibility changes, so those > > > "real" > > > > > > > calls > > > > > > > > > > overwrite the results of our "test" calls while the test is in > > > progress. > > > > > > > > > > > > > > > > > > > > What if you call WasShown() after calls to > WaitForPossibleUpdate() > > > and > > > > > > any > > > > > > > > call > > > > > > > > > > that might keep the test idle so an event such a visibility > change > > > might > > > > > > > get > > > > > > > > > > handled by the window. > > > > > > > > > > > > > > > > > > If I called WasShown() after WaitForPossibleUpdate(), I would > have > > > to call > > > > > > > > WaitForPossibleUpdate() again, because visibility for the purpose > of > > > wake > > > > > > lock > > > > > > > > is handled in Blink by ScreenWakeLock and the new wake lock status > is > > > > > > > > communicated back to the browser process via mojo channel which is > not > > > > > > > > synchronized with any IPC (that's the original reason for > > > > > > > > WaitForPossibleUpdate()). > > > > > > > > > > > > > > > > > > I looked into hooking into RenderWidgetHostImpl / > RenderProcessHost > > > (for > > > > > > the > > > > > > > > purpose of blocking ViewMsg_WasShown / ViewMsg_WasHidden messages) > but > > > > > > > couldn't > > > > > > > > find a way to do it without modifying any of the existing code. > > > > > > > > > > > > > > > > Then maybe your last bet might be to add something like > > > > > > > > ::SetVisibilityForTest(bool visibility) that will override > visibility. > > > > > > Though, > > > > > > > I > > > > > > > > would do that in a different CL and maybe disable the tests in the > > > time > > > > > > being > > > > > > > > while a solution is found so you can land. > > > > > > > > > > > > > > I disabled visibility-dependent tests on Mac as you suggested, also > > > rolled > > > > > > back > > > > > > > previous attempt on forcing visibility. > > > > > > > > > > > > > > I think it would be nice to have some general visibility mocking > > > mechanism for > > > > > > > browser tests (including those in residing in content) because > > > visibility > > > > > > > information is exposed to Web APIs in Blink and to page scripts so > it > > > can be a > > > > > > > factor in behavior correctness, hence the need to be able to mock > it. > > > > > > > > > > > > yeah we have avoided adding an interactive target in content. > > > > > > > > > > > > I didn't look at the test in details, but is it possible to put it > inside > > > > > > src/chrome? i.e. if it doesn't need to muck around with any non-public > > > content > > > > > > api. > > > > > > > > > > I tried that, but it seems that I can't use non-public content stuff > such as > > > WebContentsImpl and WakeLockServiceContext in interactive ui tests? I cannot > > > check actual screen blocker status without having access to those classes. > > > > > > > > maybe a layout test was better for this? I think we just never create an > > > window manager window, so we can control whether it feels visible or not > (see > > > headless_ in shell_mac.mm) > > > > > > What about using ContentBrowserClient::OverridePageVisibilityState()? You > could > > > use this to make a frame always visible or always hidden I believe. WDYT? > > > > This is flaky on Mac as well (see > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...), > I've mentioned it in https://codereview.chromium.org/1393203004/#msg2 > > The flakyness of that test isn't the usage of ::OverridePageVisibilityState() > but the very first call to GetVisibilityState(). At that point, ::WasShown() > needs to be called to guarantee that the WebContents is actually visible. This > is a test I wrote and I have a fix pending reviews actually. I tried ::OverridePageVisibilityState(), but the tests are still flaky: http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_dbg_... Are you sure that ::OverridePageVisibilityState() at all affects renderer's view of page visibility? As far as I can see, RenderFrameHostImpl::GetVisibilityState() first checks whether RenderWidgetHostImpl is hidden, and then calls ContentBrowserClient::OverridePageVisibilityState on a local variable. The overridden value is not communicated back to RenderWidgetHostImpl which handles visibility state-related communication with the renderer.
It seems that I've finally found a magic combo that isn't flaky on Mac! Calling WebContents::WasHidden() appears to put the native view into a steady hidden state so changes in occlusion don't come through anymore. After that, visibility can be driven manually via RenderWidgetHostImpl::WasShown()/RenderWidgetHostImpl::WasHidden().
On 2015/10/28 15:37:19, alogvinov wrote: > It seems that I've finally found a magic combo that isn't flaky on Mac! Calling > WebContents::WasHidden() appears to put the native view into a steady hidden > state so changes in occlusion don't come through anymore. After that, visibility > can be driven manually via > RenderWidgetHostImpl::WasShown()/RenderWidgetHostImpl::WasHidden(). I can see one particular test is still flaky (WakeLockTest.BrowserInitiatedFrameNavigation), but this may be not related to visibility (I'll look into it). By the way, another option is to prevent occlusion altogether by calling WebContents::IncrementCapturerCount(), but then it would also be not possible to hide web contents (i.e. will have to remove VisibilityStateChange test).
On 2015/10/28 17:12:06, alogvinov wrote: > On 2015/10/28 15:37:19, alogvinov wrote: > > It seems that I've finally found a magic combo that isn't flaky on Mac! > Calling > > WebContents::WasHidden() appears to put the native view into a steady hidden > > state so changes in occlusion don't come through anymore. After that, > visibility > > can be driven manually via > > RenderWidgetHostImpl::WasShown()/RenderWidgetHostImpl::WasHidden(). > > I can see one particular test is still flaky > (WakeLockTest.BrowserInitiatedFrameNavigation), but this may be not related to > visibility (I'll look into it). > By the way, another option is to prevent occlusion altogether by calling > WebContents::IncrementCapturerCount(), but then it would also be not possible to > hide web contents (i.e. will have to remove VisibilityStateChange test). I've added WebContents::IncrementCapturerCount() and now I am confident that the tests are no longer flaky on Mac. Mounir, Nasko, can you please take another look?
The CQ bit was checked by alogvinov@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393203004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393203004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alogvinov@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393203004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393203004/200001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alogvinov@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1393203004/#ps200001 (title: "Fix presubmit errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393203004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393203004/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/f50445a583e4b7f5f4c865a061c83a4d7b57e5c5 Cr-Commit-Position: refs/heads/master@{#357087} |