Add base::RunLoop and update ui_test_utils to use it to reduce flakiness

Add base::RunLoop and update ui_test_utils to use it to reduce flakiness

Timeout flakiness has been observed in multiple tests that use Quit. This changes various test utility APIs to use QuitNow via base::RunLoop instead. Some instances of Quit are left as-is where it appears they may have a use case.

The ui_test_utils QuitThisRunLoop function does a safer form of MessageLoop::QuitWhenIdle that allows a few generations of tasks to run before actually quitting the MessageLoop. This addresses the design assumptions of many existing tests while hopefully reducing flaky timeouts by moving away from QuitWhenIdle.

This fixes throughput_tests.cc which is currently timing out on Mac.

BUG=124906, 130141, 131220, 128305, 132932
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144824

[jbates, 2012-06-05 01:20:04]

phajdan: chrome/test/OWNERS
jar: base/OWNERS

[jar (doing other things), 2012-06-05 02:10:53]

One of the reasons for doing a Quit() is to ensure you run the pending tasks...
which can often do cleanup.  Have you tried to run this with a bot to detect
what leaks might be caused by this landing?

I also see a lot of tests failing on your current bot runs... and perhaps the
Quit() was selected to allow final tasks to run to completion... but I'm not
sure.

IMO, you should carefully change the test you have diagnosed, and be very
cautious about applying this technique to other tests.  This is a delicate area
IMO, and should not be treated (changed) lightly.

[Paweł Hajdan Jr., 2012-06-05 11:57:44]

LGTM

