First step in enabling creating tabs without an Activity on Android.

First step in enabling creating tabs without an Activity on Android.

This patch modifies the ContentBrowserClient::OpenURL method to pass a
callback, to be invoked when the content::WebContents* is available,
rather than returning it immediately as this will be an asynchronous
process on Android.

For actually opening a new tab, we create a ServiceTabCreator class which
has the ability to create tabs from Services. This method will fire
an intent to start the activity asynchronously.

The follow-up patch will make the activity communicate back a reference
to the created TabAndroid / WebContents, which allows the Service to
get it, and finalize the callback as expected.

BUG=454809
Committed: https://crrev.com/bbcccc162d55f19e30fced510a701d44e96e86f8
Cr-Commit-Position: refs/heads/master@{#315855}

[Peter Beverloo, 2015-02-03 15:54:52]

peter@chromium.org changed reviewers:
+ miguelg@chromium.org, mlamouri@chromium.org

[Peter Beverloo, 2015-02-03 15:54:53]

+miguelg for Android
+mlamouri for OpenURL

This patch depends on https://codereview.chromium.org/843583005.

[Miguel Garcia, 2015-02-03 16:58:40]

mostly nits on my end.

https://codereview.chromium.org/897673002/diff/40001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabCreator.java
(right):

https://codereview.chromium.org/897673002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabCreator.java:19: *
Activity is available. It works by sending an intent to launch a new tab, and
then getting
It does no work like this yet right? Perhaps keep the comment describing the
current functionality and change it as you itereate.

https://codereview.chromium.org/897673002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabCreator.java:32:
private static ServiceTabCreator create(long nativeServiceTabCreator, Context
context) {
shouldn't we make this a singleton so that we have only one of this hanging
around instead of one per call? There is only one natveServiceTabCreator

https://codereview.chromium.org/897673002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabCreator.java:47:
private void createTab(String url) {
perhaps rename it to launchTab, it would be mode descriptive.

https://codereview.chromium.org/897673002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabCreator.java:48:
Intent intent = new Intent(mContext, getBrowserActivityClassFromManifest());
perhaps get the class first and fail gracefully if
getBrowserActivityClassFromManifest returns null.

https://codereview.chromium.org/897673002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabCreator.java:73:
// Context.getPackageName() refers to an invalid package name, which is not
possible.
You might still want to log it just in case.

https://codereview.chromium.org/897673002/diff/40001/chrome/browser/android/s...
File chrome/browser/android/service_tab_creator.h (right):

https://codereview.chromium.org/897673002/diff/40001/chrome/browser/android/s...
chrome/browser/android/service_tab_creator.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
There is already an intentHelper c++/java that does similar things, perhaps it's
worth combining them. Up to you.

https://codereview.chromium.org/897673002/diff/40001/chrome/browser/android/s...
chrome/browser/android/service_tab_creator.h:17: 
namespace it?

https://codereview.chromium.org/897673002/diff/40001/chrome/browser/android/s...
chrome/browser/android/service_tab_creator.h:20: // able to report back to the
user when the tab has been created.
same thing, we are not quite able to report back yet so let's keep the comments
in par with the implementation?

[Peter Beverloo, 2015-02-03 17:24:54]

Thanks for the review!

https://codereview.chromium.org/897673002/diff/40001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabCreator.java
(right):

https://codereview.chromium.org/897673002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabCreator.java:19: *
Activity is available. It works by sending an intent to launch a new tab, and
then getting
On 2015/02/03 16:58:40, Miguel Garcia wrote:
> It does no work like this yet right? Perhaps keep the comment describing the
> current functionality and change it as you itereate.

Done. I added a TODO.

https://codereview.chromium.org/897673002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabCreator.java:32:
private static ServiceTabCreator create(long nativeServiceTabCreator, Context
context) {
On 2015/02/03 16:58:40, Miguel Garcia wrote:
> shouldn't we make this a singleton so that we have only one of this hanging
> around instead of one per call? There is only one natveServiceTabCreator 

What else would initialize it, given @CalledByNative to a Singleton'd class? Do
you think the overhead of keeping a sInstance member around (+ errors and
logging) for the sake of asserting that it's a singleton is worth it? I don't
think it is.

https://codereview.chromium.org/897673002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabCreator.java:47:
private void createTab(String url) {
On 2015/02/03 16:58:40, Miguel Garcia wrote:
> perhaps rename it to launchTab, it would be mode descriptive.

Done.

https://codereview.chromium.org/897673002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabCreator.java:48:
Intent intent = new Intent(mContext, getBrowserActivityClassFromManifest());
On 2015/02/03 16:58:40, Miguel Garcia wrote:
> perhaps get the class first and fail gracefully if
> getBrowserActivityClassFromManifest returns null.

The only realistic failure-case for gBACFM() is that the manifest key has not
been set correctly, nothing based on user-input. I'd argue that we want that to
explode so that we can catch it in tests prior to rolling it out to users?

https://codereview.chromium.org/897673002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabCreator.java:73:
// Context.getPackageName() refers to an invalid package name, which is not
possible.
On 2015/02/03 16:58:40, Miguel Garcia wrote:
> You might still want to log it just in case.

Done. I also removed the comments.

https://codereview.chromium.org/897673002/diff/40001/chrome/browser/android/s...
File chrome/browser/android/service_tab_creator.h (right):

https://codereview.chromium.org/897673002/diff/40001/chrome/browser/android/s...
chrome/browser/android/service_tab_creator.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/02/03 16:58:40, Miguel Garcia wrote:
> There is already an intentHelper c++/java that does similar things, perhaps
it's
> worth combining them. Up to you.

Nah, because this class will need to keep request Ids as well - it'll get some
more logic in patch 2.

https://codereview.chromium.org/897673002/diff/40001/chrome/browser/android/s...
chrome/browser/android/service_tab_creator.h:17: 
On 2015/02/03 16:58:40, Miguel Garcia wrote:
> namespace it? 

Done. "namespace chrome {}" eeuw :-(

https://codereview.chromium.org/897673002/diff/40001/chrome/browser/android/s...
chrome/browser/android/service_tab_creator.h:20: // able to report back to the
user when the tab has been created.
On 2015/02/03 16:58:40, Miguel Garcia wrote:
> same thing, we are not quite able to report back yet so let's keep the
comments
> in par with the implementation?

Done.

[Miguel Garcia, 2015-02-03 17:27:22]

lgtm for the android bits

[Peter Beverloo, 2015-02-06 11:38:18]

peter@chromium.org changed reviewers:
+ avi@chromium.org

[Peter Beverloo, 2015-02-06 11:38:19]

+avi for content/ API change.

[Peter Beverloo, 2015-02-06 14:01:45]

peter@chromium.org changed reviewers:
+ jochen@chromium.org

[Peter Beverloo, 2015-02-06 14:01:45]

+Jochen per Mounir's suggestion since this modifies CBC::OpenURL.

[Avi (use Gerrit), 2015-02-06 15:20:08]

lgtm

I'm cool with that.

[Peter Beverloo, 2015-02-06 16:33:59]

New patchsets have been uploaded after l-g-t-m from avi@chromium.org

[Peter Beverloo, 2015-02-06 16:50:40]

Cheers! I'll fix the unit test and will wait a bit to see if Jochen has
opinions.

[mlamouri (slow - plz ping), 2015-02-09 12:07:37]

It looks good. Thanks! A few comments below.

https://codereview.chromium.org/897673002/diff/120001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java
(right):

https://codereview.chromium.org/897673002/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:53:
intent.setData(Uri.parse(url));
Which other params value could be passed there? (see comment below). Could we
pass a Params object trough JNI? There are a few parameter bags related to
opening urls/tabs. There might be one that the Java side understand?

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
File chrome/browser/android/service_tab_launcher.cc (right):

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
chrome/browser/android/service_tab_launcher.cc:40: env, params.url.spec());
Why not pass some other params value? if not all?

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
File chrome/browser/android/service_tab_launcher.h (right):

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
chrome/browser/android/service_tab_launcher.h:31: void
LaunchTab(content::BrowserContext* browser_context,
nit: why not OpenURL() to keep symmetry with ContentClientBrowser?

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2533: // See
https://crbug.com/448409.
I think you can remove the TODO and the mention of the BUG. Just change the
comment to note that we do not support this on iOS.

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:286:
base::Callback<void(content::WebContents*)> callback) override;
Use const T& please.

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client_unittest.cc (right):

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client_unittest.cc:73: // Android and iOS
this is synchronous, which is the assumption this test
nit: I wouldn't make any assumption about iOS implementation in the comments.

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client_unittest.cc:79:
EXPECT_TRUE(web_contents());
It's a bit odd to have web_contents() being a test method even though it doesn't
mean anything for the test itself. Probably not a big deal but I wonder why you
preferred that compared to have a DidOpenURL anonymous method that would take a
WebContents** and set it based on the callback return value. It would also make
the synchronous requirement way more obvious because you would set the
WebContents* on the stack, dereference it to send it to the callback, then read
the value again on the stack.

WDYT? Feel free to keep the current design if you prefer it.

https://codereview.chromium.org/897673002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/897673002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:222: void
DidOpenWindowOnUI(
nit: DidOpenURL would be the right name for a callback called by OpenURL.
Specifying "OnUI" would be confusing because that naming convention is usually
used when there is an associated method on another thread.

https://codereview.chromium.org/897673002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:262:
base::Bind(&DidOpenWindowOnUI, callback));
Alternatively, you could pass browser_context and params to
WindowOpenedObserver() and have it call OpenURL() then when the call succeeds,
start listening to the returned WebContents. Keep it as is if you don't like
that.

https://codereview.chromium.org/897673002/diff/120001/content/public/browser/...
File content/public/browser/content_browser_client.cc (left):

https://codereview.chromium.org/897673002/diff/120001/content/public/browser/...
content/public/browser/content_browser_client.cc:347: return nullptr;
May I suggest to keep a default implementation and do something like:
  callback->Run(nullptr);

It would be unfortunate to have someone not implementing that method and then
have a pending callback.

https://codereview.chromium.org/897673002/diff/120001/content/shell/browser/s...
File content/shell/browser/shell_content_browser_client.cc (right):

https://codereview.chromium.org/897673002/diff/120001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.cc:314:
callback.Run(contents);
nit: you could do |callback.Run(Shell::CreateWindow(...));|

[Peter Beverloo, 2015-02-10 17:49:37]

Thanks for the review. PTAL

https://codereview.chromium.org/897673002/diff/120001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java
(right):

https://codereview.chromium.org/897673002/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:53:
intent.setData(Uri.parse(url));
On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> Which other params value could be passed there? (see comment below). Could we
> pass a Params object trough JNI? There are a few parameter bags related to
> opening urls/tabs. There might be one that the Java side understand?

There's nothing we can pass beyond the referrer unfortunately, which has the
TODO just below here.

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
File chrome/browser/android/service_tab_launcher.cc (right):

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
chrome/browser/android/service_tab_launcher.cc:40: env, params.url.spec());
On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> Why not pass some other params value? if not all?

There are only very few of them we can do something with since this is coming
from intents. In the longer run we can probably use more of them if we do
another STL -> Tab.loadUrl <--> STL round-trip, but that has some interesting
timing issues.

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
File chrome/browser/android/service_tab_launcher.h (right):

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
chrome/browser/android/service_tab_launcher.h:31: void
LaunchTab(content::BrowserContext* browser_context,
On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> nit: why not OpenURL() to keep symmetry with ContentClientBrowser?

One opens a URL, but launches a tab. This was a suggestion by Miguel which I
agree with -- this class is in charge of the latter.

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2533: // See
https://crbug.com/448409.
On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> I think you can remove the TODO and the mention of the BUG. Just change the
> comment to note that we do not support this on iOS.

Done.

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:286:
base::Callback<void(content::WebContents*)> callback) override;
On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> Use const T& please.

Done.

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client_unittest.cc (right):

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client_unittest.cc:73: // Android and iOS
this is synchronous, which is the assumption this test
On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> nit: I wouldn't make any assumption about iOS implementation in the comments.

Done.

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client_unittest.cc:79:
EXPECT_TRUE(web_contents());
On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> It's a bit odd to have web_contents() being a test method even though it
doesn't
> mean anything for the test itself. Probably not a big deal but I wonder why
you
> preferred that compared to have a DidOpenURL anonymous method that would take
a
> WebContents** and set it based on the callback return value. It would also
make
> the synchronous requirement way more obvious because you would set the
> WebContents* on the stack, dereference it to send it to the callback, then
read
> the value again on the stack.
> 
> WDYT? Feel free to keep the current design if you prefer it.

Done.

https://codereview.chromium.org/897673002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/897673002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:222: void
DidOpenWindowOnUI(
On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> nit: DidOpenURL would be the right name for a callback called by OpenURL.
> Specifying "OnUI" would be confusing because that naming convention is usually
> used when there is an associated method on another thread.

Done.

https://codereview.chromium.org/897673002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:262:
base::Bind(&DidOpenWindowOnUI, callback));
On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> Alternatively, you could pass browser_context and params to
> WindowOpenedObserver() and have it call OpenURL() then when the call succeeds,
> start listening to the returned WebContents. Keep it as is if you don't like
> that.

I don't feel strongly about this code, so I'll leave it as it is.

https://codereview.chromium.org/897673002/diff/120001/content/public/browser/...
File content/public/browser/content_browser_client.cc (left):

https://codereview.chromium.org/897673002/diff/120001/content/public/browser/...
content/public/browser/content_browser_client.cc:347: return nullptr;
On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> May I suggest to keep a default implementation and do something like:
>   callback->Run(nullptr);
> 
> It would be unfortunate to have someone not implementing that method and then
> have a pending callback.

I'd prefer not to, as that breaks the API contract that the callback will be
invoked when the WC* is available. If someone uses OpenURL() on a platform where
it's not supported then they'll get the NOTIMPLEMENTED() message in either case.

https://codereview.chromium.org/897673002/diff/120001/content/shell/browser/s...
File content/shell/browser/shell_content_browser_client.cc (right):

https://codereview.chromium.org/897673002/diff/120001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.cc:314:
callback.Run(contents);
On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> nit: you could do |callback.Run(Shell::CreateWindow(...));|

Done.

[mlamouri (slow - plz ping), 2015-02-11 13:50:24]

Thanks for applying the review comments. Unfortunately, I realised that you were
missing the incognito case: we don't want to open a new regular tab if the
browser context is incognito.

https://codereview.chromium.org/897673002/diff/120001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java
(right):

https://codereview.chromium.org/897673002/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:53:
intent.setData(Uri.parse(url));
On 2015/02/10 at 17:49:37, Peter Beverloo wrote:
> On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> > Which other params value could be passed there? (see comment below). Could
we
> > pass a Params object trough JNI? There are a few parameter bags related to
> > opening urls/tabs. There might be one that the Java side understand?
> 
> There's nothing we can pass beyond the referrer unfortunately, which has the
TODO just below here.

We could probably get the WindowDisposition and do a best effort here. Something
we surely need though is whether the new tab need to be INCOGNITO. It would be
great if you could explicitly pass Browser.EXTRA_CREATE_NEW_TAB or
EXTRA_OPEN_NEW_INCOGNITO_TAB depending on the context.

Note: EXTRA_OPEN_NEW_INCOGNITO_TAB is defined in the Chrome Android private
repository. I wouldn't mind any hack that would make your life easier here ;)

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
File chrome/browser/android/service_tab_launcher.cc (right):

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
chrome/browser/android/service_tab_launcher.cc:40: env, params.url.spec());
On 2015/02/10 at 17:49:37, Peter Beverloo wrote:
> On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> > Why not pass some other params value? if not all?
> 
> There are only very few of them we can do something with since this is coming
from intents. In the longer run we can probably use more of them if we do
another STL -> Tab.loadUrl <--> STL round-trip, but that has some interesting
timing issues.

Given my comment above, you might want to at least pass whether we want the tab
to be in incognito using the BrowserContext.

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
File chrome/browser/android/service_tab_launcher.h (right):

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
chrome/browser/android/service_tab_launcher.h:31: void
LaunchTab(content::BrowserContext* browser_context,
On 2015/02/10 at 17:49:37, Peter Beverloo wrote:
> On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> > nit: why not OpenURL() to keep symmetry with ContentClientBrowser?
> 
> One opens a URL, but launches a tab. This was a suggestion by Miguel which I
agree with -- this class is in charge of the latter.

I will not block on that but given that we pass OpenURLParams, we clearly don't
mean to always create a new tab. This is a limitation of the Java implementation
that makes us do that. OpenURLParams might ask the URL to be open in the current
tab.

https://codereview.chromium.org/897673002/diff/120001/content/public/browser/...
File content/public/browser/content_browser_client.cc (left):

https://codereview.chromium.org/897673002/diff/120001/content/public/browser/...
content/public/browser/content_browser_client.cc:347: return nullptr;
On 2015/02/10 at 17:49:37, Peter Beverloo wrote:
> On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> > May I suggest to keep a default implementation and do something like:
> >   callback->Run(nullptr);
> > 
> > It would be unfortunate to have someone not implementing that method and
then
> > have a pending callback.
> 
> I'd prefer not to, as that breaks the API contract that the callback will be
invoked when the WC* is available. If someone uses OpenURL() on a platform where
it's not supported then they'll get the NOTIMPLEMENTED() message in either case.

How would they get NOTIMPLEMENTED()? That's in the chrome/ layer, right? If
someone, let say Opera is implementing a ContentBrowserClient, this call would
silently fail.

Maybe you could just have a NOTIMPLEMENTED() in there?

https://codereview.chromium.org/897673002/diff/160001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java
(right):

https://codereview.chromium.org/897673002/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:72: 
nit: empty line

https://codereview.chromium.org/897673002/diff/160001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/897673002/diff/160001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:287: override;
nit: override in its line alone, is that fine per coding style?

https://codereview.chromium.org/897673002/diff/160001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client_unittest.cc (right):

https://codereview.chromium.org/897673002/diff/160001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client_unittest.cc:66: // only be ran on
platforms where OpenURL is implemented synchronously.
nit: mind opening a bug?

[Peter Beverloo, 2015-02-11 16:18:27]

Thanks for the thorough review! Please take another look. Mind that these values
are not currently handled in ChromeShell.

https://codereview.chromium.org/897673002/diff/120001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java
(right):

https://codereview.chromium.org/897673002/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:53:
intent.setData(Uri.parse(url));
On 2015/02/11 13:50:23, Mounir Lamouri wrote:
> On 2015/02/10 at 17:49:37, Peter Beverloo wrote:
> > On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> > > Which other params value could be passed there? (see comment below). Could
> we
> > > pass a Params object trough JNI? There are a few parameter bags related to
> > > opening urls/tabs. There might be one that the Java side understand?
> > 
> > There's nothing we can pass beyond the referrer unfortunately, which has the
> TODO just below here.
> 
> We could probably get the WindowDisposition and do a best effort here.
Something
> we surely need though is whether the new tab need to be INCOGNITO. It would be
> great if you could explicitly pass Browser.EXTRA_CREATE_NEW_TAB or
> EXTRA_OPEN_NEW_INCOGNITO_TAB depending on the context.
> 
> Note: EXTRA_OPEN_NEW_INCOGNITO_TAB is defined in the Chrome Android private
> repository. I wouldn't mind any hack that would make your life easier here ;)

Done.

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
File chrome/browser/android/service_tab_launcher.cc (right):

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
chrome/browser/android/service_tab_launcher.cc:40: env, params.url.spec());
On 2015/02/11 13:50:23, Mounir Lamouri wrote:
> On 2015/02/10 at 17:49:37, Peter Beverloo wrote:
> > On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> > > Why not pass some other params value? if not all?
> > 
> > There are only very few of them we can do something with since this is
coming
> from intents. In the longer run we can probably use more of them if we do
> another STL -> Tab.loadUrl <--> STL round-trip, but that has some interesting
> timing issues.
> 
> Given my comment above, you might want to at least pass whether we want the
tab
> to be in incognito using the BrowserContext.

Done.

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
File chrome/browser/android/service_tab_launcher.h (right):

https://codereview.chromium.org/897673002/diff/120001/chrome/browser/android/...
chrome/browser/android/service_tab_launcher.h:31: void
LaunchTab(content::BrowserContext* browser_context,
On 2015/02/11 13:50:23, Mounir Lamouri wrote:
> On 2015/02/10 at 17:49:37, Peter Beverloo wrote:
> > On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> > > nit: why not OpenURL() to keep symmetry with ContentClientBrowser?
> > 
> > One opens a URL, but launches a tab. This was a suggestion by Miguel which I
> agree with -- this class is in charge of the latter.
> 
> I will not block on that but given that we pass OpenURLParams, we clearly
don't
> mean to always create a new tab. This is a limitation of the Java
implementation
> that makes us do that. OpenURLParams might ask the URL to be open in the
current
> tab.

Good point. Applied per the other comment. I'll maintain the Tab Launcher name -
if I "open a tab" I'd expect to see a new tab, whereas "launching a tab" is more
ambiguous.

https://codereview.chromium.org/897673002/diff/120001/content/public/browser/...
File content/public/browser/content_browser_client.cc (left):

https://codereview.chromium.org/897673002/diff/120001/content/public/browser/...
content/public/browser/content_browser_client.cc:347: return nullptr;
On 2015/02/11 13:50:23, Mounir Lamouri wrote:
> On 2015/02/10 at 17:49:37, Peter Beverloo wrote:
> > On 2015/02/09 12:07:37, Mounir Lamouri wrote:
> > > May I suggest to keep a default implementation and do something like:
> > >   callback->Run(nullptr);
> > > 
> > > It would be unfortunate to have someone not implementing that method and
> then
> > > have a pending callback.
> > 
> > I'd prefer not to, as that breaks the API contract that the callback will be
> invoked when the WC* is available. If someone uses OpenURL() on a platform
where
> it's not supported then they'll get the NOTIMPLEMENTED() message in either
case.
> 
> How would they get NOTIMPLEMENTED()? That's in the chrome/ layer, right? If
> someone, let say Opera is implementing a ContentBrowserClient, this call would
> silently fail.
> 
> Maybe you could just have a NOTIMPLEMENTED() in there?

Right. That's true. I've added the "callback.Run(nullptr);" you initially
suggested since that's what the rest of the ContentBrowserClient default
implementations do. The DCHECK will lead them up the stack to the new method.

https://codereview.chromium.org/897673002/diff/160001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java
(right):

https://codereview.chromium.org/897673002/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:72: 
On 2015/02/11 13:50:23, Mounir Lamouri wrote:
> nit: empty line

Done.

https://codereview.chromium.org/897673002/diff/160001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/897673002/diff/160001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:287: override;
On 2015/02/11 13:50:23, Mounir Lamouri wrote:
> nit: override in its line alone, is that fine per coding style?

I don't see why not, it's a continuation with 4-space indent. 141 results on
codesearch seem to agree.

https://codereview.chromium.org/897673002/diff/160001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client_unittest.cc (right):

https://codereview.chromium.org/897673002/diff/160001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client_unittest.cc:66: // only be ran on
platforms where OpenURL is implemented synchronously.
On 2015/02/11 13:50:23, Mounir Lamouri wrote:
> nit: mind opening a bug?

Done.

[mlamouri (slow - plz ping), 2015-02-11 20:49:52]

lgtm with the const T& callback applied, do whatever you prefer for the nits.

Thanks! :)

https://codereview.chromium.org/897673002/diff/200001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java
(right):

https://codereview.chromium.org/897673002/diff/200001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:92:
return Class.forName(className);
nit:
return Class.forName(info.metaData.getString(BROWSER_ACTIVITY_KEY));

https://codereview.chromium.org/897673002/diff/200001/chrome/browser/android/...
File chrome/browser/android/service_tab_launcher.cc (right):

https://codereview.chromium.org/897673002/diff/200001/chrome/browser/android/...
chrome/browser/android/service_tab_launcher.cc:38:
base::Callback<void(content::WebContents*)> callback) {
ditto

https://codereview.chromium.org/897673002/diff/200001/chrome/browser/android/...
File chrome/browser/android/service_tab_launcher.h (right):

https://codereview.chromium.org/897673002/diff/200001/chrome/browser/android/...
chrome/browser/android/service_tab_launcher.h:33:
base::Callback<void(content::WebContents*)> callback);
const T& for callbacks, see base/callback.h

Sorry, I should have seen that one.

[Peter Beverloo, 2015-02-11 21:11:52]

New patchsets have been uploaded after l-g-t-m from mlamouri@chromium.org

[Peter Beverloo, 2015-02-11 21:12:13]

Thanks for your reviews!

https://codereview.chromium.org/897673002/diff/200001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java
(right):

https://codereview.chromium.org/897673002/diff/200001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:92:
return Class.forName(className);
On 2015/02/11 20:49:52, Mounir Lamouri wrote:
> nit:
> return Class.forName(info.metaData.getString(BROWSER_ACTIVITY_KEY));

I actually prefer the temporary variable since it benefits readability. Line
88/89 already is rather long as well. I'll keep it like this :-).

https://codereview.chromium.org/897673002/diff/200001/chrome/browser/android/...
File chrome/browser/android/service_tab_launcher.cc (right):

https://codereview.chromium.org/897673002/diff/200001/chrome/browser/android/...
chrome/browser/android/service_tab_launcher.cc:38:
base::Callback<void(content::WebContents*)> callback) {
On 2015/02/11 20:49:52, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/897673002/diff/200001/chrome/browser/android/...
File chrome/browser/android/service_tab_launcher.h (right):

https://codereview.chromium.org/897673002/diff/200001/chrome/browser/android/...
chrome/browser/android/service_tab_launcher.h:33:
base::Callback<void(content::WebContents*)> callback);
On 2015/02/11 20:49:52, Mounir Lamouri wrote:
> const T& for callbacks, see base/callback.h
> 
> Sorry, I should have seen that one.

Done.

[Peter Beverloo, 2015-02-11 21:12:36]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2015-02-11 21:13:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/897673002/220001

[commit-bot: I haz the power, 2015-02-11 22:23:42]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-02-11 22:24:19]

Patchset 12 (id:??) landed as
https://crrev.com/bbcccc162d55f19e30fced510a701d44e96e86f8
Cr-Commit-Position: refs/heads/master@{#315855}