base::Bind: Cleanup in automation.

base::Bind: Cleanup in automation.

BUG=none
TEST=none

R=csilv@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105761

[csilv, 2011-10-10 17:29:44]

Need to also include:

ui_test_utils_mac.mm
automation_provider_win.cc

[csilv, 2011-10-10 18:36:47]

http://codereview.chromium.org/8212006/diff/4001/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_gtk.cc (right):

http://codereview.chromium.org/8212006/diff/4001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_gtk.cc:137: task = &task5;
This looks problematic.  'task' will point to task5 which will be passed into
task6 on line 139.  However, task5 will go out of scope and leave task pointing
to a disposed object.

http://codereview.chromium.org/8212006/diff/4001/chrome/test/base/ui_test_uti...
File chrome/test/base/ui_test_utils_linux.cc (right):

http://codereview.chromium.org/8212006/diff/4001/chrome/test/base/ui_test_uti...
chrome/test/base/ui_test_utils_linux.cc:11: #include "base/logging.h"
include base/callback.h

http://codereview.chromium.org/8212006/diff/4001/chrome/test/base/ui_test_uti...
File chrome/test/base/ui_test_utils_mac.mm (right):

http://codereview.chromium.org/8212006/diff/4001/chrome/test/base/ui_test_uti...
chrome/test/base/ui_test_utils_mac.mm:10: #include "base/logging.h"
include base/callback.h

[James Hawkins, 2011-10-11 00:49:45]

+ajwong

Made a ton of changes, please take another look.

[awong, 2011-10-11 01:18:04]

Nice!

Got a few comments.  One that is critical.

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_gtk.cc (right):

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_gtk.cc:87: task = &task5;
Why bother with a task* at all?  Just copy?

In fact, I'd rename a bunch of these.

task1 -> drag_response_cb