https://chromiumcodereview.appspot.com/10479018/diff/7001/chrome/test/base/ui...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/7001/chrome/test/base/ui...
chrome/test/base/ui_test_utils.cc:334: base::Bind(&MessageLoop::QuitNow,
nit: Would QuitNowClosure be slightly prettier here?

https://chromiumcodereview.appspot.com/10479018/diff/7001/chrome/test/base/ui...
chrome/test/base/ui_test_utils.cc:394: base::Bind(&MessageLoop::QuitNow,
nit: Would QuitNowClosure be slightly prettier here?

https://chromiumcodereview.appspot.com/10479018/diff/7001/chrome/test/base/ui...
chrome/test/base/ui_test_utils.cc:468: base::Bind(&MessageLoop::QuitNow,
nit: Would QuitNowClosure be slightly prettier here?

[jbates, 2012-06-07 00:24:02]

After discussing things with Jim and observing the test failures, it's clear
that this can't be a quick fix. It seems clear that test code shouldn't be using
QuitWhenIdle (aka Quit), because it's flaky. The tasks processed after the call
to QuitWhenIdle are indeterminate, which is not a good characteristic for unit
tests.

QuitAfterPending is almost determinate (almost because of task posting across
threads), because it only processes the existing set of pending tasks at the
time that it executes. That has exposed some test failures due to subtle bugs
(multiple Quits at ui_test_utils.cc:1112) or dependence on the cascade effect of
QuitWhenIdle (ex: FindMovesWhenObscuring in find_bar_host_browsertest.cc).

I'll continue to tackle the remaining test failures, but it's getting close to
working so PTAL.

https://chromiumcodereview.appspot.com/10479018/diff/7001/chrome/test/base/ui...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/7001/chrome/test/base/ui...
chrome/test/base/ui_test_utils.cc:334: base::Bind(&MessageLoop::QuitNow,
On 2012/06/05 11:57:44, Paweł Hajdan Jr. wrote:
> nit: Would QuitNowClosure be slightly prettier here?

Done.

https://chromiumcodereview.appspot.com/10479018/diff/7001/chrome/test/base/ui...
chrome/test/base/ui_test_utils.cc:394: base::Bind(&MessageLoop::QuitNow,
On 2012/06/05 11:57:44, Paweł Hajdan Jr. wrote:
> nit: Would QuitNowClosure be slightly prettier here?

Done.

https://chromiumcodereview.appspot.com/10479018/diff/7001/chrome/test/base/ui...
chrome/test/base/ui_test_utils.cc:468: base::Bind(&MessageLoop::QuitNow,
On 2012/06/05 11:57:44, Paweł Hajdan Jr. wrote:
> nit: Would QuitNowClosure be slightly prettier here?

Done.

[jar (doing other things), 2012-06-08 01:05:37]

https://chromiumcodereview.appspot.com/10479018/diff/24004/base/message_loop.h
File base/message_loop.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/24004/base/message_loop....
base/message_loop.h:252: void QuitAfterPending();
nit: I think this is now too terse.  I read it as:

Do a "Quit" after we have some how finished "Pending."

Perhaps (sadly) better would be:

QuitAfterRunningAllCurrentlyPendingTasks()

or

RunAllCurrentlyPendingTasksAndThenQuit()

https://chromiumcodereview.appspot.com/10479018/diff/24004/base/message_loop....
base/message_loop.h:257: // Invokes QuitWhenIdle on the current MessageLoop when
run. Useful to
nit: I don't think "current MessageLoop" should be in the comment.  Perhaps you
mean on the "then current..." but that seems similarly confusing.  

Perhaps better is:

Construct a Closure that will call QuitWhenIdle().  Useful to schedule an
arbitrary MessageLoop termination.

https://chromiumcodereview.appspot.com/10479018/diff/24004/chrome/test/base/u...
File chrome/test/base/ui_test_utils.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/24004/chrome/test/base/u...
chrome/test/base/ui_test_utils.h:317: : loop_(loop), owned_(false) {}
nit: one constructor init per line (even if the old code was missing this).

https://chromiumcodereview.appspot.com/10479018/diff/24004/ui/ui_controls/ui_...
File ui/ui_controls/ui_controls_internal_win.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/24004/ui/ui_controls/ui_...
ui/ui_controls/ui_controls_internal_win.cc:115: : task_(task),
message_waiting_for_(message_waiting_for) {
nit: one init per line

https://chromiumcodereview.appspot.com/10479018/diff/24004/ui/ui_controls/ui_...
ui/ui_controls/ui_controls_internal_win.cc:140: task_.Reset();
Why did this change?

[jbates, 2012-06-13 22:11:20]

PTAL, I've added the ability to Quit a specific nesting level of
MessageLoop::Run so that the test code can be more explicit.

https://chromiumcodereview.appspot.com/10479018/diff/24004/base/message_loop.h
File base/message_loop.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/24004/base/message_loop....
base/message_loop.h:252: void QuitAfterPending();
On 2012/06/08 01:05:37, jar wrote:
> nit: I think this is now too terse.  I read it as:
> 
> Do a "Quit" after we have some how finished "Pending."
> 
> Perhaps (sadly) better would be:
> 
> QuitAfterRunningAllCurrentlyPendingTasks()
> 
> or
> 
> RunAllCurrentlyPendingTasksAndThenQuit()

Removed it since it wasn't adding more than QuitNow or PostTask(QuitNow) already
offered.

https://chromiumcodereview.appspot.com/10479018/diff/24004/base/message_loop....
base/message_loop.h:257: // Invokes QuitWhenIdle on the current MessageLoop when
run. Useful to
On 2012/06/08 01:05:37, jar wrote:
> nit: I don't think "current MessageLoop" should be in the comment.  Perhaps
you
> mean on the "then current..." but that seems similarly confusing.  
> 
> Perhaps better is:
> 
> Construct a Closure that will call QuitWhenIdle().  Useful to schedule an
> arbitrary MessageLoop termination.

Done.

https://chromiumcodereview.appspot.com/10479018/diff/24004/chrome/test/base/u...
File chrome/test/base/ui_test_utils.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/24004/chrome/test/base/u...
chrome/test/base/ui_test_utils.h:317: : loop_(loop), owned_(false) {}
On 2012/06/08 01:05:37, jar wrote:
> nit: one constructor init per line (even if the old code was missing this).

Done.

https://chromiumcodereview.appspot.com/10479018/diff/24004/ui/ui_controls/ui_...
File ui/ui_controls/ui_controls_internal_win.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/24004/ui/ui_controls/ui_...
ui/ui_controls/ui_controls_internal_win.cc:140: task_.Reset();
On 2012/06/08 01:05:37, jar wrote:
> Why did this change?

I'll undo this.  I just noticed there's a DCHECK in Release if it's called too
many times so no need for this code.

[jbates, 2012-06-13 22:12:27]

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ui/...
File chrome/browser/ui/fullscreen_controller_interactive_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ui/...
chrome/browser/ui/fullscreen_controller_interactive_browsertest.cc:88:
fprintf(stderr, "global %s:%s:%d\n", __FILE__, __FUNCTION__, (int)__LINE__);
Of course, I'll remove these printfs before commit :) Just trying to find the
cause of a timeout in this test.

[jar (doing other things), 2012-06-14 04:50:28]

Aside from some nits, the major request is a suggested change in your new API. 
Please feel free to argue.

Note... I'm not liking this support for this evil... but I guess when it must be
done... helping is better than making life impossible.... I think. :-/.

Thanks,

Jim

https://chromiumcodereview.appspot.com/10479018/diff/20024/base/message_loop.cc
File base/message_loop.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/base/message_loop....
base/message_loop.cc:361: static void QuitNowByIDCurrent(int run_id) {
nit: I think the word "current" is in the wrong place, as it seems to be linked
to "IdCurrent".  Perhaps better is:

QuitCurrentNowByID

<sigh> all names are hard to read... but that might be an improvement.  WDYT?

https://chromiumcodereview.appspot.com/10479018/diff/20024/base/message_loop.h
File base/message_loop.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/base/message_loop....
base/message_loop.h:255: void QuitNowByID(int run_id);
This is an interesting extension.
My general feeling is that I want to always massively discourage nested message
loops, and although this helps some scenarios, it is still murderously hard to
use these without messing up (use only with fully re-entrant tasks!!!).

From an API perspective, I'd rather not expose next_run_id(), which gives a
caller the ability to mess-up, and get the wrong id (some how).  I'd rather have
a 

Run(int* generated_run_id)  // arg is output value only.

This would put all the onus of manipulating these ids as a private affair in the
message loop, but allow a thread that called Run() to get a copy of the id
pushed to an accessible place, for use in a Quit(int
previously_generated_run_id).

https://chromiumcodereview.appspot.com/10479018/diff/20024/base/message_loop....
base/message_loop.h:277: //  MessageLoop::current()->Run();
With the proposed API (preventing others from manipulating run_ids).  This could
would read:
int run_id = 0;
MessageLoop::current()->Run(&run_id);

IMO, if you failed to pass in a run_id pointer, or passed in NULL, then *no*
run_id would even be generated (and hence no need for a global lock, etc.).

https://chromiumcodereview.appspot.com/10479018/diff/20024/base/message_loop....
base/message_loop.h:570: // instances (AutoRunState is associated with calls to
Run).
It might be nice to expand on this statement.  Is the ID globally unique for the
process? Is it generated automagically for the message loop (thread?), or is it
the responsibility of the caller of Run() to select one?

https://chromiumcodereview.appspot.com/10479018/diff/20024/base/message_loop....
base/message_loop.h:583: 
nit: extra blank line probably wasn't intentional.

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ext...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ext...
chrome/browser/extensions/extension_browsertest.cc:310:
installer->set_allow_off_store_install(true);
nit: I couldn't guess what this method does.  I suspect it is a bad name.

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ref...
File chrome/browser/referrer_policy_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ref...
chrome/browser/referrer_policy_browsertest.cc:258:
IDC_CONTENT_CONTEXT_OPENLINKNEWTAB);
nit: Is wrap needed?

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ref...
chrome/browser/referrer_policy_browsertest.cc:267:
IDC_CONTENT_CONTEXT_OPENLINKNEWTAB);
nit: wrap needed?

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ref...
chrome/browser/referrer_policy_browsertest.cc:353:
IDC_CONTENT_CONTEXT_OPENLINKNEWTAB);
nit: wrap?

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ref...
chrome/browser/referrer_policy_browsertest.cc:362:
IDC_CONTENT_CONTEXT_OPENLINKNEWTAB);
nit: wrap?

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/tab...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/tab...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:104:
IDC_CONTENT_CONTEXT_OPENLINKNEWTAB);
nit: wrap needed?

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/tab...
File chrome/browser/tab_contents/render_view_context_menu_browsertest_util.cc
(right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/tab...
chrome/browser/tab_contents/render_view_context_menu_browsertest_util.cc:14: int
command_to_execute)
nit: arg wrap not needed (I think)

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ui/...
File chrome/browser/ui/fullscreen_controller_interactive_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ui/...
chrome/browser/ui/fullscreen_controller_interactive_browsertest.cc:88:
fprintf(stderr, "global %s:%s:%d\n", __FILE__, __FUNCTION__, (int)__LINE__);
You could probably use a DLOG() function, and not have to worry about yanking it
in time.

On 2012/06/13 22:12:27, jbates wrote:
> Of course, I'll remove these printfs before commit :) Just trying to find the
> cause of a timeout in this test.

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ui/...
chrome/browser/ui/fullscreen_controller_interactive_browsertest.cc:145:
fprintf(stderr, "global %s:%s:%d\n", __FILE__, __FUNCTION__, (int)__LINE__);
... I see... you hav a LOT of printfs..... probably should have a final review
when you yank these (before commit).

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/test/base/u...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:841: CHECK(message_loop_run_id_ == 0) <<
"Nested runs must use another instance";
I think you need to be explicit in the message loop code that a 0 is never used.

https://chromiumcodereview.appspot.com/10479018/diff/20024/content/test/test_...
File content/test/test_navigation_observer.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/content/test/test_...
content/test/test_navigation_observer.cc:87: int message_loop_run_id =
MessageLoop::current()->next_run_id();
re: discussion of changing API from next_run_id().

Although I think this code works... this hints at why this next_run_id() API
could be problematic.  The issue is that the call has global impact (it changes
some global?), and hence is not re-entrant.  If any of the calls in lines 88-94
happened to induce nested runs, and need to call next_run_id(), then the value
would be "wrong" by the time it was used in line 94.

I think the API needs to be clearly re-entrant... and should  atomically do the
Run() and establish the ID (rather than doing them separately).

[jbates, 2012-06-15 02:20:38]

jar: ptal

I ended up with a simple RefCounted class to store the data for RunID. It works
well since it can be created *before* anyone calls Run.

https://chromiumcodereview.appspot.com/10479018/diff/20024/base/message_loop.cc
File base/message_loop.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/base/message_loop....
base/message_loop.cc:361: static void QuitNowByIDCurrent(int run_id) {
On 2012/06/14 04:50:29, jar wrote:
> nit: I think the word "current" is in the wrong place, as it seems to be
linked
> to "IdCurrent".  Perhaps better is:
> 
> QuitCurrentNowByID
> 
> <sigh> all names are hard to read... but that might be an improvement.  WDYT?

Done.

https://chromiumcodereview.appspot.com/10479018/diff/20024/base/message_loop.h
File base/message_loop.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/base/message_loop....
base/message_loop.h:255: void QuitNowByID(int run_id);
On 2012/06/14 04:50:29, jar wrote:
> This is an interesting extension.
> My general feeling is that I want to always massively discourage nested
message
> loops, and although this helps some scenarios, it is still murderously hard to
> use these without messing up (use only with fully re-entrant tasks!!!).
> 
> From an API perspective, I'd rather not expose next_run_id(), which gives a
> caller the ability to mess-up, and get the wrong id (some how).  I'd rather
have
> a 
> 
> Run(int* generated_run_id)  // arg is output value only.
> 
> This would put all the onus of manipulating these ids as a private affair in
the
> message loop, but allow a thread that called Run() to get a copy of the id
> pushed to an accessible place, for use in a Quit(int
> previously_generated_run_id).
> 

Done as discussed over chat.

https://chromiumcodereview.appspot.com/10479018/diff/20024/base/message_loop....
base/message_loop.h:570: // instances (AutoRunState is associated with calls to
Run).
On 2012/06/14 04:50:29, jar wrote:
> It might be nice to expand on this statement.  Is the ID globally unique for
the
> process? Is it generated automagically for the message loop (thread?), or is
it
> the responsibility of the caller of Run() to select one?

Removed.

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ext...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ext...
chrome/browser/extensions/extension_browsertest.cc:310:
installer->set_allow_off_store_install(true);
On 2012/06/14 04:50:29, jar wrote:
> nit: I couldn't guess what this method does.  I suspect it is a bad name.

I'm just in here to fix some flakiness, not all that sure what this code does.

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ref...
File chrome/browser/referrer_policy_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/ref...
chrome/browser/referrer_policy_browsertest.cc:258:
IDC_CONTENT_CONTEXT_OPENLINKNEWTAB);
On 2012/06/14 04:50:29, jar wrote:
> nit: Is wrap needed?

wrap still needed (here and below).

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/tab...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/tab...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:104:
IDC_CONTENT_CONTEXT_OPENLINKNEWTAB);
On 2012/06/14 04:50:29, jar wrote:
> nit: wrap needed?

