Autoscroll new background tabs on iPad

Autoscroll new background tabs on iPad

On iPads, when many tabs are open, additional tabs opened in the background 
(through the context menu) will collapse together and remain hidden. Users will
have to manually scroll through to see newly opened ones.

This CL fixes the issue by enabling the scenario that every time a new tab is
inserted, we auto scroll the tab strip view to make it immediately visible.

BUG=228419
Review-Url: https://codereview.chromium.org/2611003002
Cr-Commit-Position: refs/heads/master@{#442106}
Committed: https://chromium.googlesource.com/chromium/src/+/df4f7e43c1e79a85246f8297f1f0111a967e6a56

[liaoyuke, 2017-01-05 21:13:55]

Description was changed from

==========
Proof of Concept: Make background tab open through link visible

Not for review. Only serves as a demon to play with.

BUG=
==========

to

==========
Autoscroll new background tabs on iPad

On iPads, when many tabs are open, additional tabs opened in the background 
(through the context menu) will collapse together and remain hidden. Users will
have to manually scroll through to see newly opened ones.

This CL fixes the issue by enabling the scenario that every time a new tab is
inserted, we auto scroll the tab strip view to make it immediately visible.

BUG=228419
==========

[liaoyuke, 2017-01-05 21:13:55]

liaoyuke@chromium.org changed reviewers:
+ rohitrao@chromium.org

[liaoyuke, 2017-01-05 21:16:50]

Patchset #1 (id:1) has been deleted

[liaoyuke, 2017-01-05 21:19:46]

Hi Rohit,

Please take a look. 

Tomorrow is the feature freeze day, I really want to have this landed by
tomorrow, so it would be great if you can take a quick look before the end of
the today or early tomorrow.

Thank you very much :)

[liaoyuke, 2017-01-05 23:12:28]

liaoyuke@chromium.org changed reviewers:
+ edchin@chromium.org

[liaoyuke, 2017-01-05 23:24:27]

liaoyuke@chromium.org changed reviewers:
- edchin@chromium.org

[liaoyuke, 2017-01-05 23:30:48]

Addressed Jason's offline comments to turn on the feature by default.

PTAL!

[rohitrao (ping after 24h), 2017-01-06 15:35:22]

Looks pretty good!

I found some edge cases that we probably want to address before landing:

1) Open a bunch of tabs, say 10 - 15.  Select the second to last one, then kill
the app from xcode to simulate a crash.  Restart and choose to restore from the
infobar.

Something weird happens in this case and we only lay out tabs in half the
tabstrip, rather than using the full width.

2) Open a bunch of tabs, say 10 - 15.  Select the last one, scroll all the way
to the right in the tabstrip, then background the app and bring it back to the
foreground.

When you foreground the app, you'll see that the tabstrip was scrolled away from
where you were originally.

3) Open a bunch of tabs, say 10 - 15.  Select the second to last one, scroll all
the way to the left in the tabstrip, then tap on the selected tab.

Selecting the current tab will cause the tabstrip to scroll all the way to the
right.

https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/chro...
File ios/chrome/browser/chrome_switches.h (right):

https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/chro...
ios/chrome/browser/chrome_switches.h:43: extern const char
kEnableTabStripAutoScrollNewTabs[];
I would remove this flag completely, since it's never really used.  If --disable
is passed, we'll turn off the feature.  If nothing is passed, it will be on by
default.

https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1257: NSUInteger
numTabsToLeft = 0;
This could use a big comment explaining the goal of this code and how the
algorithm works.

