Allow chromedriver to install an extension and get all installed extension IDs.

Allow chromedriver to install an extension and get all installed extension IDs.
Fix install extension hook to only watch for internal extension install.
BUG=103033, 93571
TEST=none


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

[kkania, 2011-11-22 22:11:50]

This is the start of adding extension support to webdriver, so that
extension/app authors can use webdriver for testing.

hnguyen: chrome/test/webdriver/* and gypi
dennisjeffrey: all else

[dennis_jeffrey, 2011-11-22 23:32:16]

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.cc:626: case
chrome::NOTIFICATION_EXTENSION_LOADED: {
Is there a reason this block is enclosed in braces, whereas the block above is
not?

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.h:340:
ExtensionReadyNotificationObserver(ExtensionProcessManager* manager,
Do we still need this old version of the function? Can we get rid of it?

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.h:365: bool use_json_;
nit: Should we move this down below the definition of reply_message_ below, to
separate the variables for constructor arguments from the other class variables?

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

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:2486: //
InstallExtension() present in pyauto.py.
Maybe remove this comment or update it, since InstallExtension is now in the
current file.

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4311: if
(extension_path.MatchesExtension(FILE_PATH_LITERAL(".crx"))) {
Please add a comment to describe what's happening in the "if" versus the "else"
part of this selection structure.

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4325:
Send(reply_message);
What's the reason you're replying this way as compared to using
AutomationJSONReply like at line 4296 above?

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

http://codereview.chromium.org/8649004/diff/2001/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:1724: Otherwise, throws an exception. The
extension must not already be installed.
Mention the possibility of an exception in the "Raises:" field of this
docstring.

http://codereview.chromium.org/8649004/diff/2001/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:1731: The ID of the extension.
'of the installed extension'

http://codereview.chromium.org/8649004/diff/2001/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:1744: Example:
Maybe we could modify this example now that there are some additional fields
returned by the automation hook, such as "is_component_extension" and
"is_internal_extension"

http://codereview.chromium.org/8649004/diff/2001/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:2433: If the install is unsuccessful, will throw
an exception.
Mention this in the "Raises:" section of this docstring

http://codereview.chromium.org/8649004/diff/2001/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:3127: def InstallApp(self, app_crx_file_path):
I think we should get rid of this function now (and also UninstallApp below) -
we'll need to change the places where it's used (it's used in at least ntp.py).

http://codereview.chromium.org/8649004/diff/2001/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/chrome_commands.cc (right):

http://codereview.chromium.org/8649004/diff/2001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/chrome_commands.cc:41: for (size_t i = 0; i <
extension_ids.size(); ++i) {
nit: can remove the curly braces here

http://codereview.chromium.org/8649004/diff/2001/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/chrome_commands.h (right):

http://codereview.chromium.org/8649004/diff/2001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/chrome_commands.h:14: class DictionaryValue;
nit: maybe add a blank line above and below this?

[Huyen, 2011-11-23 00:53:26]

lgtm for the webdriver and gypi

[kkania, 2011-11-23 17:31:23]

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.cc:626: case
chrome::NOTIFICATION_EXTENSION_LOADED: {
On 2011/11/22 23:32:16, dennis_jeffrey wrote:
> Is there a reason this block is enclosed in braces, whereas the block above is
> not?

I need the brace because I am defining a new var in this scope

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.h:340:
ExtensionReadyNotificationObserver(ExtensionProcessManager* manager,
On 2011/11/22 23:32:16, dennis_jeffrey wrote:
> Do we still need this old version of the function? Can we get rid of it?

Yes, there is an install extension func in automation_provider that uses it.
That func itself is used by a ui test and an old version of chromedriver.

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.h:365: bool use_json_;
On 2011/11/22 23:32:16, dennis_jeffrey wrote:
> nit: Should we move this down below the definition of reply_message_ below, to
> separate the variables for constructor arguments from the other class
variables?

Done.

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

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:2486: //
InstallExtension() present in pyauto.py.
On 2011/11/22 23:32:16, dennis_jeffrey wrote:
> Maybe remove this comment or update it, since InstallExtension is now in the
> current file.

Done.

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4311: if
(extension_path.MatchesExtension(FILE_PATH_LITERAL(".crx"))) {
On 2011/11/22 23:32:16, dennis_jeffrey wrote:
> Please add a comment to describe what's happening in the "if" versus the
"else"
> part of this selection structure.

Done.

http://codereview.chromium.org/8649004/diff/2001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4325:
Send(reply_message);
On 2011/11/22 23:32:16, dennis_jeffrey wrote:
> What's the reason you're replying this way as compared to using
> AutomationJSONReply like at line 4296 above?

oops, meant to delete that...

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

http://codereview.chromium.org/8649004/diff/2001/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:1724: Otherwise, throws an exception. The
extension must not already be installed.
On 2011/11/22 23:32:16, dennis_jeffrey wrote:
> Mention the possibility of an exception in the "Raises:" field of this
> docstring.

Done.

http://codereview.chromium.org/8649004/diff/2001/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:1731: The ID of the extension.
On 2011/11/22 23:32:16, dennis_jeffrey wrote:
> 'of the installed extension'

Done.

http://codereview.chromium.org/8649004/diff/2001/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:1744: Example:
On 2011/11/22 23:32:16, dennis_jeffrey wrote:
> Maybe we could modify this example now that there are some additional fields
> returned by the automation hook, such as "is_component_extension" and
> "is_internal_extension"

Done.

http://codereview.chromium.org/8649004/diff/2001/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:2433: If the install is unsuccessful, will throw
an exception.
On 2011/11/22 23:32:16, dennis_jeffrey wrote:
> Mention this in the "Raises:" section of this docstring

Done.

http://codereview.chromium.org/8649004/diff/2001/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:3127: def InstallApp(self, app_crx_file_path):
On 2011/11/22 23:32:16, dennis_jeffrey wrote:
> I think we should get rid of this function now (and also UninstallApp below) -
> we'll need to change the places where it's used (it's used in at least
ntp.py).

Done.

http://codereview.chromium.org/8649004/diff/2001/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/chrome_commands.cc (right):

http://codereview.chromium.org/8649004/diff/2001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/chrome_commands.cc:41: for (size_t i = 0; i <
extension_ids.size(); ++i) {
On 2011/11/22 23:32:16, dennis_jeffrey wrote:
> nit: can remove the curly braces here

Done.

http://codereview.chromium.org/8649004/diff/2001/chrome/test/webdriver/comman...
File chrome/test/webdriver/commands/chrome_commands.h (right):

http://codereview.chromium.org/8649004/diff/2001/chrome/test/webdriver/comman...
chrome/test/webdriver/commands/chrome_commands.h:14: class DictionaryValue;
On 2011/11/22 23:32:16, dennis_jeffrey wrote:
> nit: maybe add a blank line above and below this?

i think this is the style for forward declaring in a namespace.  I don't think
its in the style guide, but I haven't seen it done otherwise in chrome.

[dennis_jeffrey, 2011-11-23 18:04:48]

LGTM, just 1 fix to make before submitting (caused by changes in the latest
patch set).

http://codereview.chromium.org/8649004/diff/12001/chrome/test/functional/chro...
File chrome/test/functional/chromeos_security.py (right):

http://codereview.chromium.org/8649004/diff/12001/chrome/test/functional/chro...
chrome/test/functional/chromeos_security.py:157: ext['is_component_extension']]
'is_component_extension' --> 'is_component'

[commit-bot: I haz the power, 2011-11-29 11:07:24]

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

[commit-bot: I haz the power, 2011-11-29 11:07:37]

Presubmit check for 8649004-20005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
The old callback system is deprecated. If possible, use base::Bind and
base::Callback instead.
    chrome/test/webdriver/webdriver_session.cc:1013
    chrome/test/webdriver/webdriver_session.cc:1024

Presubmit checks took 3.4s to calculate.

[commit-bot: I haz the power, 2011-11-29 12:07:08]

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

[commit-bot: I haz the power, 2011-11-29 13:27:32]

Change committed as 111927

[Avi (use Gerrit), 2011-11-29 17:00:14]

http://codereview.chromium.org/8649004/diff/26001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/chrome_commands.h (right):

http://codereview.chromium.org/8649004/diff/26001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/chrome_commands.h:31: virtual void
ExecutePost(Response* const response);
In future CLs, please annotate overriding functions with OVERRIDE. (I'll fix
this one.)