yeah, wrap still needed.

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/tab...
File chrome/browser/tab_contents/render_view_context_menu_browsertest_util.cc
(right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/browser/tab...
chrome/browser/tab_contents/render_view_context_menu_browsertest_util.cc:14: int
command_to_execute)
On 2012/06/14 04:50:29, jar wrote:
> nit: arg wrap not needed (I think)

Done.

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/test/base/u...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:841: CHECK(message_loop_run_id_ == 0) <<
"Nested runs must use another instance";
On 2012/06/14 04:50:29, jar wrote:
> I think you need to be explicit in the message loop code that a 0 is never
used.

Fixed by removing this code.

https://chromiumcodereview.appspot.com/10479018/diff/20024/content/test/test_...
File content/test/test_navigation_observer.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/20024/content/test/test_...
content/test/test_navigation_observer.cc:87: int message_loop_run_id =
MessageLoop::current()->next_run_id();
On 2012/06/14 04:50:29, jar wrote:
> re: discussion of changing API from next_run_id().
> 
> Although I think this code works... this hints at why this next_run_id() API
> could be problematic.  The issue is that the call has global impact (it
changes
> some global?), and hence is not re-entrant.  If any of the calls in lines
88-94
> happened to induce nested runs, and need to call next_run_id(), then the value
> would be "wrong" by the time it was used in line 94.
> 
> I think the API needs to be clearly re-entrant... and should  atomically do
the
> Run() and establish the ID (rather than doing them separately).

Done.

[jbates, 2012-06-16 01:27:32]

phajdan: ptal (ui_test_utils changes)

[Paweł Hajdan Jr., 2012-06-18 15:56:25]

+Darin, the MessageLoop changes look quite complex; feel free to defer the
review to Jim, just thought more eyes looking at these changes could help