https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1259: for (TabView* tab in
_tabArray.get()) {
What is this for loop trying to do?  I think can be replaced with:

[self indexForModelIndex:tabIndex]

[liaoyuke, 2017-01-06 20:44:42]

On 2017/01/06 15:35:22, rohitrao wrote:
> Looks pretty good!
> 
> I found some edge cases that we probably want to address before landing:
> 
> 1) Open a bunch of tabs, say 10 - 15.  Select the second to last one, then
kill
> the app from xcode to simulate a crash.  Restart and choose to restore from
the
> infobar.
> 
> Something weird happens in this case and we only lay out tabs in half the
> tabstrip, rather than using the full width.
> 
> 2) Open a bunch of tabs, say 10 - 15.  Select the last one, scroll all the way
> to the right in the tabstrip, then background the app and bring it back to the
> foreground.
> 
> When you foreground the app, you'll see that the tabstrip was scrolled away
from
> where you were originally.
> 
> 3) Open a bunch of tabs, say 10 - 15.  Select the second to last one, scroll
all
> the way to the left in the tabstrip, then tap on the selected tab.
> 
> Selecting the current tab will cause the tabstrip to scroll all the way to the
> right.
> 
>
https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/chro...
> File ios/chrome/browser/chrome_switches.h (right):
> 
>
https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/chro...
> ios/chrome/browser/chrome_switches.h:43: extern const char
> kEnableTabStripAutoScrollNewTabs[];
> I would remove this flag completely, since it's never really used.  If
--disable
> is passed, we'll turn off the feature.  If nothing is passed, it will be on by
> default.
> 
>
https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/ui/t...
> File ios/chrome/browser/ui/tabs/tab_strip_controller.mm (right):
> 
>
https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/ui/t...
> ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1257: NSUInteger
> numTabsToLeft = 0;
> This could use a big comment explaining the goal of this code and how the
> algorithm works.
> 
>
https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/ui/t...
> ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1259: for (TabView* tab in
> _tabArray.get()) {
> What is this for loop trying to do?  I think can be replaced with:
> 
> [self indexForModelIndex:tabIndex]


Fixed case (1) and (3). For (2), it seems that this issue is not brought in by
autoscroll, even without the change, this issue still happens.
Currently, I'm still not sure why, but I would like to open a new bug and fix it
in a separate CL.

Please take another quick look.

Thank you very much!

[liaoyuke, 2017-01-06 20:46:49]

Fixed (1) (3), and I will open a new bug to fix (2) separately as it is not
related to the autoscroll new tabs function.

PTAL.

Thank you very much!

https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/chro...
File ios/chrome/browser/chrome_switches.h (right):

https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/chro...
ios/chrome/browser/chrome_switches.h:43: extern const char
kEnableTabStripAutoScrollNewTabs[];
On 2017/01/06 15:35:21, rohitrao wrote:
> I would remove this flag completely, since it's never really used.  If
--disable
> is passed, we'll turn off the feature.  If nothing is passed, it will be on by
> default.

Done.

https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1257: NSUInteger
numTabsToLeft = 0;
On 2017/01/06 15:35:21, rohitrao wrote:
> This could use a big comment explaining the goal of this code and how the
> algorithm works.

Done.

https://codereview.chromium.org/2611003002/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1259: for (TabView* tab in
_tabArray.get()) {
On 2017/01/06 15:35:21, rohitrao wrote:
> What is this for loop trying to do?  I think can be replaced with:
> 
> [self indexForModelIndex:tabIndex]

To find the number of non-closing tabs to the left of the current tab. It seems
to be different from indexForModelIndex. For example, say the index of current
tab is 3, and tab of index 0 is closing. So, in this case, indexForModelIndex
would return 4, while what we need is 2.

[liaoyuke, 2017-01-06 21:01:12]

Patchset #4 (id:80001) has been deleted

[rohitrao (ping after 24h), 2017-01-06 23:11:04]

https://codereview.chromium.org/2611003002/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2611003002/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/tabs/tab_strip_controller.mm:656: [self
updateContentOffsetForTabIndex:index isNewTab:NO];
One thing you might want to experiment with is changing the isNewTab argument to
be isSelectedTab.  Maybe we want to scroll a tab to be fully visible when it is
first selected, but not for subsequent taps.