base::Closure move_chain_cb = IgnoreResult<bool>(
    base::Bind(&ui_controls::SendMouseMoveNotifyWhenDone,
        ui_controls::LEFT, ui_controls::UP, draw_resonse_cb);

move_chain_cb = IgnoreResult<bool>(
    base::Bind(&ui_controls::SendMouseMoveNotifyWhenDone,
        ui_controls::LEFT, ui_controls::UP, move_chain_cb);

for (size_t i = drag_path.size() - 1; i > 0; --i) {
   ...
   move_chain_cb = IgnoreResult<bool>(
       base::Bind(&ui_controls::SendMouseMoveNotifyWhenDone,
           dest_x, dest_y, move_chain_cb);
   move_chain_cb = IgnoreResult<bool>(
       base::Bind(&ui_controls::SendMouseMoveNotifyWhenDone,
           half_step_x, half_step_y, move_chain_cb);
}


move_chain_cb = IgnoreResult<bool>(
    base::Bind(&ui_controls::SendMouseMoveNotifyWhenDone,
        ui_controls::LEFT, ui_controls::DOWN, move_chain_cb);

ui_controls::SendMouseMoveNotifyWhenDone(x + drag_path[0].x(),
    y + drag_path[0].y(),
    move_chain_cb);

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
File chrome/browser/automation/ui_controls_gtk.cc (right):

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
chrome/browser/automation/ui_controls_gtk.cc:75: base::Closure task_;
That comment scares me slightly.  Are we okay with releasing task_ on this
thread?

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
File chrome/browser/automation/ui_controls_internal.h (right):

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
chrome/browser/automation/ui_controls_internal.h:15: void ClickTask(MouseButton
button, int state, const base::Closure& followup) {
Don't inline this function?

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
File chrome/browser/automation/ui_controls_mac.mm (right):

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
chrome/browser/automation/ui_controls_mac.mm:193: class EventQueueWatcher :
public base::Closure {
Eek. bad.

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
File chrome/browser/automation/ui_controls_win.cc (right):

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
chrome/browser/automation/ui_controls_win.cc:366: base::Closure& task) {
const base::Closure&

[James Hawkins, 2011-10-11 21:11:22]

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_gtk.cc (right):

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_gtk.cc:87: task = &task5;
On 2011/10/11 01:18:04, awong wrote:
> Why bother with a task* at all?  Just copy?
> 
> In fact, I'd rename a bunch of these.
> 
> task1 -> drag_response_cb
> 
> base::Closure move_chain_cb = IgnoreResult<bool>(
>     base::Bind(&ui_controls::SendMouseMoveNotifyWhenDone,
>         ui_controls::LEFT, ui_controls::UP, draw_resonse_cb);
> 
> move_chain_cb = IgnoreResult<bool>(
>     base::Bind(&ui_controls::SendMouseMoveNotifyWhenDone,
>         ui_controls::LEFT, ui_controls::UP, move_chain_cb);
> 
> for (size_t i = drag_path.size() - 1; i > 0; --i) {
>    ...
>    move_chain_cb = IgnoreResult<bool>(
>        base::Bind(&ui_controls::SendMouseMoveNotifyWhenDone,
>            dest_x, dest_y, move_chain_cb);
>    move_chain_cb = IgnoreResult<bool>(
>        base::Bind(&ui_controls::SendMouseMoveNotifyWhenDone,
>            half_step_x, half_step_y, move_chain_cb);
> }
> 
> 
> move_chain_cb = IgnoreResult<bool>(
>     base::Bind(&ui_controls::SendMouseMoveNotifyWhenDone,
>         ui_controls::LEFT, ui_controls::DOWN, move_chain_cb);
> 
> ui_controls::SendMouseMoveNotifyWhenDone(x + drag_path[0].x(),
>     y + drag_path[0].y(),
>     move_chain_cb);

Done.

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
File chrome/browser/automation/ui_controls_gtk.cc (right):

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
chrome/browser/automation/ui_controls_gtk.cc:75: base::Closure task_;
On 2011/10/11 01:18:04, awong wrote:
> That comment scares me slightly.  Are we okay with releasing task_ on this
> thread?

Yes.

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
File chrome/browser/automation/ui_controls_internal.h (right):

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
chrome/browser/automation/ui_controls_internal.h:15: void ClickTask(MouseButton
button, int state, const base::Closure& followup) {
On 2011/10/11 01:18:04, awong wrote:
> Don't inline this function?

Done.

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
File chrome/browser/automation/ui_controls_mac.mm (right):

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
chrome/browser/automation/ui_controls_mac.mm:193: class EventQueueWatcher :
public base::Closure {
On 2011/10/11 01:18:04, awong wrote:
> Eek. bad.

Yeah, forgot to fix this. Done.

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
File chrome/browser/automation/ui_controls_win.cc (right):

http://codereview.chromium.org/8212006/diff/7002/chrome/browser/automation/ui...
chrome/browser/automation/ui_controls_win.cc:366: base::Closure& task) {
On 2011/10/11 01:18:04, awong wrote:
> const base::Closure&

Done.

[csilv, 2011-10-11 21:48:00]

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_gtk.cc (right):

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_gtk.cc:28: void
WindowDragResponse(AutomationProvider* provider,
maybe 'SendWindowDragResponse' would be a better name?

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_mac.mm (right):

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:260: return
SendMouseMoveNotifyWhenDone(x, y, NULL);
Can't pass null for last argument, so probably change method contents to:

  base::Closure null_task;
  return SendMouseMoveNotifyWhenDone(x, y, null_task);

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:267: bool
SendMouseMoveNotifyWhenDone(long x, long y, Task* task) {
Task* -> const base::Closure&

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:298: return
SendMouseEventsNotifyWhenDone(type, state, NULL);
Can't pass null for last argument, see previous comment.

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:301: bool
SendMouseEventsNotifyWhenDone(MouseButton type, int state, Task* task) {
Task* -> const base::Closure&

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:356: return
SendMouseEventsNotifyWhenDone(type, UP|DOWN, NULL);
Can't pass null for last argument, see previous comment.

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:379: ClickTask(button, state,
task));
ClickTask(button, state, task) ->base::Bind(&ui_controls::ClickTask, button,
state, task)

[James Hawkins, 2011-10-11 21:54:20]

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_gtk.cc (right):

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_gtk.cc:28: void
WindowDragResponse(AutomationProvider* provider,
On 2011/10/11 21:48:00, csilv wrote:
> maybe 'SendWindowDragResponse' would be a better name?

Done.

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_mac.mm (right):

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:260: return
SendMouseMoveNotifyWhenDone(x, y, NULL);
On 2011/10/11 21:48:00, csilv wrote:
> Can't pass null for last argument, so probably change method contents to:
> 
>   base::Closure null_task;
>   return SendMouseMoveNotifyWhenDone(x, y, null_task);

Done. Used base::Closure() here and elsewhere.

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:267: bool
SendMouseMoveNotifyWhenDone(long x, long y, Task* task) {
On 2011/10/11 21:48:00, csilv wrote:
> Task* -> const base::Closure&

Done.

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:298: return
SendMouseEventsNotifyWhenDone(type, state, NULL);
On 2011/10/11 21:48:00, csilv wrote:
> Can't pass null for last argument, see previous comment.

Done.

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:301: bool
SendMouseEventsNotifyWhenDone(MouseButton type, int state, Task* task) {
On 2011/10/11 21:48:00, csilv wrote:
> Task* -> const base::Closure&

Done.

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:356: return
SendMouseEventsNotifyWhenDone(type, UP|DOWN, NULL);
On 2011/10/11 21:48:00, csilv wrote:
> Can't pass null for last argument, see previous comment.

Done.

http://codereview.chromium.org/8212006/diff/12001/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:379: ClickTask(button, state,
task));
On 2011/10/11 21:48:00, csilv wrote:
> ClickTask(button, state, task) ->base::Bind(&ui_controls::ClickTask, button,
> state, task)

Done.

[csilv, 2011-10-11 22:20:52]

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_mac.mm (right):

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:306: return
(SendMouseEventsNotifyWhenDone(type, DOWN, NULL) &&
NULL -> base::Closure()

[awong, 2011-10-12 08:20:03]

the big chain looks much nicer. :)

Here's a few more comments.

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_gtk.cc (right):

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_gtk.cc:36: void
SendMouseEventsNotifyWhenDoneWithoutResult(
Do you still need these?

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_gtk.cc (right):

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_gtk.cc:305:
base::Bind(ui_controls::ClickTask, button, state, task));
Add &?

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_mac.mm (right):

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:224: MessageLoop::QuitClosure());
Are we sure we want to change this to QuitClosure()?

[James Hawkins, 2011-10-12 17:12:39]

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_gtk.cc (right):

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_gtk.cc:36: void
SendMouseEventsNotifyWhenDoneWithoutResult(
On 2011/10/12 08:20:03, awong wrote:
> Do you still need these?

Done.

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_gtk.cc (right):

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_gtk.cc:305:
base::Bind(ui_controls::ClickTask, button, state, task));
On 2011/10/12 08:20:03, awong wrote:
> Add &?

Done.

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_mac.mm (right):

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:224: MessageLoop::QuitClosure());
On 2011/10/12 08:20:03, awong wrote:
> Are we sure we want to change this to QuitClosure()?

Oops. Done.

http://codereview.chromium.org/8212006/diff/11002/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_mac.mm:306: return
(SendMouseEventsNotifyWhenDone(type, DOWN, NULL) &&
On 2011/10/11 22:20:52, csilv wrote:
> NULL -> base::Closure()

Done.

[awong, 2011-10-12 17:15:33]

LGTM

[csilv, 2011-10-12 20:51:32]

lgtm

[commit-bot: I haz the power, 2011-10-13 02:44:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jhawkins@chromium.org/8212006/23001

[commit-bot: I haz the power, 2011-10-13 02:45:03]

Can't apply patch for file chrome/browser/ui/views/menu_item_view_test.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/views/menu_item_view_test.cc
Hunk #1 FAILED at 2.
Hunk #2 succeeded at 82 (offset 2 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/menu_item_view_test.cc.rej

[commit-bot: I haz the power, 2011-10-16 02:05:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jhawkins@chromium.org/8212006/29001

[commit-bot: I haz the power, 2011-10-16 02:05:17]

Presubmit check for 8212006-29001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: base/message_loop.cc,base/message_loop.h

Presubmit checks took 3.2s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[James Hawkins, 2011-10-16 02:31:27]

+willchan for base/ approval.  I'd git cl dcommit -f, but I can't connect to my
work machine for some reason.

[willchan no longer on Chromium, 2011-10-16 02:51:32]

http://codereview.chromium.org/8212006/diff/29001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/8212006/diff/29001/base/message_loop.h#newcode267
base/message_loop.h:267: return base::Bind(&MessageLoop::Quit,
We're trying to keep bind.h out of header files. Can you move this to the .cc
file? I see that this rule has already been broken, but message_loop.h is pretty
far-reaching, so I think it's worthwhile to keep it out of this header too
unless there's a good reason to keep it here (optimizing this function seems
unnecessary).

[James Hawkins, 2011-10-16 22:00:24]

http://codereview.chromium.org/8212006/diff/29001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/8212006/diff/29001/base/message_loop.h#newcode267
base/message_loop.h:267: return base::Bind(&MessageLoop::Quit,
Oops. Done.

[James Hawkins, 2011-10-17 01:17:52]

Will, I'm going to go ahead and commit.  If you have any further review
comments, I'll follow those up with another CL.

[willchan no longer on Chromium, 2011-10-17 20:29:37]

OK, sounds good. Thanks.

On Sun, Oct 16, 2011 at 6:17 PM, <jhawkins@chromium.org> wrote:

> Will, I'm going to go ahead and commit.  If you have any further review
> comments, I'll follow those up with another CL.
>
>
http://codereview.chromium.**org/8212006/<http://codereview.chromium.org/8212...
>