Feed back to the ServiceTabLauncher when the tab has been created.

Feed back to the ServiceTabLauncher when the tab has been created.

This is the second part of being able to open new windows on Android from
a background service, where no Activity may be available. New tabs are
launched with a request ID, which will be used to feed back creation of
the tab to the ServiceTabLauncher.

TBR=jbudorick@chromium.org
BUG=454809
Committed: https://crrev.com/244c761f6ef763dabf708ac6794afd39b1ea7b5e
Cr-Commit-Position: refs/heads/master@{#317313}

[Peter Beverloo, 2015-02-04 17:45:45]

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

[Peter Beverloo, 2015-02-04 17:45:45]

+mlamouri for opening window semantics
+bauerb, tedchoc and dfalcantara for the Android tab bits.

This patch depends on the first CL[1], and needs some tests. I'm mostly
searching for some ideas from the more knowledgeable folks for now :-).

My goal is to be able to launch tabs from an Android Service, and get feedback
(in the form of the content::WebContents*) when that has been completed, where
the WC* won't have started the provisional load yet. There may not be an
activity when the Service launches a tab. The feedback is reliable due to an
integral request Id being included as an intent extra, but sensitive to timing.

Rather than changing the intent handling paths for Chrome Shell (as done in this
patch) and the tab types we support downstream, would there be a better way to
support this in shared parts of Tab[Model[Selector]|Manager], so that we only
have to implement it once?

For completency, this code path is enabled through ContentBrowserClient::OpenURL
which Mounir is adding in [2].

[1] https://codereview.chromium.org/897673002/
[2] https://codereview.chromium.org/843583005

[Bernhard Bauer, 2015-02-04 18:09:12]

lgtm

https://codereview.chromium.org/904453002/diff/1/chrome/browser/android/servi...
File chrome/browser/android/service_tab_launcher.cc (right):

https://codereview.chromium.org/904453002/diff/1/chrome/browser/android/servi...
chrome/browser/android/service_tab_launcher.cc:45: content::BrowserContext*
browser_context,
You don't really seem to need the browser context.

[gone, 2015-02-06 01:04:24]

dfalcantara@chromium.org changed reviewers:
+ dtrainor@chromium.org, mariakhomenko@chromium.org

[gone, 2015-02-06 01:04:24]

+Maria, David

Honestly not sure.  Downstream you have to deal with the DocumentTabModel or the
TabModelImpl, depending on whether you're starting in document mode or in tabbed
mode.  They both, however, branch from the same launching point of
ChromeLauncherActivity.  Document mode currently has the concept of creating a
WebContents, then launching a new DocumentTab with that new WebContents, but
ChromeTabbedActivity doesn't have that type of code path.  We'd been discussing
having ChromeTabbedActivity gain similar functionality, though, which seems to
be what you need.

[Ted C, 2015-02-06 01:18:19]

I agree with Dan that if this could be in charge of creating the web contents
(and that there was some observer method that could be hooked into to trigger
what you want) that would be much better than the contract of each calling app
to make the call before issuing a load.

Requiring the various code paths to call that just seems extremely brittle. 
Even if we can't manage to do that...we should figure out some way to inform Tab
of the need to talk to this service and ensure it does it on startup.

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

https://codereview.chromium.org/904453002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:65:
intent.putExtra(LAUNCH_REQUEST_ID_EXTRA, requestId);
for chrome to actually act on this, we'll probably need to mark the intent as
trusted as this could come from any external app and cause weird things to
happen.

See downstream IntentHandler.

https://codereview.chromium.org/904453002/diff/1/chrome/browser/android/servi...
File chrome/browser/android/service_tab_launcher.cc (right):

https://codereview.chromium.org/904453002/diff/1/chrome/browser/android/servi...
chrome/browser/android/service_tab_launcher.cc:60: content::WebContents*
web_contents) {
+1 indent

[Peter Beverloo, 2015-02-19 13:51:09]

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

[mlamouri (slow - plz ping), 2015-02-19 14:51:17]

https://codereview.chromium.org/904453002/diff/20001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
(right):

https://codereview.chromium.org/904453002/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:254:
Tab newTab = mTabManager.createTab(url, TabLaunchType.FROM_EXTERNAL_APP);
FROM_EXTERNAL_APP isn't quite right...

https://codereview.chromium.org/904453002/diff/20001/chrome/browser/android/s...
File chrome/browser/android/service_tab_launcher.h (right):

https://codereview.chromium.org/904453002/diff/20001/chrome/browser/android/s...
chrome/browser/android/service_tab_launcher.h:56: std::map<int,
TabLaunchedCallback> tab_launched_callbacks_;
Could you use an IDMap? It will prevent having to make a copy of the callback
and you can delegate the unique ID work to it.

[Bernhard Bauer, 2015-02-19 15:03:05]

https://codereview.chromium.org/904453002/diff/20001/chrome/browser/android/s...
File chrome/browser/android/service_tab_launcher.h (right):

https://codereview.chromium.org/904453002/diff/20001/chrome/browser/android/s...
chrome/browser/android/service_tab_launcher.h:56: std::map<int,
TabLaunchedCallback> tab_launched_callbacks_;
On 2015/02/19 14:51:17, Mounir Lamouri wrote:
> Could you use an IDMap? It will prevent having to make a copy of the callback
> and you can delegate the unique ID work to it.

FTR: Copies of callbacks are cheap, as they just contain a scoped_refptr to a
refcounted immutable object.

[Peter Beverloo, 2015-02-19 16:03:07]

Thanks for your responses, everyone. The patch is now in better shape. I'll send
an e-mail to make sure everyone's got full context.

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

https://codereview.chromium.org/904453002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:65:
intent.putExtra(LAUNCH_REQUEST_ID_EXTRA, requestId);
On 2015/02/06 01:18:18, Ted C wrote:
> for chrome to actually act on this, we'll probably need to mark the intent as
> trusted as this could come from any external app and cause weird things to
> happen.
> 
> See downstream IntentHandler.

Done.

https://codereview.chromium.org/904453002/diff/1/chrome/browser/android/servi...
File chrome/browser/android/service_tab_launcher.cc (right):

https://codereview.chromium.org/904453002/diff/1/chrome/browser/android/servi...
chrome/browser/android/service_tab_launcher.cc:45: content::BrowserContext*
browser_context,
On 2015/02/04 18:09:12, Bernhard Bauer wrote:
> You don't really seem to need the browser context.

Rebased. We do now per a previous patch, to check whether we're in incognito
mode :-).

