Upstream TabModel and related classes.

Upstream TabModel and related classes.

BUG=318769
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269598

[Bernhard Bauer, 2014-03-24 16:26:48]

Please review.

[Yaron, 2014-03-24 18:01:13]

On 2014/03/24 16:26:48, Bernhard Bauer wrote:
> Please review.

Do you have a follow-up CL for how this is used? This seems like a good step of
moving the code to chromium but without any client access of any form, it's kind
of just a "does it compile?" check. Are you planning to make ChromeShell use
this at all?

[Bernhard Bauer, 2014-03-24 18:19:20]

On 2014/03/24 18:01:13, Yaron wrote:
> On 2014/03/24 16:26:48, Bernhard Bauer wrote:
> > Please review.
> 
> Do you have a follow-up CL for how this is used? This seems like a good step
of
> moving the code to chromium but without any client access of any form, it's
kind
> of just a "does it compile?" check. Are you planning to make ChromeShell use
> this at all?

Yes, ChromeShell is going to use this for the accessibility tab switcher. That
requires an upstream implementation of TabModel and TabModelSelector though,
which I'm still working on (see
https://chrome-internal-review.googlesource.com/#/c/157872/ for part of that).

[Yaron, 2014-03-25 02:10:44]

On 2014/03/24 18:19:20, Bernhard Bauer wrote:
> On 2014/03/24 18:01:13, Yaron wrote:
> > On 2014/03/24 16:26:48, Bernhard Bauer wrote:
> > > Please review.
> > 
> > Do you have a follow-up CL for how this is used? This seems like a good step
> of
> > moving the code to chromium but without any client access of any form, it's
> kind
> > of just a "does it compile?" check. Are you planning to make ChromeShell use
> > this at all?
> 
> Yes, ChromeShell is going to use this for the accessibility tab switcher. That
> requires an upstream implementation of TabModel and TabModelSelector though,
> which I'm still working on (see
> https://chrome-internal-review.googlesource.com/#/c/157872/ for part of that).

So I think this could wait until we have a client. I appreciate small changes
but this is basically a no-op and just code dump at this point. It also makes
internal changes harder to make because they're across repos without the benefit
of being in chromium.

[Yaron, 2014-03-25 02:12:20]

newt: see question I raised

https://codereview.chromium.org/207743004/diff/1/chrome/android/java/src/org/...
File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java
(right):

https://codereview.chromium.org/207743004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:16:
public static final String NTP_CACHED_TAG = "#cached_ntp";
Hmm. I think this is also dead at this point with the "Classic" NTP. +Newton

[Bernhard Bauer, 2014-03-25 10:36:13]

On 2014/03/25 02:10:44, Yaron wrote:
> On 2014/03/24 18:19:20, Bernhard Bauer wrote:
> > On 2014/03/24 18:01:13, Yaron wrote:
> > > On 2014/03/24 16:26:48, Bernhard Bauer wrote:
> > > > Please review.
> > > 
> > > Do you have a follow-up CL for how this is used? This seems like a good
step
> > of
> > > moving the code to chromium but without any client access of any form,
it's
> > kind
> > > of just a "does it compile?" check. Are you planning to make ChromeShell
use
> > > this at all?
> > 
> > Yes, ChromeShell is going to use this for the accessibility tab switcher.
That
> > requires an upstream implementation of TabModel and TabModelSelector though,
> > which I'm still working on (see
> > https://chrome-internal-review.googlesource.com/#/c/157872/ for part of
that).
> 
> So I think this could wait until we have a client. I appreciate small changes
> but this is basically a no-op and just code dump at this point. It also makes
> internal changes harder to make because they're across repos without the
benefit
> of being in chromium.

Umm, how can I land a client upstream if one of its dependencies isn't upstream
yet?

On a more general note, would it be worth chatting about the general strategy
for tabs, tab model and tab switcher some time? Now that The Big Renaming is
done, it might be a good time to make sure we're all on the same page about the
next steps.

[Yaron, 2014-03-25 20:34:58]

On 2014/03/25 10:36:13, Bernhard Bauer wrote:
> On 2014/03/25 02:10:44, Yaron wrote:
> > On 2014/03/24 18:19:20, Bernhard Bauer wrote:
> > > On 2014/03/24 18:01:13, Yaron wrote:
> > > > On 2014/03/24 16:26:48, Bernhard Bauer wrote:
> > > > > Please review.
> > > > 
> > > > Do you have a follow-up CL for how this is used? This seems like a good
> step
> > > of
> > > > moving the code to chromium but without any client access of any form,
> it's
> > > kind
> > > > of just a "does it compile?" check. Are you planning to make ChromeShell
> use
> > > > this at all?
> > > 
> > > Yes, ChromeShell is going to use this for the accessibility tab switcher.
> That
> > > requires an upstream implementation of TabModel and TabModelSelector
though,
> > > which I'm still working on (see
> > > https://chrome-internal-review.googlesource.com/#/c/157872/ for part of
> that).
> > 
> > So I think this could wait until we have a client. I appreciate small
changes
> > but this is basically a no-op and just code dump at this point. It also
makes
> > internal changes harder to make because they're across repos without the
> benefit
> > of being in chromium.
> 
> Umm, how can I land a client upstream if one of its dependencies isn't
upstream
> yet?

