Last_n: do not save snapshot of custom tabs.

Last_n: do not save snapshot of custom tabs.

Custom tabs are run almost exactly as regular tabs and last_n was saving
snapshots of them when they were being hidden. This is not the right thing
to do because these tabs are tabs are generally short lived unless they
are transferred into Chrome by the user.

This change adds the ability for native code to check if an Android tab
is a custom tab and uses this new functionality to avoid saving last_n
snapshots if that's the case.

BUG=636048
Review-Url: https://codereview.chromium.org/2773273002
Cr-Commit-Position: refs/heads/master@{#460242}
Committed: https://chromium.googlesource.com/chromium/src/+/6619bb4ebd6d8a47b7a0a0185d86bc90d7f80f56

[carlosk, 2017-03-27 19:47:00]

The CQ bit was checked by carlosk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 19:47:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[carlosk, 2017-03-27 19:51:40]

carlosk@chromium.org changed reviewers:
+ dewittj@chromium.org, dfalcantara@chromium.org, yusufo@chromium.org

[carlosk, 2017-03-27 19:51:41]

dewittj@: PTAL at chrome/browser/android/offline_pages/ files.

dfalcantara@: PTAL at remaining chrome/browser/ and chrome/android files

yusufo@: PTAL at how I'm exposing custom tab info.

[commit-bot: I haz the power, 2017-03-27 20:30:19]

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

[commit-bot: I haz the power, 2017-03-27 20:30:20]

Dry run: This issue passed the CQ dry run.

[dewittj, 2017-03-27 20:37:45]

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_utils.h (right):

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_utils.h:73: static bool
IsCurrentlyACustomTab(content::WebContents* web_contents);
nit: a web contents isn't a custom tab, it is shown inside one. So, maybe
CurrentlyShownInCustomTab is better?

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right):

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:412: 
Is there a way to test with a real TabAndroid, and tell it to go from CustomTab
to NotCustomTab?  Could be an integration test.  This would be a nice safeguard
against unexpected WasHidden events.

[gone, 2017-03-27 21:35:22]

Deferring to Yusuf on everything because he owns custom tabs.

[Yusuf, 2017-03-27 21:42:45]

customtabs related bits in tab_android and Tab.java lgtm

Thanks Carlos!

[gone, 2017-03-27 22:22:10]

OWNERS lgtm

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_utils.h (right):

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_utils.h:73: static bool
IsCurrentlyACustomTab(content::WebContents* web_contents);
On 2017/03/27 20:37:45, dewittj wrote:
> nit: a web contents isn't a custom tab, it is shown inside one. So, maybe
> CurrentlyShownInCustomTab is better?

+1

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right):

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:412: 
On 2017/03/27 20:37:45, dewittj wrote:
> Is there a way to test with a real TabAndroid, and tell it to go from
CustomTab
> to NotCustomTab?  Could be an integration test.  This would be a nice
safeguard
> against unexpected WasHidden events.

You could probably do this using a Java test... could be potentially really
flaky though because you'd need to start and track two different Android
Activities.

[Yusuf, 2017-03-27 22:28:27]

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right):

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:412: 
On 2017/03/27 22:22:10, dfalcantara (load balance plz) wrote:
> On 2017/03/27 20:37:45, dewittj wrote:
> > Is there a way to test with a real TabAndroid, and tell it to go from
> CustomTab
> > to NotCustomTab?  Could be an integration test.  This would be a nice
> safeguard
> > against unexpected WasHidden events.
> 
> You could probably do this using a Java test... could be potentially really
> flaky though because you'd need to start and track two different Android
> Activities.

See CustomTabActivityTest#testTabReparentingBasic and the other ones there for
examples. Maybe you can call this API there and assert it gives the right value
before and after?

[carlosk, 2017-03-28 01:47:56]

The CQ bit was checked by carlosk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 01:48:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[carlosk, 2017-03-28 02:00:32]

