Determine the element location and click synchronously on the renderer.

[chromedriver] Determine the element location and click synchronously in the renderer.

This is only the browser-side implementation. In this patch we send some scripts from the client which the renderer evaluates to determine where to move or click the mouse. I tried to keep the renderer code as small and as simple as possible. I plan on eventually removing the script-based location finding in favor of some scrollIntoView-like method in WebKit. Currently scrollIntoView does not work. See http://code.google.com/p/chromium/issues/detail?id=73953.

BUG=chromedriver:22, chromedriver:36, chromedriver:44
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138479

[dennis_jeffrey, 2012-05-08 19:18:48]

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1923:
SendMessage(true, automation::Error());
To make sure I understand correctly: if OnWillProcessMouseEventAt is invoked
(and we haven't yet gotten the mouse event ACK), but then a modal dialog is
shown, we want to return success in this case?

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:5924: }
no need for curly braces

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:6013: if
(!tab_contents) {
Could you add a comment to explain the difference in how we're processing things
depending on whether or not tab_contents exist?  If tab_contents don't exist,
we're forwarding the mouse event, but if tab_contents does exist, we're using
the new MouseEventProcessedObserver.  I think a comment explaining why could be
helpful.

http://codereview.chromium.org/10384023/diff/2002/chrome/common/automation_ev...
File chrome/common/automation_events.h (right):

http://codereview.chromium.org/10384023/diff/2002/chrome/common/automation_ev...
chrome/common/automation_events.h:35: // provides the input for the next.
Would it be helpful to show an example of how a simple sequence of scripts might
look?

http://codereview.chromium.org/10384023/diff/2002/chrome/common/automation_me...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/10384023/diff/2002/chrome/common/automation_me...
chrome/common/automation_messages_internal.h:1527: AutomationMouseEvent)
This will change the line numbers of the entries below.  I was concerned about
the message at lines 12-18 above, but I think it's ok here, since this is a
browser <--> renderer message.  However, just want to bring this to your
attention to double-check, since I'm not sure about this.

http://codereview.chromium.org/10384023/diff/2002/chrome/common/automation_me...
chrome/common/automation_messages_internal.h:1559: 
nit: remove this blank line

http://codereview.chromium.org/10384023/diff/2002/chrome/renderer/automation/...
File chrome/renderer/automation/automation_renderer_helper.cc (right):

http://codereview.chromium.org/10384023/diff/2002/chrome/renderer/automation/...
chrome/renderer/automation/automation_renderer_helper.cc:18: #include
"content/public/renderer/render_view.h"
swap the above 2 lines to keep them in alphabetical order.

[craigdh, 2012-05-08 21:18:40]

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.h:1340: virtual void
Observe(int type,
Why are the three preceding functions private when they are public in the parent
classes?

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.h:1342: const
content::NotificationDetails& details);
Add OVERRIDE.

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.h:1345: void
SendMessage(bool success, const automation::Error& error);
Start private function names with an _.

[craigdh, 2012-05-08 21:21:45]

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.h:1345: void
SendMessage(bool success, const automation::Error& error);
On 2012/05/08 21:18:40, craigdh wrote:
> Start private function names with an _.

nvm.

[kkania, 2012-05-09 18:19:59]

Addressed comments. Also changed from using AutomationTabHelper to using
AutomationMouseEventProcessor, which gets rid of a hack.

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1923:
SendMessage(true, automation::Error());
On 2012/05/08 19:18:49, dennis_jeffrey wrote:
> To make sure I understand correctly: if OnWillProcessMouseEventAt is invoked
> (and we haven't yet gotten the mouse event ACK), but then a modal dialog is
> shown, we want to return success in this case?

Yes

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.h:1340: virtual void
Observe(int type,
On 2012/05/08 21:18:40, craigdh wrote:
> Why are the three preceding functions private when they are public in the
parent
> classes?

The idea is to communicate to the client of this class that these methods should
not be called directly. The method is only called through the appropriate
interface. I don't think there's a hard rule about whether to do this or not,
and browser.h
(http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/browser.h...)
seems to have some of its overrides public and private. I'd be fine moving it to
public.

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.h:1342: const
content::NotificationDetails& details);
On 2012/05/08 21:18:40, craigdh wrote:
> Add OVERRIDE.

Done.

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:5924: }
On 2012/05/08 19:18:49, dennis_jeffrey wrote:
> no need for curly braces

Done.