Either in the same change or back to back. It wasn't clear if the client will be
ready on the order of days or weeks..
> 
> On a more general note, would it be worth chatting about the general strategy
> for tabs, tab model and tab switcher some time? Now that The Big Renaming is
> done, it might be a good time to make sure we're all on the same page about
the
> next steps.

sgtm. Any design doc or class diagram you have would be helpful as well

[Ted C, 2014-03-25 21:34:50]

On 2014/03/25 20:34:58, Yaron wrote:
> On 2014/03/25 10:36:13, Bernhard Bauer wrote:
> > On 2014/03/25 02:10:44, Yaron wrote:
> > > On 2014/03/24 18:19:20, Bernhard Bauer wrote:
> > > > On 2014/03/24 18:01:13, Yaron wrote:
> > > > > On 2014/03/24 16:26:48, Bernhard Bauer wrote:
> > > > > > Please review.
> > > > > 
> > > > > Do you have a follow-up CL for how this is used? This seems like a
good
> > step
> > > > of
> > > > > moving the code to chromium but without any client access of any form,
> > it's
> > > > kind
> > > > > of just a "does it compile?" check. Are you planning to make
ChromeShell
> > use
> > > > > this at all?
> > > > 
> > > > Yes, ChromeShell is going to use this for the accessibility tab
switcher.
> > That
> > > > requires an upstream implementation of TabModel and TabModelSelector
> though,
> > > > which I'm still working on (see
> > > > https://chrome-internal-review.googlesource.com/#/c/157872/ for part of
> > that).
> > > 
> > > So I think this could wait until we have a client. I appreciate small
> changes
> > > but this is basically a no-op and just code dump at this point. It also
> makes
> > > internal changes harder to make because they're across repos without the
> > benefit
> > > of being in chromium.
> > 
> > Umm, how can I land a client upstream if one of its dependencies isn't
> upstream
> > yet?
> 
> Either in the same change or back to back. It wasn't clear if the client will
be
> ready on the order of days or weeks..
> > 
> > On a more general note, would it be worth chatting about the general
strategy
> > for tabs, tab model and tab switcher some time? Now that The Big Renaming is
> > done, it might be a good time to make sure we're all on the same page about
> the
> > next steps.
> 
> sgtm. Any design doc or class diagram you have would be helpful as well

I concur :-)

[newt (away), 2014-03-25 23:19:19]

http://youtu.be/i5j1wWY-qus?t=31s

https://codereview.chromium.org/207743004/diff/1/chrome/android/java/src/org/...
File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java
(right):

https://codereview.chromium.org/207743004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:16:
public static final String NTP_CACHED_TAG = "#cached_ntp";
On 2014/03/25 02:12:20, Yaron wrote:
> Hmm. I think this is also dead at this point with the "Classic" NTP. +Newton

Let's keep this around for now. We're waiting until after the M35 branch point
to start removing WebUI NTP code. (see
https://code.google.com/p/chromium/issues/detail?id=284770 )

[Bernhard Bauer, 2014-03-26 17:13:33]

On 2014/03/25 20:34:58, Yaron wrote:
> On 2014/03/25 10:36:13, Bernhard Bauer wrote:
> > On 2014/03/25 02:10:44, Yaron wrote:
> > > On 2014/03/24 18:19:20, Bernhard Bauer wrote:
> > > > On 2014/03/24 18:01:13, Yaron wrote:
> > > > > On 2014/03/24 16:26:48, Bernhard Bauer wrote:
> > > > > > Please review.
> > > > > 
> > > > > Do you have a follow-up CL for how this is used? This seems like a
good
> > step
> > > > of
> > > > > moving the code to chromium but without any client access of any form,
> > it's
> > > > kind
> > > > > of just a "does it compile?" check. Are you planning to make
ChromeShell
> > use
> > > > > this at all?
> > > > 
> > > > Yes, ChromeShell is going to use this for the accessibility tab
switcher.
> > That
> > > > requires an upstream implementation of TabModel and TabModelSelector
> though,
> > > > which I'm still working on (see
> > > > https://chrome-internal-review.googlesource.com/#/c/157872/ for part of
> > that).
> > > 
> > > So I think this could wait until we have a client. I appreciate small
> changes
> > > but this is basically a no-op and just code dump at this point. It also
> makes
> > > internal changes harder to make because they're across repos without the
> > benefit
> > > of being in chromium.
> > 
> > Umm, how can I land a client upstream if one of its dependencies isn't
> upstream
> > yet?
> 
> Either in the same change or back to back. It wasn't clear if the client will
be
> ready on the order of days or weeks..