Mostly minor comments, thanks for working on this.

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ext...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ext...
chrome/browser/extensions/extension_browsertest.cc:142: // After
chrome::NOTIFICATION_EXTENSION_LOADED there are still remaining tasks
Have you checked all tests where we wait for NOTIFICATION_EXTENSION_LOADED? Just
checking.

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ref...
File chrome/browser/referrer_policy_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ref...
chrome/browser/referrer_policy_browsertest.cc:165:
ui_test_utils::RunAllPendingInMessageLoop();
Is this needed after every WaitForLoadStop? If so, make it a part of
WaitForLoadStop.

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ui/...
File chrome/browser/ui/fullscreen_controller_interactive_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ui/...
chrome/browser/ui/fullscreen_controller_interactive_browsertest.cc:88:
fprintf(stderr, "global %s:%s:%d\n", __FILE__, __FUNCTION__, (int)__LINE__);
Ouch, some debugging code? This can't be a part of the commit - or maybe you
want to convert these to LOG?

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ui/...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest_win.cc
(right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ui/...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest_win.cc:250:
ui_test_utils::RunAllPendingInMessageLoop();
If this is needed after every SendMouseEventsSync, please make it part of
SendMouseEventsSync.

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/test/base/b...
File chrome/test/base/bookmark_load_observer.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/test/base/b...
chrome/test/base/bookmark_load_observer.h:44: MessageLoop::RunID
message_loop_run_id_;
nit: Indentation is off (3 instead of 2).

[jar (doing other things), 2012-06-18 19:43:21]

I just focused on the actual message_loop changes (not on all the api uses).

Comments below.

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop.cc
File base/message_loop.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop....
base/message_loop.cc:313: if (run_id->quit_now_received_)
If you want to make the DCHECK in line 311 more valuable, you should add to this
clause:
run_id->used_ = true;

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop....
base/message_loop.cc:353: // begun; in that case, this will cause the Run to
immediately return.
Is the case of calling quit before the RunWithId() useful?  IF so, this
justifies the separate bool.

Does this let us run exactly one task in a message loop? Is there another
scenario for this?

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop.h
File base/message_loop.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop....
base/message_loop.h:238: bool quit_now_received_;
Why not use the bool in the run_state?  

I later saw that you handle calling quit before a Run(), which I guess means we
need this... but  I wasn't clear about that use case.

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop....
base/message_loop.h:287: // all nested Run calls have completed.
Please add the following additional warning.  I don't think we can say this
enough :-/  :

WARNING: You must NEVER assume that a call to QuitNowWithID will terminate the
targetted message loop. If a nested message loop continues running, the target
may NEVER terminate.  It is very easy to deadlock (run forever) in such a case.

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop....
base/message_loop.h:443: RunID run_state_id;
nit: member names need a trailing underscore.

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop_...
File base/message_loop_unittest.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop_...
base/message_loop_unittest.cc:602: ORDERERD,
nit: Although you didn't type this.... I think this is a misspelling of:

ORDERED

[jbates, 2012-06-18 23:14:02]

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop.cc
File base/message_loop.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop....
base/message_loop.cc:313: if (run_id->quit_now_received_)
On 2012/06/18 19:43:21, jar wrote:
> If you want to make the DCHECK in line 311 more valuable, you should add to
this
> clause:
> run_id->used_ = true;

It's done by the AutoRunState constructor. I moved the DCHECK there to be more
clear.

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop....
base/message_loop.cc:353: // begun; in that case, this will cause the Run to
immediately return.
On 2012/06/18 19:43:21, jar wrote:
> Is the case of calling quit before the RunWithId() useful?  IF so, this
> justifies the separate bool.
> 
> Does this let us run exactly one task in a message loop? Is there another
> scenario for this?

Explained in response to your other comment in message_loop.h. This feature
should allow some simplification of test code that is currently responsible for
tracking run/quit states. I added some new tests to verify behavior when
QuitWithID is used before, during and after RunWithID.

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop.h
File base/message_loop.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop....
base/message_loop.h:238: bool quit_now_received_;
On 2012/06/18 19:43:21, jar wrote:
> Why not use the bool in the run_state?  
> 
> I later saw that you handle calling quit before a Run(), which I guess means
we
> need this... but  I wasn't clear about that use case.

Added this use case to the QuitNowWithID comments. I think it's helpful to make
the API easier to use and less error prone. Often, tests will kick off an
asynchronous process and then later wait for the process to complete by calling
ML::RunWithID(id). It simplifies the test code if they don't have to track
whether they have called run yet, or whether they have called quit yet. This
design allows them to quit at any time, even multiple times on the same RunID
and it will always affect a specific past/present/future call to RunWithID.

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop....
base/message_loop.h:287: // all nested Run calls have completed.
On 2012/06/18 19:43:21, jar wrote:
> Please add the following additional warning.  I don't think we can say this
> enough :-/  :
> 
> WARNING: You must NEVER assume that a call to QuitNowWithID will terminate the
> targetted message loop. If a nested message loop continues running, the target
> may NEVER terminate.  It is very easy to deadlock (run forever) in such a
case.

Done.

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop....
base/message_loop.h:443: RunID run_state_id;
On 2012/06/18 19:43:21, jar wrote:
> nit: member names need a trailing underscore.

Done.

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop_...
File base/message_loop_unittest.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/base/message_loop_...
base/message_loop_unittest.cc:602: ORDERERD,
On 2012/06/18 19:43:21, jar wrote:
> nit: Although you didn't type this.... I think this is a misspelling of:
> 
> ORDERED

Done.

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ext...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ext...
chrome/browser/extensions/extension_browsertest.cc:142: // After
chrome::NOTIFICATION_EXTENSION_LOADED there are still remaining tasks
On 2012/06/18 15:56:25, Paweł Hajdan Jr. wrote:
> Have you checked all tests where we wait for NOTIFICATION_EXTENSION_LOADED?
Just
> checking.

Removed to see if reposting Quit task is enough.

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ref...
File chrome/browser/referrer_policy_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ref...
chrome/browser/referrer_policy_browsertest.cc:165:
ui_test_utils::RunAllPendingInMessageLoop();
On 2012/06/18 15:56:25, Paweł Hajdan Jr. wrote:
> Is this needed after every WaitForLoadStop? If so, make it a part of
> WaitForLoadStop.

Removed.

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ui/...
File chrome/browser/ui/fullscreen_controller_interactive_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ui/...
chrome/browser/ui/fullscreen_controller_interactive_browsertest.cc:88:
fprintf(stderr, "global %s:%s:%d\n", __FILE__, __FUNCTION__, (int)__LINE__);
On 2012/06/18 15:56:25, Paweł Hajdan Jr. wrote:
> Ouch, some debugging code? This can't be a part of the commit - or maybe you
> want to convert these to LOG?

Done.

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ui/...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest_win.cc
(right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/browser/ui/...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest_win.cc:250:
ui_test_utils::RunAllPendingInMessageLoop();
On 2012/06/18 15:56:25, Paweł Hajdan Jr. wrote:
> If this is needed after every SendMouseEventsSync, please make it part of
> SendMouseEventsSync.

Removed it, because the reposting QuitNow task in ui_test_utils should be
enough.

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/test/base/b...
File chrome/test/base/bookmark_load_observer.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/39002/chrome/test/base/b...
chrome/test/base/bookmark_load_observer.h:44: MessageLoop::RunID
message_loop_run_id_;
On 2012/06/18 15:56:25, Paweł Hajdan Jr. wrote:
> nit: Indentation is off (3 instead of 2).

Done.

[darin (slow to review), 2012-06-19 17:55:51]

https://chromiumcodereview.appspot.com/10479018/diff/43001/base/message_loop.h
File base/message_loop.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/43001/base/message_loop....
base/message_loop.h:252: void RunWithID(const RunID& run_id);
[Sorry, detour incoming!!]

I think it would be helpful to abstract this API a bit more.

What we really have are two distinct things:

  1-  A queue of messages / tasks to process.
  2-  A function that loops over the queue of messages / tasks and runs them.

Perhaps part of the problem stems from the fact that we are trying to use
a single class to represent both things.

Imagine if we could write code like this:

  RunLoop run_loop;
  run_loop.Start();
  ...
  WeakPtr<RunLoop> weak_run_loop(run_loop.AsWeakPtr());
  ...
  if (weak_run_loop)
    weak_run_loop->Stop();

The RunLoop constructor would use MessageLoop::current() to locate
the queue of messages / tasks for the current thread.

If we switched to this model, then the approach for running a nested
loop would look like this:

  void StopRunLoop(const base::WeakPtr<RunLoop>& run_loop) {
    run_loop->Stop();
  }

  void RunNestedLoop() {
    RunLoop nested_run_loop;
    MessageLoop::current()->PostDelayedTask(
        FROM_HERE,
        base::Bind(&StopNestedLoop, nested_run_loop.AsWeakPtr()));
    nested_run_loop.Start();
  }

  void main() {
    RunLoop main_run_loop;
    MessageLoop::current()->PostTask(
        FROM_HERE,
        base::Bind(&RunNestedLoop));
    main_run_loop.Start();
  }

In other words, the RunLoop embodies the concept of MessageLoop::RunID.
I think this creates a much simpler programming model for consumers.

[jbates, 2012-06-22 02:12:51]

ptal, getting closer...

https://chromiumcodereview.appspot.com/10479018/diff/43001/base/message_loop.h
File base/message_loop.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/43001/base/message_loop....
base/message_loop.h:252: void RunWithID(const RunID& run_id);
On 2012/06/19 17:55:51, darin wrote:
> [Sorry, detour incoming!!]
> 
> I think it would be helpful to abstract this API a bit more.
> 
> What we really have are two distinct things:
> 
>   1-  A queue of messages / tasks to process.
>   2-  A function that loops over the queue of messages / tasks and runs them.
> 
> Perhaps part of the problem stems from the fact that we are trying to use
> a single class to represent both things.
> 
> Imagine if we could write code like this:
> 
>   RunLoop run_loop;
>   run_loop.Start();
>   ...
>   WeakPtr<RunLoop> weak_run_loop(run_loop.AsWeakPtr());
>   ...
>   if (weak_run_loop)
>     weak_run_loop->Stop();
> 
> The RunLoop constructor would use MessageLoop::current() to locate
> the queue of messages / tasks for the current thread.
> 
> If we switched to this model, then the approach for running a nested
> loop would look like this:
> 
>   void StopRunLoop(const base::WeakPtr<RunLoop>& run_loop) {
>     run_loop->Stop();
>   }
> 
>   void RunNestedLoop() {
>     RunLoop nested_run_loop;
>     MessageLoop::current()->PostDelayedTask(
>         FROM_HERE,
>         base::Bind(&StopNestedLoop, nested_run_loop.AsWeakPtr()));
>     nested_run_loop.Start();
>   }
> 
>   void main() {
>     RunLoop main_run_loop;
>     MessageLoop::current()->PostTask(
>         FROM_HERE,
>         base::Bind(&RunNestedLoop));
>     main_run_loop.Start();
>   }
> 
> In other words, the RunLoop embodies the concept of MessageLoop::RunID.
> I think this creates a much simpler programming model for consumers.

RunLoop done with some slight semantic changes.

I started with RunLoop::Start, but kept getting tripped up by a blocking "Start"
method. Start sounds like it kicks off some asynchronous thing and then returns,
while Run sounds more like it blocks to me. Also, RunUntilIdle sounds better
than StartUntilIdle :)

https://chromiumcodereview.appspot.com/10479018/diff/54001/base/message_loop.cc
File base/message_loop.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/54001/base/message_loop....
base/message_loop.cc:488: if (run_loop_->run_depth_ != 1)
Moved this down here, because it's cheaper to check if the queue is empty than
to access a WeakPtr...

[darin (slow to review), 2012-06-22 04:35:45]

I'm happy with Run() too.  It is also more consistent with the current
mental model people have.

You might consider sticking w/ RunAllPending() to avoid having to define
what it means to be idle.  It wouldn't be correct to say that the message
loop is idle when RunUntilIdle() returns, right? :-/

-Darin


On Thu, Jun 21, 2012 at 7:12 PM, <jbates@chromium.org> wrote:

> ptal, getting closer...
>
>
>
> https://chromiumcodereview.**appspot.com/10479018/diff/**
>
43001/base/message_loop.h<https://chromiumcodereview.appspot.com/10479018/diff/43001/base/message_loop.h>
> File base/message_loop.h (right):
>
> https://chromiumcodereview.**appspot.com/10479018/diff/**
>
43001/base/message_loop.h#**newcode252<https://chromiumcodereview.appspot.com/10479018/diff/43001/base/message_loop.h#newcode252>
> base/message_loop.h:252: void RunWithID(const RunID& run_id);
> On 2012/06/19 17:55:51, darin wrote:
>
>> [Sorry, detour incoming!!]
>>
>
>  I think it would be helpful to abstract this API a bit more.
>>
>
>  What we really have are two distinct things:
>>
>
>    1-  A queue of messages / tasks to process.
>>   2-  A function that loops over the queue of messages / tasks and
>>
> runs them.
>
>  Perhaps part of the problem stems from the fact that we are trying to
>>
> use
>
>> a single class to represent both things.
>>
>
>  Imagine if we could write code like this:
>>
>
>    RunLoop run_loop;
>>   run_loop.Start();
>>   ...
>>   WeakPtr<RunLoop> weak_run_loop(run_loop.**AsWeakPtr());
>>   ...
>>   if (weak_run_loop)
>>     weak_run_loop->Stop();
>>
>
>  The RunLoop constructor would use MessageLoop::current() to locate
>> the queue of messages / tasks for the current thread.
>>
>
>  If we switched to this model, then the approach for running a nested
>> loop would look like this:
>>
>
>    void StopRunLoop(const base::WeakPtr<RunLoop>& run_loop) {
>>     run_loop->Stop();
>>   }
>>
>
>    void RunNestedLoop() {
>>     RunLoop nested_run_loop;
>>     MessageLoop::current()->**PostDelayedTask(
>>         FROM_HERE,
>>         base::Bind(&StopNestedLoop, nested_run_loop.AsWeakPtr()));
>>     nested_run_loop.Start();
>>   }
>>
>
>    void main() {
>>     RunLoop main_run_loop;
>>     MessageLoop::current()->**PostTask(
>>         FROM_HERE,
>>         base::Bind(&RunNestedLoop));
>>     main_run_loop.Start();
>>   }
>>
>
>  In other words, the RunLoop embodies the concept of
>>
> MessageLoop::RunID.
>
>> I think this creates a much simpler programming model for consumers.
>>
>
> RunLoop done with some slight semantic changes.
>
> I started with RunLoop::Start, but kept getting tripped up by a blocking
> "Start" method. Start sounds like it kicks off some asynchronous thing
> and then returns, while Run sounds more like it blocks to me. Also,
> RunUntilIdle sounds better than StartUntilIdle :)
>
> https://chromiumcodereview.**appspot.com/10479018/diff/**
>
54001/base/message_loop.cc<https://chromiumcodereview.appspot.com/10479018/diff/54001/base/message_loop.cc>
> File base/message_loop.cc (right):
>
> https://chromiumcodereview.**appspot.com/10479018/diff/**
>
54001/base/message_loop.cc#**newcode488<https://chromiumcodereview.appspot.com/10479018/diff/54001/base/message_loop.cc#newcode488>
> base/message_loop.cc:488: if (run_loop_->run_depth_ != 1)
> Moved this down here, because it's cheaper to check if the queue is
> empty than to access a WeakPtr...
>
>
https://chromiumcodereview.**appspot.com/10479018/<https://chromiumcodereview...
>

