Adding a LaunchApp command to Chromedriver

Adding a LaunchApp command to Chromedriver using the Chrome management API in the background extension, this will give developers an improved way to launch apps over having to navigate to the chrome://apps page and click on the app tile.

BUG=

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

[chrisgao (Use stgao instead), 2014-01-07 23:36:35]

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/automation_extension.h (right):

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/automation_extension.h:30: // Launches an app
with the specified id
nit: comment ends with a dot.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/client/chromedriver.py (right):

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/client/chromedriver.py:173:
self.ExecuteCommand(Command.LAUNCH_APP, {'id': app_id})
If you do not plan to add an end to end test in
chrome/test/chromedriver/test/run_py_tests.py, you may revert
chrome/test/chromedriver/client/chromedriver.py and
chrome/test/chromedriver/client/command_executor.py. Because the python client
under chrome/test/chromedriver/client is for chromedriver developer only and not
for public users.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/client/chromedriver.py:174: 
And what's your plan for the change on the Java client side in the opensource
selenium project?

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/extension/background.js (right):

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/extension/background.js:78: * @param {Object} Id The ID
of the app to launch
string instead of an Object?
nit: comment ends with a dot.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/extension/background.js:79: * @param {function()}
callback Invoked when the updating is complete.
It seems the comment is not correct.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/server/http_handler.cc (right):

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/server/http_handler.cc:122:
"session/:sessionId/launch",
As this is a chrome-specific command, we'd better put it after a browser-vendor
name like this session/:sessionId/chromium/launch_app

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/server/http_handler.cc:123: WrapToCommand("Navigate",
base::Bind(&ExecuteLaunchApp))),
I prefer "LaunchApp" as it is more accurate than "Navigate".

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/window_commands.cc:250: Status ExecuteLaunchApp(
Is the app always opened in the current target window (a tab in chrome) ?
If not, do we want to switch to the window of the app by default?

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/window_commands.cc:261:
session->chrome->GetAsDesktop()->GetAutomationExtension(&extension);
We may need to test whether we are on desktop or android like
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/chrome....
If not, return an error saying LaunchApp unsupported?

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/window_commands.cc:263: return
extension->LaunchApp(id);
We may also need to check the returned status from GetAutomationExtension above,
as there might be some error already and |extension| could be NULL in that case.

[bustamante, 2014-01-13 21:19:29]

Thanks for taking a look!

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/automation_extension.h (right):

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/automation_extension.h:30: // Launches an app
with the specified id
On 2014/01/07 23:36:36, chrisgao wrote:
> nit: comment ends with a dot.

Done.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/client/chromedriver.py (right):

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/client/chromedriver.py:173:
self.ExecuteCommand(Command.LAUNCH_APP, {'id': app_id})
On 2014/01/07 23:36:36, chrisgao wrote:
> If you do not plan to add an end to end test in
> chrome/test/chromedriver/test/run_py_tests.py, you may revert
> chrome/test/chromedriver/client/chromedriver.py and
> chrome/test/chromedriver/client/command_executor.py. Because the python client
> under chrome/test/chromedriver/client is for chromedriver developer only and
not
> for public users.

Ok, Done.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/client/chromedriver.py:174: 
On 2014/01/07 23:36:36, chrisgao wrote:
> And what's your plan for the change on the Java client side in the opensource
> selenium project?

It looks like org.openqa.selenium.chrome.ChromeDriver would be the place to go. 
I wanted to get this in first, and then tackle that.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/extension/background.js (right):

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/extension/background.js:78: * @param {Object} Id The ID
of the app to launch
On 2014/01/07 23:36:36, chrisgao wrote:
> string instead of an Object?
> nit: comment ends with a dot.

Done.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/extension/background.js:79: * @param {function()}
callback Invoked when the updating is complete.
On 2014/01/07 23:36:36, chrisgao wrote:
> It seems the comment is not correct.

Yeah copy/paste error, Done.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/server/http_handler.cc (right):

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/server/http_handler.cc:122:
"session/:sessionId/launch",
On 2014/01/07 23:36:36, chrisgao wrote:
> As this is a chrome-specific command, we'd better put it after a
browser-vendor
> name like this session/:sessionId/chromium/launch_app

Sounds good, Done.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/server/http_handler.cc:123: WrapToCommand("Navigate",
base::Bind(&ExecuteLaunchApp))),
On 2014/01/07 23:36:36, chrisgao wrote:
> I prefer "LaunchApp" as it is more accurate than "Navigate".

