Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2540683002:
Don't defer quitting in ExecuteScript and other helpers that use DOMMessageQueue. (Closed)
|
Created:
4 years ago by Alexander Semashko Modified:
4 years ago CC:
chromium-reviews, alemate+watch_chromium.org, creis+watch_chromium.org, achuith+watch_chromium.org, nasko+codewatch_chromium.org, jam, pam+watch_chromium.org, oshima+watch_chromium.org, darin-cc_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't defer quitting in ExecuteScript and other helpers that use DOMMessageQueue.
BUG=668707
Committed: https://crrev.com/eab9ac50b601e4d8c810554233a1d2f8e9d61048
Cr-Commit-Position: refs/heads/master@{#437249}
Patch Set 1 #Patch Set 2 : Fix races in supervised user tests. #Patch Set 3 : Try to fix failure during teardown. #Patch Set 4 : Spin the loop in PRE_CreateAndCancelSupervisedUser. #
Total comments: 10
Patch Set 5 : Change FIXME to TODO. #
Total comments: 12
Patch Set 6 : Address comments. #
Total comments: 2
Patch Set 7 : Move ctor & dtor to .cc #
Total comments: 2
Patch Set 8 : rebase #Patch Set 9 : Restore reload in FeatureUsageSucceeds. #
Total comments: 4
Patch Set 10 : Address comments. #Patch Set 11 : rebase #
Messages
Total messages: 66 (41 generated)
The CQ bit was checked by ahest@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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ahest@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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ahest@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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ahest@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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
ahest@yandex-team.ru changed reviewers: + achuith@chromium.org, nasko@chromium.org, tommycli@chromium.org
nasko@chromium.org: Please review changes in content tommycli@chromium.org: Please review changes in chrome/browser/plugins achuith@chromium.org: Please review changes in chromeos/ and chrome/browser/chromeos/
tommycli@chromium.org changed reviewers: + raymes@chromium.org
+cc raymes since he wrote that plugins browsertest. I'll stamp once he does.
https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc (right): https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc:194: // FIXME: There should probably be a wait for a specific event. TODO(): .... https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/supervised/supervised_user_test_base.cc (right): https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/supervised/supervised_user_test_base.cc:365: // FIXME: There should probably be a wait for a specific event. FIXME is not used in Chromium, please use // TODO(): ... format. https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/plugins/... File chrome/browser/plugins/flash_permission_browsertest.cc (right): https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/plugins/... chrome/browser/plugins/flash_permission_browsertest.cc:22: class PageReloadWaiter { Why do we need this wrapper for TestNavigationManager? The goal of TNM is to be as simple to use, so I see this wrapper as unnecessary complexity. If a wait for full load completion is needed, there is TestNavigationObserver class that accomplishes exactly that.
https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc (right): https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc:194: // FIXME: There should probably be a wait for a specific event. On 2016/11/30 17:38:56, nasko wrote: > TODO(): .... Done. https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/supervised/supervised_user_test_base.cc (right): https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/supervised/supervised_user_test_base.cc:365: // FIXME: There should probably be a wait for a specific event. On 2016/11/30 17:38:56, nasko wrote: > FIXME is not used in Chromium, please use // TODO(): ... format. Ok. I just did not know who to assign on these TODOs. I put here achuith for now. achuith, please comment if these TODOs should be assigned on someone else. https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/plugins/... File chrome/browser/plugins/flash_permission_browsertest.cc (left): https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/plugins/... chrome/browser/plugins/flash_permission_browsertest.cc:59: ui_test_utils::NavigateToURL(browser(), Just for the information. This was an "extra" navigation, seemingly not needed in any of the tests. And in TriggerPromptViaNewWindow auto-reload could happen after entering NavigateToURL, which produced the most weird results (either interrupting the navigation at a random point, or happening while we wait for script result). https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/plugins/... File chrome/browser/plugins/flash_permission_browsertest.cc (right): https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/plugins/... chrome/browser/plugins/flash_permission_browsertest.cc:22: class PageReloadWaiter { On 2016/11/30 17:38:56, nasko wrote: > Why do we need this wrapper for TestNavigationManager? The goal of TNM is to be > as simple to use, so I see this wrapper as unnecessary complexity. > If a wait for full load completion is needed, there is TestNavigationObserver > class that accomplishes exactly that. Yes, waiting only for commit here seems wrong. TestNavigationObserver does not fit well, because in some cases there are intermediate navigations to urls like https://get.adobe.com/flashplayer/, which are then dropped. TNO stops waiting at this moment, but the reload that we're waiting for starts later, which brings us to an awkward construction with two observers. The current way filters navigations by URL, which looks a bit clearer to me. By the way, don't you think that the name TestNavigationObserver is somewhat misleading? It sort of implies that this class waits for DidFinishNavigation at most, not the whole page load.
You should have a reviewer more familiar with MessageLoopRunner and RunLoop review this. I'm happy to provide an owner stamp, but I don't know the intricacies to catch subtle errors here. https://codereview.chromium.org/2540683002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc (right): https://codereview.chromium.org/2540683002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc:168: base::RunLoop add_key_wait_loop; Any reason to not put both on the same line? https://codereview.chromium.org/2540683002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/supervised/supervised_user_test_base.cc (right): https://codereview.chromium.org/2540683002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/supervised/supervised_user_test_base.cc:339: base::RunLoop add_key_wait_loop; ditto https://codereview.chromium.org/2540683002/diff/80001/chrome/browser/plugins/... File chrome/browser/plugins/flash_permission_browsertest.cc (right): https://codereview.chromium.org/2540683002/diff/80001/chrome/browser/plugins/... chrome/browser/plugins/flash_permission_browsertest.cc:31: EXPECT_TRUE(content::WaitForLoadStop(web_contents_)); Could you return content::WaitForLoadStop(web_contents_)) here instead? The caller should invoke EXPECT_TRUE - this way you get proper line numbers when the expectation fails. https://codereview.chromium.org/2540683002/diff/80001/chromeos/cryptohome/moc... File chromeos/cryptohome/mock_homedir_methods.cc (right): https://codereview.chromium.org/2540683002/diff/80001/chromeos/cryptohome/moc... chromeos/cryptohome/mock_homedir_methods.cc:19: : success_(false), return_code_(cryptohome::MOUNT_ERROR_NONE) {} Please move this initialization to the header since you're here. https://codereview.chromium.org/2540683002/diff/80001/chromeos/cryptohome/moc... File chromeos/cryptohome/mock_homedir_methods.h (right): https://codereview.chromium.org/2540683002/diff/80001/chromeos/cryptohome/moc... chromeos/cryptohome/mock_homedir_methods.h:62: void set_mount_callback(base::Closure callback) { pass by const reference? Will save an unnecessary copy https://codereview.chromium.org/2540683002/diff/80001/chromeos/cryptohome/moc... chromeos/cryptohome/mock_homedir_methods.h:65: void set_add_key_callback(base::Closure callback) { ditto
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/plugins/... File chrome/browser/plugins/flash_permission_browsertest.cc (right): https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/plugins/... chrome/browser/plugins/flash_permission_browsertest.cc:22: class PageReloadWaiter { On 2016/11/30 22:23:08, Alexander Semashko wrote: > On 2016/11/30 17:38:56, nasko wrote: > > Why do we need this wrapper for TestNavigationManager? The goal of TNM is to > be > > as simple to use, so I see this wrapper as unnecessary complexity. > > If a wait for full load completion is needed, there is TestNavigationObserver > > class that accomplishes exactly that. > > Yes, waiting only for commit here seems wrong. > TestNavigationObserver does not fit well, because in some cases there are > intermediate navigations to urls like > https://get.adobe.com/flashplayer/, which are then dropped. TNO stops waiting at > this moment, but the reload that we're waiting for starts later, which brings us > to an awkward construction with two observers. Maybe UrlLoadObserver then fits the picture better? > The current way filters > navigations by URL, which looks a bit clearer to me. In general, we should reuse code as much as possible and avoiding one-off implementations whenever we can. It makes fixing issues much easier later on. > By the way, don't you think that the name TestNavigationObserver is somewhat > misleading? It sort of implies that this class waits for DidFinishNavigation at > most, not the whole page load. Welcome to reality and history :). TestNavigationObserver existed long before DidFinishNavigation and our effort in being more precise/explicit about what is navigation and what is a document load.
https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/plugins/... File chrome/browser/plugins/flash_permission_browsertest.cc (right): https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/plugins/... chrome/browser/plugins/flash_permission_browsertest.cc:22: class PageReloadWaiter { On 2016/12/01 01:17:28, nasko wrote: > On 2016/11/30 22:23:08, Alexander Semashko wrote: > > On 2016/11/30 17:38:56, nasko wrote: > > > Why do we need this wrapper for TestNavigationManager? The goal of TNM is to > > be > > > as simple to use, so I see this wrapper as unnecessary complexity. > > > If a wait for full load completion is needed, there is > TestNavigationObserver > > > class that accomplishes exactly that. > > > > Yes, waiting only for commit here seems wrong. > > TestNavigationObserver does not fit well, because in some cases there are > > intermediate navigations to urls like > > https://get.adobe.com/flashplayer/, which are then dropped. TNO stops waiting > at > > this moment, but the reload that we're waiting for starts later, which brings > us > > to an awkward construction with two observers. > > Maybe UrlLoadObserver then fits the picture better? Unfortunately, no. When an intermediate navigation gets cancelled, UrlLoadObserver sees that navigation stopped, and that the last committed url is the "right" one, and quits. I'm not happy to add this class too. It even makes me think that probably we need a generic solution with something like "navigation matchers" that can encode and check various conditions for different navigation steps and can be combined to handle complex cases. If only this could be lightweight and easy to use... > > > The current way filters > > navigations by URL, which looks a bit clearer to me. > > In general, we should reuse code as much as possible and avoiding one-off > implementations whenever we can. It makes fixing issues much easier later on. > > > By the way, don't you think that the name TestNavigationObserver is somewhat > > misleading? It sort of implies that this class waits for DidFinishNavigation > at > > most, not the whole page load. > > Welcome to reality and history :). TestNavigationObserver existed long before > DidFinishNavigation and our effort in being more precise/explicit about what is > navigation and what is a document load. https://codereview.chromium.org/2540683002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc (right): https://codereview.chromium.org/2540683002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc:168: base::RunLoop add_key_wait_loop; On 2016/11/30 22:39:52, achuithb wrote: > Any reason to not put both on the same line? Done. I thought it was discouraged by the style guide; it seems I was mistaken. https://codereview.chromium.org/2540683002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/supervised/supervised_user_test_base.cc (right): https://codereview.chromium.org/2540683002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/supervised/supervised_user_test_base.cc:339: base::RunLoop add_key_wait_loop; On 2016/11/30 22:39:52, achuithb wrote: > ditto Done. https://codereview.chromium.org/2540683002/diff/80001/chrome/browser/plugins/... File chrome/browser/plugins/flash_permission_browsertest.cc (right): https://codereview.chromium.org/2540683002/diff/80001/chrome/browser/plugins/... chrome/browser/plugins/flash_permission_browsertest.cc:31: EXPECT_TRUE(content::WaitForLoadStop(web_contents_)); On 2016/11/30 22:39:52, achuithb wrote: > Could you return content::WaitForLoadStop(web_contents_)) here instead? > > The caller should invoke EXPECT_TRUE - this way you get proper line numbers when > the expectation fails. Done. https://codereview.chromium.org/2540683002/diff/80001/chromeos/cryptohome/moc... File chromeos/cryptohome/mock_homedir_methods.cc (right): https://codereview.chromium.org/2540683002/diff/80001/chromeos/cryptohome/moc... chromeos/cryptohome/mock_homedir_methods.cc:19: : success_(false), return_code_(cryptohome::MOUNT_ERROR_NONE) {} On 2016/11/30 22:39:52, achuithb wrote: > Please move this initialization to the header since you're here. Done. https://codereview.chromium.org/2540683002/diff/80001/chromeos/cryptohome/moc... File chromeos/cryptohome/mock_homedir_methods.h (right): https://codereview.chromium.org/2540683002/diff/80001/chromeos/cryptohome/moc... chromeos/cryptohome/mock_homedir_methods.h:62: void set_mount_callback(base::Closure callback) { On 2016/11/30 22:39:52, achuithb wrote: > pass by const reference? Will save an unnecessary copy Done. https://codereview.chromium.org/2540683002/diff/80001/chromeos/cryptohome/moc... chromeos/cryptohome/mock_homedir_methods.h:65: void set_add_key_callback(base::Closure callback) { On 2016/11/30 22:39:52, achuithb wrote: > ditto Done.
LGTM https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/plugins/... File chrome/browser/plugins/flash_permission_browsertest.cc (right): https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/plugins/... chrome/browser/plugins/flash_permission_browsertest.cc:22: class PageReloadWaiter { On 2016/12/01 12:24:59, Alexander Semashko wrote: > On 2016/12/01 01:17:28, nasko wrote: > > On 2016/11/30 22:23:08, Alexander Semashko wrote: > > > On 2016/11/30 17:38:56, nasko wrote: > > > > Why do we need this wrapper for TestNavigationManager? The goal of TNM is > to > > > be > > > > as simple to use, so I see this wrapper as unnecessary complexity. > > > > If a wait for full load completion is needed, there is > > TestNavigationObserver > > > > class that accomplishes exactly that. > > > > > > Yes, waiting only for commit here seems wrong. > > > TestNavigationObserver does not fit well, because in some cases there are > > > intermediate navigations to urls like > > > https://get.adobe.com/flashplayer/, which are then dropped. TNO stops > waiting > > at > > > this moment, but the reload that we're waiting for starts later, which > brings > > us > > > to an awkward construction with two observers. > > > > Maybe UrlLoadObserver then fits the picture better? > > Unfortunately, no. When an intermediate navigation gets cancelled, > UrlLoadObserver sees that navigation stopped, and that the last committed url is > the "right" one, and quits. : ( Is the intermediate navigation in the same frame as the last committed one? Do you know what cancels it? > I'm not happy to add this class too. It even makes me think that probably we > need a generic solution with something like "navigation matchers" that can > encode and check various conditions for different navigation steps and can be > combined to handle complex cases. If only this could be lightweight and easy to > use... nick@ is working on a more generic way to have control over navigations in tests, maybe this scenario can be factored in that. I do think that it is best to fix the loop quit issue, so let's proceed with this as is and come back in the future to see if it can be generalized. Also, if you end up hitting the same requirements in other tests, we can do more general work at that time. > > > The current way filters > > > navigations by URL, which looks a bit clearer to me. > > > > In general, we should reuse code as much as possible and avoiding one-off > > implementations whenever we can. It makes fixing issues much easier later on. > > > > > By the way, don't you think that the name TestNavigationObserver is somewhat > > > misleading? It sort of implies that this class waits for DidFinishNavigation > > at > > > most, not the whole page load. > > > > Welcome to reality and history :). TestNavigationObserver existed long before > > DidFinishNavigation and our effort in being more precise/explicit about what > is > > navigation and what is a document load. >
On 2016/12/01 17:23:09, nasko wrote: > LGTM > > https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/plugins/... > File chrome/browser/plugins/flash_permission_browsertest.cc (right): > > https://codereview.chromium.org/2540683002/diff/60001/chrome/browser/plugins/... > chrome/browser/plugins/flash_permission_browsertest.cc:22: class > PageReloadWaiter { > On 2016/12/01 12:24:59, Alexander Semashko wrote: > > On 2016/12/01 01:17:28, nasko wrote: > > > On 2016/11/30 22:23:08, Alexander Semashko wrote: > > > > On 2016/11/30 17:38:56, nasko wrote: > > > > > Why do we need this wrapper for TestNavigationManager? The goal of TNM > is > > to > > > > be > > > > > as simple to use, so I see this wrapper as unnecessary complexity. > > > > > If a wait for full load completion is needed, there is > > > TestNavigationObserver > > > > > class that accomplishes exactly that. > > > > > > > > Yes, waiting only for commit here seems wrong. > > > > TestNavigationObserver does not fit well, because in some cases there are > > > > intermediate navigations to urls like > > > > https://get.adobe.com/flashplayer/, which are then dropped. TNO stops > > waiting > > > at > > > > this moment, but the reload that we're waiting for starts later, which > > brings > > > us > > > > to an awkward construction with two observers. > > > > > > Maybe UrlLoadObserver then fits the picture better? > > > > Unfortunately, no. When an intermediate navigation gets cancelled, > > UrlLoadObserver sees that navigation stopped, and that the last committed url > is > > the "right" one, and quits. > > : ( Is the intermediate navigation in the same frame as the last committed one? > Do you know what cancels it? Yes, they are in the main frame. I did not trace down to the exact point of cancellation, but it does not look like we can easily make use of it in the test, so I'd defer any refactoring to the owners. > > > I'm not happy to add this class too. It even makes me think that probably we > > need a generic solution with something like "navigation matchers" that can > > encode and check various conditions for different navigation steps and can be > > combined to handle complex cases. If only this could be lightweight and easy > to > > use... > > nick@ is working on a more generic way to have control over navigations in > tests, maybe this scenario can be factored in that. Good to hear that. Is there a bug or a design doc? > > I do think that it is best to fix the loop quit issue, so let's proceed with > this as is and come back in the future to see if it can be generalized. Also, if > you end up hitting the same requirements in other tests, we can do more general > work at that time. > > > > > The current way filters > > > > navigations by URL, which looks a bit clearer to me. > > > > > > In general, we should reuse code as much as possible and avoiding one-off > > > implementations whenever we can. It makes fixing issues much easier later > on. > > > > > > > By the way, don't you think that the name TestNavigationObserver is > somewhat > > > > misleading? It sort of implies that this class waits for > DidFinishNavigation > > > at > > > > most, not the whole page load. > > > > > > Welcome to reality and history :). TestNavigationObserver existed long > before > > > DidFinishNavigation and our effort in being more precise/explicit about what > > is > > > navigation and what is a document load. > >
On 2016/12/01 19:18:41, Alexander Semashko wrote: > > > > nick@ is working on a more generic way to have control over navigations in > > tests, maybe this scenario can be factored in that. > > Good to hear that. Is there a bug or a design doc? I don't think so at this time. I'll cc you on his CLs if you are interested in following along and providing feedback.
On 2016/12/01 19:25:05, nasko wrote: > On 2016/12/01 19:18:41, Alexander Semashko wrote: > > > > > > nick@ is working on a more generic way to have control over navigations in > > > tests, maybe this scenario can be factored in that. > > > > Good to hear that. Is there a bug or a design doc? > > I don't think so at this time. I'll cc you on his CLs if you are interested in > following along and providing feedback. Thank you, I'm interested.
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2540683002/diff/100001/chromeos/cryptohome/mo... File chromeos/cryptohome/mock_homedir_methods.h (right): https://codereview.chromium.org/2540683002/diff/100001/chromeos/cryptohome/mo... chromeos/cryptohome/mock_homedir_methods.h:19: MockHomedirMethods() = default; This doesn't work. I think these need to be in the cc file as before
https://codereview.chromium.org/2540683002/diff/100001/chromeos/cryptohome/mo... File chromeos/cryptohome/mock_homedir_methods.h (right): https://codereview.chromium.org/2540683002/diff/100001/chromeos/cryptohome/mo... chromeos/cryptohome/mock_homedir_methods.h:19: MockHomedirMethods() = default; On 2016/12/01 21:02:31, achuithb wrote: > This doesn't work. I think these need to be in the cc file as before Done.
raymes, take a look, please.
Sorry for the delay! https://codereview.chromium.org/2540683002/diff/120001/chrome/browser/plugins... File chrome/browser/plugins/flash_permission_browsertest.cc (left): https://codereview.chromium.org/2540683002/diff/120001/chrome/browser/plugins... chrome/browser/plugins/flash_permission_browsertest.cc:60: GetWebContents()->GetLastCommittedURL()); I think we want to keep this because if the user clicks "block" the page won't be refreshed. In that case we need to refresh the page to make sure that flash really wasn't enabled.
https://codereview.chromium.org/2540683002/diff/120001/chrome/browser/plugins... File chrome/browser/plugins/flash_permission_browsertest.cc (left): https://codereview.chromium.org/2540683002/diff/120001/chrome/browser/plugins... chrome/browser/plugins/flash_permission_browsertest.cc:60: GetWebContents()->GetLastCommittedURL()); On 2016/12/05 01:46:26, raymes wrote: > I think we want to keep this because if the user clicks "block" the page won't > be refreshed. In that case we need to refresh the page to make sure that flash > really wasn't enabled. Done. Please note that I updated the comment above to make things a bit more clear.
lgtm https://codereview.chromium.org/2540683002/diff/160001/chrome/browser/plugins... File chrome/browser/plugins/flash_permission_browsertest.cc (right): https://codereview.chromium.org/2540683002/diff/160001/chrome/browser/plugins... chrome/browser/plugins/flash_permission_browsertest.cc:79: // NB: In cases where flash is allowed the page reloads automatically, nit: fill 80 chars above or add a newline between paragraphs https://codereview.chromium.org/2540683002/diff/160001/chrome/browser/plugins... chrome/browser/plugins/flash_permission_browsertest.cc:80: // and tests should always wait for this reload to finish before calling nit: this->that
rs lgtm since raymes lgtmed
Thanks for the review. JFYI: I want to hold on landing this some time because there is some uncertainty on whether previous CLs for this issue behave well, see https://bugs.chromium.org/p/chromium/issues/detail?id=670844 https://codereview.chromium.org/2540683002/diff/160001/chrome/browser/plugins... File chrome/browser/plugins/flash_permission_browsertest.cc (right): https://codereview.chromium.org/2540683002/diff/160001/chrome/browser/plugins... chrome/browser/plugins/flash_permission_browsertest.cc:79: // NB: In cases where flash is allowed the page reloads automatically, On 2016/12/06 00:02:53, raymes wrote: > nit: fill 80 chars above or add a newline between paragraphs Done. https://codereview.chromium.org/2540683002/diff/160001/chrome/browser/plugins... chrome/browser/plugins/flash_permission_browsertest.cc:80: // and tests should always wait for this reload to finish before calling On 2016/12/06 00:02:53, raymes wrote: > nit: this->that Done.
The CQ bit was checked by ahest@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/v2/patch-status/codereview.chromium.or...
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 ahest@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/v2/patch-status/codereview.chromium.or...
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 ahest@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, nasko@chromium.org, tommycli@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2540683002/#ps180001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc:
While running git apply --index -p1;
error: patch failed:
chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc:182
error:
chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc:
patch does not apply
Patch:
chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc
Index:
chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc
diff --git
a/chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc
b/chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc
index
d0b0e410e99776f683003aaf202fcfb8ea9f9e71..c9fbed8acbcba4828fcb1c757aafb87457562756
100644
---
a/chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc
+++
b/chrome/browser/chromeos/login/supervised/supervised_user_creation_browsertest.cc
@@ -164,12 +164,19 @@
IN_PROC_BROWSER_TEST_F(SupervisedUserTransactionCleanupTest,
StartFlowLoginAsManager();
FillNewUserData(kTestSupervisedUserDisplayName);
+ base::RunLoop mount_wait_loop, add_key_wait_loop;
+ mock_homedir_methods_->set_mount_callback(mount_wait_loop.QuitClosure());
+ mock_homedir_methods_->set_add_key_callback(add_key_wait_loop.QuitClosure());
EXPECT_CALL(*mock_homedir_methods_, MountEx(_, _, _, _)).Times(1);
EXPECT_CALL(*mock_homedir_methods_, AddKeyEx(_, _, _, _, _)).Times(1);
JSEval("$('supervised-user-creation-next-button').click()");
+ mount_wait_loop.Run();
+ add_key_wait_loop.Run();
testing::Mock::VerifyAndClearExpectations(mock_homedir_methods_);
+ mock_homedir_methods_->set_mount_callback(base::Closure());
+ mock_homedir_methods_->set_add_key_callback(base::Closure());
EXPECT_TRUE(registration_utility_stub_->register_was_called());
EXPECT_EQ(registration_utility_stub_->display_name(),
@@ -182,6 +189,9 @@ IN_PROC_BROWSER_TEST_F(SupervisedUserTransactionCleanupTest,
// We wait for token now. Press cancel button at this point.
JSEvalOrExitBrowser("$('supervised-user-creation').cancel()");
+
+ // TODO(achuith): There should probably be a wait for a specific event.
+ content::RunAllPendingInMessageLoop();
}
IN_PROC_BROWSER_TEST_(
The CQ bit was checked by ahest@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, achuith@chromium.org, raymes@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2540683002/#ps200001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1481207935414330,
"parent_rev": "b1c5935183d492324b272feaece4d247f7df2e46", "commit_rev":
"6f2bea4f0fb474a8ba665ac2a1de555da47aed2e"}
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Don't defer quitting in ExecuteScript and other helpers that use DOMMessageQueue. BUG=668707 ========== to ========== Don't defer quitting in ExecuteScript and other helpers that use DOMMessageQueue. BUG=668707 Committed: https://crrev.com/eab9ac50b601e4d8c810554233a1d2f8e9d61048 Cr-Commit-Position: refs/heads/master@{#437249} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/eab9ac50b601e4d8c810554233a1d2f8e9d61048 Cr-Commit-Position: refs/heads/master@{#437249} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
