Use correct snapshot for the tab transition animation on iPad

Use correct snapshot for the tab transition animation on iPad.

The TabSwitcherTransitionContextContent stores the index of the tab from
which the user animated in. However the tabs can be manipulated in the
switcher (added removed) and so the tab at index i will not necessarily
be the same as the tab that was originally at index i when first
entering the switcher.

This can lead to an incorrect snapshot being used.

To reproduce:

1. Have a clean setup, just a single tab.
2. Navigate to a website which looks different to Google.com
e.g. cnn.com
3. Enter the tab switcher (there should only be one tab)
4. Close the tab in the switcher
5. Hit the plus (+) button to create a new tab
6. The new tab will animate to full screen with the old (cnn.com)
snapshot

This change removes the use of an index to track the validity of the
tab snapshots. Instead using the tabID of the Tab
object itself to see if it's different.

BUG=706891
TEST=Have a clean setup, just a single tab. Navigate to (e.g.) cnn.com. 
Enter the tab switcher. Close the tab in the switcher. Hit the plus (+) 
button to create a new tab. The new tab will animate to full screen with 
a correct snapshot.

Review-Url: https://codereview.chromium.org/2776083003
Cr-Commit-Position: refs/heads/master@{#460905}
Committed: https://chromium.googlesource.com/chromium/src/+/b6d1b157721d10aebaca323f514ba7d5a09bdcef

[PL, 2017-03-27 22:00:02]

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

[commit-bot: I haz the power, 2017-03-27 22:00:41]

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

[PL, 2017-03-27 22:21:29]

Description was changed from

==========
There is a bug which reuses an incorrect snapshot for the tab transition
animation on iPad.

The TabSwitcherTransitionContextContent stores the index of the tab from
which the user animated in. However the tabs can be manipulated in the
switcher (added removed) and so the tab at index i will not necessarily
be the same as the tab that was originally at index i when first
entering the switcher.

This can lead to an incorrect snapshot being used.

To reproduce:

1. Have a clean setup, just a single tab.
2. Navigate to a website which looks different to Google.com
e.g. cnn.com
3. Enter the tab switcher (there should only be one tab)
4. Close the tab in the switcher
5. Hit the plus (+) button to create a new tab
6. The new tab will animate to full screen with the old (cnn.com)
snapshot

This change removes the use of an index to track the validity of the
tab snapshots. Instead we are using the whole reference to the tab
object itself to see if it's different.

The original bug is not immediately unit-testable, as this change
really removes the scope of the original bug altogether. There is
a new method on TabSwitcherController that compares transition
objects with ones passed in to see if they're different which
it would be possible to write a unit test for, however it feels
that this may just be testing a basic equality between transition
contexts so I'm not convinced the utility is very high. Very open
to any feedback on this part!

I'm using pointer equality rather than isEqual: calls, which
might be something to reconsider in a different CL, however
this is the technique that is used in the shipping code so I think
this is ok for now.

The context object is storing a lot of data (images for example) but
adding a strong reference to an entire tab may keep a Tab object in
memory longer than before and there is a concern about any memory
growth; however I haven't seen an increase in memory usage in
Instruments. Feedback sought on this approach!
==========

to

==========
There is a bug which reuses an incorrect snapshot for the tab transition
animation on iPad.

The TabSwitcherTransitionContextContent stores the index of the tab from
which the user animated in. However the tabs can be manipulated in the
switcher (added removed) and so the tab at index i will not necessarily
be the same as the tab that was originally at index i when first
entering the switcher.

This can lead to an incorrect snapshot being used.

To reproduce:

1. Have a clean setup, just a single tab.
2. Navigate to a website which looks different to Google.com
e.g. cnn.com
3. Enter the tab switcher (there should only be one tab)
4. Close the tab in the switcher
5. Hit the plus (+) button to create a new tab
6. The new tab will animate to full screen with the old (cnn.com)
snapshot

This change removes the use of an index to track the validity of the
tab snapshots. Instead we are using the whole reference to the tab
object itself to see if it's different.

The original bug is not immediately unit-testable, as this change
really removes the scope of the original bug altogether. There is
a new method on TabSwitcherController that compares transition
objects with ones passed in to see if they're different which
it would be possible to write a unit test for, however it feels
that this may just be testing a basic equality between transition
contexts so I'm not convinced the utility is very high. Very open
to any feedback on this part!

I'm using pointer equality rather than isEqual: calls, which
might be something to reconsider in a different CL, however
this is the technique that is used in the shipping code so I think
this is ok for now.

The context object is storing a lot of data (images for example) but
adding a strong reference to an entire tab may keep a Tab object in
memory longer than before and there is a concern about any memory
growth; however I haven't seen an increase in memory usage in
Instruments. Feedback sought on this approach!

BUG=none

TEST=Have a clean setup, just a single tab. Navigate to (e.g.) cnn.com. Enter
the tab switcher. Close the tab in the switcher. Hit the plus (+) button to
create a new tab. The new tab will animate to full screen with a correct
snapshot.
==========

[PL, 2017-03-27 22:22:04]

Description was changed from

==========
There is a bug which reuses an incorrect snapshot for the tab transition
animation on iPad.

The TabSwitcherTransitionContextContent stores the index of the tab from
which the user animated in. However the tabs can be manipulated in the
switcher (added removed) and so the tab at index i will not necessarily
be the same as the tab that was originally at index i when first
entering the switcher.

This can lead to an incorrect snapshot being used.

To reproduce:

1. Have a clean setup, just a single tab.
2. Navigate to a website which looks different to Google.com
e.g. cnn.com
3. Enter the tab switcher (there should only be one tab)
4. Close the tab in the switcher
5. Hit the plus (+) button to create a new tab
6. The new tab will animate to full screen with the old (cnn.com)
snapshot

This change removes the use of an index to track the validity of the
tab snapshots. Instead we are using the whole reference to the tab
object itself to see if it's different.

The original bug is not immediately unit-testable, as this change
really removes the scope of the original bug altogether. There is
a new method on TabSwitcherController that compares transition
objects with ones passed in to see if they're different which
it would be possible to write a unit test for, however it feels
that this may just be testing a basic equality between transition
contexts so I'm not convinced the utility is very high. Very open
to any feedback on this part!

I'm using pointer equality rather than isEqual: calls, which
might be something to reconsider in a different CL, however
this is the technique that is used in the shipping code so I think
this is ok for now.

The context object is storing a lot of data (images for example) but
adding a strong reference to an entire tab may keep a Tab object in
memory longer than before and there is a concern about any memory
growth; however I haven't seen an increase in memory usage in
Instruments. Feedback sought on this approach!

BUG=none

TEST=Have a clean setup, just a single tab. Navigate to (e.g.) cnn.com. Enter
the tab switcher. Close the tab in the switcher. Hit the plus (+) button to
create a new tab. The new tab will animate to full screen with a correct
snapshot.
==========

to

==========
There is a bug which reuses an incorrect snapshot for the tab transition
animation on iPad.

The TabSwitcherTransitionContextContent stores the index of the tab from
which the user animated in. However the tabs can be manipulated in the
switcher (added removed) and so the tab at index i will not necessarily
be the same as the tab that was originally at index i when first
entering the switcher.

This can lead to an incorrect snapshot being used.

To reproduce:

1. Have a clean setup, just a single tab.
2. Navigate to a website which looks different to Google.com
e.g. cnn.com
3. Enter the tab switcher (there should only be one tab)
4. Close the tab in the switcher
5. Hit the plus (+) button to create a new tab
6. The new tab will animate to full screen with the old (cnn.com)
snapshot

This change removes the use of an index to track the validity of the
tab snapshots. Instead we are using the whole reference to the tab
object itself to see if it's different.

The original bug is not immediately unit-testable, as this change
really removes the scope of the original bug altogether. There is
a new method on TabSwitcherController that compares transition
objects with ones passed in to see if they're different which
it would be possible to write a unit test for, however it feels
that this may just be testing a basic equality between transition
contexts so I'm not convinced the utility is very high. Very open
to any feedback on this part!

I'm using pointer equality rather than isEqual: calls, which
might be something to reconsider in a different CL, however
this is the technique that is used in the shipping code so I think
this is ok for now.

The context object is storing a lot of data (images for example) but
adding a strong reference to an entire tab may keep a Tab object in
memory longer than before and there is a concern about any memory
growth; however I haven't seen an increase in memory usage in
Instruments. Feedback sought on this approach!

BUG=none

TEST=Have a clean setup, just a single tab. Navigate to (e.g.) cnn.com. 
Enter the tab switcher. Close the tab in the switcher. Hit the plus (+) 
button to create a new tab. The new tab will animate to full screen with 
a correct snapshot.
==========

[PL, 2017-03-27 22:28:13]

peterlaurens@chromium.org changed reviewers:
+ jif@chromium.org

[PL, 2017-03-27 22:28:14]

Hi Jean-François,

This is my first CL! 

I am very open to feedback, especially on any process I'm missing (e.g. do I
need a CR bug?).

