Use a scrollable tab strip on tablets at small widths

Use a scrollable tab strip on tablets at small widths

Introduces a new tab strip used when the available width is < 600dp.
Rather than cascading tabs behind each other, the new ScrollingStripStacker
places all tabs side by side and the entire strip scrolls.

Also moves the incognito badge from the right to left side in RTL and
makes strip padding consistent between RTL and LTR.

BUG=595519
Committed: https://crrev.com/1c147020cf3e777cc7522b49e3d7504732087bef
Cr-Commit-Position: refs/heads/master@{#385562}

[Theresa, 2016-03-25 17:31:19]

twellington@chromium.org changed reviewers:
+ changwan@chromium.org

[Theresa, 2016-03-25 17:31:20]

changwan@ ptal

Normally I'd send this tab strip change to dtrainor@, but he's out for two more
weeks and this needs to make it in before then. Do you feel comfortable
reviewing?

[Theresa, 2016-03-25 17:42:31]

https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java
(right):

https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:22:
public void setTabOffsets(int selectedIndex, StripLayoutTab[] indexOrderedTabs,
This used to the StripLayoutHelper#computeTabOffsetHelper. None of the logic has
changed - just a simple copy/paste w/ some variable renaming.

https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:120:
float stripWidth) {
This method is the old StaticStripStacker#performOcculsionPass() with the
exception of line 143, which was added to fix a bug w/ tab index 0 not being
visible after switching back from the ScrollingStripStacker if it was selected
but offscreen before the switch.

https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:150:
float leftEdge = stripWidth - stripRightMargin;
This and the method below are the old
StripLayoutHelper#updateNewTabButtonState() logic. I split it into two methods
but otherwise the logic is (almost) identical (see comment a few lines down).

https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:161:
leftEdge += tabOverlapWidth / 2;
This line is new. I removed the newTabButtonWidth adjustment in
StripLayoutHelper's constructor, so this gets the new tab button positioned in
the same spot.

Before, in StripLayoutHelper's constructor we had this line:
mNewTabButtonWidth -= tabOverlapWidth /2 

So line 165 below was really:
leftEdge - (newTabButtonWidth - tabOverlapWidth / 2).

https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java
(left):

https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:164:
mNewTabButtonWidth -= mTabOverlapWidth / 2;
This is now accounted for in the StripStacker's. It makes more sense to change
the new tab button position there rather than setting the new tab button width
to something less than it actually is.

https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperManager.java
(right):

https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperManager.java:139:
mModelSelectorButton.setX(
This code is changing because I discussed w/ cleer@ and the incognito badge
should be on the left side while in rtl.

[Changwan Ryu, 2016-03-29 09:16:26]

On 2016/03/25 17:42:31, Theresa Wellington wrote:
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java
> (right):
> 
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:22:
> public void setTabOffsets(int selectedIndex, StripLayoutTab[]
indexOrderedTabs,
> This used to the StripLayoutHelper#computeTabOffsetHelper. None of the logic
has
> changed - just a simple copy/paste w/ some variable renaming.
> 
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:120:
> float stripWidth) {
> This method is the old StaticStripStacker#performOcculsionPass() with the
> exception of line 143, which was added to fix a bug w/ tab index 0 not being
> visible after switching back from the ScrollingStripStacker if it was selected
> but offscreen before the switch.
> 
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:150:
> float leftEdge = stripWidth - stripRightMargin;
> This and the method below are the old
> StripLayoutHelper#updateNewTabButtonState() logic. I split it into two methods
> but otherwise the logic is (almost) identical (see comment a few lines down).
> 
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:161:
> leftEdge += tabOverlapWidth / 2;
> This line is new. I removed the newTabButtonWidth adjustment in
> StripLayoutHelper's constructor, so this gets the new tab button positioned in
> the same spot.
> 
> Before, in StripLayoutHelper's constructor we had this line:
> mNewTabButtonWidth -= tabOverlapWidth /2 
> 
> So line 165 below was really:
> leftEdge - (newTabButtonWidth - tabOverlapWidth / 2).
> 
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java
> (left):
> 
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:164:
> mNewTabButtonWidth -= mTabOverlapWidth / 2;
> This is now accounted for in the StripStacker's. It makes more sense to change
> the new tab button position there rather than setting the new tab button width
> to something less than it actually is.
> 
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperManager.java
> (right):
> 
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperManager.java:139:
> mModelSelectorButton.setX(
> This code is changing because I discussed w/ cleer@ and the incognito badge
> should be on the left side while in rtl.

Sorry I couldn't look at this yet, there were other urgent issues. I'll start
taking a look later this week.
In the meantime, could you consider adding JUnit tests if there are new behavior
/ changes in behavior that you want to test?
Please take a look at https://chromiumcodereview.appspot.com/1763713003/ for an
example.

[Theresa, 2016-03-29 15:52:50]

On 2016/03/29 09:16:26, Changwan Ryu wrote:
> On 2016/03/25 17:42:31, Theresa Wellington wrote:
> >
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:22:
> > public void setTabOffsets(int selectedIndex, StripLayoutTab[]
> indexOrderedTabs,
> > This used to the StripLayoutHelper#computeTabOffsetHelper. None of the logic
> has
> > changed - just a simple copy/paste w/ some variable renaming.
> > 
> >
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:120:
> > float stripWidth) {
> > This method is the old StaticStripStacker#performOcculsionPass() with the
> > exception of line 143, which was added to fix a bug w/ tab index 0 not being
> > visible after switching back from the ScrollingStripStacker if it was
selected
> > but offscreen before the switch.
> > 
> >
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:150:
> > float leftEdge = stripWidth - stripRightMargin;
> > This and the method below are the old
> > StripLayoutHelper#updateNewTabButtonState() logic. I split it into two
methods
> > but otherwise the logic is (almost) identical (see comment a few lines
down).
> > 
> >
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:161:
> > leftEdge += tabOverlapWidth / 2;
> > This line is new. I removed the newTabButtonWidth adjustment in
> > StripLayoutHelper's constructor, so this gets the new tab button positioned
in
> > the same spot.
> > 
> > Before, in StripLayoutHelper's constructor we had this line:
> > mNewTabButtonWidth -= tabOverlapWidth /2 
> > 
> > So line 165 below was really:
> > leftEdge - (newTabButtonWidth - tabOverlapWidth / 2).
> > 
> >
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java
> > (left):
> > 
> >
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:164:
> > mNewTabButtonWidth -= mTabOverlapWidth / 2;
> > This is now accounted for in the StripStacker's. It makes more sense to
change
> > the new tab button position there rather than setting the new tab button
width
> > to something less than it actually is.
> > 
> >
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperManager.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1836453003/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperManager.java:139:
> > mModelSelectorButton.setX(
> > This code is changing because I discussed w/ cleer@ and the incognito badge
> > should be on the left side while in rtl.
> 
> Sorry I couldn't look at this yet, there were other urgent issues. I'll start
> taking a look later this week.
> In the meantime, could you consider adding JUnit tests if there are new
behavior
> / changes in behavior that you want to test?
> Please take a look at
https://chromiumcodereview.appspot.com/1763713003/?_ga=1.132734222.1097695852...
for an
> example.

I'll look into adding unit tests, but it might come in a follow-up CL. Since
this is for Android N multi-window, we have a hard deadline, and I have a couple
of other things that need to land in the next couple of weeks as well.  I will
circle back to tests one those features are done. I also plan to do
instrumentation testing once we have a bot running N on tablets up and running.

[Theresa, 2016-03-29 16:04:58]

On 2016/03/29 15:52:50, Theresa Wellington wrote:
> On 2016/03/29 09:16:26, Changwan Ryu wrote:
> > Sorry I couldn't look at this yet, there were other urgent issues. I'll
start
> > taking a look later this week.
> > In the meantime, could you consider adding JUnit tests if there are new
> behavior
> > / changes in behavior that you want to test?
> > Please take a look at
>
https://chromiumcodereview.appspot.com/1763713003/?_ga=1.132734222.1097695852...
> for an
> > example.
> 
> I'll look into adding unit tests, but it might come in a follow-up CL. Since
> this is for Android N multi-window, we have a hard deadline, and I have a
couple
> of other things that need to land in the next couple of weeks as well.  I will
> circle back to tests one those features are done. I also plan to do
> instrumentation testing once we have a bot running N on tablets up and
running.

Or if the CL you referenced lands soon, I'll rebase and try to add a couple of
quick tests for the new ScrollingStripStacker :).

[Theresa, 2016-03-31 15:42:05]

friendly ping

[Changwan Ryu, 2016-04-04 09:37:59]

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java
(right):

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:17:
public boolean canSlideTitleText() {
This seems like a remainder of previous refactoring. Could you remove this?

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:114:
: computeNewTabButtonOffsetLtr(indexOrderedTabs, tabOverlapWidth,
shouldn't indentation be 8 characters ahead?

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:143:
if (i - 1 == selectedIndex) prevTab.setVisible(true);
hmm... why did the logic change from StaticStripStacker?

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/ScrollingStripStacker.java
(right):

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/ScrollingStripStacker.java:12:
* side and the entire strip will scroll. Tabs will will never completely overlap
each other.
s/will will/will

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/ScrollingStripStacker.java:40:
: computeNewTabButtonOffsetLtr(indexOrderedTabs, tabOverlapWidth,
shouldn't indentation be 8 characters ahead?

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/ScrollingStripStacker.java:49:
if ((tab.getDrawX() + tab.getWidth()) < 0 || tab.getDrawX() > layoutWidth) {
how about tab.setVisible(tab.getDrawX() + tab.getWidth() >= 0 && tab.getDrawX()
<= layoutWidth);

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/ScrollingStripStacker.java:57:
private float computeNewTabButtonOffsetRtl(StripLayoutTab[] indexOrderedTabs,
nit: could you keep the methods order same across files? ltl < rtl

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/ScrollingStripStacker.java:80:
for (int i = 0; i < indexOrderedTabs.length; i++) {
these are ordered tabs, do we still need to loop through them? could we just
probe the last tab? (same could be said for rtl)

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StaticStripStacker.java
(right):

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StaticStripStacker.java:13:
public abstract class StaticStripStacker implements StripStacker {
hmm... actually, do we need to keep both StaticStripStacker and StripStacker?
Would it be possible to merge them into one abstract class?

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java
(right):

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:294:
boolean useUnadjustedScrollOffset = (!isRtl && isLeft) || (isRtl && !isLeft);
How about isRtl != isLeft; ?

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:298:
if (offset == 0) {
0.f

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:303:
return offset / FADE_FULL_OPACITY_THRESHOLD_DP * 1.f;
no need to multiply by 1.f?

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:380:
// Scroll to make the selected tab visible.
Is this only applicable to scrolling strip stacker? If so could you guard it?

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:542:

nit: no need for blank line

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperManager.java
(right):

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperManager.java:286:
mNormalHelper.setRightMargin(margin);
could you change these names as well?

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabStripSceneLayer.java
(right):

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabStripSceneLayer.java:116:
? R.drawable.tab_strip_fade_incognito : R.drawable.tab_strip_fade;
is this a typo? why do we use incognito here?
also, when modelSelectorButtonVisible isn't visible, shouldn't we remove it from
the screen?

[Theresa, 2016-04-05 03:09:58]

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java
(right):

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:17:
public boolean canSlideTitleText() {
On 2016/04/04 09:37:58, Changwan Ryu wrote:
> This seems like a remainder of previous refactoring. Could you remove this?

Done. Removed since its not used out side of this class.

Without dtrainor@ here in general this is mostly speculation, but I think the
StripStacker interface was intended to be built up a bit more (based on a TODO
comment in StripLayoutHelper). I left canShowCloseButton() with that in mind.
I'm okay with this method getting removed since calculating the tab content
offset is now the responsibility of StripStacker classes.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:114:
: computeNewTabButtonOffsetLtr(indexOrderedTabs, tabOverlapWidth,
On 2016/04/04 09:37:58, Changwan Ryu wrote:
> shouldn't indentation be 8 characters ahead?

I was thinking it should be 8 chars from the ? it corresponds to, but presubmit
seems happy with either. Changed it to 8 ahead of where it is here.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/CascadingStripStacker.java:143:
if (i - 1 == selectedIndex) prevTab.setVisible(true);
On 2016/04/04 09:37:58, Changwan Ryu wrote:
> hmm... why did the logic change from StaticStripStacker?

Previously the selected tab was always guaranteed to be visible. In the
ScrollingStripStacker that's no longer true. The user may have scrolled the
selected tab out of view, causing its visibility to be false. If the first tab
is selected but not visible when we change back to the CascadingStripStacker,
this line is required to set the visibility of the first tab correctly.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/ScrollingStripStacker.java
(right):

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/ScrollingStripStacker.java:12:
* side and the entire strip will scroll. Tabs will will never completely overlap
each other.
On 2016/04/04 09:37:58, Changwan Ryu wrote:
> s/will will/will

Done.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/ScrollingStripStacker.java:40:
: computeNewTabButtonOffsetLtr(indexOrderedTabs, tabOverlapWidth,
On 2016/04/04 09:37:58, Changwan Ryu wrote:
> shouldn't indentation be 8 characters ahead?

Done.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/ScrollingStripStacker.java:49:
if ((tab.getDrawX() + tab.getWidth()) < 0 || tab.getDrawX() > layoutWidth) {
On 2016/04/04 09:37:58, Changwan Ryu wrote:
> how about tab.setVisible(tab.getDrawX() + tab.getWidth() >= 0 &&
tab.getDrawX()
> <= layoutWidth);

Done.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/ScrollingStripStacker.java:57:
private float computeNewTabButtonOffsetRtl(StripLayoutTab[] indexOrderedTabs,
On 2016/04/04 09:37:58, Changwan Ryu wrote:
> nit: could you keep the methods order same across files? ltl < rtl

Done. Now ltr comes first in both.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/ScrollingStripStacker.java:80:
for (int i = 0; i < indexOrderedTabs.length; i++) {
On 2016/04/04 09:37:58, Changwan Ryu wrote:
> these are ordered tabs, do we still need to loop through them? could we just
> probe the last tab? (same could be said for rtl)


In rtl it only needs to check the last tab (changed). In ltr, it needs to check
the last two tabs so that the new tab button gets positioned correctly when the
last tab is dying. Changed.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StaticStripStacker.java
(right):

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StaticStripStacker.java:13:
public abstract class StaticStripStacker implements StripStacker {
On 2016/04/04 09:37:58, Changwan Ryu wrote:
> hmm... actually, do we need to keep both StaticStripStacker and StripStacker?
> Would it be possible to merge them into one abstract class?

We don't technically need both. I merged them into StripStacker.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java
(right):

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:294:
boolean useUnadjustedScrollOffset = (!isRtl && isLeft) || (isRtl && !isLeft);
On 2016/04/04 09:37:58, Changwan Ryu wrote:
> How about isRtl != isLeft; ?

Done.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:298:
if (offset == 0) {
On 2016/04/04 09:37:58, Changwan Ryu wrote:
> 0.f

Done.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:303:
return offset / FADE_FULL_OPACITY_THRESHOLD_DP * 1.f;
On 2016/04/04 09:37:58, Changwan Ryu wrote:
> no need to multiply by 1.f?

Done.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:380:
// Scroll to make the selected tab visible.
On 2016/04/04 09:37:58, Changwan Ryu wrote:
> Is this only applicable to scrolling strip stacker? If so could you guard it?

I think it's still useful for the cascading strip stacker. It ensures that the
tabs close to the selected tab are also visible. I updated the comment. Can send
you screenshots if you'd like.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:542:

On 2016/04/04 09:37:59, Changwan Ryu wrote:
> nit: no need for blank line

Done.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperManager.java
(right):

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperManager.java:286:
mNormalHelper.setRightMargin(margin);
On 2016/04/04 09:37:59, Changwan Ryu wrote:
> could you change these names as well?

Done.

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabStripSceneLayer.java
(right):

https://codereview.chromium.org/1836453003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabStripSceneLayer.java:116:
? R.drawable.tab_strip_fade_incognito : R.drawable.tab_strip_fade;
On 2016/04/04 09:37:59, Changwan Ryu wrote:
> is this a typo? why do we use incognito here?
> also, when modelSelectorButtonVisible isn't visible, shouldn't we remove it
from
> the screen?

The logic is correct. If the model selector button is visible and the layout is
rtl, then the model selector button is on the left and the
"tab_strip_fade_incognito" should be used. Otherwise we still want a fade, it's
just the shorter "tab_strip_fade"

I renamed the asset "tab_strip_fade_for_model_selector" so that this makes more
sense.

[Changwan Ryu, 2016-04-05 06:20:48]

If you have WIP tests, could you also add them here?
I think it may take more time to check in
https://chromiumcodereview.appspot.com/1763713003/,
but in the meantime we could continue the review.

[Theresa, 2016-04-05 06:26:11]

On 2016/04/05 06:20:48, Changwan Ryu wrote:
> If you have WIP tests, could you also add them here?
> I think it may take more time to check in
> https://chromiumcodereview.appspot.com/1763713003/,
> but in the meantime we could continue the review.

I don't have any tests in progress yet. I want to write instrumentation tests
for the StripStacker switching and auto scrolling logic but the infrastructure
isn't in place yet (requires N bots w/ tablets).

[Theresa, 2016-04-05 21:11:10]

ptal

As discussed offline, I added tests for some of the core functionality.

Will add end-to-end tests when infra is in place.

https://codereview.chromium.org/1836453003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java
(right):

https://codereview.chromium.org/1836453003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:539:
// Select the fifth tab by setting the index directly. It may not be visible, so
don't
nit: s/fifth/sixth

https://codereview.chromium.org/1836453003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:810:
* Checks that the left and right fade opacities are as expected based on the
tab's current
s/tab's/strip's

[Theresa, 2016-04-05 21:11:10]

ptal

As discussed offline, I added tests for some of the core functionality.

Will add end-to-end tests when infra is in place.

https://codereview.chromium.org/1836453003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java
(right):

https://codereview.chromium.org/1836453003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:539:
// Select the fifth tab by setting the index directly. It may not be visible, so
don't
nit: s/fifth/sixth

https://codereview.chromium.org/1836453003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:810:
* Checks that the left and right fade opacities are as expected based on the
tab's current
s/tab's/strip's

[Changwan Ryu, 2016-04-06 01:26:02]

On 2016/04/05 21:11:10, Theresa Wellington wrote:
> ptal
> 
> As discussed offline, I added tests for some of the core functionality.
> 
> Will add end-to-end tests when infra is in place.
> 
>
https://codereview.chromium.org/1836453003/diff/100001/chrome/android/javates...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java
> (right):
> 
>
https://codereview.chromium.org/1836453003/diff/100001/chrome/android/javates...
>
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:539:
> // Select the fifth tab by setting the index directly. It may not be visible,
so
> don't
> nit: s/fifth/sixth
> 
>
https://codereview.chromium.org/1836453003/diff/100001/chrome/android/javates...
>
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:810:
> * Checks that the left and right fade opacities are as expected based on the
> tab's current
> s/tab's/strip's

Hmm... I feel that the only thing that is being tested is 'crash'. There's
slight visibility check but
I'd like to see the following structure instead:

1) set up
2) check the status (could be part of set up if repeated)
3) do something that will change the status, e.g. scroll offset, change tab
index, etc
4) check the status, make sure that it's changed, e.g. tab visibility, whether
fade resource is being applied or not.

I've referred to the JUnit4 tests because it may be difficult to write those
tests as instrumentation tests.

If we don't have enough time, I don't mind adding tests separately and merge
them later.

[Changwan Ryu, 2016-04-06 02:56:19]

On 2016/04/06 01:26:02, Changwan Ryu wrote:
> On 2016/04/05 21:11:10, Theresa Wellington wrote:
> > ptal
> > 
> > As discussed offline, I added tests for some of the core functionality.
> > 
> > Will add end-to-end tests when infra is in place.
> > 
> >
>
https://codereview.chromium.org/1836453003/diff/100001/chrome/android/javates...
> > File
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1836453003/diff/100001/chrome/android/javates...
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:539:
> > // Select the fifth tab by setting the index directly. It may not be
visible,
> so
> > don't
> > nit: s/fifth/sixth
> > 
> >
>
https://codereview.chromium.org/1836453003/diff/100001/chrome/android/javates...
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:810:
> > * Checks that the left and right fade opacities are as expected based on the
> > tab's current
> > s/tab's/strip's
> 
> Hmm... I feel that the only thing that is being tested is 'crash'. There's
> slight visibility check but
> I'd like to see the following structure instead:
> 
> 1) set up
> 2) check the status (could be part of set up if repeated)
> 3) do something that will change the status, e.g. scroll offset, change tab
> index, etc
> 4) check the status, make sure that it's changed, e.g. tab visibility, whether
> fade resource is being applied or not.
> 
> I've referred to the JUnit4 tests because it may be difficult to write those
> tests as instrumentation tests.
> 
> If we don't have enough time, I don't mind adding tests separately and merge
> them later.

Talked to twellington@ offline. I have overlooked checks in the helper function
in the test.
I still think that the current 'check' approach needs revision.
It seems to me that we're just verifying f(x) with f'(x), when f(x) = f'(x).

I'm wondering if you could use more concrete input and output? Something like
the following:

setRtl(false);

assertTabStripOffsets(0, true, 0.0f, 10.0f);
assertTabStripOffsets(1, true, 10.0f, 20.0f);
assertTabStripOffsets(2, false, 20.0f, 30.0f);
assertFadeOnRight();
assertTabCloseButtonOffset(35.0f, 40.0f);

scrollHorizontally(15.0);

assertTabStripOffsets(0, false, -15.0f, -5.0f);
assertTabStripOffsets(1, true, 5.0f, 5.0f);
assertTabStripOffsets(2, true, 5.0f, 15.0f);
assertFadeOnLeft();
assertTabCloseButtonOffset(35.0f, 40.0f);

[Changwan Ryu, 2016-04-06 02:56:43]

https://codereview.chromium.org/1836453003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java
(right):

https://codereview.chromium.org/1836453003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:209:
mStripStacker = mCascadingStripStacker;
Hmm... Can we set the correct value from the beginning?
In case of starting width < 600, we may see flickering in some situation.

[Changwan Ryu, 2016-04-06 03:02:47]

On 2016/04/06 02:56:43, Changwan Ryu wrote:
>
https://codereview.chromium.org/1836453003/diff/100001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java
> (right):
> 
>
https://codereview.chromium.org/1836453003/diff/100001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:209:
> mStripStacker = mCascadingStripStacker;
> Hmm... Can we set the correct value from the beginning?
> In case of starting width < 600, we may see flickering in some situation.

I just tried the scrolling feature per your instruction over email and it looks
really awesome!

BTW, one nit issue is that we may need to scroll to the active tab strip in the
following situations:
1) when swiping on omnibox changes the current activation
2) when gmail opens the existing tab using tab index which was in the back stack
(sorry I haven't actually tested this...)

[Theresa, 2016-04-06 06:49:51]

Per more offline discussions:

setRtl(false) isn't possible and the tab close button pos isn't set by the new
ScrollingStripStacker (it's set by the StripLayoutTab itself).

I added tests for
- asserting fade on the left/right sides explicitly (and removed the
checkTabStripFadeOpacitiy() step in compareTabStripWidth)
- checking that previously invisible tab is visible once selected
- checking tab draw X positions after scrolling various amounts

The expected draw X positions are based on the width the first tab (the width is
dynamic, so it has to be dynamically retrieved), but it's a simple calculation
that's different from the one used in the StripLayoutHelper itself.

https://chromiumcodereview.appspot.com/1836453003/diff/100001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java
(right):

https://chromiumcodereview.appspot.com/1836453003/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:209:
mStripStacker = mCascadingStripStacker;
On 2016/04/06 02:56:43, Changwan Ryu wrote:
> Hmm... Can we set the correct value from the beginning?
> In case of starting width < 600, we may see flickering in some situation.

Done. Also added logic to handle auto-scrolling when a tab is selected and a
TODO note to add logic for auto-scrolling on cold start and when the tab model
scrolls.

https://chromiumcodereview.appspot.com/1836453003/diff/100001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java
(right):

https://chromiumcodereview.appspot.com/1836453003/diff/100001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:539:
// Select the fifth tab by setting the index directly. It may not be visible, so
don't
On 2016/04/05 21:11:10, Theresa Wellington wrote:
> nit: s/fifth/sixth

Done.

https://chromiumcodereview.appspot.com/1836453003/diff/100001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:810:
* Checks that the left and right fade opacities are as expected based on the
tab's current
On 2016/04/05 21:11:10, Theresa Wellington wrote:
> s/tab's/strip's

Done.

[Changwan Ryu, 2016-04-06 07:05:38]

lgtm

https://chromiumcodereview.appspot.com/1836453003/diff/140001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java
(right):

https://chromiumcodereview.appspot.com/1836453003/diff/140001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:565:
boolean isLeft = !LocalizationUtils.isLayoutRtl();
Actually, there seems to be a way to test RTL / LTR:

https://codereview.chromium.org/1763713003/diff/80001/ui/android/java/src/org...

You can add a TODO for now.

https://chromiumcodereview.appspot.com/1836453003/diff/140001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:844:
private void setShouldCascadeTabs(final boolean shouldCascadeTabs) {
how about renaming this as setShouldCascadeTabsAndCheckTabStrips?

https://chromiumcodereview.appspot.com/1836453003/diff/140001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:861:
: "Expected ScrollingStripStacker but was CascadingStripStacker.",
nit: indentation

https://chromiumcodereview.appspot.com/1836453003/diff/140001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:883:
private void scrollTabStrip(final int scrollOffset) {
How about renaming this as assertSetTabStripScrollOffset?

[Theresa, 2016-04-06 07:13:47]

twellington@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[Theresa, 2016-04-06 07:13:48]

+dfalcantara@ for OWNERS review on java_sources.gni, TabStripTest.java and new
drawables

[gone, 2016-04-06 18:13:29]

lgtm on the files you asked me to take a look at % changwan's comments

https://chromiumcodereview.appspot.com/1836453003/diff/140001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java
(right):

https://chromiumcodereview.appspot.com/1836453003/diff/140001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:475:
getInstrumentation().waitForIdleSync();
I don't know if these are necessary -- that function already waits for idle sync
at the end of it.

[Theresa, 2016-04-06 20:24:14]

https://codereview.chromium.org/1836453003/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java
(right):

https://codereview.chromium.org/1836453003/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:475:
getInstrumentation().waitForIdleSync();
On 2016/04/06 18:13:29, dfalcantara wrote:
> I don't know if these are necessary -- that function already waits for idle
sync
> at the end of it.

You are correct. Removed.

https://codereview.chromium.org/1836453003/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:565:
boolean isLeft = !LocalizationUtils.isLayoutRtl();
On 2016/04/06 07:05:38, Changwan Ryu wrote:
> Actually, there seems to be a way to test RTL / LTR:
> 
>
https://codereview.chromium.org/1763713003/diff/80001/ui/android/java/src/org...
> 
> You can add a TODO for now.

Done.

https://codereview.chromium.org/1836453003/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:844:
private void setShouldCascadeTabs(final boolean shouldCascadeTabs) {
On 2016/04/06 07:05:38, Changwan Ryu wrote:
> how about renaming this as setShouldCascadeTabsAndCheckTabStrips?

Done.

https://codereview.chromium.org/1836453003/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:861:
: "Expected ScrollingStripStacker but was CascadingStripStacker.",
On 2016/04/06 07:05:38, Changwan Ryu wrote:
> nit: indentation

The indentation on this line should be +8 from the line above and the rest of
the lines should align with the ?. I think it's correct as is.

https://codereview.chromium.org/1836453003/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:883:
private void scrollTabStrip(final int scrollOffset) {
On 2016/04/06 07:05:38, Changwan Ryu wrote:
> How about renaming this as assertSetTabStripScrollOffset?

Done.

[Theresa, 2016-04-06 20:24:19]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2016-04-06 20:24:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
changwan@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1836453003/#ps180001
(title: "Tiny bit of clean up")

[commit-bot: I haz the power, 2016-04-06 20:24:44]

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

[commit-bot: I haz the power, 2016-04-06 21:50:48]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-04-06 21:52:30]

Description was changed from

==========
Use a scrollable tab strip on tablets at small widths

Introduces a new tab strip used when the available width is < 600dp.
Rather than cascading tabs behind each other, the new ScrollingStripStacker
places all tabs side by side and the entire strip scrolls.

Also moves the incognito badge from the right to left side in RTL and
makes strip padding consistent between RTL and LTR.

BUG=595519
==========

to

==========
Use a scrollable tab strip on tablets at small widths

Introduces a new tab strip used when the available width is < 600dp.
Rather than cascading tabs behind each other, the new ScrollingStripStacker
places all tabs side by side and the entire strip scrolls.

Also moves the incognito badge from the right to left side in RTL and
makes strip padding consistent between RTL and LTR.

BUG=595519

Committed: https://crrev.com/1c147020cf3e777cc7522b49e3d7504732087bef
Cr-Commit-Position: refs/heads/master@{#385562}
==========

[commit-bot: I haz the power, 2016-04-06 21:52:32]

Patchset 10 (id:??) landed as
https://crrev.com/1c147020cf3e777cc7522b49e3d7504732087bef
Cr-Commit-Position: refs/heads/master@{#385562}