Fix restore of a tab from a recently-closed browser window

Fix restore of a tab from a recently-closed browser window

https://codereview.chromium.org/1343833002 introduced a manually-written copy
constructor and assignment operator for TabRestoreService::Tab in order to
accommodate a new member that had to be copied via an explicit Clone() method.
However, that CL failed to copy the fields of TabRestoreService::Entry, the
base class of TabRestoreService::Tab. This problem resulted in not being able
to restore individual tabs from a recently-closed browser window because the ID
of the tab got lost in a copy somewhere along the way and didn't match the one
of the menu item that the user clicked on.

This CL also adds a browsertest that fails without this change.

BUG=622752
TEST=On a desktop system, open 2 tabs, close the browser window and open a new
browser window. In that new window go into the "Recently Closed" section of the
History menu, find the entry that says "2 Tabs", and click on one of the
individual tabs. That tab should be restored in the new window.

Committed: https://crrev.com/3d931371a8a5f795e7b9a559489b50a877f3db8a
Cr-Commit-Position: refs/heads/master@{#406523}

[blundell, 2016-07-05 12:56:05]

Description was changed from

==========
Fix restore of a tab from a recently-closed browser window

https://codereview.chromium.org/1343833002 introduced a manually-written copy
constructor and assignment operator for TabRestoreService::Tab in order to
accommodate a new member that had to be copied via an explicit Clone() method.
However, that CL failed to copy the fields of TabRestoreService::Entry, the
base class of TabRestoreService::Tab. This problem resulted in not being able
to restore individual tabs from a recently-closed browser window because the ID
of the tab got lost in a copy somewhere along the way and didn't match the one
of the menu item that the user clicked on.


BUG=622752
TEST=On a desktop system, open 2 tabs, close the browser window and open a new
browser window. In that new window go into the "Recently Closed" section of the
History menu, find the entry that says "2 Tabs", and click on one of the
individual tabs. That tab should be restored in the new window.
==========

to

==========
Fix restore of a tab from a recently-closed browser window

https://codereview.chromium.org/1343833002 introduced a manually-written copy
constructor and assignment operator for TabRestoreService::Tab in order to
accommodate a new member that had to be copied via an explicit Clone() method.
However, that CL failed to copy the fields of TabRestoreService::Entry, the
base class of TabRestoreService::Tab. This problem resulted in not being able
to restore individual tabs from a recently-closed browser window because the ID
of the tab got lost in a copy somewhere along the way and didn't match the one
of the menu item that the user clicked on.

This CL also adds a browsertest that fails without this change.

BUG=622752
TEST=On a desktop system, open 2 tabs, close the browser window and open a new
browser window. In that new window go into the "Recently Closed" section of the
History menu, find the entry that says "2 Tabs", and click on one of the
individual tabs. That tab should be restored in the new window.
==========

[blundell, 2016-07-05 13:49:16]

blundell@chromium.org changed reviewers:
+ sky@chromium.org

[blundell, 2016-07-05 13:49:41]

I believe that the red bots are unrelated.

[sky, 2016-07-06 16:09:07]

The browser_test is fine, but given we have a manual assignment operator perhaps
we should have a test for that?

https://codereview.chromium.org/2114673002/diff/20001/components/sessions/cor...
File components/sessions/core/tab_restore_service.cc (right):

https://codereview.chromium.org/2114673002/diff/20001/components/sessions/cor...
components/sessions/core/tab_restore_service.cc:60: from_last_session =
tab.from_last_session;
What about timestamp?

[blundell, 2016-07-06 16:12:50]

On 2016/07/06 16:09:07, sky wrote:
> The browser_test is fine, but given we have a manual assignment operator
perhaps
> we should have a test for that?
> 

I thought about that but I don't really know how to write a meaningful unit test
of that. All we can test is that there's equality in all the fields we know
about. If someone adds a field, forgets to copy it, and forgets to add a check
for it in the test (which they presumably would if they forgot to copy it), then
the test will still pass. Or did you have something in mind that I'm missing?

>
https://codereview.chromium.org/2114673002/diff/20001/components/sessions/cor...
> File components/sessions/core/tab_restore_service.cc (right):
> 
>
https://codereview.chromium.org/2114673002/diff/20001/components/sessions/cor...
> components/sessions/core/tab_restore_service.cc:60: from_last_session =
> tab.from_last_session;
> What about timestamp?

[sky, 2016-07-06 19:35:08]

Such a test would have caught the regression, right?

On Wed, Jul 6, 2016 at 9:12 AM,  <blundell@chromium.org> wrote:
> On 2016/07/06 16:09:07, sky wrote:
>> The browser_test is fine, but given we have a manual assignment operator
> perhaps
>> we should have a test for that?
>>
>
> I thought about that but I don't really know how to write a meaningful unit
> test
> of that. All we can test is that there's equality in all the fields we know
> about. If someone adds a field, forgets to copy it, and forgets to add a
> check
> for it in the test (which they presumably would if they forgot to copy it),
> then
> the test will still pass. Or did you have something in mind that I'm
> missing?
>
>>
>
https://codereview.chromium.org/2114673002/diff/20001/components/sessions/cor...
>> File components/sessions/core/tab_restore_service.cc (right):
>>
>>
>
https://codereview.chromium.org/2114673002/diff/20001/components/sessions/cor...
>> components/sessions/core/tab_restore_service.cc:60: from_last_session =
>> tab.from_last_session;
>> What about timestamp?
>
>
>
> https://codereview.chromium.org/2114673002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blundell, 2016-07-19 09:55:16]

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

[commit-bot: I haz the power, 2016-07-19 09:55:43]

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

[commit-bot: I haz the power, 2016-07-19 11:19:44]

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

[commit-bot: I haz the power, 2016-07-19 11:19:45]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-07-19 11:21:54]