Thanks for your help!

- Peter

[commit-bot: I haz the power, 2017-03-27 22:47:16]

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

[commit-bot: I haz the power, 2017-03-27 22:47:17]

Dry run: This issue passed the CQ dry run.

[jif, 2017-03-28 21:19:36]

On 2017/03/27 22:28:14, PL wrote:
> Hi Jean-François,
> 
> This is my first CL! 
> 
> I am very open to feedback, especially on any process I'm missing (e.g. do I
> need a CR bug?).
> 
> Thanks for your help!
> 
> - Peter

Nice to meet you Peter :)

I'm not super confortable with the idea of retaining a Tab.
What if we simply never reuse the tabSnapshotImage? Does that introduce any kind
of jank?

[PL, 2017-03-28 22:21:55]

On 2017/03/28 21:19:36, jif wrote:
> On 2017/03/27 22:28:14, PL wrote:
> > Hi Jean-François,
> > 
> > This is my first CL! 
> > 
> > I am very open to feedback, especially on any process I'm missing (e.g. do I
> > need a CR bug?).
> > 
> > Thanks for your help!
> > 
> > - Peter
> 
> Nice to meet you Peter :)
> 
> I'm not super confortable with the idea of retaining a Tab.
> What if we simply never reuse the tabSnapshotImage? Does that introduce any
kind
> of jank?

