Fix tabs duplication when restoring last closed window.

Fix tabs duplication when restoring last closed window.

Restoration of a last closed window is done by TabRestoreService.
However if that window is going to be the first window for a profile,
SessionService will perform restoration and there will be conflict
between services. It leads to tabs duplication.

BUG=500083
R=sky@chromium.org

Committed: https://crrev.com/d19beb53ae69324f842ba8efdbdd0967034f38c4
Cr-Commit-Position: refs/heads/master@{#420006}

[Michael K. (Yandex Team), 2016-09-15 18:46:21]

On 2016/09/15 18:42:08, mkolom wrote:

PTAL

[Michael K. (Yandex Team), 2016-09-15 18:56:27]

(btw I have another variant to fix it: via
chrome/browser/app_controller_mac.h/mm; It's similar to how
chrome::NewEmptyWindow already works on Mac. But I've decided that 2nd variant
is worse: it introduces _implicit_ dependency rather than explicit one. In this
fix there is the explicit connection between services).

[Michael K. (Yandex Team), 2016-09-15 18:56:28]

(btw I have another variant to fix it: via
chrome/browser/app_controller_mac.h/mm; It's similar to how
chrome::NewEmptyWindow already works on Mac. But I've decided that 2nd variant
is worse: it introduces _implicit_ dependency rather than explicit one. In this
fix there is the explicit connection between services).

[Michael K. (Yandex Team), 2016-09-15 18:56:30]

(btw I have another variant to fix it: via
chrome/browser/app_controller_mac.h/mm; It's similar to how
chrome::NewEmptyWindow already works on Mac. But I've decided that 2nd variant
is worse: it introduces _implicit_ dependency rather than explicit one. In this
fix there is the explicit connection between services).

[sky, 2016-09-16 17:03:23]

Thanks for the fix!
Adding the extra boolean to restore is confusing and easy to get wrong. Can we
make SessionService knows we're in the middle of tab restore and not restore?
One possible way for this is to have a setter on SessionService for the
TabRestoreService and add functions to TabRestoreService to know if in process
of restoring.

[Michael K. (Yandex Team), 2016-09-16 17:17:59]

Ok.
I'll add setter to SessionService. And browser_live_tab_context.cc will use that
setter.
TabRestoreService won't know anything about that logic and remain unmodified
('cause it creates tabs/wins through a LiveTabContext).

[sky, 2016-09-16 17:21:03]

I think you're going down the path of a setter on SessionService,
which is still easy to get wrong. Can SessionService have a setter for
TabRestoreServiceHelper and TabRestoreServiceHelper gets a function to
query if in the process of restoring?

On Fri, Sep 16, 2016 at 10:17 AM,  <mkolom@yandex-team.ru> wrote:
> Ok.
> I'll add setter to SessionService. And browser_live_tab_context.cc will use
> that
> setter.
> TabRestoreService won't know anything about that logic and remain unmodified
> ('cause it creates tabs/wins through a LiveTabContext).
>
>
> https://codereview.chromium.org/2345763002/

-- 
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.

[Michael K. (Yandex Team), 2016-09-20 04:29:15]

Sky, I've fixed some of your notes.
Sorry but I can't understand what setter and why you want to implement for
SessionService.
SessionService can use TabRestoreService directly to check whether it is in the
middle of restoring or not.
PTAL

[sky, 2016-09-20 13:18:37]

https://codereview.chromium.org/2345763002/diff/20001/chrome/browser/sessions...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/2345763002/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:596: (!tab_restore_service ||
!tab_restore_service->IsRestoring())) {
The problem with this is that you're assuming tab restore is going to restore
everything from the last session. That is not necessarily the case.

[Michael K. (Yandex Team), 2016-09-20 14:53:58]

https://codereview.chromium.org/2345763002/diff/1/chrome/browser/sessions/tab...
File chrome/browser/sessions/tab_restore_browsertest.cc (right):

https://codereview.chromium.org/2345763002/diff/1/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_browsertest.cc:753: EXPECT_EQ(4,
browser->tab_strip_model()->count());
Without that fix there will be 8 tabs.

https://codereview.chromium.org/2345763002/diff/20001/chrome/browser/sessions...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/2345763002/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:596: (!tab_restore_service ||
!tab_restore_service->IsRestoring())) {
Hmm, I see...But now we cannot find a match between tabs/windows from
TabRestoreService and tabs/windows from SessionService: both services give IDs
to tabs/wins independently from each other and using URLs will be erroneously...
Maybe TabRestoreService should use IDs from SessionTabHelper as SessionService
does.

[sky, 2016-09-20 16:47:46]

In theory tab restore shouldn't restore things out of order, so lets go with
your fix. LGTM

and thanks!

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

https://codereview.chromium.org/2345763002/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/tab_restore_browsertest.cc:724: // Check that
TabRestoreService and SessionService do not conflict with each
conflict with each ... -> try to restore the same thing.

https://codereview.chromium.org/2345763002/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/tab_restore_browsertest.cc:748: // Restore browser (that
what Cmd-Shift-T does on Mac).
that -> this is

[Michael K. (Yandex Team), 2016-09-21 06:36:40]

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

https://codereview.chromium.org/2345763002/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/tab_restore_browsertest.cc:724: // Check that
TabRestoreService and SessionService do not conflict with each
On 2016/09/20 16:47:46, sky wrote:
> conflict with each ... -> try to restore the same thing.

Done.

https://codereview.chromium.org/2345763002/diff/20001/chrome/browser/sessions...
chrome/browser/sessions/tab_restore_browsertest.cc:748: // Restore browser (that
what Cmd-Shift-T does on Mac).
On 2016/09/20 16:47:46, sky wrote:
> that -> this is

Done.

[Michael K. (Yandex Team), 2016-09-21 06:36:53]

The CQ bit was checked by mkolom@yandex-team.ru

[Michael K. (Yandex Team), 2016-09-21 06:36:53]

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/2345763002/#ps40001
(title: "Fix comments.")

[commit-bot: I haz the power, 2016-09-21 06:37:24]

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-09-21 08:10:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-21 08:12:30]

Description was changed from

==========
Fix tabs duplication when restoring last closed window.

Restoration of a last closed window is done by TabRestoreService.
However if that window is going to be the first window for a profile,
SessionService will perform restoration and there will be conflict
between services. It leads to tabs duplication.

BUG=500083

R=sky@chromium.org
==========

to

==========
Fix tabs duplication when restoring last closed window.

Restoration of a last closed window is done by TabRestoreService.
However if that window is going to be the first window for a profile,
SessionService will perform restoration and there will be conflict
between services. It leads to tabs duplication.

BUG=500083

R=sky@chromium.org

Committed: https://crrev.com/d19beb53ae69324f842ba8efdbdd0967034f38c4
Cr-Commit-Position: refs/heads/master@{#420006}
==========

[commit-bot: I haz the power, 2016-09-21 08:12:31]

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