Thanks!

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_utils.h (right):

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_utils.h:73: static bool
IsCurrentlyACustomTab(content::WebContents* web_contents);
On 2017/03/27 22:22:10, dfalcantara (load balance plz) wrote:
> On 2017/03/27 20:37:45, dewittj wrote:
> > nit: a web contents isn't a custom tab, it is shown inside one. So, maybe
> > CurrentlyShownInCustomTab is better?
> 
> +1

Done.

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right):

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:412: 
On 2017/03/27 22:28:27, Yusuf wrote:
> On 2017/03/27 22:22:10, dfalcantara (load balance plz) wrote:
> > On 2017/03/27 20:37:45, dewittj wrote:
> > > Is there a way to test with a real TabAndroid, and tell it to go from
> > CustomTab
> > > to NotCustomTab?  Could be an integration test.  This would be a nice
> > safeguard
> > > against unexpected WasHidden events.
> > 
> > You could probably do this using a Java test... could be potentially really
> > flaky though because you'd need to start and track two different Android
> > Activities.
> 
> See CustomTabActivityTest#testTabReparentingBasic and the other ones there for
> examples. Maybe you can call this API there and assert it gives the right
value
> before and after?

I added asserts to CustomTabActivityTestBase.startCustomTabActivityWithIntent
and CustomTabActivityTest.reparentAndVerifyTab.

dewittj@: I think these changes should cover your concern. As long as that
method returns the correct state, WasHidden can be called at will. But while
updating these tests I found out that upon Activity reparenting there is a
specific assert to verify that WasHidden is *not* called [1]. So we should be
double-safe here.

yusufo@: should I also change CustomTabActivityTest.testOpenInBrowser to run
those checks? I experienced with it and it seemed it would make it very similar
to reparentAndVerifyTab. WDYT?


[1]
https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu...

[commit-bot: I haz the power, 2017-03-28 02:32:52]

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

[commit-bot: I haz the power, 2017-03-28 02:32:53]

Dry run: This issue passed the CQ dry run.

[dewittj, 2017-03-28 16:44:45]

lgtm

[Yusuf, 2017-03-28 17:17:54]

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right):

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:412: 
On 2017/03/28 02:00:32, carlosk wrote:
> On 2017/03/27 22:28:27, Yusuf wrote:
> > On 2017/03/27 22:22:10, dfalcantara (load balance plz) wrote:
> > > On 2017/03/27 20:37:45, dewittj wrote:
> > > > Is there a way to test with a real TabAndroid, and tell it to go from
> > > CustomTab
> > > > to NotCustomTab?  Could be an integration test.  This would be a nice
> > > safeguard
> > > > against unexpected WasHidden events.
> > > 
> > > You could probably do this using a Java test... could be potentially
really
> > > flaky though because you'd need to start and track two different Android
> > > Activities.
> > 
> > See CustomTabActivityTest#testTabReparentingBasic and the other ones there
for
> > examples. Maybe you can call this API there and assert it gives the right
> value
> > before and after?
> 
> I added asserts to CustomTabActivityTestBase.startCustomTabActivityWithIntent
> and CustomTabActivityTest.reparentAndVerifyTab.
> 
> dewittj@: I think these changes should cover your concern. As long as that
> method returns the correct state, WasHidden can be called at will. But while
> updating these tests I found out that upon Activity reparenting there is a
> specific assert to verify that WasHidden is *not* called [1]. So we should be
> double-safe here.
> 
> yusufo@: should I also change CustomTabActivityTest.testOpenInBrowser to run
> those checks? I experienced with it and it seemed it would make it very
similar
> to reparentAndVerifyTab. WDYT?
> 
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu...

testOpenInBrowser actually tests the default behavior only (where we dont have
any default browser set on the bot), so "Open in Browser" means no default is
set. "Open in Chrome" means the default is Chrome. So reparenting wont happen in
that test.

[carlosk, 2017-03-28 23:22:44]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2017-03-28 23:22:44]

The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2773273002/#ps20001
(title: "Renamed method; added custom tab checks to tests.")