Thanks Jean-François,

There might be a performance issue if we don't use the context object but I am
not
sure, I can investigate never going down this branch.
It will remove a dozen or two lines of code, so that might be nice.

Although I have two other options you might prefer:

1. Instead of retaining a Tab, we retain the Tab object's hash value. Will just
be a string,
and compare hash values (this is kind of a poor man's isEqual: on the Tab object
but without the danger of retaining a whole Tab object for a long time).

or:

2. We set the TabSwitcherController's context object to nil when animating out.

I've been playing with this (#2) today as I was wanting to see what the memory
impact 
might be in isolation. Currently the context object hangs around even when just 
browsing around after returning from the switcher. Nil-ing out the transition
context
after returning from the switcher doesn't appear to cause any issue in my
testing,
currently old transition context objects just get thrown out by the creation of
a new 
one when next entering the switcher.

I haven't done a thorough analysis yet, but early signs show maybe a ~100KB
memory
saving.

If we do this with the transition context object then maybe your concerns about
it retaining a Tab object will be diminished.

I'm quite in favor of option #2 :-).

Thanks for your feedback so far! What do you think?

[PL, 2017-03-29 00:18:03]

On 2017/03/28 22:21:55, PL wrote:
> On 2017/03/28 21:19:36, jif wrote:
> > On 2017/03/27 22:28:14, PL wrote:
> > > Hi Jean-François,
> > > 
> > > This is my first CL! 
> > > 
> > > I am very open to feedback, especially on any process I'm missing (e.g. do
I
> > > need a CR bug?).
> > > 
> > > Thanks for your help!
> > > 
> > > - Peter
> > 
> > Nice to meet you Peter :)
> > 
> > I'm not super confortable with the idea of retaining a Tab.
> > What if we simply never reuse the tabSnapshotImage? Does that introduce any
> kind
> > of jank?
> 
> Thanks Jean-François,
> 
> There might be a performance issue if we don't use the context object but I am
> not
> sure, I can investigate never going down this branch.
> It will remove a dozen or two lines of code, so that might be nice.
> 
> Although I have two other options you might prefer:
> 
> 1. Instead of retaining a Tab, we retain the Tab object's hash value. Will
just
> be a string,
> and compare hash values (this is kind of a poor man's isEqual: on the Tab
object
> but without the danger of retaining a whole Tab object for a long time).
> 
> or:
> 
> 2. We set the TabSwitcherController's context object to nil when animating
out.
> 
> I've been playing with this (#2) today as I was wanting to see what the memory
> impact 
> might be in isolation. Currently the context object hangs around even when
just 
> browsing around after returning from the switcher. Nil-ing out the transition
> context
> after returning from the switcher doesn't appear to cause any issue in my
> testing,
> currently old transition context objects just get thrown out by the creation
of
> a new 
> one when next entering the switcher.
> 
> I haven't done a thorough analysis yet, but early signs show maybe a ~100KB
> memory
> saving.
> 
> If we do this with the transition context object then maybe your concerns
about
> it retaining a Tab object will be diminished.
> 
> I'm quite in favor of option #2 :-).
> 
> Thanks for your feedback so far! What do you think?

