Add PyAuto tests for triggering browser/page action.

Add PyAuto tests for triggering browser/page action.

BUG=chromium:104530
TEST=PyAuto tests

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

[dennis_jeffrey, 2011-11-17 00:11:31]

Is it possible to package up the two new extensions here into two .crx files,
and check those in instead?  The pyauto test can directly install the extension
using the path to the .crx file.

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

http://codereview.chromium.org/8587004/diff/2002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4496: int tab_id =
ExtensionTabUtil::GetTabId(browser->GetSelectedTabContents());
The hook assumes that the page action should be triggered for the
currently-selected tab.  We should either add a comment in pyauto.py to make
that requirement clear, or else modify the hook to take the tab_index of the tab
for which we want to trigger the page action.

http://codereview.chromium.org/8587004/diff/2002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4498:
browser->profile(), id, "action", tab_id, "", 1);
Is there anyone you know who is an expert on the extension code and can review
the use of it here?  I'm probably not the best one to review this part of the
code.

http://codereview.chromium.org/8587004/diff/2002/chrome/test/data/extensions/...
File chrome/test/data/extensions/trigger_actions/browser_action/background.html
(right):

http://codereview.chromium.org/8587004/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/trigger_actions/browser_action/background.html:7:
null, {code:"document.body.style.backgroundColor='red'"});
nit: space right after ':'

http://codereview.chromium.org/8587004/diff/2002/chrome/test/data/extensions/...
File chrome/test/data/extensions/trigger_actions/page_action/background.html
(right):

http://codereview.chromium.org/8587004/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/trigger_actions/page_action/background.html:16:
null, {code:"document.body.style.backgroundColor='red'"});
nit: add space right after the ':'

http://codereview.chromium.org/8587004/diff/2002/chrome/test/functional/exten...
File chrome/test/functional/extensions.py (right):

http://codereview.chromium.org/8587004/diff/2002/chrome/test/functional/exten...
chrome/test/functional/extensions.py:201: expect_retval='red'))
add a second parameter to the assert:

msg='Browser action was not triggered.'

http://codereview.chromium.org/8587004/diff/2002/chrome/test/functional/exten...
chrome/test/functional/extensions.py:211: # Page action icon is displayed when a
tab is created.
Is it required that the page action icon be displayed in a created tab before we
trigger it via the automation hook?  If so, we should probably add a comment to
pyauto.py to make that clear for people who use the hook in the future.

http://codereview.chromium.org/8587004/diff/2002/chrome/test/functional/exten...
chrome/test/functional/extensions.py:219: lambda:
self.GetDOMValue('document.body.style.backgroundColor', 1),
tab_index=1

http://codereview.chromium.org/8587004/diff/2002/chrome/test/functional/exten...
chrome/test/functional/extensions.py:220: expect_retval='red'))
add a second parameter to the assert:

msg='Page action was not triggered.'

http://codereview.chromium.org/8587004/diff/2002/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/8587004/diff/2002/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:1797: id: The string id of the extension.
document the "windex" variable here too

[frankf, 2011-11-17 00:25:03]

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

http://codereview.chromium.org/8587004/diff/2002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4496: int tab_id =
ExtensionTabUtil::GetTabId(browser->GetSelectedTabContents());
It is mentioned in pyauto.py :)

http://codereview.chromium.org/8587004/diff/2002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4498:
browser->profile(), id, "action", tab_id, "", 1);
On 2011/11/17 00:11:32, dennis_jeffrey wrote:
> Is there anyone you know who is an expert on the extension code and can review
> the use of it here?  I'm probably not the best one to review this part of the
> code.

Done.

http://codereview.chromium.org/8587004/diff/2002/chrome/test/data/extensions/...
File chrome/test/data/extensions/trigger_actions/browser_action/background.html
(right):

http://codereview.chromium.org/8587004/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/trigger_actions/browser_action/background.html:7:
null, {code:"document.body.style.backgroundColor='red'"});
On 2011/11/17 00:11:32, dennis_jeffrey wrote:
> nit: space right after ':'

