[Custom Tabs]Fix a bug that activateContents will pop up a blank chrome

[Custom Tabs]Fix a bug that activateContents will pop up a blank chrome

ChromeTab#activateContents brings chrome app to foreground, by sending
an intent to chrome. Yet in CustomTabs, it will generate erroneous
behavior.

BUG=524700
Committed: https://crrev.com/60b78e3530d0d0ec91e2c70f2be92c4c506623e4
Cr-Commit-Position: refs/heads/master@{#345784}

[Ian Wen, 2015-08-26 21:20:57]

ianwen@chromium.org changed reviewers:
+ dfalcantara@chromium.org, yusufo@chromium.org

[Ian Wen, 2015-08-26 21:20:58]

PTAL

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java
(right):

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:376:
protected void bringTabToForeground() { }
In L or M, ActivityManager.getAppTasks will only give us tasks that are created
by chrome, which does not include the client's task. Then I wonder how we can
bring the client app to foreground?

[gone, 2015-08-26 21:22:19]

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java
(right):

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:376:
protected void bringTabToForeground() { }
On 2015/08/26 21:20:58, Ian Wen wrote:
> In L or M, ActivityManager.getAppTasks will only give us tasks that are
created
> by chrome, which does not include the client's task. Then I wonder how we can
> bring the client app to foreground?

If that's the case, then we're completely hosed.  I don't know how we can.

[Yusuf, 2015-08-26 21:24:15]

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java:422: if
(index == TabModel.INVALID_TAB_INDEX) return;
do we need these in customtabs? It is a single tab model, so this is essentially
resetting the current tab to be the current tab.

I would say we should just override activateContents in customtab. Dan, what do
you think?

[gone, 2015-08-26 21:35:37]

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java:422: if
(index == TabModel.INVALID_TAB_INDEX) return;
On 2015/08/26 21:24:15, Yusuf wrote:
> do we need these in customtabs? It is a single tab model, so this is
essentially
> resetting the current tab to be the current tab.
> 
> I would say we should just override activateContents in customtab. Dan, what
do
> you think?

It's a no-op.  I'm worried about divergence if we ever add extra things to
activateContents() and the overridden CustomTab doesn't call super().

[Ian Wen, 2015-08-26 21:57:10]

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java:422: if
(index == TabModel.INVALID_TAB_INDEX) return;
On 2015/08/26 21:35:37, dfalcantara wrote:
> On 2015/08/26 21:24:15, Yusuf wrote:
> > do we need these in customtabs? It is a single tab model, so this is
> essentially
> > resetting the current tab to be the current tab.
> > 
> > I would say we should just override activateContents in customtab. Dan, what
> do
> > you think?
> 
> It's a no-op.  I'm worried about divergence if we ever add extra things to
> activateContents() and the overridden CustomTab doesn't call super().

I am okay with both. Yet maybe doing a no-op is a slightly cleaner way? We
haven't changed activateContents() since the mass upstreaming.

Dan wdyt? I will upload a new patch.

[gone, 2015-08-26 21:59:52]

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java:422: if
(index == TabModel.INVALID_TAB_INDEX) return;
On 2015/08/26 21:57:10, Ian Wen wrote:
> On 2015/08/26 21:35:37, dfalcantara wrote:
> > On 2015/08/26 21:24:15, Yusuf wrote:
> > > do we need these in customtabs? It is a single tab model, so this is
> > essentially
> > > resetting the current tab to be the current tab.
> > > 
> > > I would say we should just override activateContents in customtab. Dan,
what
> > do
> > > you think?
> > 
> > It's a no-op.  I'm worried about divergence if we ever add extra things to
> > activateContents() and the overridden CustomTab doesn't call super().
> 
> I am okay with both. Yet maybe doing a no-op is a slightly cleaner way? We
> haven't changed activateContents() since the mass upstreaming.
> 
> Dan wdyt? I will upload a new patch.

I'd still vote to call super() to avoid divergence.  I'd go with whatever Dave
says.

[Yusuf, 2015-08-26 22:25:36]

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java:422: if
(index == TabModel.INVALID_TAB_INDEX) return;
On 2015/08/26 21:59:52, dfalcantara wrote:
> On 2015/08/26 21:57:10, Ian Wen wrote:
> > On 2015/08/26 21:35:37, dfalcantara wrote:
> > > On 2015/08/26 21:24:15, Yusuf wrote:
> > > > do we need these in customtabs? It is a single tab model, so this is
> > > essentially
> > > > resetting the current tab to be the current tab.
> > > > 
> > > > I would say we should just override activateContents in customtab. Dan,
> what
> > > do
> > > > you think?
> > > 
> > > It's a no-op.  I'm worried about divergence if we ever add extra things to
> > > activateContents() and the overridden CustomTab doesn't call super().
> > 
> > I am okay with both. Yet maybe doing a no-op is a slightly cleaner way? We
> > haven't changed activateContents() since the mass upstreaming.
> > 
> > Dan wdyt? I will upload a new patch.
> 
> I'd still vote to call super() to avoid divergence.  I'd go with whatever Dave
> says.

OK then. I am not feeling strongly about either solution.

[Yusuf, 2015-08-26 23:09:57]

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1318853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java:422: if
(index == TabModel.INVALID_TAB_INDEX) return;
On 2015/08/26 22:25:36, Yusuf wrote:
> On 2015/08/26 21:59:52, dfalcantara wrote:
> > On 2015/08/26 21:57:10, Ian Wen wrote:
> > > On 2015/08/26 21:35:37, dfalcantara wrote:
> > > > On 2015/08/26 21:24:15, Yusuf wrote:
> > > > > do we need these in customtabs? It is a single tab model, so this is
> > > > essentially
> > > > > resetting the current tab to be the current tab.
> > > > > 
> > > > > I would say we should just override activateContents in customtab.
Dan,
> > what
> > > > do
> > > > > you think?
> > > > 
> > > > It's a no-op.  I'm worried about divergence if we ever add extra things
to
> > > > activateContents() and the overridden CustomTab doesn't call super().
> > > 
> > > I am okay with both. Yet maybe doing a no-op is a slightly cleaner way? We
> > > haven't changed activateContents() since the mass upstreaming.
> > > 
> > > Dan wdyt? I will upload a new patch.
> > 
> > I'd still vote to call super() to avoid divergence.  I'd go with whatever
Dave
> > says.
> 
> OK then. I am not feeling strongly about either solution.

And Dave said we should go with the solution in Patch set 1. Sorry for the back
and forth!

[Ian Wen, 2015-08-26 23:35:32]

Fell back to first patch.

PTAL. :)