Unittest added :).

https://codereview.chromium.org/2114673002/diff/20001/components/sessions/cor...
File components/sessions/core/tab_restore_service.cc (right):

https://codereview.chromium.org/2114673002/diff/20001/components/sessions/cor...
components/sessions/core/tab_restore_service.cc:60: from_last_session =
tab.from_last_session;
On 2016/07/06 16:09:07, sky wrote:
> What about timestamp?

Done, thanks.

[sky, 2016-07-19 17:42:41]

LGTM

https://codereview.chromium.org/2114673002/diff/60001/chrome/browser/sessions...
File chrome/browser/sessions/tab_restore_browsertest.cc (right):

https://codereview.chromium.org/2114673002/diff/60001/chrome/browser/sessions...
chrome/browser/sessions/tab_restore_browsertest.cc:445: EXPECT_EQ(entry->type,
sessions::TabRestoreService::WINDOW);
nit: assertions have expected, actual. You want WINDOW first.
Also, make this an ASSERT as if it's not a window then the case on the next line
is likely to cause bad things to happen.

https://codereview.chromium.org/2114673002/diff/60001/chrome/browser/sessions...
chrome/browser/sessions/tab_restore_browsertest.cc:461:
EXPECT_TRUE(found_tab_to_restore);
ASSERT, as if you didn't find it tab_to_restore is bad.

https://codereview.chromium.org/2114673002/diff/60001/components/sessions/cor...
File components/sessions/core/tab_restore_service_unittest.cc (right):