[commit-bot: I haz the power, 2017-03-28 23:23:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[carlosk, 2017-03-28 23:26:07]

Thanks.

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right):

https://codereview.chromium.org/2773273002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:412: 
On 2017/03/28 17:17:54, Yusuf wrote:
> On 2017/03/28 02:00:32, carlosk wrote:
> > On 2017/03/27 22:28:27, Yusuf wrote:
> > > On 2017/03/27 22:22:10, dfalcantara (load balance plz) wrote:
> > > > On 2017/03/27 20:37:45, dewittj wrote:
> > > > > Is there a way to test with a real TabAndroid, and tell it to go from
> > > > CustomTab
> > > > > to NotCustomTab?  Could be an integration test.  This would be a nice
> > > > safeguard
> > > > > against unexpected WasHidden events.
> > > > 
> > > > You could probably do this using a Java test... could be potentially
> really
> > > > flaky though because you'd need to start and track two different Android
> > > > Activities.
> > > 
> > > See CustomTabActivityTest#testTabReparentingBasic and the other ones there
> for
> > > examples. Maybe you can call this API there and assert it gives the right
> > value
> > > before and after?
> > 
> > I added asserts to
CustomTabActivityTestBase.startCustomTabActivityWithIntent
> > and CustomTabActivityTest.reparentAndVerifyTab.
> > 
> > dewittj@: I think these changes should cover your concern. As long as that
> > method returns the correct state, WasHidden can be called at will. But while
> > updating these tests I found out that upon Activity reparenting there is a
> > specific assert to verify that WasHidden is *not* called [1]. So we should
be
> > double-safe here.
> > 
> > yusufo@: should I also change CustomTabActivityTest.testOpenInBrowser to run
> > those checks? I experienced with it and it seemed it would make it very
> similar
> > to reparentAndVerifyTab. WDYT?
> > 
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu...
> 
> testOpenInBrowser actually tests the default behavior only (where we dont have
> any default browser set on the bot), so "Open in Browser" means no default is
> set. "Open in Chrome" means the default is Chrome. So reparenting wont happen
in
> that test.

I see. It's confusing though that in the comment of that test it is mentioned
"Open in Chrome".

As I couldn't find any test specifically named after Chrome I assumed you were
satisfied with the test changes as they are. Let me know if I missed something.

[commit-bot: I haz the power, 2017-03-28 23:50:54]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490743364263350,
"parent_rev": "509a48c8e7a2e5c1db065d49fb1b8453acd54a02", "commit_rev":
"6619bb4ebd6d8a47b7a0a0185d86bc90d7f80f56"}

[commit-bot: I haz the power, 2017-03-28 23:52:03]

Description was changed from

==========
Last_n: do not save snapshot of custom tabs.

Custom tabs are run almost exactly as regular tabs and last_n was saving
snapshots of them when they were being hidden. This is not the right thing
to do because these tabs are tabs are generally short lived unless they
are transferred into Chrome by the user.

This change adds the ability for native code to check if an Android tab
is a custom tab and uses this new functionality to avoid saving last_n
snapshots if that's the case.

BUG=636048
==========

to

==========
Last_n: do not save snapshot of custom tabs.

Custom tabs are run almost exactly as regular tabs and last_n was saving
snapshots of them when they were being hidden. This is not the right thing
to do because these tabs are tabs are generally short lived unless they
are transferred into Chrome by the user.

This change adds the ability for native code to check if an Android tab
is a custom tab and uses this new functionality to avoid saving last_n
snapshots if that's the case.

BUG=636048

Review-Url: https://codereview.chromium.org/2773273002
Cr-Commit-Position: refs/heads/master@{#460242}
Committed:
https://chromium.googlesource.com/chromium/src/+/6619bb4ebd6d8a47b7a0a0185d86...
==========

[commit-bot: I haz the power, 2017-03-28 23:52:04]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/6619bb4ebd6d8a47b7a0a0185d86...