[Sync] Add support for identifying tabbed activites

[Sync] Add support for identifying tabbed activites

Tabbed activites are different from non-tabbed activities in that they get
preserved on cold start start if no other tabbed activities are present,
as it's assumed they just haven't been loaded. Other activities, like
custom tabs, are not restored by sync.

This patch adds the plumbing for detecting if the activity is tabbed, and
persists that information within the sync window type. Windows are now
either tabbed, popup, or custom tabs.

BUG=639009
Committed: https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f
Cr-Commit-Position: refs/heads/master@{#431163}

[Nicolas Zea, 2016-11-04 22:51:29]

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

[commit-bot: I haz the power, 2016-11-04 22:52:28]

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

[commit-bot: I haz the power, 2016-11-04 23:08:11]

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

[commit-bot: I haz the power, 2016-11-04 23:08:12]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Nicolas Zea, 2016-11-05 00:21:09]

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

[commit-bot: I haz the power, 2016-11-05 00:21:52]

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

[commit-bot: I haz the power, 2016-11-05 00:26:04]

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

[commit-bot: I haz the power, 2016-11-05 00:26:05]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Nicolas Zea, 2016-11-05 19:03:58]

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

[commit-bot: I haz the power, 2016-11-05 19:04:04]

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

[Nicolas Zea, 2016-11-05 19:32:35]

Patchset #4 (id:60001) has been deleted

[Nicolas Zea, 2016-11-05 19:36:36]

Description was changed from

==========
[Sync] Add support for identifying tabbed activites

Tabbed activites are different from non-tabbed activities in that they get
restored on cold start restart. Other activities, like custom tabs, are not
restored, and are therefore considered transient from Sync's perspective.

This patch adds the plumbing for detecting if the activity is tabbed, and
persists that information within the sync window type. Windows are now
either tabbed, popup, or transient.

BUG=639009
==========

to

==========
[Sync] Add support for identifying tabbed activites

Tabbed activites are different from non-tabbed activities in that they get
preserved on cold start start if no other tabbed activities are present,
as it's assumed they just haven't been loaded. Other activities, like
custom tabs, are not restored by sync.

This patch adds the plumbing for detecting if the activity is tabbed, and
persists that information within the sync window type. Windows are now
either tabbed, popup, or custom tabs.

BUG=639009
==========

[Nicolas Zea, 2016-11-05 19:38:14]

zea@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Nicolas Zea, 2016-11-05 19:38:15]

Ted, PTAL. Checking whether the tab model is part of a tabbed activity or not
seemed like the simplest approach. I opted to query the tab persistence policy
type, but let me know if you think there's a better way (in general I tried to
keep things simple/avoid introducing new state).

[Ted C, 2016-11-08 00:41:45]

https://codereview.chromium.org/2479683006/diff/80001/components/sync/protoco...
File components/sync/protocol/session_specifics.proto (right):

