Modifying extension automation so that it is done through a particular...

Modifying extension automation so that it is done through a particular
tab for all extension views.  Previously, API routing to the
automation client would only work for extension views that were
contained in the particular ExternalTab instance being automated. This
meant you couldn't test API calls from e.g. background pages.

Also using async functions instead of the previous RVH-based hack.

Updating one of the UI tests to make the API calls from a background
page, to provide an end-to-end test of the new routing.

This makes AutomationProvider::SetEnableAutomationExtension
Windows-only, but it effectively always was since it was already
dependent on ExternalTabContainer (indirectly, to provide a non-empty
implementation of TabContentsDelegate::ForwardMessageToExternalHost).

BUG=none
TEST=ui_tests.exe, chrome_frame_tests.exe, chrome_frame_unittests.exe


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

[MAD, 2009-11-05 16:19:49]

Cool stuff...

Small nits and a suggestion to add comments.

BYT
MAD

http://codereview.chromium.org/366025/diff/1/16
File chrome/browser/automation/automation_extension_function.cc (right):

http://codereview.chromium.org/366025/diff/1/16#newcode36
Line 36: "Why is this function still enabled if no target tab?";
2 more spaces indent.

http://codereview.chromium.org/366025/diff/1/16#newcode164
Line 164: if (api_handler_tab_) {
Would be nice to explain why we hold on the tab as opposed to holding on to its
render view host, if all we do with the tab is to get its render view host...
Unless I missed something...

http://codereview.chromium.org/366025/diff/1/12
File chrome/browser/extensions/extension_uitest.cc (right):

http://codereview.chromium.org/366025/diff/1/12#newcode60
Line 60: static_cast<AutomationProxyForExternalTab*>(automation());
2 more spaces indent.

http://codereview.chromium.org/366025/diff/1/12#newcode73
Line 73: static_cast<AutomationProxyForExternalTab*>(automation());
2 more spaces indent.

[Jói, 2009-11-05 16:25:53]

Thanks MAD.

http://codereview.chromium.org/366025/diff/1/16
File chrome/browser/automation/automation_extension_function.cc (right):

http://codereview.chromium.org/366025/diff/1/16#newcode36
Line 36: "Why is this function still enabled if no target tab?";
On 2009/11/05 16:19:49, mad1 wrote:
> 2 more spaces indent.

Done.

http://codereview.chromium.org/366025/diff/1/16#newcode164
Line 164: if (api_handler_tab_) {
On 2009/11/05 16:19:49, mad1 wrote:
> Would be nice to explain why we hold on the tab as opposed to holding on to
its
> render view host, if all we do with the tab is to get its render view host...
> Unless I missed something...

Added comment:

  // We get the RVH from the tab's NavigationController when it's needed
  // rather than storing the RVH, since the RVH will change when the
  // tab is navigated.  Each new RVH will be attached to the automation
  // client attached to the tab.

http://codereview.chromium.org/366025/diff/1/12
File chrome/browser/extensions/extension_uitest.cc (right):

http://codereview.chromium.org/366025/diff/1/12#newcode60
Line 60: static_cast<AutomationProxyForExternalTab*>(automation());
On 2009/11/05 16:19:49, mad1 wrote:
> 2 more spaces indent.

Done.

http://codereview.chromium.org/366025/diff/1/12#newcode73
Line 73: static_cast<AutomationProxyForExternalTab*>(automation());
On 2009/11/05 16:19:49, mad1 wrote:
> 2 more spaces indent.

Done.

[Paweł Hajdan Jr., 2009-11-05 16:40:45]

Drive-by with automation-related comments.

http://codereview.chromium.org/366025/diff/1/15
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/366025/diff/1/15#newcode1946
Line 1946: NOTREACHED();
Not a good idea. Better would be to pass failure return value to the caller.

This is going to crash the browser process (in debug mode) if someone issues a
bad automation call.

http://codereview.chromium.org/366025/diff/1/12
File chrome/browser/extensions/extension_uitest.cc (right):

http://codereview.chromium.org/366025/diff/1/12#newcode84
Line 84: if (tab_) {
How about if (!tab_) return; ... ?

http://codereview.chromium.org/366025/diff/1/7
File chrome/test/automation/automation_messages_internal.h (right):

http://codereview.chromium.org/366025/diff/1/7#newcode911
Line 911: // Tab handle.
Please explain more what exactly the tab handle means (you can adjust the
comment above, probably to say to put a specific tab into extension automation
mode).

http://codereview.chromium.org/366025/diff/1/9
File chrome/test/automation/tab_proxy.h (right):

http://codereview.chromium.org/366025/diff/1/9#newcode94
Line 94: // Enabling extension automation from more than one tab is an error.
Is that checked somewhere?

http://codereview.chromium.org/366025/diff/1/9#newcode96
Line 96: // You must disable extension automation before destroying the tab.
Is that checked somewhere?

[Jói, 2009-11-05 19:06:08]

Hi Pawel,

Thanks for the drive-bys.  The code is not 100% complete, thanks for catching a
couple of points that I will address.

Cheers,
Jói

http://codereview.chromium.org/366025/diff/1/15
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/366025/diff/1/15#newcode1946
Line 1946: NOTREACHED();
On 2009/11/05 16:40:45, Paweł Hajdan Jr. wrote:
> Not a good idea. Better would be to pass failure return value to the caller.
> 
> This is going to crash the browser process (in debug mode) if someone issues a
> bad automation call.

I don't think it would be good to make it a synchronous call so we can pass back
an error, and I'm not sure why it would be a problem to crash the browser in
debug mode (with an explanatory callstack) when the automation interface is used
incorrectly?

I can make the DCHECK a LOG(WARNING) instead if you prefer but I don't want to
make it a synch message just to pass back an error.  Does that make sense?

http://codereview.chromium.org/366025/diff/1/12
File chrome/browser/extensions/extension_uitest.cc (right):

http://codereview.chromium.org/366025/diff/1/12#newcode84
Line 84: if (tab_) {
On 2009/11/05 16:40:45, Paweł Hajdan Jr. wrote:
> How about if (!tab_) return; ... ?

That would be fine too, it's a question of style and I don't think we mandate
one way or another.

Personally, my habit is to avoid early return unless it makes the nesting really
deep, since in some cases (not here) it can lead to programming errors where you
forget cleanup.

http://codereview.chromium.org/366025/diff/1/7
File chrome/test/automation/automation_messages_internal.h (right):

http://codereview.chromium.org/366025/diff/1/7#newcode911
Line 911: // Tab handle.
On 2009/11/05 16:40:45, Paweł Hajdan Jr. wrote:
> Please explain more what exactly the tab handle means (you can adjust the
> comment above, probably to say to put a specific tab into extension automation
> mode).

Good point, comment updated.

http://codereview.chromium.org/366025/diff/1/9
File chrome/test/automation/tab_proxy.h (right):

http://codereview.chromium.org/366025/diff/1/9#newcode94
Line 94: // Enabling extension automation from more than one tab is an error.
On 2009/11/05 16:40:45, Paweł Hajdan Jr. wrote:
> Is that checked somewhere?

Yes, that would DCHECK inside this function.

http://codereview.chromium.org/366025/diff/1/9#newcode96
Line 96: // You must disable extension automation before destroying the tab.
On 2009/11/05 16:40:45, Paweł Hajdan Jr. wrote:
> Is that checked somewhere?

Actually no, not at the moment, thanks for the catch.  I can't find a place
where the AutomationProvider observes tabs going away (even though it has a
CloseTab function, it doesn't remove the tab from its list of handles), and the
tab might be closed by other means.  I'll look into this and either update the
documentation to remove this line (as the AutomationProvider does cleanup on
exit) or try to enforce it somehow better.

[Paweł Hajdan Jr., 2009-11-05 20:01:24]

http://codereview.chromium.org/366025/diff/1/15
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/366025/diff/1/15#newcode1946
Line 1946: NOTREACHED();
On 2009/11/05 19:06:08, Jói wrote:
> I don't think it would be good to make it a synchronous call so we can pass
back
> an error, and I'm not sure why it would be a problem to crash the browser in
> debug mode (with an explanatory callstack) when the automation interface is
used
> incorrectly?

When the call is synchronous, you have higher chance to see helpful output in
the logs (when the failure is flaky). Actually, with a DCHECK "crash" you may or
may not get a stack trace, which may or may not have symbols etc. If an
ASSERT_TRUE(ShouldSucceed()); fails in a test, you have a line number, etc.
which proved to be more helpful.

For calls that are not synchronous... hm, I don't know. However, introducing the
NOTREACHED() pattern here may make someone erroneously use it for synchronous
calls.

> I can make the DCHECK a LOG(WARNING) instead if you prefer but I don't want to
> make it a synch message just to pass back an error.  Does that make sense?

For now I think LOG(WARNING) would be better, but it doesn't look like a perfect
solution. I'll think about something better for the long term (it's beyond the
scope of this patch obviously).

http://codereview.chromium.org/366025/diff/1/12
File chrome/browser/extensions/extension_uitest.cc (right):

http://codereview.chromium.org/366025/diff/1/12#newcode84
Line 84: if (tab_) {
On 2009/11/05 19:06:08, Jói wrote:
> On 2009/11/05 16:40:45, Paweł Hajdan Jr. wrote:
> > How about if (!tab_) return; ... ?
> 
> That would be fine too, it's a question of style and I don't think we mandate
> one way or another.

OK, that's fine.

http://codereview.chromium.org/366025/diff/1/7
File chrome/test/automation/automation_messages_internal.h (right):

http://codereview.chromium.org/366025/diff/1/7#newcode911
Line 911: // Tab handle.
On 2009/11/05 19:06:08, Jói wrote:
> On 2009/11/05 16:40:45, Paweł Hajdan Jr. wrote:
> > Please explain more what exactly the tab handle means (you can adjust the
> > comment above, probably to say to put a specific tab into extension
automation
> > mode).
> 
> Good point, comment updated.

I don't see the update... have you uploaded the new patchset?

http://codereview.chromium.org/366025/diff/1/9
File chrome/test/automation/tab_proxy.h (right):

http://codereview.chromium.org/366025/diff/1/9#newcode94
Line 94: // Enabling extension automation from more than one tab is an error.
On 2009/11/05 19:06:08, Jói wrote:
> On 2009/11/05 16:40:45, Paweł Hajdan Jr. wrote:
> > Is that checked somewhere?
> 
> Yes, that would DCHECK inside this function.

I couldn't find it, but you know this part better so that's OK.

http://codereview.chromium.org/366025/diff/1/9#newcode96
Line 96: // You must disable extension automation before destroying the tab.
On 2009/11/05 19:06:08, Jói wrote:
> On 2009/11/05 16:40:45, Paweł Hajdan Jr. wrote:
> > Is that checked somewhere?
> 
> Actually no, not at the moment, thanks for the catch.  I can't find a place
> where the AutomationProvider observes tabs going away (even though it has a
> CloseTab function, it doesn't remove the tab from its list of handles), and
the
> tab might be closed by other means.  I'll look into this and either update the
> documentation to remove this line (as the AutomationProvider does cleanup on
> exit) or try to enforce it somehow better.

Sounds good, thanks.

[Jói, 2009-11-06 02:40:39]

Pawel, thanks again, your comments led me to a better solution for managing the
lifetime of automation.

http://codereview.chromium.org/366025/diff/1/15
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/366025/diff/1/15#newcode1946
Line 1946: NOTREACHED();
On 2009/11/05 20:01:24, Paweł Hajdan Jr. wrote:
> On 2009/11/05 19:06:08, Jói wrote:
> > I don't think it would be good to make it a synchronous call so we can pass
> back
> > an error, and I'm not sure why it would be a problem to crash the browser in
> > debug mode (with an explanatory callstack) when the automation interface is
> used
> > incorrectly?
> 
> When the call is synchronous, you have higher chance to see helpful output in
> the logs (when the failure is flaky). Actually, with a DCHECK "crash" you may
or
> may not get a stack trace, which may or may not have symbols etc. If an
> ASSERT_TRUE(ShouldSucceed()); fails in a test, you have a line number, etc.
> which proved to be more helpful.
> 
> For calls that are not synchronous... hm, I don't know. However, introducing
the
> NOTREACHED() pattern here may make someone erroneously use it for synchronous
> calls.
> 
> > I can make the DCHECK a LOG(WARNING) instead if you prefer but I don't want
to
> > make it a synch message just to pass back an error.  Does that make sense?
> 
> For now I think LOG(WARNING) would be better, but it doesn't look like a
perfect
> solution. I'll think about something better for the long term (it's beyond the
> scope of this patch obviously).

I've made it a LOG(WARNING).

http://codereview.chromium.org/366025/diff/1/7
File chrome/test/automation/automation_messages_internal.h (right):

http://codereview.chromium.org/366025/diff/1/7#newcode911
Line 911: // Tab handle.
On 2009/11/05 20:01:24, Paweł Hajdan Jr. wrote:
> On 2009/11/05 19:06:08, Jói wrote:
> > On 2009/11/05 16:40:45, Paweł Hajdan Jr. wrote:
> > > Please explain more what exactly the tab handle means (you can adjust the
> > > comment above, probably to say to put a specific tab into extension
> automation
> > > mode).
> > 
> > Good point, comment updated.
> 
> I don't see the update... have you uploaded the new patchset?

Sorry, it's updated now.  I've also updated in AutomationProvider so that we
verify that the tab in question is an external tab, and we use the
ExternalTabContainer in the AutomationExtensionFunction, and to control the
lifetime of automation.  Thanks for your comments, they led me to this better
approach.

Note that the code may still not be final, there are some failing CF tests and
while I don't think they are my fault I intend to find out for sure before I
send for final review.

http://codereview.chromium.org/366025/diff/1/9
File chrome/test/automation/tab_proxy.h (right):

http://codereview.chromium.org/366025/diff/1/9#newcode94
Line 94: // Enabling extension automation from more than one tab is an error.
On 2009/11/05 20:01:24, Paweł Hajdan Jr. wrote:
> On 2009/11/05 19:06:08, Jói wrote:
> > On 2009/11/05 16:40:45, Paweł Hajdan Jr. wrote:
> > > Is that checked somewhere?
> > 
> > Yes, that would DCHECK inside this function.
> 
> I couldn't find it, but you know this part better so that's OK.

The NOTREACHED() on line 74 is the dcheck I was referring to.

http://codereview.chromium.org/366025/diff/1/9#newcode96
Line 96: // You must disable extension automation before destroying the tab.
On 2009/11/05 20:01:24, Paweł Hajdan Jr. wrote:
> On 2009/11/05 19:06:08, Jói wrote:
> > On 2009/11/05 16:40:45, Paweł Hajdan Jr. wrote:
> > > Is that checked somewhere?
> > 
> > Actually no, not at the moment, thanks for the catch.  I can't find a place
> > where the AutomationProvider observes tabs going away (even though it has a
> > CloseTab function, it doesn't remove the tab from its list of handles), and
> the
> > tab might be closed by other means.  I'll look into this and either update
the
> > documentation to remove this line (as the AutomationProvider does cleanup on
> > exit) or try to enforce it somehow better.
> 
> Sounds good, thanks.

This is now fixed, using ExternalTabContainer as the representation of the tab
to channel back through, and using it to control the lifetime of automation.

[Paweł Hajdan Jr., 2009-11-06 06:09:16]

LGTM

[tommi (sloooow) - chröme, 2009-11-06 14:32:51]

Hi Joi,

I just saw the review request.  I'll be on a plane today so won't be able to
send you feedback until tonight.  Hope that's alright.

On Thu, Nov 5, 2009 at 7:03 PM, <joi@chromium.org> wrote:

> http://codereview.chromium.org/366025
>

[Jói, 2009-11-06 16:51:23]

Thanks Tommi, didn't realize you were traveling today. I had a chat with Ananta,
he suggested I send it to him and Amit.

Ananta & Amit, let me know what you think.

Cheers,
Jói


On 2009/11/06 14:32:51, tommi wrote:
> Hi Joi,
> 
> I just saw the review request.  I'll be on a plane today so won't be able to
> send you feedback until tonight.  Hope that's alright.
> 
> On Thu, Nov 5, 2009 at 7:03 PM, <mailto:joi@chromium.org> wrote:
> 
> > http://codereview.chromium.org/366025
> >
>

[ananta, 2009-11-06 21:50:51]

LGTM for the automation changes.

[amit, 2009-11-06 22:34:04]

lgtm

On Fri, Nov 6, 2009 at 1:50 PM, <ananta@chromium.org> wrote:

> LGTM for the automation changes.
>
>
>
> http://codereview.chromium.org/366025
>