https://codereview.chromium.org/904453002/diff/1/chrome/browser/android/servi...
chrome/browser/android/service_tab_launcher.cc:60: content::WebContents*
web_contents) {
On 2015/02/06 01:18:18, Ted C wrote:
> +1 indent

Done.

https://codereview.chromium.org/904453002/diff/20001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
(right):

https://codereview.chromium.org/904453002/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:254:
Tab newTab = mTabManager.createTab(url, TabLaunchType.FROM_EXTERNAL_APP);
On 2015/02/19 14:51:17, Mounir Lamouri wrote:
> FROM_EXTERNAL_APP isn't quite right...

Done.

https://codereview.chromium.org/904453002/diff/20001/chrome/browser/android/s...
File chrome/browser/android/service_tab_launcher.h (right):

https://codereview.chromium.org/904453002/diff/20001/chrome/browser/android/s...
chrome/browser/android/service_tab_launcher.h:56: std::map<int,
TabLaunchedCallback> tab_launched_callbacks_;
On 2015/02/19 14:51:17, Mounir Lamouri wrote:
> Could you use an IDMap? It will prevent having to make a copy of the callback
> and you can delegate the unique ID work to it.

Done.

[mlamouri (slow - plz ping), 2015-02-19 18:56:57]

lgtm

[Ted C, 2015-02-19 21:55:22]

lgtm

https://codereview.chromium.org/904453002/diff/40001/chrome/browser/android/s...
File chrome/browser/android/service_tab_launcher.cc (right):

https://codereview.chromium.org/904453002/diff/40001/chrome/browser/android/s...
chrome/browser/android/service_tab_launcher.cc:89: TabLaunchedCallback* callback
= tab_launched_callbacks_.Lookup(request_id);
the one gotcha here is that if the intent is ever redispatched from android
(because it persists it for some reason), it might not exist in the map as it
could have been cleared.

The pending data usage for document mode makes this safer, but the intents do
get slightly out of our control.

Just an FYI that this DCHECK might happen for legitimate reasons in android --
so I'm glad it's a DCHECK :-)

[Maria, 2015-02-19 22:50:50]

lgtm

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

https://codereview.chromium.org/904453002/diff/40001/chrome/browser/android/s...
chrome/browser/android/service_tab_launcher.h:27: using TabLaunchedCallback =
base::Callback<void(content::WebContents*)>;
is this C++ feature now allowed in chromium?

[Peter Beverloo, 2015-02-20 12:09:19]

https://codereview.chromium.org/904453002/diff/40001/chrome/browser/android/s...
File chrome/browser/android/service_tab_launcher.cc (right):

https://codereview.chromium.org/904453002/diff/40001/chrome/browser/android/s...
chrome/browser/android/service_tab_launcher.cc:89: TabLaunchedCallback* callback
= tab_launched_callbacks_.Lookup(request_id);
On 2015/02/19 21:55:22, Ted C wrote:
> the one gotcha here is that if the intent is ever redispatched from android
> (because it persists it for some reason), it might not exist in the map as it
> could have been cleared.
> 
> The pending data usage for document mode makes this safer, but the intents do
> get slightly out of our control.
> 
> Just an FYI that this DCHECK might happen for legitimate reasons in android --
> so I'm glad it's a DCHECK :-)

That's a really good point actually, thank you.

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

https://codereview.chromium.org/904453002/diff/40001/chrome/browser/android/s...
chrome/browser/android/service_tab_launcher.h:27: using TabLaunchedCallback =
base::Callback<void(content::WebContents*)>;
On 2015/02/19 22:50:50, Maria wrote:
> is this C++ feature now allowed in chromium?

Yeah! :-) It's much nicer than typedefs.

https://chromium-cpp.appspot.com/

[Peter Beverloo, 2015-02-20 12:09:23]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2015-02-20 12:10:39]

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

[commit-bot: I haz the power, 2015-02-20 13:46:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-20 13:46:09]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)

[Peter Beverloo, 2015-02-20 13:46:50]

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

[Peter Beverloo, 2015-02-20 13:46:58]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-02-20 13:47:00]

The CQ bit was unchecked by peter@chromium.org

[Peter Beverloo, 2015-02-20 13:47:01]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-02-20 13:47:02]

The CQ bit was unchecked by peter@chromium.org

[Peter Beverloo, 2015-02-20 13:48:22]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2015-02-20 13:49:11]

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

[commit-bot: I haz the power, 2015-02-20 14:13:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-20 14:14:24]

Patchset 4 (id:??) landed as
https://crrev.com/244c761f6ef763dabf708ac6794afd39b1ea7b5e
Cr-Commit-Position: refs/heads/master@{#317313}