[darin (slow to review), 2012-06-22 04:42:23]

https://chromiumcodereview.appspot.com/10479018/diff/51008/base/message_loop.h
File base/message_loop.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/51008/base/message_loop....
base/message_loop.h:221: class RunLoop : public base::SupportsWeakPtr<RunLoop> {
this can be a top-level type in the base:: namespace.

https://chromiumcodereview.appspot.com/10479018/diff/51008/base/message_loop....
base/message_loop.h:225: explicit RunLoop(Dispatcher* dispatcher);
cool, i had the same idea to just make the dispatcher a property of the RunLoop
to avoid the need for RunWithDispatcher :)

https://chromiumcodereview.appspot.com/10479018/diff/51008/base/message_loop....
base/message_loop.h:247: // (when there are no nested RunLoops running), leaving
any pending tasks
i think you can just define this in terms of quitting the RunLoop.  there can be
other RunLoops servicing the same task queue (MessageLoop), and quitting one
RunLoop has no bearing on the others.

https://chromiumcodereview.appspot.com/10479018/diff/51008/base/message_loop....
base/message_loop.h:255: void Stop();
probably should go back to calling this Quit.  but we can define it to mean an
immediate quit.

https://chromiumcodereview.appspot.com/10479018/diff/51008/base/message_loop....
base/message_loop.h:325: void QuitWhenIdle();
hmm... thinking about names again.  RunUntilIdle() and QuitWhenIdle() fit nicely
together.  RunAllPending() and QuitWhenIdle() are not so obviously related.  I'm
not sure how to rename QuitWhenIdle() to say "Pending" instead of "Idle".  Maybe
that means the names with "Idle" are better.  QuitWhenNonePending :-P :-P

[jbates, 2012-06-22 23:21:01]

https://chromiumcodereview.appspot.com/10479018/diff/51008/base/message_loop.h
File base/message_loop.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/51008/base/message_loop....
base/message_loop.h:221: class RunLoop : public base::SupportsWeakPtr<RunLoop> {
On 2012/06/22 04:42:23, darin wrote:
> this can be a top-level type in the base:: namespace.

Done.

https://chromiumcodereview.appspot.com/10479018/diff/51008/base/message_loop....
base/message_loop.h:247: // (when there are no nested RunLoops running), leaving
any pending tasks
On 2012/06/22 04:42:23, darin wrote:
> i think you can just define this in terms of quitting the RunLoop.  there can
be
> other RunLoops servicing the same task queue (MessageLoop), and quitting one
> RunLoop has no bearing on the others.

Done.

https://chromiumcodereview.appspot.com/10479018/diff/51008/base/message_loop....
base/message_loop.h:255: void Stop();
On 2012/06/22 04:42:23, darin wrote:
> probably should go back to calling this Quit.  but we can define it to mean an
> immediate quit.

Done.

https://chromiumcodereview.appspot.com/10479018/diff/51008/base/message_loop....
base/message_loop.h:325: void QuitWhenIdle();
On 2012/06/22 04:42:23, darin wrote:
> hmm... thinking about names again.  RunUntilIdle() and QuitWhenIdle() fit
nicely
> together.  RunAllPending() and QuitWhenIdle() are not so obviously related. 
I'm
> not sure how to rename QuitWhenIdle() to say "Pending" instead of "Idle". 
Maybe
> that means the names with "Idle" are better.  QuitWhenNonePending :-P :-P

I'm not too partial. I guess I prefer "Idle" because it might make people more
inclined to RTFM compared to Pending which seems more likely inferred as
all-currently-pending. Switching everything to Idle too see how it looks...

[jar (doing other things), 2012-06-23 02:43:19]

You wrote a lot of nice code here!! ;-)

A few questions, and a few nits below.

https://chromiumcodereview.appspot.com/10479018/diff/63001/base/message_loop.cc
File base/message_loop.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/base/message_loop....
base/message_loop.cc:368: return !!run_loop_;
nit: I couldn't find it... but I think the style guide suggests comparison with
NULL, rather than use of "!", so I *think* the old format is preferred.