https://codereview.chromium.org/2114673002/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service_unittest.cc:24: void
TestEntryEquality(TabRestoreService::Entry* reference,
nit: how about naming the parameters here and on 32 as expected, actual to match
general assertions.

[blundell, 2016-07-20 08:18:12]

Thanks, Scott!

https://codereview.chromium.org/2114673002/diff/60001/chrome/browser/sessions...
File chrome/browser/sessions/tab_restore_browsertest.cc (right):

https://codereview.chromium.org/2114673002/diff/60001/chrome/browser/sessions...
chrome/browser/sessions/tab_restore_browsertest.cc:445: EXPECT_EQ(entry->type,
sessions::TabRestoreService::WINDOW);
On 2016/07/19 17:42:40, sky wrote:
> nit: assertions have expected, actual. You want WINDOW first.
> Also, make this an ASSERT as if it's not a window then the case on the next
line
> is likely to cause bad things to happen.

Done.

https://codereview.chromium.org/2114673002/diff/60001/chrome/browser/sessions...
chrome/browser/sessions/tab_restore_browsertest.cc:461:
EXPECT_TRUE(found_tab_to_restore);
On 2016/07/19 17:42:40, sky wrote:
> ASSERT, as if you didn't find it tab_to_restore is bad.

Done.

https://codereview.chromium.org/2114673002/diff/60001/components/sessions/cor...
File components/sessions/core/tab_restore_service_unittest.cc (right):

https://codereview.chromium.org/2114673002/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service_unittest.cc:24: void
TestEntryEquality(TabRestoreService::Entry* reference,
On 2016/07/19 17:42:40, sky wrote:
> nit: how about naming the parameters here and on 32 as expected, actual to
match
> general assertions.

Done.

[blundell, 2016-07-20 08:18:40]

The CQ bit was checked by blundell@chromium.org

[blundell, 2016-07-20 08:18:40]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2114673002/#ps80001
(title: "Response to review")

[commit-bot: I haz the power, 2016-07-20 08:19:01]

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

[commit-bot: I haz the power, 2016-07-20 09:15:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-20 09:15:58]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 09:17:41]

Description was changed from

==========
Fix restore of a tab from a recently-closed browser window

https://codereview.chromium.org/1343833002 introduced a manually-written copy
constructor and assignment operator for TabRestoreService::Tab in order to
accommodate a new member that had to be copied via an explicit Clone() method.
However, that CL failed to copy the fields of TabRestoreService::Entry, the
base class of TabRestoreService::Tab. This problem resulted in not being able
to restore individual tabs from a recently-closed browser window because the ID
of the tab got lost in a copy somewhere along the way and didn't match the one
of the menu item that the user clicked on.

This CL also adds a browsertest that fails without this change.

BUG=622752
TEST=On a desktop system, open 2 tabs, close the browser window and open a new
browser window. In that new window go into the "Recently Closed" section of the
History menu, find the entry that says "2 Tabs", and click on one of the
individual tabs. That tab should be restored in the new window.
==========

to

==========
Fix restore of a tab from a recently-closed browser window

https://codereview.chromium.org/1343833002 introduced a manually-written copy
constructor and assignment operator for TabRestoreService::Tab in order to
accommodate a new member that had to be copied via an explicit Clone() method.
However, that CL failed to copy the fields of TabRestoreService::Entry, the
base class of TabRestoreService::Tab. This problem resulted in not being able
to restore individual tabs from a recently-closed browser window because the ID
of the tab got lost in a copy somewhere along the way and didn't match the one
of the menu item that the user clicked on.

This CL also adds a browsertest that fails without this change.

BUG=622752
TEST=On a desktop system, open 2 tabs, close the browser window and open a new
browser window. In that new window go into the "Recently Closed" section of the
History menu, find the entry that says "2 Tabs", and click on one of the
individual tabs. That tab should be restored in the new window.

Committed: https://crrev.com/3d931371a8a5f795e7b9a559489b50a877f3db8a
Cr-Commit-Position: refs/heads/master@{#406523}
==========

[commit-bot: I haz the power, 2016-07-20 09:17:42]

Patchset 5 (id:??) landed as
https://crrev.com/3d931371a8a5f795e7b9a559489b50a877f3db8a
Cr-Commit-Position: refs/heads/master@{#406523}