http://codereview.chromium.org/10384023/diff/2002/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:6013: if
(!tab_contents) {
On 2012/05/08 19:18:49, dennis_jeffrey wrote:
> Could you add a comment to explain the difference in how we're processing
things
> depending on whether or not tab_contents exist?  If tab_contents don't exist,
> we're forwarding the mouse event, but if tab_contents does exist, we're using
> the new MouseEventProcessedObserver.  I think a comment explaining why could
be
> helpful.

Got rid of this hack. I didn't actually need it anyway.

http://codereview.chromium.org/10384023/diff/2002/chrome/common/automation_ev...
File chrome/common/automation_events.h (right):

http://codereview.chromium.org/10384023/diff/2002/chrome/common/automation_ev...
chrome/common/automation_events.h:35: // provides the input for the next.
On 2012/05/08 19:18:49, dennis_jeffrey wrote:
> Would it be helpful to show an example of how a simple sequence of scripts
might
> look?

Done.

http://codereview.chromium.org/10384023/diff/2002/chrome/common/automation_me...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/10384023/diff/2002/chrome/common/automation_me...
chrome/common/automation_messages_internal.h:1527: AutomationMouseEvent)
On 2012/05/08 19:18:49, dennis_jeffrey wrote:
> This will change the line numbers of the entries below.  I was concerned about
> the message at lines 12-18 above, but I think it's ok here, since this is a
> browser <--> renderer message.  However, just want to bring this to your
> attention to double-check, since I'm not sure about this. 

Yes, this is ok.

http://codereview.chromium.org/10384023/diff/2002/chrome/common/automation_me...
chrome/common/automation_messages_internal.h:1559: 
On 2012/05/08 19:18:49, dennis_jeffrey wrote:
> nit: remove this blank line

Done.

http://codereview.chromium.org/10384023/diff/2002/chrome/renderer/automation/...
File chrome/renderer/automation/automation_renderer_helper.cc (right):

http://codereview.chromium.org/10384023/diff/2002/chrome/renderer/automation/...
chrome/renderer/automation/automation_renderer_helper.cc:18: #include
"content/public/renderer/render_view.h"
On 2012/05/08 19:18:49, dennis_jeffrey wrote:
> swap the above 2 lines to keep them in alphabetical order.

Done.

[dennis_jeffrey, 2012-05-09 19:05:36]

LGTM with a couple nits.

http://codereview.chromium.org/10384023/diff/19/chrome/browser/automation/aut...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/10384023/diff/19/chrome/browser/automation/aut...
chrome/browser/automation/automation_provider_observers.cc:1933: }
no need for curly braces

http://codereview.chromium.org/10384023/diff/19/chrome/browser/automation/aut...
chrome/browser/automation/automation_provider_observers.cc:1950: }
no need for curly braces in the if/else

[kkania, 2012-05-09 19:11:50]

http://codereview.chromium.org/10384023/diff/19/chrome/browser/automation/aut...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/10384023/diff/19/chrome/browser/automation/aut...
chrome/browser/automation/automation_provider_observers.cc:1933: }
On 2012/05/09 19:05:36, dennis_jeffrey wrote:
> no need for curly braces

You're supposed to be curly braces around macros, since they could theoretically
expand to multiple lines

http://codereview.chromium.org/10384023/diff/19/chrome/browser/automation/aut...
chrome/browser/automation/automation_provider_observers.cc:1950: }
On 2012/05/09 19:05:36, dennis_jeffrey wrote:
> no need for curly braces in the if/else

Done.

[dennis_jeffrey, 2012-05-09 19:12:49]

http://codereview.chromium.org/10384023/diff/19/chrome/browser/automation/aut...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/10384023/diff/19/chrome/browser/automation/aut...
chrome/browser/automation/automation_provider_observers.cc:1933: }
On 2012/05/09 19:11:50, kkania wrote:
> On 2012/05/09 19:05:36, dennis_jeffrey wrote:
> > no need for curly braces
> 
> You're supposed to be curly braces around macros, since they could
theoretically
> expand to multiple lines

Ah, good point.  Thanks for letting me know!

[kkania, 2012-05-09 21:15:31]

On 2012/05/09 19:12:49, dennis_jeffrey wrote:
>
http://codereview.chromium.org/10384023/diff/19/chrome/browser/automation/aut...
> File chrome/browser/automation/automation_provider_observers.cc (right):
> 
>
http://codereview.chromium.org/10384023/diff/19/chrome/browser/automation/aut...
> chrome/browser/automation/automation_provider_observers.cc:1933: }
> On 2012/05/09 19:11:50, kkania wrote:
> > On 2012/05/09 19:05:36, dennis_jeffrey wrote:
> > > no need for curly braces
> > 
> > You're supposed to be curly braces around macros, since they could
> theoretically
> > expand to multiple lines
> 
> Ah, good point.  Thanks for letting me know!

Alright, just fyi, I'm still waiting on craigdh's approval.

[craigdh, 2012-05-10 15:49:13]

lgtm (sorry, I was OOO yesterday)

[kkania, 2012-05-10 15:54:39]

On 2012/05/10 15:49:13, craigdh wrote:
> lgtm (sorry, I was OOO yesterday)

No worries. Thanks

[commit-bot: I haz the power, 2012-05-17 20:10:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkania@chromium.org/10384023/11007

[commit-bot: I haz the power, 2012-05-17 20:59:21]

Try job failure for 10384023-11007 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-22 15:57:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkania@chromium.org/10384023/22001

[commit-bot: I haz the power, 2012-05-22 17:50:09]

Try job failure for 10384023-22001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-05-23 01:18:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkania@chromium.org/10384023/22001

[commit-bot: I haz the power, 2012-05-23 05:26:41]

Try job failure for 10384023-22001 (retry) on win_rel for step
"sync_unit_tests".
It's a second try, previously, step "sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...