[gone, 2015-08-26 23:43:40]

https://codereview.chromium.org/1318853002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1318853002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java:424:
bringTabToForeground();
This is actually about bringing the Activity back to the foreground, not the
Tab.  Please rename accordingly.

[gone, 2015-08-26 23:44:10]

https://codereview.chromium.org/1318853002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java
(right):

https://codereview.chromium.org/1318853002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:376:
protected void bringTabToForeground() { }
Also explain why this does nothing.

[Ian Wen, 2015-08-27 00:27:57]

https://codereview.chromium.org/1318853002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java
(right):

https://codereview.chromium.org/1318853002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:376:
protected void bringTabToForeground() { }
On 2015/08/26 23:44:10, dfalcantara wrote:
> Also explain why this does nothing.

Done.

https://codereview.chromium.org/1318853002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1318853002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java:424:
bringTabToForeground();
On 2015/08/26 23:43:40, dfalcantara wrote:
> This is actually about bringing the Activity back to the foreground, not the
> Tab.  Please rename accordingly.

Done.

[gone, 2015-08-27 02:37:45]

Lgtm, but you need to update the javadoc for the function to say it brings the
activity back and not the tab.

[Ian Wen, 2015-08-27 02:43:31]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2015-08-27 02:43:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1318853002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-08-27 02:43:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1318853002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1318853002/80001

[Ian Wen, 2015-08-27 02:43:44]

Done.

[commit-bot: I haz the power, 2015-08-27 03:35:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-27 03:36:23]

Patchset 5 (id:??) landed as
https://crrev.com/60b78e3530d0d0ec91e2c70f2be92c4c506623e4
Cr-Commit-Position: refs/heads/master@{#345784}