Make service tab launcher a component

Make service tab launcher a component

BUG=473011
Committed: https://crrev.com/3ecfdfafd58ba9a8422b5fc7c4b128d31d8ff28f
Cr-Commit-Position: refs/heads/master@{#324850}

[Mathias Hällman, 2015-04-02 08:07:58]

mathiash@opera.com changed reviewers:
+ jochen@chromium.org, tedchoc@chromium.org

[Mathias Hällman, 2015-04-02 08:13:10]

mathiash@opera.com changed reviewers:
+ peter@chromium.org

[Mathias Hällman, 2015-04-02 08:13:11]

tedchoc, could you look at all things android?
jochen, I think you're the guy for the rest?
peter, maybe you'd be interested in weighing in?

I haven't been able to build using GN, but I took inspiration from looking at
web_contents_delegate_android.. I hope I wasn't too far off the mark.

[Peter Beverloo, 2015-04-02 14:27:47]

This mostly lg % the gyp comments, but please add some OWNERS for the component.
(I'd nominate tedchoc@, miguelg@ and myself for now.)

Additionally, this will break our downstream build, so ideally we can postpone
landing this CL until some time next week so that I can take care of the
breakage.

https://codereview.chromium.org/1057533005/diff/1/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/1057533005/diff/1/components/components.gyp#n...
components/components.gyp:77: 'service_tab_launcher.gypi',
Since we only use the service_tab_launcher on Android, why not have a
['OS=="android"] block?

That way the top-level condition in the gypi file can go as well, and it matches
(roughly) what you're doing in GN.

https://codereview.chromium.org/1057533005/diff/1/components/service_tab_laun...
File components/service_tab_launcher.gypi (right):

https://codereview.chromium.org/1057533005/diff/1/components/service_tab_laun...
components/service_tab_launcher.gypi:1: # Copyright (c) 2015 The Chromium
Authors. All rights reserved.
nit: no more (c), and no blank line.

https://codereview.chromium.org/1057533005/diff/1/components/service_tab_laun...
File components/service_tab_launcher/component_jni_registrar.h (right):

https://codereview.chromium.org/1057533005/diff/1/components/service_tab_laun...
components/service_tab_launcher/component_jni_registrar.h:1: // Copyright (c)
2015 The Chromium Authors. All rights reserved.
nit: no (c), elsewhere too

[Ted C, 2015-04-02 16:48:07]

https://codereview.chromium.org/1057533005/diff/1/components/service_tab_laun...
File
components/service_tab_launcher/android/java/src/org/chromium/components/service_tab_launcher/ServiceTabLauncher.java
(right):

https://codereview.chromium.org/1057533005/diff/1/components/service_tab_laun...
components/service_tab_launcher/android/java/src/org/chromium/components/service_tab_launcher/ServiceTabLauncher.java:24:
private static final String SERVICE_TAB_LAUNCHER_KEY =
just for extra certainty, are persisted anywhere are they (i.e. an intent that
will persist across upgrades/installs)?  Just want to make sure they don't need
aliases or anything for previous usage.

[Mathias Hällman, 2015-04-07 07:27:40]

Peter, could you answer Ted's question?

Would either of you know what I need to add to which DEPS file to fix the errors
I got when trying PS1? I only added the first DEPS few files because git cl
wouldn't let me upload it otherwise.

[Mathias Hällman, 2015-04-13 12:36:35]

jochen, are you having a look? Should I ask someone else?
peter, did you have an answer for Ted? I'm just moving the parts around, I
haven't looked much into how any of it works.

[Peter Beverloo, 2015-04-13 12:53:21]

Oops, sorry! The answer is rather innocent :).

https://codereview.chromium.org/1057533005/diff/1/components/service_tab_laun...
File
components/service_tab_launcher/android/java/src/org/chromium/components/service_tab_launcher/ServiceTabLauncher.java
(right):

https://codereview.chromium.org/1057533005/diff/1/components/service_tab_laun...
components/service_tab_launcher/android/java/src/org/chromium/components/service_tab_launcher/ServiceTabLauncher.java:24:
private static final String SERVICE_TAB_LAUNCHER_KEY =
On 2015/04/02 16:48:07, Ted C (OOO 4.13 - 4.21) wrote:
> just for extra certainty, are persisted anywhere are they (i.e. an intent that
> will persist across upgrades/installs)?  Just want to make sure they don't
need
> aliases or anything for previous usage.

No, they are not. The intents are only used for immediate tab fires.

[jochen (gone - plz use gerrit), 2015-04-13 13:23:43]

lgtm

https://codereview.chromium.org/1057533005/diff/40001/components/service_tab_...
File components/service_tab_launcher/android/DEPS (right):

https://codereview.chromium.org/1057533005/diff/40001/components/service_tab_...
components/service_tab_launcher/android/DEPS:4: 
nit remove empty line

[jochen (gone - plz use gerrit), 2015-04-13 13:30:50]

(but please wait for Peter/Ted to also approve)

[Mathias Hällman, 2015-04-13 13:38:05]

Will do

I'm confused by the failing build though, some test which shouldn't be affected
by this. Is it possible the patch will land anyway?

[Peter Beverloo, 2015-04-13 13:53:18]

lgtm

I'll take care of our downstream build breakage.

On 2015/04/13 13:38:05, Mathias Hällman wrote:
> Will do
> 
> I'm confused by the failing build though, some test which shouldn't be
affected
> by this. Is it possible the patch will land anyway?

That looks like a flake to me.

[Mathias Hällman, 2015-04-13 13:57:00]

The CQ bit was checked by mathiash@opera.com

[Mathias Hällman, 2015-04-13 13:57:01]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1057533005/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-04-13 13:57:12]

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

[Mathias Hällman, 2015-04-13 13:57:52]

On 2015/04/13 13:53:18, Peter Beverloo wrote:
> That looks like a flake to me.

It happened a few times though.. well, let's see how this goes now.

[commit-bot: I haz the power, 2015-04-13 15:06:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-13 15:07:23]

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