I can upstream the accessibility tab switcher classes almost immediately (even
in this CL, if you prefer that), and that would be a client of
TabModel(Selector). Of course, then those classes wouldn't actually get used
upstream yet...

> > On a more general note, would it be worth chatting about the general
strategy
> > for tabs, tab model and tab switcher some time? Now that The Big Renaming is
> > done, it might be a good time to make sure we're all on the same page about
> the
> > next steps.
> 
> sgtm. Any design doc or class diagram you have would be helpful as well

I have a list of tasks I have done so far and am planning on doing at go/pbrml.
That is only what's necessary for the tab switcher though. There are some other
things about the future of TabModel/TabModelImpl and ChromeTab/Tab etc. that I'm
not quite sure about.

[Yaron, 2014-04-01 05:55:37]

On 2014/03/26 17:13:33, Bernhard (OOO until April 2) wrote:
> On 2014/03/25 20:34:58, Yaron wrote:
> > On 2014/03/25 10:36:13, Bernhard Bauer wrote:
> > > On 2014/03/25 02:10:44, Yaron wrote:
> > > > On 2014/03/24 18:19:20, Bernhard Bauer wrote:
> > > > > On 2014/03/24 18:01:13, Yaron wrote:
> > > > > > On 2014/03/24 16:26:48, Bernhard Bauer wrote:
> > > > > > > Please review.
> > > > > > 
> > > > > > Do you have a follow-up CL for how this is used? This seems like a
> good
> > > step
> > > > > of
> > > > > > moving the code to chromium but without any client access of any
form,
> > > it's
> > > > > kind
> > > > > > of just a "does it compile?" check. Are you planning to make
> ChromeShell
> > > use
> > > > > > this at all?
> > > > > 
> > > > > Yes, ChromeShell is going to use this for the accessibility tab
> switcher.
> > > That
> > > > > requires an upstream implementation of TabModel and TabModelSelector
> > though,
> > > > > which I'm still working on (see
> > > > > https://chrome-internal-review.googlesource.com/#/c/157872/ for part
of
> > > that).
> > > > 
> > > > So I think this could wait until we have a client. I appreciate small
> > changes
> > > > but this is basically a no-op and just code dump at this point. It also
> > makes
> > > > internal changes harder to make because they're across repos without the
> > > benefit
> > > > of being in chromium.
> > > 
> > > Umm, how can I land a client upstream if one of its dependencies isn't
> > upstream
> > > yet?
> > 
> > Either in the same change or back to back. It wasn't clear if the client
will
> be
> > ready on the order of days or weeks..
> 
> I can upstream the accessibility tab switcher classes almost immediately (even
> in this CL, if you prefer that), and that would be a client of
> TabModel(Selector). Of course, then those classes wouldn't actually get used
> upstream yet...

If these are just straight copies from internal repo, I don't mind it being a
large CL as it's essentially a rubberstamp review.
I guess I'd like to see a client usage cleaned/prepared downstream and then we
can bring the logical component over in a CL.
wdyt? Happy to meet and discuss when you're back
 
> 
> > > On a more general note, would it be worth chatting about the general
> strategy
> > > for tabs, tab model and tab switcher some time? Now that The Big Renaming
is
> > > done, it might be a good time to make sure we're all on the same page
about
> > the
> > > next steps.
> > 
> > sgtm. Any design doc or class diagram you have would be helpful as well
> 
> I have a list of tasks I have done so far and am planning on doing at
go/pbrml.
> That is only what's necessary for the tab switcher though. There are some
other
> things about the future of TabModel/TabModelImpl and ChromeTab/Tab etc. that
I'm
> not quite sure about.

[Bernhard Bauer, 2014-04-02 17:32:18]

On 2014/04/01 05:55:37, Yaron wrote:
> If these are just straight copies from internal repo, I don't mind it being a
> large CL as it's essentially a rubberstamp review.
> I guess I'd like to see a client usage cleaned/prepared downstream and then we
> can bring the logical component over in a CL.
> wdyt? Happy to meet and discuss when you're back

Yes, that'd be good. I sent y'all an invite for tomorrow; feel free to
reschedule.

[Bernhard Bauer, 2014-05-09 15:33:03]

PTAL; This now contains only the interfaces.

[Yaron, 2014-05-09 16:03:01]

lgtm

[Bernhard Bauer, 2014-05-09 16:08:00]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2014-05-09 16:10:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bauerb@chromium.org/207743004/50001

[commit-bot: I haz the power, 2014-05-10 18:34:41]

Change committed as 269598