Done.

http://codereview.chromium.org/8587004/diff/2002/chrome/test/data/extensions/...
File chrome/test/data/extensions/trigger_actions/page_action/background.html
(right):

http://codereview.chromium.org/8587004/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/trigger_actions/page_action/background.html:16:
null, {code:"document.body.style.backgroundColor='red'"});
On 2011/11/17 00:11:32, dennis_jeffrey wrote:
> nit: add space right after the ':'

Done.

http://codereview.chromium.org/8587004/diff/2002/chrome/test/functional/exten...
File chrome/test/functional/extensions.py (right):

http://codereview.chromium.org/8587004/diff/2002/chrome/test/functional/exten...
chrome/test/functional/extensions.py:201: expect_retval='red'))
On 2011/11/17 00:11:32, dennis_jeffrey wrote:
> add a second parameter to the assert:
> 
> msg='Browser action was not triggered.'

Done.

http://codereview.chromium.org/8587004/diff/2002/chrome/test/functional/exten...
chrome/test/functional/extensions.py:211: # Page action icon is displayed when a
tab is created.
Yes, done.

http://codereview.chromium.org/8587004/diff/2002/chrome/test/functional/exten...
chrome/test/functional/extensions.py:219: lambda:
self.GetDOMValue('document.body.style.backgroundColor', 1),
On 2011/11/17 00:11:32, dennis_jeffrey wrote:
> tab_index=1

Done.

http://codereview.chromium.org/8587004/diff/2002/chrome/test/functional/exten...
chrome/test/functional/extensions.py:220: expect_retval='red'))
On 2011/11/17 00:11:32, dennis_jeffrey wrote:
> add a second parameter to the assert:
> 
> msg='Page action was not triggered.'

Done.

http://codereview.chromium.org/8587004/diff/2002/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/8587004/diff/2002/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:1797: id: The string id of the extension.
On 2011/11/17 00:11:32, dennis_jeffrey wrote:
> document the "windex" variable here too

Done.

[frankf, 2011-11-17 00:26:43]

@aa, Could you take a look at how extension actions are triggered in
testing_automation_provider.cc?

[dennis_jeffrey, 2011-11-17 00:53:17]

LGTM - just a couple nits and one optional consideration.

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

http://codereview.chromium.org/8587004/diff/2002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4496: int tab_id =
ExtensionTabUtil::GetTabId(browser->GetSelectedTabContents());
On 2011/11/17 00:25:03, frankf wrote:
> It is mentioned in pyauto.py :)

Oops, you're right - I missed it.

http://codereview.chromium.org/8587004/diff/7002/chrome/test/data/extensions/...
File chrome/test/data/extensions/trigger_actions/browser_action/background.html
(right):

http://codereview.chromium.org/8587004/diff/7002/chrome/test/data/extensions/...
chrome/test/data/extensions/trigger_actions/browser_action/background.html:1:
<html>
(optional) Do you think it would be worthwhile to package up the 2 new
extensions in this CL into 2 .crx files, then check those in instead?  The
pyauto tests can directly install extensions from the .crx files.

http://codereview.chromium.org/8587004/diff/7002/chrome/test/functional/exten...
File chrome/test/functional/extensions.py (right):

http://codereview.chromium.org/8587004/diff/7002/chrome/test/functional/exten...
chrome/test/functional/extensions.py:223: msg='Browser action was not
triggered.')
'Browser' --> 'Page'

http://codereview.chromium.org/8587004/diff/7002/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/8587004/diff/7002/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:1796: If the extension has a page action, the
page action icon must be displayed.
nit: ...the page action icon must be displayed before invoking this function.

[kkania, 2011-11-17 17:49:09]

http://codereview.chromium.org/8587004/diff/7002/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/8587004/diff/7002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4464: Browser* browser,
could you use the new non-browser style for this and pass in a windex (see
GetBrowserFromJSONArgs) instead of passing in a browser handle. That would make
it more clean to use in chromedriver.