Done.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/window_commands.cc:250: Status ExecuteLaunchApp(
On 2014/01/07 23:36:36, chrisgao wrote:
> Is the app always opened in the current target window (a tab in chrome) ?
> If not, do we want to switch to the window of the app by default?

Depending on the app type there's different behaviors, v1 packaged/hosted apps
will open in a new window.  Packaged apps will have the id in the url, while
hosted (bookmark) app will open like a normal window created by the user.

v2 apps will just received an "OnLaunched" event and decide what to do from
there.  That usually will involve creating a new window, but they aren't
required too.

I added code that will switch the active window to the last one opened, which
works for each app type.  There's a couple caveats that if a new window is
simultaneously created independent of chromedriver it may get focus, and if the
app doesn't create a window when launching this will return an unrelated window.
 But those seem like rare use cases.

What do you think?

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/window_commands.cc:261:
session->chrome->GetAsDesktop()->GetAutomationExtension(&extension);
On 2014/01/07 23:36:36, chrisgao wrote:
> We may need to test whether we are on desktop or android like
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/chrome....
> If not, return an error saying LaunchApp unsupported?

That's a great point, Done.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/window_commands.cc:263: return
extension->LaunchApp(id);
On 2014/01/07 23:36:36, chrisgao wrote:
> We may also need to check the returned status from GetAutomationExtension
above,
> as there might be some error already and |extension| could be NULL in that
case.

Done.

[chrisgao (Use stgao instead), 2014-01-15 21:39:55]

I suggest filing a bug on the issue tracker
(https://code.google.com/p/chromedriver/issues/list) with pri=0 and mark it as
"ToBeReleased" once this CL is committed. In that way, users could know about
this feature in the release note, although they could use it after the Java
client supports the new command.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/client/chromedriver.py (right):

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/client/chromedriver.py:174: 
On 2014/01/13 21:19:29, bustamante wrote:
> On 2014/01/07 23:36:36, chrisgao wrote:
> > And what's your plan for the change on the Java client side in the
opensource
> > selenium project?
> 
> It looks like org.openqa.selenium.chrome.ChromeDriver would be the place to
go. 
> I wanted to get this in first, and then tackle that.

Sounds good, thanks.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/window_commands.cc:250: Status ExecuteLaunchApp(
On 2014/01/13 21:19:29, bustamante wrote:
> On 2014/01/07 23:36:36, chrisgao wrote:
> > Is the app always opened in the current target window (a tab in chrome) ?
> > If not, do we want to switch to the window of the app by default?
> 
> Depending on the app type there's different behaviors, v1 packaged/hosted apps
> will open in a new window.  Packaged apps will have the id in the url, while
> hosted (bookmark) app will open like a normal window created by the user.
> 
> v2 apps will just received an "OnLaunched" event and decide what to do from
> there.  That usually will involve creating a new window, but they aren't
> required too.
> 
> I added code that will switch the active window to the last one opened, which
> works for each app type.  There's a couple caveats that if a new window is
> simultaneously created independent of chromedriver it may get focus, and if
the
> app doesn't create a window when launching this will return an unrelated
window.
>  But those seem like rare use cases.
> 
> What do you think?

Sorry, I might mislead you on this.

After looking into the code, I found session->chrome->GetWebViewIds is not
reliable to return the list of opened windows in order, especially when two or
more windows are opened between two calls of session->chrome->GetWebViewIds.
This method could only do a best guess. And the underlying devtools command
returns the list of windows ordered by "last activity time" which is defined by
the time when the window is shown or selected. If the user tries to debug the
test and interact with the opened windows, then session->chrome->GetWebViewIds
won't live up to its API document. I will update the comment of this method in
chrome.h.

And after thinking it more thoroughly, I feel that it might be better to leave
the switchTo app window to the users.

First, we could not reliably tell which window is the last one opened (we assume
it is the app window) while the user could. The user could do a diff of two
calls to command driver.window_handles (rely on session->chrome->GetWebViewIds)
if there is only one window opened. If two or more windows are opened, the user
could still make it by querying the url or title of the window, or checking some
text in the page of that window.
I think this also applies to app window.

Second, it might be better to deal with an app window the same way as normal web
page window for consistency. For normal web page window (it could be opened due
to a click or javascript), we leave the switchTo to the user. For app, when it's
launched, although I believe almost 100% the user is going to test and interact
with it, the user might still do the switchTo in the same way as normal web page
window (they get used to that).

Third, some apps will be opened in the current window/tab while others (and most
of them) are in a new one. If we try our best to do the switchTo, we might end
in the wrong window. If the user handles that corner case in their code, this
might lead to two code paths for different types or implementation of apps.

What do you think?

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
File chrome/test/chromedriver/extension/background.js (right):

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
chrome/test/chromedriver/extension/background.js:83: function launchApp(Id,
callback, errCallback) {
whoops, nit: |Id| -> |id|?

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
File chrome/test/chromedriver/server/http_handler.cc (right):

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
chrome/test/chromedriver/server/http_handler.cc:122:
"session/:sessionId/chromium/launch",
how about "launch" -> "launch_app"?
It'll also match the name of the command below.

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
chrome/test/chromedriver/window_commands.cc:279:
session->chrome->ActivateWebView(web_view_ids.back());
The activation might fail. maybe need to check the returned status code.

btw, maybe we don't have to active the app window, because clicking, typing and
others will still work on the app window even it is not activated and focused.

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
File chrome/test/chromedriver/window_commands.h (right):

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
chrome/test/chromedriver/window_commands.h:43: Status ExecuteLaunchApp(
window_commands.cc/h are for commands that require a target window, like
navigating to a new url in the current target window, finding some element in
the page. If the target window was closed, we will return an error to the client
side. That's why the |WebView* web_view| is passed in and it won't be NULL.

session_commands.cc/h are for commands that require a session only. As long as
the session is not deleted, the command could be run.

"LaunchApp" seems to fall into the category of session_commands.cc/h.
Maybe we'd better move to those two files.

[bustamante, 2014-01-15 23:51:38]

Thanks!  I addressed the comments, and I'll create an issue once this is
committed.

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/114403005/diff/70001/chrome/test/chromedriver...
chrome/test/chromedriver/window_commands.cc:250: Status ExecuteLaunchApp(
On 2014/01/15 21:39:56, chrisgao wrote:
> On 2014/01/13 21:19:29, bustamante wrote:
> > On 2014/01/07 23:36:36, chrisgao wrote:
> > > Is the app always opened in the current target window (a tab in chrome) ?
> > > If not, do we want to switch to the window of the app by default?
> > 
> > Depending on the app type there's different behaviors, v1 packaged/hosted
apps
> > will open in a new window.  Packaged apps will have the id in the url, while
> > hosted (bookmark) app will open like a normal window created by the user.
> > 
> > v2 apps will just received an "OnLaunched" event and decide what to do from
> > there.  That usually will involve creating a new window, but they aren't
> > required too.
> > 
> > I added code that will switch the active window to the last one opened,
which
> > works for each app type.  There's a couple caveats that if a new window is
> > simultaneously created independent of chromedriver it may get focus, and if
> the
> > app doesn't create a window when launching this will return an unrelated
> window.
> >  But those seem like rare use cases.
> > 
> > What do you think?
> 
> Sorry, I might mislead you on this.
> 
> After looking into the code, I found session->chrome->GetWebViewIds is not
> reliable to return the list of opened windows in order, especially when two or
> more windows are opened between two calls of session->chrome->GetWebViewIds.
> This method could only do a best guess. And the underlying devtools command
> returns the list of windows ordered by "last activity time" which is defined
by
> the time when the window is shown or selected. If the user tries to debug the
> test and interact with the opened windows, then session->chrome->GetWebViewIds
> won't live up to its API document. I will update the comment of this method in
> chrome.h.
> 
> And after thinking it more thoroughly, I feel that it might be better to leave
> the switchTo app window to the users.
> 
> First, we could not reliably tell which window is the last one opened (we
assume
> it is the app window) while the user could. The user could do a diff of two
> calls to command driver.window_handles (rely on
session->chrome->GetWebViewIds)
> if there is only one window opened. If two or more windows are opened, the
user
> could still make it by querying the url or title of the window, or checking
some
> text in the page of that window.
> I think this also applies to app window.
> 
> Second, it might be better to deal with an app window the same way as normal
web
> page window for consistency. For normal web page window (it could be opened
due
> to a click or javascript), we leave the switchTo to the user. For app, when
it's
> launched, although I believe almost 100% the user is going to test and
interact
> with it, the user might still do the switchTo in the same way as normal web
page
> window (they get used to that).
> 
> Third, some apps will be opened in the current window/tab while others (and
most
> of them) are in a new one. If we try our best to do the switchTo, we might end
> in the wrong window. If the user handles that corner case in their code, this
> might lead to two code paths for different types or implementation of apps.
> 
> What do you think?

Yeah that makes sense, if users won't mind switching to the window themselves I
agree that's the best to make sure they get desired behavior.  Thanks for
catching that taking the last window from GetWebViewIds wouldn't be reliable.

Done.

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
File chrome/test/chromedriver/extension/background.js (right):

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
chrome/test/chromedriver/extension/background.js:83: function launchApp(Id,
callback, errCallback) {
On 2014/01/15 21:39:56, chrisgao wrote:
> whoops, nit: |Id| -> |id|?

Done.

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
File chrome/test/chromedriver/server/http_handler.cc (right):

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
chrome/test/chromedriver/server/http_handler.cc:122:
"session/:sessionId/chromium/launch",
On 2014/01/15 21:39:56, chrisgao wrote:
> how about "launch" -> "launch_app"?
> It'll also match the name of the command below.

Sure, Done.

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
chrome/test/chromedriver/window_commands.cc:279:
session->chrome->ActivateWebView(web_view_ids.back());
On 2014/01/15 21:39:56, chrisgao wrote:
> The activation might fail. maybe need to check the returned status code.
> 
> btw, maybe we don't have to active the app window, because clicking, typing
and
> others will still work on the app window even it is not activated and focused.

Thanks, per the other comment I removed this code as we don't need to switch
focus anymore.

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
File chrome/test/chromedriver/window_commands.h (right):

https://codereview.chromium.org/114403005/diff/390001/chrome/test/chromedrive...
chrome/test/chromedriver/window_commands.h:43: Status ExecuteLaunchApp(
On 2014/01/15 21:39:56, chrisgao wrote:
> window_commands.cc/h are for commands that require a target window, like
> navigating to a new url in the current target window, finding some element in
> the page. If the target window was closed, we will return an error to the
client
> side. That's why the |WebView* web_view| is passed in and it won't be NULL.
> 
> session_commands.cc/h are for commands that require a session only. As long as
> the session is not deleted, the command could be run.
> 
> "LaunchApp" seems to fall into the category of session_commands.cc/h.
> Maybe we'd better move to those two files.

Agree, Done.

[chrisgao (Use stgao instead), 2014-01-16 00:05:30]

lgtm.
Many thanks for the quick patch!

[commit-bot: I haz the power, 2014-01-16 00:13:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bustamante@chromium.org/114403005/520001

[commit-bot: I haz the power, 2014-01-16 02:24:38]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[bustamante, 2014-01-16 04:08:49]

The failures don't look like they're related to this change (and don't fail
consistently), I'm going to retry.

[commit-bot: I haz the power, 2014-01-16 04:09:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bustamante@chromium.org/114403005/520001

[commit-bot: I haz the power, 2014-01-16 06:24:50]

Change committed as 245167