https://codereview.chromium.org/2611003002/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1214: if
(experimental_flags::IsTabStripAutoScrollNewTabsEnabled() &&
Per IM discussion, this feels like a lot of added complication and it would be
nice if we could detect and handle this scenario without needing these extra
ivars.  I'll think about this as well.

[rohitrao (ping after 24h), 2017-01-06 23:21:37]

Let's go back to patchset #2 and add the isNewTab fix on top of that, but leave
out the fix for the tab restore issue.  That code looks good, so I'm happy to
approve that.

[liaoyuke, 2017-01-06 23:29:21]

Patchset #4 (id:100001) has been deleted

[liaoyuke, 2017-01-06 23:29:26]

Patchset #3 (id:60001) has been deleted

[liaoyuke, 2017-01-06 23:41:23]

On 2017/01/06 23:21:37, rohitrao wrote:
> Let's go back to patchset #2 and add the isNewTab fix on top of that, but
leave
> out the fix for the tab restore issue.  That code looks good, so I'm happy to
> approve that.

Done. PTAL!

[liaoyuke, 2017-01-06 23:42:05]

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

[commit-bot: I haz the power, 2017-01-06 23:42:41]

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

[rohitrao (ping after 24h), 2017-01-06 23:46:54]

lgtm

https://codereview.chromium.org/2611003002/diff/120001/ios/chrome/browser/exp...
File ios/chrome/browser/experimental_flags.h (right):

https://codereview.chromium.org/2611003002/diff/120001/ios/chrome/browser/exp...
ios/chrome/browser/experimental_flags.h:109: // Whether the tab strip auto
scroll new tabs enabled for iPad or not.
// Whether or not the tab strip scrolls new tabs to be visible.

https://codereview.chromium.org/2611003002/diff/120001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2611003002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1260: // The following code
calculates the amount of scroll amount needed to make
Typo: "amount" appears twice.

https://codereview.chromium.org/2611003002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1266: NSUInteger
numNonClosingTabsToLeft = 0;
Much clearer, thank you.

[liaoyuke, 2017-01-06 23:55:01]

Thank you for reviewing the code! It's really nice working with you :)

Thank you very much!

https://codereview.chromium.org/2611003002/diff/120001/ios/chrome/browser/exp...
File ios/chrome/browser/experimental_flags.h (right):

https://codereview.chromium.org/2611003002/diff/120001/ios/chrome/browser/exp...
ios/chrome/browser/experimental_flags.h:109: // Whether the tab strip auto
scroll new tabs enabled for iPad or not.
On 2017/01/06 23:46:54, rohitrao wrote:
> // Whether or not the tab strip scrolls new tabs to be visible.

Done.

https://codereview.chromium.org/2611003002/diff/120001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/2611003002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/tabs/tab_strip_controller.mm:1266: NSUInteger
numNonClosingTabsToLeft = 0;
On 2017/01/06 23:46:54, rohitrao wrote:
> Much clearer, thank you.

Acknowledged.

[liaoyuke, 2017-01-07 00:09:59]

The CQ bit was checked by liaoyuke@chromium.org

[liaoyuke, 2017-01-07 00:09:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2611003002/#ps140001
(title: "Updated comments")

[commit-bot: I haz the power, 2017-01-07 00:10:38]

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, 2017-01-07 00:21:15]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483747799096290,
"parent_rev": "d890f99cb3e60ad71c087b4d9a628c0c400aa196", "commit_rev":
"df4f7e43c1e79a85246f8297f1f0111a967e6a56"}

[commit-bot: I haz the power, 2017-01-07 00:21:44]

Description was changed from

==========
Autoscroll new background tabs on iPad

On iPads, when many tabs are open, additional tabs opened in the background 
(through the context menu) will collapse together and remain hidden. Users will
have to manually scroll through to see newly opened ones.

This CL fixes the issue by enabling the scenario that every time a new tab is
inserted, we auto scroll the tab strip view to make it immediately visible.

BUG=228419
==========

to

==========
Autoscroll new background tabs on iPad

On iPads, when many tabs are open, additional tabs opened in the background 
(through the context menu) will collapse together and remain hidden. Users will
have to manually scroll through to see newly opened ones.

This CL fixes the issue by enabling the scenario that every time a new tab is
inserted, we auto scroll the tab strip view to make it immediately visible.

BUG=228419

Review-Url: https://codereview.chromium.org/2611003002
Cr-Commit-Position: refs/heads/master@{#442106}
Committed:
https://chromium.googlesource.com/chromium/src/+/df4f7e43c1e79a85246f8297f1f0...
==========

[commit-bot: I haz the power, 2017-01-07 00:21:45]

Committed patchset #4 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/df4f7e43c1e79a85246f8297f1f0...