I ran some tests in the simulator and option #2 and it looks like on average we 
would save ~130KB memory after leaving the tab switcher. If you think this is
interesting I can put up a new diff here (it's very simple).

[sdefresne, 2017-03-29 09:50:54]

On 2017/03/28 22:21:55, PL wrote:
> On 2017/03/28 21:19:36, jif wrote:
> > On 2017/03/27 22:28:14, PL wrote:
> > > Hi Jean-François,
> > > 
> > > This is my first CL! 
> > > 
> > > I am very open to feedback, especially on any process I'm missing (e.g. do
I
> > > need a CR bug?).
> > > 
> > > Thanks for your help!
> > > 
> > > - Peter
> > 
> > Nice to meet you Peter :)
> > 
> > I'm not super confortable with the idea of retaining a Tab.
> > What if we simply never reuse the tabSnapshotImage? Does that introduce any
> kind
> > of jank?
> 
> Thanks Jean-François,
> 
> There might be a performance issue if we don't use the context object but I am
> not
> sure, I can investigate never going down this branch.
> It will remove a dozen or two lines of code, so that might be nice.
> 
> Although I have two other options you might prefer:
> 
> 1. Instead of retaining a Tab, we retain the Tab object's hash value. Will
just
> be a string,
> and compare hash values (this is kind of a poor man's isEqual: on the Tab
object
> but without the danger of retaining a whole Tab object for a long time).
> 
> or:
> 
> 2. We set the TabSwitcherController's context object to nil when animating
out.
> 
> I've been playing with this (#2) today as I was wanting to see what the memory
> impact 
> might be in isolation. Currently the context object hangs around even when
just 
> browsing around after returning from the switcher. Nil-ing out the transition
> context
> after returning from the switcher doesn't appear to cause any issue in my
> testing,
> currently old transition context objects just get thrown out by the creation
of
> a new 
> one when next entering the switcher.
> 
> I haven't done a thorough analysis yet, but early signs show maybe a ~100KB
> memory
> saving.
> 
> If we do this with the transition context object then maybe your concerns
about
> it retaining a Tab object will be diminished.
> 
> I'm quite in favor of option #2 :-).
> 
> Thanks for your feedback so far! What do you think?

Instead of keeping a reference to the Tab, I would suggest keeping a reference
to Tab's WebState instead. There is a 1:1 mapping between them, so it should
give you the same results and fix the bug. We're trying to get rid of Tab in
favour of WebState, so adding new uses of Tab should be avoided if possible. In
addition, this will avoid retaining the Tab and preventing proper destruction.

As WebState is a C++ object, care should be taken that the reference is cleared
when the WebState is destroyed. This can be achieved by the following two ways:
1. use a WebStateObserver and reset the pointer when WebStateDestroyed() method
is invoked (or in Objective-C uses CRWWebStateObserver and -webStateDestroyed:)
2. use a WebStateUserData, attach it to Webstate, and on destruction of the
object reset the pointer (you can use the self as the key, guaranteed to be
unique).

[jif, 2017-03-29 16:20:09]

On 2017/03/29 09:50:54, sdefresne wrote:
> On 2017/03/28 22:21:55, PL wrote:
> > On 2017/03/28 21:19:36, jif wrote:
> > > On 2017/03/27 22:28:14, PL wrote:
> > > > Hi Jean-François,
> > > > 
> > > > This is my first CL! 
> > > > 
> > > > I am very open to feedback, especially on any process I'm missing (e.g.
do
> I
> > > > need a CR bug?).
> > > > 
> > > > Thanks for your help!
> > > > 
> > > > - Peter
> > > 
> > > Nice to meet you Peter :)
> > > 
> > > I'm not super confortable with the idea of retaining a Tab.
> > > What if we simply never reuse the tabSnapshotImage? Does that introduce
any
> > kind
> > > of jank?
> > 
> > Thanks Jean-François,
> > 
> > There might be a performance issue if we don't use the context object but I
am
> > not
> > sure, I can investigate never going down this branch.
> > It will remove a dozen or two lines of code, so that might be nice.
> > 
> > Although I have two other options you might prefer:
> > 
> > 1. Instead of retaining a Tab, we retain the Tab object's hash value. Will
> just
> > be a string,
> > and compare hash values (this is kind of a poor man's isEqual: on the Tab
> object
> > but without the danger of retaining a whole Tab object for a long time).
> > 
> > or:
> > 
> > 2. We set the TabSwitcherController's context object to nil when animating
> out.
> > 
> > I've been playing with this (#2) today as I was wanting to see what the
memory
> > impact 
> > might be in isolation. Currently the context object hangs around even when
> just 
> > browsing around after returning from the switcher. Nil-ing out the
transition
> > context
> > after returning from the switcher doesn't appear to cause any issue in my
> > testing,
> > currently old transition context objects just get thrown out by the creation
> of
> > a new 
> > one when next entering the switcher.
> > 
> > I haven't done a thorough analysis yet, but early signs show maybe a ~100KB
> > memory
> > saving.
> > 
> > If we do this with the transition context object then maybe your concerns
> about
> > it retaining a Tab object will be diminished.
> > 
> > I'm quite in favor of option #2 :-).
> > 
> > Thanks for your feedback so far! What do you think?
> 
> Instead of keeping a reference to the Tab, I would suggest keeping a reference
> to Tab's WebState instead. There is a 1:1 mapping between them, so it should
> give you the same results and fix the bug. We're trying to get rid of Tab in
> favour of WebState, so adding new uses of Tab should be avoided if possible.
In
> addition, this will avoid retaining the Tab and preventing proper destruction.
> 
> As WebState is a C++ object, care should be taken that the reference is
cleared
> when the WebState is destroyed. This can be achieved by the following two
ways:
> 1. use a WebStateObserver and reset the pointer when WebStateDestroyed()
method
> is invoked (or in Objective-C uses CRWWebStateObserver and
-webStateDestroyed:)
> 2. use a WebStateUserData, attach it to Webstate, and on destruction of the
> object reset the pointer (you can use the self as the key, guaranteed to be
> unique).