http://codereview.chromium.org/8587004/diff/7002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4497:
service->browser_event_router()->PageActionExecuted(
I think you should throw an error if the page action is not visible for the
current page.
Optionally, it might be nice to have a func that checks whether the page action
is displayed.

http://codereview.chromium.org/8587004/diff/7002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4501:
service->browser_event_router()->BrowserActionExecuted(
this doesn't work for popups

[frankf, 2011-11-21 02:01:46]

Please review this CL, major changes:

1. Triggering browser/page action through events doesn't work with popups so
we're emulating clicking through the UI layer
2. Added tests for extension popups

http://codereview.chromium.org/8587004/diff/7002/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/8587004/diff/7002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4464: Browser* browser,
On 2011/11/17 17:49:09, kkania wrote:
> could you use the new non-browser style for this and pass in a windex (see
> GetBrowserFromJSONArgs) instead of passing in a browser handle. That would
make
> it more clean to use in chromedriver.

Done.

http://codereview.chromium.org/8587004/diff/7002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4497:
service->browser_event_router()->PageActionExecuted(
On 2011/11/17 17:49:09, kkania wrote:
> I think you should throw an error if the page action is not visible for the
> current page.
> Optionally, it might be nice to have a func that checks whether the page
action
> is displayed.

Done.

http://codereview.chromium.org/8587004/diff/7002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4501:
service->browser_event_router()->BrowserActionExecuted(
On 2011/11/17 17:49:09, kkania wrote:
> this doesn't work for popups

Done.

http://codereview.chromium.org/8587004/diff/7002/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/8587004/diff/7002/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:1796: If the extension has a page action, the
page action icon must be displayed.
On 2011/11/17 00:53:17, dennis_jeffrey wrote:
> nit: ...the page action icon must be displayed before invoking this function.

Done.

[kkania, 2011-11-21 18:53:58]

http://codereview.chromium.org/8587004/diff/9012/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/8587004/diff/9012/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:2334:
handler_map["TriggerPageActionById"] =
thanks!

http://codereview.chromium.org/8587004/diff/9012/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4491:
reply.SendError(base::StringPrintf("Extension does not exist: %s.",
how about 'Extension %s is not installed'?

http://codereview.chromium.org/8587004/diff/9012/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4532: std::string id;
Maybe move this and below to GetEnabledExtensionFromJSONArgs(args, &extension,
&error)? (next to GetBrowserFromJSONArgs)

http://codereview.chromium.org/8587004/diff/9012/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8587004/diff/9012/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:921: 'browser/extensions/browser_action_test_util.h',
if these files are going to be built with the browser, you need to remove them
from browser_tests or you'll get re-definition errors (I think)

http://codereview.chromium.org/8587004/diff/9012/chrome/test/functional/exten...
File chrome/test/functional/extensions.py (right):

http://codereview.chromium.org/8587004/diff/9012/chrome/test/functional/exten...
chrome/test/functional/extensions.py:228: # Page action icon is displayed when a
tab is created.
I think we need another function for GetVisiblePageActions that the user can
wait upon. The page action icon is not guaranteed to appear on tab creation or
by onload, since the hiding/showing is handled by the extension in whatever way
it wants.

[frankf, 2011-11-22 22:26:10]

http://codereview.chromium.org/8587004/diff/9012/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/8587004/diff/9012/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4491:
reply.SendError(base::StringPrintf("Extension does not exist: %s.",
On 2011/11/21 18:53:58, kkania wrote:
> how about 'Extension %s is not installed'?

Done.

http://codereview.chromium.org/8587004/diff/9012/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4532: std::string id;
Have to revisit in a different CL for refactoring all such calls.

On 2011/11/21 18:53:58, kkania wrote:
> Maybe move this and below to GetEnabledExtensionFromJSONArgs(args, &extension,
> &error)? (next to GetBrowserFromJSONArgs)

http://codereview.chromium.org/8587004/diff/9012/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8587004/diff/9012/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:921: 'browser/extensions/browser_action_test_util.h',
I don't think this is the case. When you link with a static library, you only
pull in undefined references. Try bots didn't complain.

On 2011/11/21 18:53:58, kkania wrote:
> if these files are going to be built with the browser, you need to remove them
> from browser_tests or you'll get re-definition errors (I think)

http://codereview.chromium.org/8587004/diff/9012/chrome/test/functional/exten...
File chrome/test/functional/extensions.py (right):

http://codereview.chromium.org/8587004/diff/9012/chrome/test/functional/exten...
chrome/test/functional/extensions.py:228: # Page action icon is displayed when a
tab is created.
I've added this to GetBrowserInfo, same thing should be done for browser actions
once we have a way to tie browser actions to extensions across platforms.

[Aaron Boodman, 2011-11-23 00:24:09]

On 2011/11/17 00:26:43, frankf wrote:
> @aa, Could you take a look at how extension actions are triggered in
> testing_automation_provider.cc?

Seems ok.

[Aaron Boodman, 2011-11-23 00:24:50]

On Wed, Nov 16, 2011 at 4:11 PM,  <dennisjeffrey@chromium.org> wrote:
> Is it possible to package up the two new extensions here into two .crx
> files,
> and check those in instead?  The pyauto test can directly install the
> extension
> using the path to the .crx file.

It's not generally a good idea to do this because then it is hard to
edit them subsequently. Better to check in the source files and create
a CRX on the fly (less efficient in machine time, but more efficient
in developer time).

- a

[dennis_jeffrey, 2011-11-23 00:38:38]

On 2011/11/23 00:24:50, Aaron Boodman wrote:
> On Wed, Nov 16, 2011 at 4:11 PM,  <mailto:dennisjeffrey@chromium.org> wrote:
> > Is it possible to package up the two new extensions here into two .crx
> > files,
> > and check those in instead? &nbsp;The pyauto test can directly install the
> > extension
> > using the path to the .crx file.
> 
> It's not generally a good idea to do this because then it is hard to
> edit them subsequently. Better to check in the source files and create
> a CRX on the fly (less efficient in machine time, but more efficient
> in developer time).
> 
> - a

Makes sense - and from pyauto there's no need to even make the CRX on the fly. 
Extensions can be installed via either a packaged CRX or from a source
directory.

[dennis_jeffrey, 2011-11-23 21:54:54]

LGTM after looking over the most recent changes.  Just a handful of minor
comments.

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

http://codereview.chromium.org/8587004/diff/19001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2818: for (size_t i =
0; i < page_action_visible_count; i++) {
nit: other chrome code generally uses '++i' in the loop control variable update,
instead of 'i++' (I believe it's for efficiency reasons, but I don't understand
why yet).

http://codereview.chromium.org/8587004/diff/19001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4517: for (size_t i =
0; i < page_action_visible_count; i++) {
nit: '++i' instead of 'i++'

http://codereview.chromium.org/8587004/diff/19001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4571: //
BrowserActionTestUtil
nit: period at end of sentence

http://codereview.chromium.org/8587004/diff/19001/chrome/test/functional/exte...
File chrome/test/functional/extensions.py (right):

http://codereview.chromium.org/8587004/diff/19001/chrome/test/functional/exte...
chrome/test/functional/extensions.py:190: ext_id =
self.InstallExtension(dir_path, False)
I think these calls to InstallExtension() will have to be modified slightly once
the following CL is submitted (which will be soon):
http://codereview.chromium.org/8649004/

http://codereview.chromium.org/8587004/diff/19001/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/8587004/diff/19001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:1859: EXTENSION_DIALOG]
Should these be strings?

[kkania, 2011-11-29 19:25:03]

LGTM

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

http://codereview.chromium.org/8587004/diff/19001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4568:
BrowserActionTestUtil browser_actions(browser);
fix indent

http://codereview.chromium.org/8587004/diff/19001/chrome/browser/extensions/b...
File chrome/browser/extensions/browser_action_test_util_views.cc (right):

http://codereview.chromium.org/8587004/diff/19001/chrome/browser/extensions/b...
chrome/browser/extensions/browser_action_test_util_views.cc:41: #ifdef UNIT_TEST
add in header too