Revert "Revert of Reland: Switch WindowedNotificationObserver to use base::RunLoop. (patchset #3 id…

Reland #2: Switch WindowedNotificationObserver to use base::RunLoop.

Now it will quit the message loop immediately after receiving the
notification. Also it does not allow nested tasks anymore.

Previous CL (https://codereview.chromium.org/2720513003) is in patchset #1.
Added fixes for:
 - browser_tests.NavigatingExtensionPopupBrowserTest.DownloadViaPost with PlzNavigate;
 - interactive_ui_tests.ConstrainedWindowViewTest.NavigationOnBackspace.
Also rephrased comment in print_preview_dialog_controller_browsertest.cc.

BUG=668707
Review-Url: https://codereview.chromium.org/2740783003
Cr-Commit-Position: refs/heads/master@{#466016}
Committed: https://chromium.googlesource.com/chromium/src/+/25911df55b8f939560df1f664f8cc8941ddc4677

[Alexander Semashko, 2017-03-09 09:50:03]

Description was changed from

==========
Revert "Revert of Reland: Switch WindowedNotificationObserver to use
base::RunLoop. (patchset #3 id:40001 of
https://codereview.chromium.org/2720513003/ )"

BUG=
==========

to

==========
Reland #2: Switch WindowedNotificationObserver to use base::RunLoop.

Now it will quit the message loop immediately after receiving the
notification. Also it does not allow nested tasks anymore.

Previous CL (https://codereview.chromium.org/2720513003) is in patchset #1.
Added fixes for:
 - browser_tests.NavigatingExtensionPopupBrowserTest.DownloadViaPost with
PlzNavigate;
 - interactive_ui_tests.ConstrainedWindowViewTest.NavigationOnBackspace.

BUG=668707
==========

[Alexander Semashko, 2017-03-09 10:31:46]

ahest@yandex-team.ru changed reviewers:
+ jam@chromium.org

[Alexander Semashko, 2017-03-09 10:33:03]

Description was changed from

==========
Reland #2: Switch WindowedNotificationObserver to use base::RunLoop.

Now it will quit the message loop immediately after receiving the
notification. Also it does not allow nested tasks anymore.

Previous CL (https://codereview.chromium.org/2720513003) is in patchset #1.
Added fixes for:
 - browser_tests.NavigatingExtensionPopupBrowserTest.DownloadViaPost with
PlzNavigate;
 - interactive_ui_tests.ConstrainedWindowViewTest.NavigationOnBackspace.

BUG=668707
==========

to

==========
Reland #2: Switch WindowedNotificationObserver to use base::RunLoop.

Now it will quit the message loop immediately after receiving the
notification. Also it does not allow nested tasks anymore.

Previous CL (https://codereview.chromium.org/2720513003) is in patchset #1.
Added fixes for:
 - browser_tests.NavigatingExtensionPopupBrowserTest.DownloadViaPost with
PlzNavigate;
 - interactive_ui_tests.ConstrainedWindowViewTest.NavigationOnBackspace.
Also rephrased comment in print_preview_dialog_controller_browsertest.cc.

BUG=668707
==========

[Alexander Semashko, 2017-03-09 11:10:27]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 11:10:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 12:19:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 12:19:51]

Dry run: This issue passed the CQ dry run.

[jam, 2017-03-09 16:52:57]

https://codereview.chromium.org/2740783003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2740783003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:911: //
TODO(https://crbug.com/695073): Find out why tests fail without it.
no one is assigned that bug, so I worry that we'll never get to it. 

Since the test fails consistently without this with PlzNavigate, seems like a
repro scenario that can help track this down?

otherwise the advantage of using RunLoop seems to be lost if we have to sprinkle
this.

[Alexander Semashko, 2017-03-09 19:47:48]

https://codereview.chromium.org/2740783003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2740783003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:911: //
TODO(https://crbug.com/695073): Find out why tests fail without it.
On 2017/03/09 16:52:57, jam wrote:
> no one is assigned that bug, so I worry that we'll never get to it. 
> 
> Since the test fails consistently without this with PlzNavigate, seems like a
> repro scenario that can help track this down?
> 
> otherwise the advantage of using RunLoop seems to be lost if we have to
sprinkle
> this.

I was hoping that an extensions/ owner would pick it, and currently
planning to address it later, if nobody chimes in. Because I'm quite
overloaded now, and the investigation can be very time consuming (I
did a quick attempt earlier).

I'm afraid that if we postpone this CL, there might appear new tests
and/or helpers that implicitly depend on deferred quit, and it will
cause another sad series of commits and reverts. OTOH, the repro
scenario will likely remain.

[jam, 2017-03-09 20:48:46]

https://codereview.chromium.org/2740783003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2740783003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:911: //
TODO(https://crbug.com/695073): Find out why tests fail without it.
On 2017/03/09 19:47:48, Alexander Semashko wrote:
> On 2017/03/09 16:52:57, jam wrote:
> > no one is assigned that bug, so I worry that we'll never get to it. 
> > 
> > Since the test fails consistently without this with PlzNavigate, seems like
a
> > repro scenario that can help track this down?
> > 
> > otherwise the advantage of using RunLoop seems to be lost if we have to
> sprinkle
> > this.
> 
> I was hoping that an extensions/ owner would pick it, and currently
> planning to address it later, if nobody chimes in. Because I'm quite
> overloaded now, and the investigation can be very time consuming (I
> did a quick attempt earlier).
> 
> I'm afraid that if we postpone this CL, there might appear new tests
> and/or helpers that implicitly depend on deferred quit, and it will
> cause another sad series of commits and reverts. OTOH, the repro
> scenario will likely remain.

While I do appreciate this change in making things more deterministic, this has
been reverted a few times already for making some tests flaky. Given we don't
have visibility in how many other tests this increased flakiness for, I think
landing this again before understanding the previous flakiness is premature. Yes
there's a small chance that new tests can depend on this behavior, but that
seems unlikely since we only have a small number of known failures out of many
thousands of browser tests.

The solution here to clear the pending tasks in some select areas is worrisome
to me: how much more flakiness is this adding in other areas that we don't know
about?

[Alexander Semashko, 2017-03-09 21:07:07]

On 2017/03/09 20:48:46, jam wrote:
>
https://codereview.chromium.org/2740783003/diff/20001/chrome/browser/extensio...
> File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
> (right):
> 
>
https://codereview.chromium.org/2740783003/diff/20001/chrome/browser/extensio...
> chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:911:
//
> TODO(https://crbug.com/695073): Find out why tests fail without it.
> On 2017/03/09 19:47:48, Alexander Semashko wrote:
> > On 2017/03/09 16:52:57, jam wrote:
> > > no one is assigned that bug, so I worry that we'll never get to it. 
> > > 
> > > Since the test fails consistently without this with PlzNavigate, seems
like
> a
> > > repro scenario that can help track this down?
> > > 
> > > otherwise the advantage of using RunLoop seems to be lost if we have to
> > sprinkle
> > > this.
> > 
> > I was hoping that an extensions/ owner would pick it, and currently
> > planning to address it later, if nobody chimes in. Because I'm quite
> > overloaded now, and the investigation can be very time consuming (I
> > did a quick attempt earlier).
> > 
> > I'm afraid that if we postpone this CL, there might appear new tests
> > and/or helpers that implicitly depend on deferred quit, and it will
> > cause another sad series of commits and reverts. OTOH, the repro
> > scenario will likely remain.
> 
> While I do appreciate this change in making things more deterministic, this
has
> been reverted a few times already for making some tests flaky. Given we don't
> have visibility in how many other tests this increased flakiness for, I think
> landing this again before understanding the previous flakiness is premature.
Yes
> there's a small chance that new tests can depend on this behavior, but that
> seems unlikely since we only have a small number of known failures out of many
> thousands of browser tests.
> 
> The solution here to clear the pending tasks in some select areas is worrisome
> to me: how much more flakiness is this adding in other areas that we don't
know
> about?

I don't see how this solution can add flakiness. Basically, without this
patch every call to WindowedNotificationObserver::Wait does wait for the
notification and (see the nit below) and then execute
RunAllPendingInMessageLoop(). In this patch I move that call to
RunAllPending closer to the identified places that depend on it. (And
there are two places where it is clear that a single RunUntilIdle is
just ok.)

Or did you mean flakiness caused by the removal of pending tasks
processing? That would be a completely different matter.

Nit: when Wait() is called when the observer has already received the
notification, no pending tasks are executed. To me, this is not a game
changer at all, just an additional source of flakiness.

[jam, 2017-03-10 16:09:18]

On 2017/03/09 21:07:07, Alexander Semashko wrote:
> On 2017/03/09 20:48:46, jam wrote:
> >
>
https://codereview.chromium.org/2740783003/diff/20001/chrome/browser/extensio...
> > File
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2740783003/diff/20001/chrome/browser/extensio...
> >
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:911:
> //
> > TODO(https://crbug.com/695073): Find out why tests fail without it.
> > On 2017/03/09 19:47:48, Alexander Semashko wrote:
> > > On 2017/03/09 16:52:57, jam wrote:
> > > > no one is assigned that bug, so I worry that we'll never get to it. 
> > > > 
> > > > Since the test fails consistently without this with PlzNavigate, seems
> like
> > a
> > > > repro scenario that can help track this down?
> > > > 
> > > > otherwise the advantage of using RunLoop seems to be lost if we have to
> > > sprinkle
> > > > this.
> > > 
> > > I was hoping that an extensions/ owner would pick it, and currently
> > > planning to address it later, if nobody chimes in. Because I'm quite
> > > overloaded now, and the investigation can be very time consuming (I
> > > did a quick attempt earlier).
> > > 
> > > I'm afraid that if we postpone this CL, there might appear new tests
> > > and/or helpers that implicitly depend on deferred quit, and it will
> > > cause another sad series of commits and reverts. OTOH, the repro
> > > scenario will likely remain.
> > 
> > While I do appreciate this change in making things more deterministic, this
> has
> > been reverted a few times already for making some tests flaky. Given we
don't
> > have visibility in how many other tests this increased flakiness for, I
think
> > landing this again before understanding the previous flakiness is premature.
> Yes
> > there's a small chance that new tests can depend on this behavior, but that
> > seems unlikely since we only have a small number of known failures out of
many
> > thousands of browser tests.
> > 
> > The solution here to clear the pending tasks in some select areas is
worrisome
> > to me: how much more flakiness is this adding in other areas that we don't
> know
> > about?
> 
> I don't see how this solution can add flakiness. Basically, without this
> patch every call to WindowedNotificationObserver::Wait does wait for the
> notification and (see the nit below) and then execute
> RunAllPendingInMessageLoop(). In this patch I move that call to
> RunAllPending closer to the identified places that depend on it. (And
> there are two places where it is clear that a single RunUntilIdle is
> just ok.)
> 
> Or did you mean flakiness caused by the removal of pending tasks
> processing? That would be a completely different matter.

Right, I meant the fact that existing tests (unfortunately) depended on this.

When you say you don't see how this can cause flakiness, I thought the previous
reverts (not mine) were because some tests started flakiness because of this
behavior change? Am I misunderstanding?

> 
> Nit: when Wait() is called when the observer has already received the
> notification, no pending tasks are executed. To me, this is not a game
> changer at all, just an additional source of flakiness.

[Alexander Semashko, 2017-03-10 16:31:50]

On 2017/03/10 16:09:18, jam wrote:
> On 2017/03/09 21:07:07, Alexander Semashko wrote:
> > On 2017/03/09 20:48:46, jam wrote:
> > >
> >
>
https://codereview.chromium.org/2740783003/diff/20001/chrome/browser/extensio...
> > > File
> chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2740783003/diff/20001/chrome/browser/extensio...
> > >
> chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:911:
> > //
> > > TODO(https://crbug.com/695073): Find out why tests fail without it.
> > > On 2017/03/09 19:47:48, Alexander Semashko wrote:
> > > > On 2017/03/09 16:52:57, jam wrote:
> > > > > no one is assigned that bug, so I worry that we'll never get to it. 
> > > > > 
> > > > > Since the test fails consistently without this with PlzNavigate, seems
> > like
> > > a
> > > > > repro scenario that can help track this down?
> > > > > 
> > > > > otherwise the advantage of using RunLoop seems to be lost if we have
to
> > > > sprinkle
> > > > > this.
> > > > 
> > > > I was hoping that an extensions/ owner would pick it, and currently
> > > > planning to address it later, if nobody chimes in. Because I'm quite
> > > > overloaded now, and the investigation can be very time consuming (I
> > > > did a quick attempt earlier).
> > > > 
> > > > I'm afraid that if we postpone this CL, there might appear new tests
> > > > and/or helpers that implicitly depend on deferred quit, and it will
> > > > cause another sad series of commits and reverts. OTOH, the repro
> > > > scenario will likely remain.
> > > 
> > > While I do appreciate this change in making things more deterministic,
this
> > has
> > > been reverted a few times already for making some tests flaky. Given we
> don't
> > > have visibility in how many other tests this increased flakiness for, I
> think
> > > landing this again before understanding the previous flakiness is
premature.
> > Yes
> > > there's a small chance that new tests can depend on this behavior, but
that
> > > seems unlikely since we only have a small number of known failures out of
> many
> > > thousands of browser tests.
> > > 
> > > The solution here to clear the pending tasks in some select areas is
> worrisome
> > > to me: how much more flakiness is this adding in other areas that we don't
> > know
> > > about?
> > 
> > I don't see how this solution can add flakiness. Basically, without this
> > patch every call to WindowedNotificationObserver::Wait does wait for the
> > notification and (see the nit below) and then execute
> > RunAllPendingInMessageLoop(). In this patch I move that call to
> > RunAllPending closer to the identified places that depend on it. (And
> > there are two places where it is clear that a single RunUntilIdle is
> > just ok.)
> > 
> > Or did you mean flakiness caused by the removal of pending tasks
> > processing? That would be a completely different matter.
> 
> Right, I meant the fact that existing tests (unfortunately) depended on this.
> 
> When you say you don't see how this can cause flakiness, I thought the
previous
> reverts (not mine) were because some tests started flakiness because of this
> behavior change? Am I misunderstanding?

Yes, some tests started flaking, but I added patches that address it.
This is not related to the known (bandaid-fixed for now) flakiness in
extensions tests which, as I understand, is what you were concerned
about.

To discover all tests that are made flaky by a change like this would
require running a huge number of try jobs, and I'm not sure whether it
is a good thing to do for an external contributor, and whether there
are tools to facilitate analyzing the outcome (e.g. to distinguish new
flakes from the ones existed before, and calculate the difference in
failure rates).

> 
> > 
> > Nit: when Wait() is called when the observer has already received the
> > notification, no pending tasks are executed. To me, this is not a game
> > changer at all, just an additional source of flakiness.

[jam, 2017-03-10 19:19:51]

On 2017/03/10 16:31:50, Alexander Semashko wrote:
> On 2017/03/10 16:09:18, jam wrote:
> > On 2017/03/09 21:07:07, Alexander Semashko wrote:
> > > On 2017/03/09 20:48:46, jam wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2740783003/diff/20001/chrome/browser/extensio...
> > > > File
> > chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2740783003/diff/20001/chrome/browser/extensio...
> > > >
> >
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:911:
> > > //
> > > > TODO(https://crbug.com/695073): Find out why tests fail without it.
> > > > On 2017/03/09 19:47:48, Alexander Semashko wrote:
> > > > > On 2017/03/09 16:52:57, jam wrote:
> > > > > > no one is assigned that bug, so I worry that we'll never get to it. 
> > > > > > 
> > > > > > Since the test fails consistently without this with PlzNavigate,
seems
> > > like
> > > > a
> > > > > > repro scenario that can help track this down?
> > > > > > 
> > > > > > otherwise the advantage of using RunLoop seems to be lost if we have
> to
> > > > > sprinkle
> > > > > > this.
> > > > > 
> > > > > I was hoping that an extensions/ owner would pick it, and currently
> > > > > planning to address it later, if nobody chimes in. Because I'm quite
> > > > > overloaded now, and the investigation can be very time consuming (I
> > > > > did a quick attempt earlier).
> > > > > 
> > > > > I'm afraid that if we postpone this CL, there might appear new tests
> > > > > and/or helpers that implicitly depend on deferred quit, and it will
> > > > > cause another sad series of commits and reverts. OTOH, the repro
> > > > > scenario will likely remain.
> > > > 
> > > > While I do appreciate this change in making things more deterministic,
> this
> > > has
> > > > been reverted a few times already for making some tests flaky. Given we
> > don't
> > > > have visibility in how many other tests this increased flakiness for, I
> > think
> > > > landing this again before understanding the previous flakiness is
> premature.
> > > Yes
> > > > there's a small chance that new tests can depend on this behavior, but
> that
> > > > seems unlikely since we only have a small number of known failures out
of
> > many
> > > > thousands of browser tests.
> > > > 
> > > > The solution here to clear the pending tasks in some select areas is
> > worrisome
> > > > to me: how much more flakiness is this adding in other areas that we
don't
> > > know
> > > > about?
> > > 
> > > I don't see how this solution can add flakiness. Basically, without this
> > > patch every call to WindowedNotificationObserver::Wait does wait for the
> > > notification and (see the nit below) and then execute
> > > RunAllPendingInMessageLoop(). In this patch I move that call to
> > > RunAllPending closer to the identified places that depend on it. (And
> > > there are two places where it is clear that a single RunUntilIdle is
> > > just ok.)
> > > 
> > > Or did you mean flakiness caused by the removal of pending tasks
> > > processing? That would be a completely different matter.
> > 
> > Right, I meant the fact that existing tests (unfortunately) depended on
this.
> > 
> > When you say you don't see how this can cause flakiness, I thought the
> previous
> > reverts (not mine) were because some tests started flakiness because of this
> > behavior change? Am I misunderstanding?
> 
> Yes, some tests started flaking, but I added patches that address it.
> This is not related to the known (bandaid-fixed for now) flakiness in
> extensions tests which, as I understand, is what you were concerned
> about.
> 
> To discover all tests that are made flaky by a change like this would
> require running a huge number of try jobs, and I'm not sure whether it
> is a good thing to do for an external contributor, and whether there
> are tools to facilitate analyzing the outcome (e.g. to distinguish new
> flakes from the ones existed before, and calculate the difference in
> failure rates).

What I meant is that since we know some tests started flaking, if we can figure
out how to solve those without the band-aid, that might a generic solution for
other flakiness that we don't know about. I'm not saying we should fix the tests
that are flaking more but we don't know about.

> 
> > 
> > > 
> > > Nit: when Wait() is called when the observer has already received the
> > > notification, no pending tasks are executed. To me, this is not a game
> > > changer at all, just an additional source of flakiness.

[Alexander Semashko, 2017-04-18 16:18:18]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 16:18:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 17:03:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 17:03:09]

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_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Alexander Semashko, 2017-04-19 11:45:39]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 11:46:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 13:00:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 13:00:06]

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_...)

[Alexander Semashko, 2017-04-19 15:39:12]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 15:39:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexander Semashko, 2017-04-19 15:41:54]

Ok, I fixed all test failures that I have seen, PTAL again.

[commit-bot: I haz the power, 2017-04-19 16:47:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 16:47:11]

Dry run: This issue passed the CQ dry run.

[jam, 2017-04-19 23:09:56]

lgtm, thanks!

https://codereview.chromium.org/2740783003/diff/180001/chrome/browser/ui/logi...
File chrome/browser/ui/login/login_handler_browsertest.cc (right):

https://codereview.chromium.org/2740783003/diff/180001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_handler_browsertest.cc:467: class
MultiRealmLoginPromptBrowserTest : public LoginPromptBrowserTest {
I'm not familiar with this, please add a reviewer from this directory's owners

[Alexander Semashko, 2017-04-20 11:03:40]

ahest@yandex-team.ru changed reviewers:
+ vabr@chromium.org

[Alexander Semashko, 2017-04-20 11:03:59]

vabr, can you take a look at chrome/browser/ui/login?

You can view the functional changes in
https://codereview.chromium.org/2701473007/patch/1/10001. The rest is
just a refactoring where I extracted common code from these two nearly
identical tests.

[vabr (Chromium), 2017-04-20 11:55:03]

Thanks, LGTM!
Vaclav

https://codereview.chromium.org/2740783003/diff/180001/chrome/browser/ui/logi...
File chrome/browser/ui/login/login_handler_browsertest.cc (right):

https://codereview.chromium.org/2740783003/diff/180001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_handler_browsertest.cc:546: EXPECT_LT(0,
login_prompt_observer()->auth_needed_count());
nit: Could you add a code comment explaining why the test only checks that there
was a positive number of auth requests and auth cancellations, but does not care
about what exactly that number was or whether those were equal?

(Similarly below in MultipleRealConfirmation.)

https://codereview.chromium.org/2740783003/diff/180001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_handler_browsertest.cc:550: // Checks that
supplying credentials works as exepcted.
nit: expected

[Alexander Semashko, 2017-04-20 14:26:32]

https://codereview.chromium.org/2740783003/diff/180001/chrome/browser/ui/logi...
File chrome/browser/ui/login/login_handler_browsertest.cc (right):

https://codereview.chromium.org/2740783003/diff/180001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_handler_browsertest.cc:546: EXPECT_LT(0,
login_prompt_observer()->auth_needed_count());
On 2017/04/20 11:55:03, vabr (Chromium) wrote:
> nit: Could you add a code comment explaining why the test only checks that
there
> was a positive number of auth requests and auth cancellations, but does not
care
> about what exactly that number was or whether those were equal?
> 
> (Similarly below in MultipleRealConfirmation.)

I don't know why is it so; I tried digging and got to the CL that added this
file 6 years ago:  https://codereview.chromium.org/5814005
Apparently, patch set 9 changed these lines, and before it there were equality
checks. I found no explanation in comments.

https://codereview.chromium.org/2740783003/diff/180001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_handler_browsertest.cc:550: // Checks that
supplying credentials works as exepcted.
On 2017/04/20 11:55:03, vabr (Chromium) wrote:
> nit: expected

Done.

[vabr (Chromium), 2017-04-20 14:29:07]

https://codereview.chromium.org/2740783003/diff/180001/chrome/browser/ui/logi...
File chrome/browser/ui/login/login_handler_browsertest.cc (right):

https://codereview.chromium.org/2740783003/diff/180001/chrome/browser/ui/logi...
chrome/browser/ui/login/login_handler_browsertest.cc:546: EXPECT_LT(0,
login_prompt_observer()->auth_needed_count());
On 2017/04/20 14:26:32, Alexander Semashko wrote:
> On 2017/04/20 11:55:03, vabr (Chromium) wrote:
> > nit: Could you add a code comment explaining why the test only checks that
> there
> > was a positive number of auth requests and auth cancellations, but does not
> care
> > about what exactly that number was or whether those were equal?
> > 
> > (Similarly below in MultipleRealConfirmation.)
> 
> I don't know why is it so; I tried digging and got to the CL that added this
> file 6 years ago:  https://codereview.chromium.org/5814005
> Apparently, patch set 9 changed these lines, and before it there were equality
> checks. I found no explanation in comments.

Ah, OK, I missed that this was already in the old code.
That's fine, then.
Thanks!

[Alexander Semashko, 2017-04-20 14:30:12]

The CQ bit was checked by ahest@yandex-team.ru

[Alexander Semashko, 2017-04-20 14:30:12]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2740783003/#ps200001
(title: "Fix comment.")

[commit-bot: I haz the power, 2017-04-20 14:30:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 15:28:37]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1492698612332120,
"parent_rev": "1e5e1dcfccdb9ec8625bda61a0afda98bf05ffb1", "commit_rev":
"25911df55b8f939560df1f664f8cc8941ddc4677"}

[commit-bot: I haz the power, 2017-04-20 15:28:51]

Description was changed from

==========
Reland #2: Switch WindowedNotificationObserver to use base::RunLoop.

Now it will quit the message loop immediately after receiving the
notification. Also it does not allow nested tasks anymore.

Previous CL (https://codereview.chromium.org/2720513003) is in patchset #1.
Added fixes for:
 - browser_tests.NavigatingExtensionPopupBrowserTest.DownloadViaPost with
PlzNavigate;
 - interactive_ui_tests.ConstrainedWindowViewTest.NavigationOnBackspace.
Also rephrased comment in print_preview_dialog_controller_browsertest.cc.

BUG=668707
==========

to

==========
Reland #2: Switch WindowedNotificationObserver to use base::RunLoop.

Now it will quit the message loop immediately after receiving the
notification. Also it does not allow nested tasks anymore.

Previous CL (https://codereview.chromium.org/2720513003) is in patchset #1.
Added fixes for:
 - browser_tests.NavigatingExtensionPopupBrowserTest.DownloadViaPost with
PlzNavigate;
 - interactive_ui_tests.ConstrainedWindowViewTest.NavigationOnBackspace.
Also rephrased comment in print_preview_dialog_controller_browsertest.cc.

BUG=668707

Review-Url: https://codereview.chromium.org/2740783003
Cr-Commit-Position: refs/heads/master@{#466016}
Committed:
https://chromium.googlesource.com/chromium/src/+/25911df55b8f939560df1f664f8c...
==========

[commit-bot: I haz the power, 2017-04-20 15:28:53]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/25911df55b8f939560df1f664f8c...

[sgurun-gerrit only, 2017-04-20 18:43:06]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/2828163004/ by sgurun@chromium.org.

The reason for reverting is: speculative revert for crbug.com/713820.

[tdanderson, 2017-04-20 19:08:57]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/2828403002/ by tdanderson@chromium.org.

The reason for reverting is: Probable cause for failing browser test
NavigatingExtensionPopupBrowserTest.DownloadViaPost in
https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.11%20Tests....

[tdanderson, 2017-04-20 21:12:56]

On 2017/04/20 19:08:57, tdanderson wrote:
> A revert of this CL (patchset #11 id:200001) has been created in
> https://codereview.chromium.org/2828403002/ by mailto:tdanderson@chromium.org.
> 
> The reason for reverting is: Probable cause for failing browser test
> NavigatingExtensionPopupBrowserTest.DownloadViaPost in
>
https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.11%20Tests....

Note: the revert did not end up landing. (The CQ wasn't picking up
the change, then it turns out the bot started cycling green.)