https://chromiumcodereview.appspot.com/10479018/diff/63001/base/message_pump_...
File base/message_pump_android.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/base/message_pump_...
base/message_pump_android.cc:78: run_loop_->BeforeRun();
Do we need to check return value here?

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/browser/tab...
File chrome/browser/tab_contents/render_view_context_menu_browsertest_util.cc
(right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/browser/tab...
chrome/browser/tab_contents/render_view_context_menu_browsertest_util.cc:14: int
command_to_execute) : command_to_execute_(command_to_execute) {
nit: When we can't fit it all on one line... init args get wrapped at the colon.

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/b...
File chrome/test/base/bookmark_load_observer.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/b...
chrome/test/base/bookmark_load_observer.h:21: const
base::WeakPtr<base::RunLoop>& run_loop);
nit: wrapping probably not needed.

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:288: void RunRunLoop(const
base::WeakPtr<base::RunLoop>& run_loop) {
nit: I didn't like the RunRun... prefix.  Perhaps better would be:
RunThisRunLoop
or
RunWeakRunLoop
or
RunWeakPtrRunLoop
or
other??

In truth, I also really wished this was a method on this weak-ptr... and then it
would just be a Run() method.  Is it worth making a local helper class
containing the weak pointer? Is that too clumsy?

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:298: run_loop->Run();
Line 296 and 298 can now be pulled out of the if blocks, and then in fact, line
291 can now go inside the #if.

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:306: if (run_loop) run_loop->Quit();
Do we gain value by iterating and posting when we don't have a run_loop any
more, or can we just return once it is gone?  After all, when we're done posting
(via line 308) and running, we'll just give up and do nothing anyway.
Could we just:
if (!run_loop)
  return;
before line 305?

It seems that if the run loop is gone, we'll just be posting into a messageloop
that has exited, and building cruft tasks there.

Or can we assert that we are the only way for this loop to finally end, and the
run_loop is always still intact?

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:313: // Quit the MessageLoop Run of the
specified |run_id|.
nit: update comment "run_id"

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:314: void QuitRunLoop(const
base::WeakPtr<base::RunLoop>& run_loop) {
This name is not as jarring... but if you did encapsulate the WeakPtr to a
RunLoop, this would indeed just be a Quit method... or perhaps a DeferredQuit()
method.

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:793: new MessageLoopRunner;
nit: no need to wrap.

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:834: new MessageLoopRunner;
nit: un-wrap (here and below in file, like line 845, 1276, etc.)

https://chromiumcodereview.appspot.com/10479018/diff/63001/content/browser/br...
File content/browser/browser_main_loop.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/content/browser/br...
content/browser/browser_main_loop.cc:659: #else
Can you move lines 652-654 into this #else clause?  It probably should have
closer to that in the past (no code in Android flavor).

https://chromiumcodereview.appspot.com/10479018/diff/63001/content/test/test_...
File content/test/test_navigation_observer.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/content/test/test_...
content/test/test_navigation_observer.cc:25: if (run_loop) run_loop->Quit();
(as commented in other file): When run_loop is null, you may as well just return
before line 24.

https://chromiumcodereview.appspot.com/10479018/diff/63001/content/test/test_...
content/test/test_navigation_observer.cc:106: const int num_quit_deferrals = 5;
Is this a hack number guess, or is this meaningful?

[jbates, 2012-06-25 20:04:57]

Thanks Jim, addressed your feedback.

https://chromiumcodereview.appspot.com/10479018/diff/63001/base/message_loop.cc
File base/message_loop.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/base/message_loop....
base/message_loop.cc:368: return !!run_loop_;
On 2012/06/23 02:43:19, jar wrote:
> nit: I couldn't find it... but I think the style guide suggests comparison
with
> NULL, rather than use of "!", so I *think* the old format is preferred. 

Done.

https://chromiumcodereview.appspot.com/10479018/diff/63001/base/message_pump_...
File base/message_pump_android.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/base/message_pump_...
base/message_pump_android.cc:78: run_loop_->BeforeRun();
On 2012/06/23 02:43:19, jar wrote:
> Do we need to check return value here?

Done.

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/browser/tab...
File chrome/browser/tab_contents/render_view_context_menu_browsertest_util.cc
(right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/browser/tab...
chrome/browser/tab_contents/render_view_context_menu_browsertest_util.cc:14: int
command_to_execute) : command_to_execute_(command_to_execute) {
On 2012/06/23 02:43:19, jar wrote:
> nit: When we can't fit it all on one line... init args get wrapped at the
colon.

Done.

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/b...
File chrome/test/base/bookmark_load_observer.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/b...
chrome/test/base/bookmark_load_observer.h:21: const
base::WeakPtr<base::RunLoop>& run_loop);
On 2012/06/23 02:43:19, jar wrote:
> nit: wrapping probably not needed.

Done.

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:288: void RunRunLoop(const
base::WeakPtr<base::RunLoop>& run_loop) {
On 2012/06/23 02:43:19, jar wrote:
> nit: I didn't like the RunRun... prefix.  Perhaps better would be:
> RunThisRunLoop
> or
> RunWeakRunLoop
> or
> RunWeakPtrRunLoop
> or
> other??
> 
> In truth, I also really wished this was a method on this weak-ptr... and then
it
> would just be a Run() method.  Is it worth making a local helper class
> containing the weak pointer? Is that too clumsy? 

Renamed to RunThisRunLoop. I tried the helper class, but it didn't seem to be
worth its code-weight.

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:298: run_loop->Run();
On 2012/06/23 02:43:19, jar wrote:
> Line 296 and 298 can now be pulled out of the if blocks, and then in fact,
line
> 291 can now go inside the #if.

Done.

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:306: if (run_loop) run_loop->Quit();
On 2012/06/23 02:43:19, jar wrote:
> Do we gain value by iterating and posting when we don't have a run_loop any
> more, or can we just return once it is gone?  After all, when we're done
posting
> (via line 308) and running, we'll just give up and do nothing anyway.
> Could we just:
> if (!run_loop)
>   return;
> before line 305?
> 
> It seems that if the run loop is gone, we'll just be posting into a
messageloop
> that has exited, and building cruft tasks there.
> 
> Or can we assert that we are the only way for this loop to finally end, and
the
> run_loop is always still intact?

Done.

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:313: // Quit the MessageLoop Run of the
specified |run_id|.
On 2012/06/23 02:43:19, jar wrote:
> nit: update comment "run_id"

Done.

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:314: void QuitRunLoop(const
base::WeakPtr<base::RunLoop>& run_loop) {
On 2012/06/23 02:43:19, jar wrote:
> This name is not as jarring... but if you did encapsulate the WeakPtr to a
> RunLoop, this would indeed just be a Quit method... or perhaps a
DeferredQuit()
> method.

Changed to QuitThisRunLoop to be consistent with RunThisRunLoop.

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:793: new MessageLoopRunner;
On 2012/06/23 02:43:19, jar wrote:
> nit: no need to wrap.

Done.

https://chromiumcodereview.appspot.com/10479018/diff/63001/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:834: new MessageLoopRunner;
On 2012/06/23 02:43:19, jar wrote:
> nit: un-wrap (here and below in file, like line 845, 1276, etc.)

Done.

https://chromiumcodereview.appspot.com/10479018/diff/63001/content/browser/br...
File content/browser/browser_main_loop.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/content/browser/br...
content/browser/browser_main_loop.cc:659: #else
On 2012/06/23 02:43:19, jar wrote:
> Can you move lines 652-654 into this #else clause?  It probably should have
> closer to that in the past (no code in Android flavor).

Done.

https://chromiumcodereview.appspot.com/10479018/diff/63001/content/test/test_...
File content/test/test_navigation_observer.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/content/test/test_...
content/test/test_navigation_observer.cc:25: if (run_loop) run_loop->Quit();
On 2012/06/23 02:43:19, jar wrote:
> (as commented in other file): When run_loop is null, you may as well just
return
> before line 24.

Done.

https://chromiumcodereview.appspot.com/10479018/diff/63001/content/test/test_...
content/test/test_navigation_observer.cc:106: const int num_quit_deferrals = 5;
On 2012/06/23 02:43:19, jar wrote:
> Is this a hack number guess, or is this meaningful?

It's approaching being meaningful. 20 worked but may have been overkill. 2
caused an uncomfortable number of test failures. Now I'm at 5 seeing if the
trybots agree.

[jar (doing other things), 2012-06-25 21:19:43]

LGTM with nits (and a potentially insignificant question).

https://chromiumcodereview.appspot.com/10479018/diff/63001/content/test/test_...
File content/test/test_navigation_observer.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/content/test/test_...
content/test/test_navigation_observer.cc:106: const int num_quit_deferrals = 5;
The larger number (20) should have no measurable impact on performance (20
minimal tasks posted and run should probably complete nicely in less than 1ms). 
The downside risk of "getting this wrong" is a flaky test, and a revisit.

IMO, you should document how you tried to pull this number form the air, mention
it can be increased if the test proves flaky, and be very conservative (choose
big... like 20?) in your selection of a constant.


On 2012/06/25 20:04:57, jbates wrote:
> On 2012/06/23 02:43:19, jar wrote:
> > Is this a hack number guess, or is this meaningful?
> 
> It's approaching being meaningful. 20 worked but may have been overkill. 2
> caused an uncomfortable number of test failures. Now I'm at 5 seeing if the
> trybots agree.

https://chromiumcodereview.appspot.com/10479018/diff/71002/base/message_pump_...
File base/message_pump_android.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/71002/base/message_pump_...
base/message_pump_android.cc:79: NOTREACHED();
Should you actually return?   I the semantics of the BeforeRun() call suggest
you would not run the message loop.  I don't mind the NOTREACHED() as well.

https://chromiumcodereview.appspot.com/10479018/diff/71002/chrome/test/base/u...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/71002/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:843: new MessageLoopRunner;
nit: unwrap line.

https://chromiumcodereview.appspot.com/10479018/diff/71002/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:1275: new MessageLoopRunner;
nit: unwrap line.

[jbates, 2012-06-27 02:06:37]

sky: OWNERS for:
    ui/views
    ash
    ui/aura
    chrome/browser/ui

https://chromiumcodereview.appspot.com/10479018/diff/63001/content/test/test_...
File content/test/test_navigation_observer.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/63001/content/test/test_...
content/test/test_navigation_observer.cc:106: const int num_quit_deferrals = 5;
On 2012/06/25 21:19:43, jar wrote:
> The larger number (20) should have no measurable impact on performance (20
> minimal tasks posted and run should probably complete nicely in less than
1ms). 
> The downside risk of "getting this wrong" is a flaky test, and a revisit.
> 
> IMO, you should document how you tried to pull this number form the air,
mention
> it can be increased if the test proves flaky, and be very conservative (choose
> big... like 20?) in your selection of a constant.
> 
> 
> On 2012/06/25 20:04:57, jbates wrote:
> > On 2012/06/23 02:43:19, jar wrote:
> > > Is this a hack number guess, or is this meaningful?
> > 
> > It's approaching being meaningful. 20 worked but may have been overkill. 2
> > caused an uncomfortable number of test failures. Now I'm at 5 seeing if the
> > trybots agree.
> 

Docs done. Went with 10, because some perf tests might have some largish 200ms
frames, and 200ms*20 is starting to seem too long of a wait per test Wait. 10
should be enough, watching the bots...

https://chromiumcodereview.appspot.com/10479018/diff/71002/base/message_pump_...
File base/message_pump_android.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/71002/base/message_pump_...
base/message_pump_android.cc:79: NOTREACHED();
On 2012/06/25 21:19:43, jar wrote:
> Should you actually return?   I the semantics of the BeforeRun() call suggest
> you would not run the message loop.  I don't mind the NOTREACHED() as well.

Added some comments.

https://chromiumcodereview.appspot.com/10479018/diff/71002/chrome/test/base/u...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/71002/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:843: new MessageLoopRunner;
On 2012/06/25 21:19:43, jar wrote:
> nit: unwrap line.

Done.

https://chromiumcodereview.appspot.com/10479018/diff/71002/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:1275: new MessageLoopRunner;
On 2012/06/25 21:19:43, jar wrote:
> nit: unwrap line.

Done.

[Paweł Hajdan Jr., 2012-06-27 07:33:28]

LGTM - this is an impressive change

I'm going on vacation, so please do not wait for me when landing. Scott has
OWNERship in the same test directories I do, so I'm happy with his review if
anything remains.

https://chromiumcodereview.appspot.com/10479018/diff/92003/chrome/test/base/i...
File chrome/test/base/in_process_browser_test.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/92003/chrome/test/base/i...
chrome/test/base/in_process_browser_test.cc:376: // Sometimes tests leave Quit
tasks in the MessageLoop (for shame), so let's
nit: Please add TODO(phajdan.jr) to maybe detect those cases and make such bad
tests fail. Feel free to make that TODO(jbates).

https://chromiumcodereview.appspot.com/10479018/diff/92003/chrome/test/base/u...
File chrome/test/base/ui_test_utils.h (left):

https://chromiumcodereview.appspot.com/10479018/diff/92003/chrome/test/base/u...
chrome/test/base/ui_test_utils.h:321: void RunFor(int ms);
It's cool to see this sleep-like thing gone from ui_test_utils. Yay! \o/

https://chromiumcodereview.appspot.com/10479018/diff/92003/content/test/test_...
File content/test/test_navigation_observer.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/92003/content/test/test_...
content/test/test_navigation_observer.cc:21: void DeferredQuitRunLoop(const
base::Closure& quit_task,
I've seen this routine in this CL before. Please de-duplicate and move it to
ui_test_utils.

https://chromiumcodereview.appspot.com/10479018/diff/92003/content/test/test_...
content/test/test_navigation_observer.cc:105: // kNumQuitDeferrals in
ui_test_utils.cc for explanation.
Why not make ui_test_utils expose its kNumQuitDeferrals constant instead of
duplicating it here?

Ah right, this is content/ vs. chrome/ thing... Feel free to create some test
utils files in content/test then.

And because this CL is already complex, feel free to do that in a separate CL
(just stick a TODO here so that things are not forgotten).

[sky, 2012-06-27 14:02:09]

https://chromiumcodereview.appspot.com/10479018/diff/92003/base/message_loop.h
File base/message_loop.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/92003/base/message_loop....
base/message_loop.h:259: static base::Closure QuitWhenIdleClosure();
MenuController, which runs a nested message loop via RunLoop needs this, so I
don't see how it can be deprecated. The problem with RunLoop is that its not
called off enough. See  MenuController::SetExitType for details.

[sky, 2012-06-27 14:05:24]

Here's a patch that resulted in the QuitNow:
http://codereview.chromium.org/10143001/ along with discussion.

  -Scott

On Wed, Jun 27, 2012 at 7:02 AM,  <sky@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/10479018/diff/92003/base/message_loop.h
> File base/message_loop.h (right):
>
>
https://chromiumcodereview.appspot.com/10479018/diff/92003/base/message_loop....
> base/message_loop.h:259: static base::Closure QuitWhenIdleClosure();
> MenuController, which runs a nested message loop via RunLoop needs this,
> so I don't see how it can be deprecated. The problem with RunLoop is
> that its not called off enough. See  MenuController::SetExitType for
> details.
>
> https://chromiumcodereview.appspot.com/10479018/

[jbates, 2012-06-27 17:23:46]

https://chromiumcodereview.appspot.com/10479018/diff/92003/base/message_loop.h
File base/message_loop.h (right):

https://chromiumcodereview.appspot.com/10479018/diff/92003/base/message_loop....
base/message_loop.h:259: static base::Closure QuitWhenIdleClosure();
On 2012/06/27 14:02:10, sky wrote:
> MenuController, which runs a nested message loop via RunLoop needs this, so I
> don't see how it can be deprecated. The problem with RunLoop is that its not
> called off enough. See  MenuController::SetExitType for details. 

Can you elaborate on "not called off enough"? The goal is to eventually remove
all Run/Quit methods from MessageLoop and provide that functionality with
RunLoop.

MenuController will eventually call Quit on a specific nested RunLoop rather
than calling MessageLoop::QuitNow. I think that requires some changes to the
aura::client::DispatcherClient API so that RunLoops can be owned by the caller
of RunWithDispatcher. There will be a few followup patches to get to the point
where MessageLoop::Run/Quit can be removed.

[sky, 2012-06-27 21:11:00]

On Wed, Jun 27, 2012 at 10:23 AM,  <jbates@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/10479018/diff/92003/base/message_loop.h
> File base/message_loop.h (right):
>
>
https://chromiumcodereview.appspot.com/10479018/diff/92003/base/message_loop....
> base/message_loop.h:259: static base::Closure QuitWhenIdleClosure();
> On 2012/06/27 14:02:10, sky wrote:
>>
>> MenuController, which runs a nested message loop via RunLoop needs
>
> this, so I
>>
>> don't see how it can be deprecated. The problem with RunLoop is that
>
> its not
>>
>> called off enough. See  MenuController::SetExitType for details.
>
>
> Can you elaborate on "not called off enough"? The goal is to eventually
> remove all Run/Quit methods from MessageLoop and provide that
> functionality with RunLoop.

Gah! That should be 'often enough'. The problem is the Dispatcher is
only called before dispatching native events. That means if the
Dispatcher needs to exit the nested message loop as the result of some
task (tasks are not native events), it can't exit until the next
native event comes in. This leads to all sorts of problems with the
menu code and is why it needs QuitNow.

> MenuController will eventually call Quit on a specific nested RunLoop
> rather than calling MessageLoop::QuitNow. I think that requires some
> changes to the aura::client::DispatcherClient API so that RunLoops can
> be owned by the caller of RunWithDispatcher. There will be a few
> followup patches to get to the point where MessageLoop::Run/Quit can be
> removed.

As long as we still get the QuitNow functionality, it shouldn't matter
who we call it on.

  -Scott
>
> https://chromiumcodereview.appspot.com/10479018/

[jbates, 2012-06-27 21:46:26]

On 2012/06/27 21:11:00, sky wrote:
> On Wed, Jun 27, 2012 at 10:23 AM,  <mailto:jbates@chromium.org> wrote:
> >
> >
> https://chromiumcodereview.appspot.com/10479018/diff/92003/base/message_loop.h
> > File base/message_loop.h (right):
> >
> >
>
https://chromiumcodereview.appspot.com/10479018/diff/92003/base/message_loop....
> > base/message_loop.h:259: static base::Closure QuitWhenIdleClosure();
> > On 2012/06/27 14:02:10, sky wrote:
> >>
> >> MenuController, which runs a nested message loop via RunLoop needs
> >
> > this, so I
> >>
> >> don't see how it can be deprecated. The problem with RunLoop is that
> >
> > its not
> >>
> >> called off enough. See &nbsp;MenuController::SetExitType for details.
> >
> >
> > Can you elaborate on "not called off enough"? The goal is to eventually
> > remove all Run/Quit methods from MessageLoop and provide that
> > functionality with RunLoop.
> 
> Gah! That should be 'often enough'. The problem is the Dispatcher is
> only called before dispatching native events. That means if the
> Dispatcher needs to exit the nested message loop as the result of some
> task (tasks are not native events), it can't exit until the next
> native event comes in. This leads to all sorts of problems with the
> menu code and is why it needs QuitNow.

It looks like MenuController::Run (which is reentrant) will need to store the
RunLoop::QuitClosure in the menu_stack_ so that the quit can be applied to the
correct menu. It does seem like it should be possible to push and pop a
dispatcher stack in the MessagePump as opposed to using nested message loops,
but I don't know all the use cases.

> 
> > MenuController will eventually call Quit on a specific nested RunLoop
> > rather than calling MessageLoop::QuitNow. I think that requires some
> > changes to the aura::client::DispatcherClient API so that RunLoops can
> > be owned by the caller of RunWithDispatcher. There will be a few
> > followup patches to get to the point where MessageLoop::Run/Quit can be
> > removed.
> 
> As long as we still get the QuitNow functionality, it shouldn't matter
> who we call it on.

OK good. QuitNow is equivalent to run_loop->Quit (or QuitClosure). The
difference being that the caller needs either the RunLoop pointer or a copy of
the run_loop->QuitClosure().

> 
>   -Scott
> >
> > https://chromiumcodereview.appspot.com/10479018/

[sky, 2012-06-27 23:09:36]

LGTM

[jbates, 2012-06-28 20:46:11]

https://chromiumcodereview.appspot.com/10479018/diff/92003/chrome/test/base/i...
File chrome/test/base/in_process_browser_test.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/92003/chrome/test/base/i...
chrome/test/base/in_process_browser_test.cc:376: // Sometimes tests leave Quit
tasks in the MessageLoop (for shame), so let's
On 2012/06/27 07:33:28, Paweł Hajdan Jr. wrote:
> nit: Please add TODO(phajdan.jr) to maybe detect those cases and make such bad
> tests fail. Feel free to make that TODO(jbates).

Done.

https://chromiumcodereview.appspot.com/10479018/diff/92003/chrome/test/base/u...
File chrome/test/base/ui_test_utils.h (left):

https://chromiumcodereview.appspot.com/10479018/diff/92003/chrome/test/base/u...
chrome/test/base/ui_test_utils.h:321: void RunFor(int ms);
On 2012/06/27 07:33:28, Paweł Hajdan Jr. wrote:
> It's cool to see this sleep-like thing gone from ui_test_utils. Yay! \o/

Yeah, turned out nobody was using it, so easy to remove :)

https://chromiumcodereview.appspot.com/10479018/diff/92003/content/test/test_...
File content/test/test_navigation_observer.cc (right):

https://chromiumcodereview.appspot.com/10479018/diff/92003/content/test/test_...
content/test/test_navigation_observer.cc:21: void DeferredQuitRunLoop(const
base::Closure& quit_task,
On 2012/06/27 07:33:28, Paweł Hajdan Jr. wrote:
> I've seen this routine in this CL before. Please de-duplicate and move it to
> ui_test_utils.

Yeah it should be moved to content/test/test_utils.h (or similar). Added TODO.

https://chromiumcodereview.appspot.com/10479018/diff/92003/content/test/test_...
content/test/test_navigation_observer.cc:105: // kNumQuitDeferrals in
ui_test_utils.cc for explanation.
On 2012/06/27 07:33:28, Paweł Hajdan Jr. wrote:
> Why not make ui_test_utils expose its kNumQuitDeferrals constant instead of
> duplicating it here?
> 
> Ah right, this is content/ vs. chrome/ thing... Feel free to create some test
> utils files in content/test then.
> 
> And because this CL is already complex, feel free to do that in a separate CL
> (just stick a TODO here so that things are not forgotten).

Sounds good, added a TODO.

[commit-bot: I haz the power, 2012-06-28 20:48:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbates@chromium.org/10479018/96003

[commit-bot: I haz the power, 2012-06-28 22:57:32]

Change committed as 144824