Discarding the old transition context is a good idea: let's do it :-) It won't
solve the bug by itself though, right?
I was going to suggest using a weak reference to the Tab, but Sylvain gave good
reasons for why we don't want to do that either.

As he explained (btw thanks Sylvain for taking the time to do that), using a
WebState takes quite a bit of boiler plate code, so I think that your idea of
using a hash (did you mean Tab's tabId method ?) is the better way to go (unless
you have other ideas).

[PL, 2017-03-29 20:20:45]

On 2017/03/29 16:20:09, jif wrote:
> On 2017/03/29 09:50:54, sdefresne wrote:
> > On 2017/03/28 22:21:55, PL wrote:
> > > On 2017/03/28 21:19:36, jif wrote:
> > > > On 2017/03/27 22:28:14, PL wrote:
> > > > > Hi Jean-François,
> > > > > 
> > > > > This is my first CL! 
> > > > > 
> > > > > I am very open to feedback, especially on any process I'm missing
(e.g.
> do
> > I
> > > > > need a CR bug?).
> > > > > 
> > > > > Thanks for your help!
> > > > > 
> > > > > - Peter
> > > > 
> > > > Nice to meet you Peter :)
> > > > 
> > > > I'm not super confortable with the idea of retaining a Tab.
> > > > What if we simply never reuse the tabSnapshotImage? Does that introduce
> any
> > > kind
> > > > of jank?
> > > 
> > > Thanks Jean-François,
> > > 
> > > There might be a performance issue if we don't use the context object but
I
> am
> > > not
> > > sure, I can investigate never going down this branch.
> > > It will remove a dozen or two lines of code, so that might be nice.
> > > 
> > > Although I have two other options you might prefer:
> > > 
> > > 1. Instead of retaining a Tab, we retain the Tab object's hash value. Will
> > just
> > > be a string,
> > > and compare hash values (this is kind of a poor man's isEqual: on the Tab
> > object
> > > but without the danger of retaining a whole Tab object for a long time).
> > > 
> > > or:
> > > 
> > > 2. We set the TabSwitcherController's context object to nil when animating
> > out.
> > > 
> > > I've been playing with this (#2) today as I was wanting to see what the
> memory
> > > impact 
> > > might be in isolation. Currently the context object hangs around even when
> > just 
> > > browsing around after returning from the switcher. Nil-ing out the
> transition
> > > context
> > > after returning from the switcher doesn't appear to cause any issue in my
> > > testing,
> > > currently old transition context objects just get thrown out by the
creation
> > of
> > > a new 
> > > one when next entering the switcher.
> > > 
> > > I haven't done a thorough analysis yet, but early signs show maybe a
~100KB
> > > memory
> > > saving.
> > > 
> > > If we do this with the transition context object then maybe your concerns
> > about
> > > it retaining a Tab object will be diminished.
> > > 
> > > I'm quite in favor of option #2 :-).
> > > 
> > > Thanks for your feedback so far! What do you think?
> > 
> > Instead of keeping a reference to the Tab, I would suggest keeping a
reference
> > to Tab's WebState instead. There is a 1:1 mapping between them, so it should
> > give you the same results and fix the bug. We're trying to get rid of Tab in
> > favour of WebState, so adding new uses of Tab should be avoided if possible.
> In
> > addition, this will avoid retaining the Tab and preventing proper
destruction.
> > 
> > As WebState is a C++ object, care should be taken that the reference is
> cleared
> > when the WebState is destroyed. This can be achieved by the following two
> ways:
> > 1. use a WebStateObserver and reset the pointer when WebStateDestroyed()
> method
> > is invoked (or in Objective-C uses CRWWebStateObserver and
> -webStateDestroyed:)
> > 2. use a WebStateUserData, attach it to Webstate, and on destruction of the
> > object reset the pointer (you can use the self as the key, guaranteed to be
> > unique).
> 
> Discarding the old transition context is a good idea: let's do it :-) It won't
> solve the bug by itself though, right?
> I was going to suggest using a weak reference to the Tab, but Sylvain gave
good
> reasons for why we don't want to do that either.
> 
> As he explained (btw thanks Sylvain for taking the time to do that), using a
> WebState takes quite a bit of boiler plate code, so I think that your idea of
> using a hash (did you mean Tab's tabId method ?) is the better way to go
(unless
> you have other ideas).

Awesome! Here's a version that uses tabID and so should be much safer!

I'll produce a separate CL for discarding the transition context as these two
things
are related but not the same.

Thanks!

[marq (ping after 24h), 2017-03-30 06:17:44]

marq@chromium.org changed reviewers:
+ marq@chromium.org

[marq (ping after 24h), 2017-03-30 06:17:45]

Hi Peter!

I'm throwing a bunch of style comments at you here. Most of them will be
resolved by letting clang-format format your code before you submit it -- just
run 'git cl format' and commit before you upload. (Usually there's a presubmit
warning if your code needs formatting).

You can see the Chromium Objective-C++ style guide here:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/objective-...

https://codereview.chromium.org/2776083003/diff/40001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm (right):

https://codereview.chromium.org/2776083003/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm:572:
andTabID:selectedTab.tabId]) {
Align colons.

https://codereview.chromium.org/2776083003/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm:573: // If we're
transitioning to a different tab than the one we animated in
Avoid using "we" in comments; it can be ambiguous. Prefer something like "When
the tab switcher is transitioning to a different tab than the one that was
animated in from ... "

https://codereview.chromium.org/2776083003/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm:723:
andTabID:(nonnull NSString*)tabID {
Align colons.

Prefer not to use 'and' to join method keywords; 'and' should only be used to
indicate a method is performing some additional action. This can just be
-initialTabModelAndTabDifferFromTabModel:tabID:

https://codereview.chromium.org/2776083003/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm:724: BOOL differ =
NO;
'differs' ? But see below.

https://codereview.chromium.org/2776083003/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm:725: TabModel
*initialTabModel = self.transitionContext.initialTabModel;
Chromium style is to have the pointer operator adhere to the type, so this
should be "TabModel* ".

https://codereview.chromium.org/2776083003/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm:730: return
differ;
Instead of using the intermediate variable, it's cleaner here to just return:

return initialTabModel != tabModel || ![initialTabID isEqualToString:tabID];

https://codereview.chromium.org/2776083003/diff/40001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tab_switcher/tab_switcher_transition_context.h
(right):

https://codereview.chromium.org/2776083003/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_transition_context.h:12: @class
Tab;
This doesn't seem to be needed.

https://codereview.chromium.org/2776083003/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_transition_context.h:34:
@property(nonatomic, retain) NSString *initialTabID;
NSString* initialTabID;

[marq (ping after 24h), 2017-03-30 09:20:06]

https://codereview.chromium.org/2776083003/diff/40001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm (right):

https://codereview.chromium.org/2776083003/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm:730: return
differ;
On 2017/03/30 06:17:45, marq wrote:
> Instead of using the intermediate variable, it's cleaner here to just return:
> 
> return initialTabModel != tabModel || ![initialTabID isEqualToString:tabID];

And, (as I should have realized), we can apply De Morgan's Law to simplify this
further to:

return !(initialTabModel == tabModel && [initialTabID isEqualToString:tabID]).

At that point, I wonder if the negation should just be hoisted out of this
method, making it into

- initialTabModelAndTabMatchesTabModel:TabID:

and having the calling code check if (![self  initialTabModelAndTabMatches ...

[PL, 2017-03-30 17:39:21]

On 2017/03/30 09:20:06, marq wrote:
>
https://codereview.chromium.org/2776083003/diff/40001/ios/chrome/browser/ui/t...
> File ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm (right):
> 
>
https://codereview.chromium.org/2776083003/diff/40001/ios/chrome/browser/ui/t...
> ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm:730: return
> differ;
> On 2017/03/30 06:17:45, marq wrote:
> > Instead of using the intermediate variable, it's cleaner here to just
return:
> > 
> > return initialTabModel != tabModel || ![initialTabID isEqualToString:tabID];
> 
> And, (as I should have realized), we can apply De Morgan's Law to simplify
this
> further to:
> 
> return !(initialTabModel == tabModel && [initialTabID isEqualToString:tabID]).
> 
> At that point, I wonder if the negation should just be hoisted out of this
> method, making it into
> 
> - initialTabModelAndTabMatchesTabModel:TabID:
> 
> and having the calling code check if (![self  initialTabModelAndTabMatches ...

Thanks Mark, I deeply appreciate these suggestions. Especially the De Morgan's,
as it will lead to fewer negations and read much more clearly.

[PL, 2017-03-30 18:07:10]

Description was changed from

==========
There is a bug which reuses an incorrect snapshot for the tab transition
animation on iPad.

The TabSwitcherTransitionContextContent stores the index of the tab from
which the user animated in. However the tabs can be manipulated in the
switcher (added removed) and so the tab at index i will not necessarily
be the same as the tab that was originally at index i when first
entering the switcher.

This can lead to an incorrect snapshot being used.

To reproduce:

1. Have a clean setup, just a single tab.
2. Navigate to a website which looks different to Google.com
e.g. cnn.com
3. Enter the tab switcher (there should only be one tab)
4. Close the tab in the switcher
5. Hit the plus (+) button to create a new tab
6. The new tab will animate to full screen with the old (cnn.com)
snapshot

This change removes the use of an index to track the validity of the
tab snapshots. Instead we are using the whole reference to the tab
object itself to see if it's different.

The original bug is not immediately unit-testable, as this change
really removes the scope of the original bug altogether. There is
a new method on TabSwitcherController that compares transition
objects with ones passed in to see if they're different which
it would be possible to write a unit test for, however it feels
that this may just be testing a basic equality between transition
contexts so I'm not convinced the utility is very high. Very open
to any feedback on this part!

I'm using pointer equality rather than isEqual: calls, which
might be something to reconsider in a different CL, however
this is the technique that is used in the shipping code so I think
this is ok for now.

The context object is storing a lot of data (images for example) but
adding a strong reference to an entire tab may keep a Tab object in
memory longer than before and there is a concern about any memory
growth; however I haven't seen an increase in memory usage in
Instruments. Feedback sought on this approach!

BUG=none

TEST=Have a clean setup, just a single tab. Navigate to (e.g.) cnn.com. 
Enter the tab switcher. Close the tab in the switcher. Hit the plus (+) 
button to create a new tab. The new tab will animate to full screen with 
a correct snapshot.
==========

to

==========
Use correct snapshot for the tab transition animation on iPad.

The TabSwitcherTransitionContextContent stores the index of the tab from
which the user animated in. However the tabs can be manipulated in the
switcher (added removed) and so the tab at index i will not necessarily
be the same as the tab that was originally at index i when first
entering the switcher.

This can lead to an incorrect snapshot being used.

To reproduce:

1. Have a clean setup, just a single tab.
2. Navigate to a website which looks different to Google.com
e.g. cnn.com
3. Enter the tab switcher (there should only be one tab)
4. Close the tab in the switcher
5. Hit the plus (+) button to create a new tab
6. The new tab will animate to full screen with the old (cnn.com)
snapshot

This change removes the use of an index to track the validity of the
tab snapshots. Instead using the tabID of the Tab
object itself to see if it's different.

BUG=706891

TEST=Have a clean setup, just a single tab. Navigate to (e.g.) cnn.com. 
Enter the tab switcher. Close the tab in the switcher. Hit the plus (+) 
button to create a new tab. The new tab will animate to full screen with 
a correct snapshot.
==========

[jif, 2017-03-30 19:50:58]

lgtm with small nits

https://codereview.chromium.org/2776083003/diff/60001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm (right):

https://codereview.chromium.org/2776083003/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm:571: if (![self
initialTabModelAndTabIDMatchesTabModel:tabModel
Can you write the code without the "!" (i.e. swap the content of the
if...else... block)?

https://codereview.chromium.org/2776083003/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm:575: // context...
no need for the "..."

https://codereview.chromium.org/2776083003/diff/60001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tab_switcher/tab_switcher_transition_context.h
(right):

https://codereview.chromium.org/2776083003/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_transition_context.h:33:
@property(nonatomic, retain) NSString* initialTabID;
Can you copy the NSString instead of retaining it? We don't want anybody
changing the string behind our back.

[PL, 2017-03-30 21:05:09]

On 2017/03/30 19:50:58, jif wrote:
> lgtm with small nits
> 
>
https://codereview.chromium.org/2776083003/diff/60001/ios/chrome/browser/ui/t...
> File ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm (right):
> 
>
https://codereview.chromium.org/2776083003/diff/60001/ios/chrome/browser/ui/t...
> ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm:571: if (![self
> initialTabModelAndTabIDMatchesTabModel:tabModel
> Can you write the code without the "!" (i.e. swap the content of the
> if...else... block)?
> 
>
https://codereview.chromium.org/2776083003/diff/60001/ios/chrome/browser/ui/t...
> ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm:575: //
context...
> no need for the "..."
> 
>
https://codereview.chromium.org/2776083003/diff/60001/ios/chrome/browser/ui/t...
> File ios/chrome/browser/ui/tab_switcher/tab_switcher_transition_context.h
> (right):
> 
>
https://codereview.chromium.org/2776083003/diff/60001/ios/chrome/browser/ui/t...
> ios/chrome/browser/ui/tab_switcher/tab_switcher_transition_context.h:33:
> @property(nonatomic, retain) NSString* initialTabID;
> Can you copy the NSString instead of retaining it? We don't want anybody
> changing the string behind our back.

Great changes, done, thanks!

[PL, 2017-03-30 21:06:05]

The CQ bit was checked by peterlaurens@chromium.org

[PL, 2017-03-30 21:06:05]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org
Link to the patchset: https://codereview.chromium.org/2776083003/#ps80001
(title: "Formatting changes")

[commit-bot: I haz the power, 2017-03-30 21:07:10]

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-03-30 22:17:40]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490907965467910,
"parent_rev": "65d11a7c591307ff2d10145a07655b53010e04d0", "commit_rev":
"b6d1b157721d10aebaca323f514ba7d5a09bdcef"}

[commit-bot: I haz the power, 2017-03-30 22:19:29]

Description was changed from

==========
Use correct snapshot for the tab transition animation on iPad.

The TabSwitcherTransitionContextContent stores the index of the tab from
which the user animated in. However the tabs can be manipulated in the
switcher (added removed) and so the tab at index i will not necessarily
be the same as the tab that was originally at index i when first
entering the switcher.

This can lead to an incorrect snapshot being used.

To reproduce:

1. Have a clean setup, just a single tab.
2. Navigate to a website which looks different to Google.com
e.g. cnn.com
3. Enter the tab switcher (there should only be one tab)
4. Close the tab in the switcher
5. Hit the plus (+) button to create a new tab
6. The new tab will animate to full screen with the old (cnn.com)
snapshot

This change removes the use of an index to track the validity of the
tab snapshots. Instead using the tabID of the Tab
object itself to see if it's different.

BUG=706891

TEST=Have a clean setup, just a single tab. Navigate to (e.g.) cnn.com. 
Enter the tab switcher. Close the tab in the switcher. Hit the plus (+) 
button to create a new tab. The new tab will animate to full screen with 
a correct snapshot.
==========

to

==========
Use correct snapshot for the tab transition animation on iPad.

The TabSwitcherTransitionContextContent stores the index of the tab from
which the user animated in. However the tabs can be manipulated in the
switcher (added removed) and so the tab at index i will not necessarily
be the same as the tab that was originally at index i when first
entering the switcher.

This can lead to an incorrect snapshot being used.

To reproduce:

1. Have a clean setup, just a single tab.
2. Navigate to a website which looks different to Google.com
e.g. cnn.com
3. Enter the tab switcher (there should only be one tab)
4. Close the tab in the switcher
5. Hit the plus (+) button to create a new tab
6. The new tab will animate to full screen with the old (cnn.com)
snapshot

This change removes the use of an index to track the validity of the
tab snapshots. Instead using the tabID of the Tab
object itself to see if it's different.

BUG=706891

TEST=Have a clean setup, just a single tab. Navigate to (e.g.) cnn.com. 
Enter the tab switcher. Close the tab in the switcher. Hit the plus (+) 
button to create a new tab. The new tab will animate to full screen with 
a correct snapshot.

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

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

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b6d1b157721d10aebaca323f514b...