https://codereview.chromium.org/2479683006/diff/80001/components/sync/protoco...
components/sync/protocol/session_specifics.proto:44: enum BrowserType {
I'll start with naive questioning here.

Can this be exposed to stuff outside of sync?  Could it then be exposed to java
(I don't know if we generate java code from our protos or not).

Ideally, I think we would expose GetSyncBrowserType in the sync delegate and we
could pass this down.

In general, I would like to avoid the instanceof check of the
TabbedModeTabPersistencePolicy because that really shouldn't belong in the
package that it is in and I want to avoid any new dependencies.

My "hope" would be that we could expose this to java, then the constructor of
TabModelSelectorImpl and TabModelImpl would take the sync type and pass it to
jni.

If we can't expose the constant, I think a boolean isTabbedTypeForSync or
something similarly gross and long would be my second option.

[Nicolas Zea, 2016-11-08 19:17:09]

ping?

[Ted C, 2016-11-08 19:18:23]

On 2016/11/08 19:17:09, Nicolas Zea wrote:
> ping?

pong?

I replied yesterday.

[Nicolas Zea, 2016-11-08 19:26:18]

On 2016/11/08 19:18:23, Ted C wrote:
> On 2016/11/08 19:17:09, Nicolas Zea wrote:
> > ping?
> 
> pong?
> 
> I replied yesterday.

blargh, sorry about that, guess I had an outdated tab open, and hadn't seen your
email :(

[Nicolas Zea, 2016-11-08 20:52:40]

PTAL

https://codereview.chromium.org/2479683006/diff/80001/components/sync/protoco...
File components/sync/protocol/session_specifics.proto (right):

https://codereview.chromium.org/2479683006/diff/80001/components/sync/protoco...
components/sync/protocol/session_specifics.proto:44: enum BrowserType {
On 2016/11/08 00:41:45, Ted C wrote:
> I'll start with naive questioning here.
> 
> Can this be exposed to stuff outside of sync?  Could it then be exposed to
java
> (I don't know if we generate java code from our protos or not).
> 
> Ideally, I think we would expose GetSyncBrowserType in the sync delegate and
we
> could pass this down.
> 
> In general, I would like to avoid the instanceof check of the
> TabbedModeTabPersistencePolicy because that really shouldn't belong in the
> package that it is in and I want to avoid any new dependencies.
> 
> My "hope" would be that we could expose this to java, then the constructor of
> TabModelSelectorImpl and TabModelImpl would take the sync type and pass it to
> jni.
> 
> If we can't expose the constant, I think a boolean isTabbedTypeForSync or
> something similarly gross and long would be my second option.

Hmm. I think there's a question of whether the TabModel should be aware of Tab
Sync semantics, or whether Tab Sync should be aware of Tab semantics. I lean
towards the latter, as Tab Sync already depends on window/tab details, and the
tab model really doesn't (and likely won't in the future) need to know anything
about Sync proto details. As such, having some way of asking the tab model "what
kind of activity are you in" is what I was thinking.

That said, we do in fact generate java code from protos, at least for
instrumentation tests (org.chromium.components.sync.protocol). Just so I
understand, what would eventually implement the GetSyncBrowserType method, and
how would it determine what to return?

I'm fine with using something different than instanceof (e.g. plumbing something
in to the TabModelSelectorImpl at construction time) if that's preferable.

[Ted C, 2016-11-08 22:55:58]

On 2016/11/08 20:52:40, Nicolas Zea wrote:
> PTAL
> 
>
https://codereview.chromium.org/2479683006/diff/80001/components/sync/protoco...
> File components/sync/protocol/session_specifics.proto (right):
> 
>
https://codereview.chromium.org/2479683006/diff/80001/components/sync/protoco...
> components/sync/protocol/session_specifics.proto:44: enum BrowserType {
> On 2016/11/08 00:41:45, Ted C wrote:
> > I'll start with naive questioning here.
> > 
> > Can this be exposed to stuff outside of sync?  Could it then be exposed to
> java
> > (I don't know if we generate java code from our protos or not).
> > 
> > Ideally, I think we would expose GetSyncBrowserType in the sync delegate and
> we
> > could pass this down.
> > 
> > In general, I would like to avoid the instanceof check of the
> > TabbedModeTabPersistencePolicy because that really shouldn't belong in the
> > package that it is in and I want to avoid any new dependencies.
> > 
> > My "hope" would be that we could expose this to java, then the constructor
of
> > TabModelSelectorImpl and TabModelImpl would take the sync type and pass it
to
> > jni.
> > 
> > If we can't expose the constant, I think a boolean isTabbedTypeForSync or
> > something similarly gross and long would be my second option.
> 
> Hmm. I think there's a question of whether the TabModel should be aware of Tab
> Sync semantics, or whether Tab Sync should be aware of Tab semantics. I lean
> towards the latter, as Tab Sync already depends on window/tab details, and the
> tab model really doesn't (and likely won't in the future) need to know
anything
> about Sync proto details. As such, having some way of asking the tab model
"what
> kind of activity are you in" is what I was thinking.
> 
> That said, we do in fact generate java code from protos, at least for
> instrumentation tests (org.chromium.components.sync.protocol). Just so I
> understand, what would eventually implement the GetSyncBrowserType method, and
> how would it determine what to return?

My thinking is that SyncedWindowDelegate would be the originator of
GetSyncBrowserType, which would replace IsTypeTabbed and IsTypePopup.

For Android, the value would be passed fully from Activity -> TabModelSelector
-> TabModel -> TabModelJniBridge -> native -> ...

And I agree that it is weird that we're mucking with sync details, but at
the same time currently that information is not needed for anything else so
coming up with an abstraction seems somewhat unnecessary as well (and it
will be clearer about the implications of changing something that it might
impact sync if it is clearly from sync).

> 
> I'm fine with using something different than instanceof (e.g. plumbing
something
> in to the TabModelSelectorImpl at construction time) if that's preferable.

I also don't feel too strongly on us using the value from the proto, but I do
want to avoid the instanceof check.  If we only have to pass a boolean isTabbed
then that is an ok "evil" in my mind.

Also, the TabModelDelegate class is very old cruft where we couldn't untangle
the dependencies, so I'd rather push it all the way through than have the
back delegate.

[Nicolas Zea, 2016-11-10 00:17:51]

Patchset #5 (id:100001) has been deleted

[Nicolas Zea, 2016-11-10 00:19:24]

Patchset #5 (id:120001) has been deleted

[Nicolas Zea, 2016-11-10 00:20:20]

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

[Nicolas Zea, 2016-11-10 00:21:00]

PTAL. I opted for plumbing a bool down into SyncedWindowGetterAndroid.

[commit-bot: I haz the power, 2016-11-10 00:21:05]

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

[commit-bot: I haz the power, 2016-11-10 00:43:22]

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

[commit-bot: I haz the power, 2016-11-10 00:43:23]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Ted C, 2016-11-10 01:13:33]

lgtm w/ one suggestion -- and fixing the failing test compilation of course :-P

https://codereview.chromium.org/2479683006/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
(right):

https://codereview.chromium.org/2479683006/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:47:
* @param isTabbedActivity Whether the {@link Activity} is tabbed.
right now this class is only used for tabbed activities, so I think you can
forgo passing it in here (and throughout this class and just pass true in
buildSelector at the bottom).

[Nicolas Zea, 2016-11-10 02:28:35]

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

[commit-bot: I haz the power, 2016-11-10 02:28:48]

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

[Nicolas Zea, 2016-11-10 02:37:42]

Done, thanks for the review!

https://codereview.chromium.org/2479683006/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
(right):

https://codereview.chromium.org/2479683006/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:47:
* @param isTabbedActivity Whether the {@link Activity} is tabbed.
On 2016/11/10 01:13:33, Ted C wrote:
> right now this class is only used for tabbed activities, so I think you can
> forgo passing it in here (and throughout this class and just pass true in
> buildSelector at the bottom).

Done.

[Nicolas Zea, 2016-11-10 02:37:49]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2016-11-10 02:37:49]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2479683006/#ps180001
(title: "Self review")

[commit-bot: I haz the power, 2016-11-10 02:38:09]

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

[commit-bot: I haz the power, 2016-11-10 03:21:17]

Description was changed from

==========
[Sync] Add support for identifying tabbed activites

Tabbed activites are different from non-tabbed activities in that they get
preserved on cold start start if no other tabbed activities are present,
as it's assumed they just haven't been loaded. Other activities, like
custom tabs, are not restored by sync.

This patch adds the plumbing for detecting if the activity is tabbed, and
persists that information within the sync window type. Windows are now
either tabbed, popup, or custom tabs.

BUG=639009
==========

to

==========
[Sync] Add support for identifying tabbed activites

Tabbed activites are different from non-tabbed activities in that they get
preserved on cold start start if no other tabbed activities are present,
as it's assumed they just haven't been loaded. Other activities, like
custom tabs, are not restored by sync.

This patch adds the plumbing for detecting if the activity is tabbed, and
persists that information within the sync window type. Windows are now
either tabbed, popup, or custom tabs.

BUG=639009
==========

[commit-bot: I haz the power, 2016-11-10 03:21:18]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2016-11-10 03:24:51]

Description was changed from

==========
[Sync] Add support for identifying tabbed activites

Tabbed activites are different from non-tabbed activities in that they get
preserved on cold start start if no other tabbed activities are present,
as it's assumed they just haven't been loaded. Other activities, like
custom tabs, are not restored by sync.

This patch adds the plumbing for detecting if the activity is tabbed, and
persists that information within the sync window type. Windows are now
either tabbed, popup, or custom tabs.

BUG=639009
==========

to

==========
[Sync] Add support for identifying tabbed activites

Tabbed activites are different from non-tabbed activities in that they get
preserved on cold start start if no other tabbed activities are present,
as it's assumed they just haven't been loaded. Other activities, like
custom tabs, are not restored by sync.

This patch adds the plumbing for detecting if the activity is tabbed, and
persists that information within the sync window type. Windows are now
either tabbed, popup, or custom tabs.

BUG=639009

Committed: https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f
Cr-Commit-Position: refs/heads/master@{#431163}
==========

[commit-bot: I haz the power, 2016-11-10 03:24:52]

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