Ensure History > Recent Tabs restore preserves window disposition.

Ensure History > Recent Tabs restore preserves window disposition.

This plumbs the window bounds, show state and workspace through the tab restore stack.
This ensure that "Window" entries are restored with their previous window parameters.

This is being submitted with NOPRESUBMIT, because the code uses a deprecated API (Time::To/FromInternal). Unfortunately, it can't be updated to not use it as their exists user data with these values. The code isn't "new", simply refactored.

BUG=718042
NOPRESUBMIT=true

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

[chrisha, 2017-05-08 22:03:10]

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

[chrisha, 2017-05-08 22:03:43]

Description was changed from

==========
Ensure History > Recent Tabs restore preserves window disposition.

This plumbs the window bounds, show state and workspace through the tab restore
stack.
This ensure that "Window" entries are restored with their previous window
parameters.

This also reworks the fix to crbug.com/56744 to make the behaviour correct when
opening with NEW_SINGLETON_TAB or UNKNOWN disposition (the tab will be restored
with
the previous window parameters).

BUG=718042
==========

to

==========
Ensure History > Recent Tabs restore preserves window disposition.

This plumbs the window bounds, show state and workspace through the tab restore
stack.
This ensure that "Window" entries are restored with their previous window
parameters.

This also reworks the fix to crbug.com/56744 to make the behaviour correct when
opening with NEW_SINGLETON_TAB or UNKNOWN disposition (the tab will be restored
with
the previous window parameters).

BUG=718042
==========

[chrisha, 2017-05-08 22:03:43]

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

[chrisha, 2017-05-08 22:04:40]

sky, PTAL?

I made an executive decision about how to fix the "single-tab window" case,
discussed briefly in the linked bug. Your input would be appreciated if you
think it should be handled differently.

Most of the code being touched has *no* tests right now. Thoughts on the best
way to test this?

[commit-bot: I haz the power, 2017-05-08 22:05:46]

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, 2017-05-08 22:15:57]

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

[commit-bot: I haz the power, 2017-05-08 22:15:59]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sky, 2017-05-08 22:51:36]

https://codereview.chromium.org/2868983003/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_live_tab_context.cc (right):

https://codereview.chromium.org/2868983003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_live_tab_context.cc:61: auto* window =
browser_->window();
Please don't use auto* here. Using auto here reduces readability. In what case
is window() null?

https://codereview.chromium.org/2868983003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_live_tab_context.cc:155: Browser* browser;
Move this to line 170 where it's used.

https://codereview.chromium.org/2868983003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_live_tab_context.cc:158: create_params =
std::make_unique<Browser::CreateParams>(
MakeUnique? We're not using c++ 14 yet are we?

https://codereview.chromium.org/2868983003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_live_tab_context.cc:171: if (browser)
|browser| should always be non-null.

https://codereview.chromium.org/2868983003/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right):

https://codereview.chromium.org/2868983003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:470: if
(window.tabs.size() > 1) {
We shouldn't need the special case here. Whether you get a tab vs a window
should depend upon how you closed the window. If you close the window, then you
should get a window entry, if you close the last tab in a window then I would
expect a tab entry.

https://codereview.chromium.org/2868983003/diff/40001/components/sessions/cor...
File components/sessions/core/tab_restore_service_helper.cc (right):

https://codereview.chromium.org/2868983003/diff/40001/components/sessions/cor...
components/sessions/core/tab_restore_service_helper.cc:92: // If this is the
last tab being closed in a window it is equivalent to a
We differentiate between closing a tab and closing a window in other places. I
think we should continue that differentiation here as well. When you close a
tab, you are closing the tab. That the tab happens to be the last tab in a
window is secondary to closing the tab.

[chrisha, 2017-05-09 14:59:31]

Regarding the closing window vs closing tab discussion, I've moved that to the
bug. Would be happy to see your feedback there.

[chrisha, 2017-05-09 15:44:55]

https://codereview.chromium.org/2868983003/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_live_tab_context.cc (right):

https://codereview.chromium.org/2868983003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_live_tab_context.cc:61: auto* window =
browser_->window();
On 2017/05/08 22:51:35, sky wrote:
> Please don't use auto* here. Using auto here reduces readability. In what case
> is window() null?

Yeah, from what I can see window() should never be null. Removed.

https://codereview.chromium.org/2868983003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_live_tab_context.cc:155: Browser* browser;
On 2017/05/08 22:51:35, sky wrote:
> Move this to line 170 where it's used.

Done.

https://codereview.chromium.org/2868983003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_live_tab_context.cc:158: create_params =
std::make_unique<Browser::CreateParams>(
On 2017/05/08 22:51:35, sky wrote:
> MakeUnique? We're not using c++ 14 yet are we?

Oops, not yet :)

https://codereview.chromium.org/2868983003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_live_tab_context.cc:171: if (browser)
On 2017/05/08 22:51:35, sky wrote:
> |browser| should always be non-null.

Done.

https://codereview.chromium.org/2868983003/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right):

https://codereview.chromium.org/2868983003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:470: if
(window.tabs.size() > 1) {
On 2017/05/08 22:51:36, sky wrote:
> We shouldn't need the special case here. Whether you get a tab vs a window
> should depend upon how you closed the window. If you close the window, then
you
> should get a window entry, if you close the last tab in a window then I would
> expect a tab entry.

Moved this discussion to the bug, as it's slightly broader scope that in this
code review.

https://codereview.chromium.org/2868983003/diff/40001/components/sessions/cor...
File components/sessions/core/tab_restore_service_helper.cc (right):

https://codereview.chromium.org/2868983003/diff/40001/components/sessions/cor...
components/sessions/core/tab_restore_service_helper.cc:92: // If this is the
last tab being closed in a window it is equivalent to a
On 2017/05/08 22:51:36, sky wrote:
> We differentiate between closing a tab and closing a window in other places. I
> think we should continue that differentiation here as well. When you close a
> tab, you are closing the tab. That the tab happens to be the last tab in a
> window is secondary to closing the tab.

Ditto with moving this discussion to the bug.

[chrisha, 2017-05-09 15:45:01]

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

[commit-bot: I haz the power, 2017-05-09 15:45:52]

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, 2017-05-09 15:56:02]

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

[commit-bot: I haz the power, 2017-05-09 15:56:03]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[chrisha, 2017-05-11 15:39:07]

Description was changed from

==========
Ensure History > Recent Tabs restore preserves window disposition.

This plumbs the window bounds, show state and workspace through the tab restore
stack.
This ensure that "Window" entries are restored with their previous window
parameters.

This also reworks the fix to crbug.com/56744 to make the behaviour correct when
opening with NEW_SINGLETON_TAB or UNKNOWN disposition (the tab will be restored
with
the previous window parameters).

BUG=718042
==========

to

==========
Ensure History > Recent Tabs restore preserves window disposition.

This plumbs the window bounds, show state and workspace through the tab restore
stack.
This ensure that "Window" entries are restored with their previous window
parameters.

BUG=718042
==========

[chrisha, 2017-05-11 15:39:33]

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

[commit-bot: I haz the power, 2017-05-11 15:40:10]

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, 2017-05-11 15:50:02]

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

[commit-bot: I haz the power, 2017-05-11 15:50:03]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[chrisha, 2017-05-11 17:02:55]

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

[commit-bot: I haz the power, 2017-05-11 17:03:52]

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, 2017-05-11 17:15:23]

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

[commit-bot: I haz the power, 2017-05-11 17:15:23]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[chrisha, 2017-05-11 17:24:26]

chrisha@chromium.org changed reviewers:
+ sdefresne@chromium.org

[chrisha, 2017-05-11 17:24:27]

sky: I've removed any UI modification and simply focused on restoring window
dimensions. I'll move the UI discussions to another bug. PTAL?
sdefresne: Plumbing for a few new parameters that are ignored on iOS. PTAL?

[chrisha, 2017-05-11 17:39:20]

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

[commit-bot: I haz the power, 2017-05-11 17:39:55]

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, 2017-05-11 18:36:38]

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

[commit-bot: I haz the power, 2017-05-11 18:36:39]

Dry run: This issue passed the CQ dry run.

[sky, 2017-05-12 02:42:07]

https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/andr...
File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right):

https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/andr...
chrome/browser/ui/android/tab_model/android_live_tab_context.cc:62: // Not
supported by android.
supported->applicable to?

https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/andr...
chrome/browser/ui/android/tab_model/android_live_tab_context.cc:74: return "";
"" -> std::string()

https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_live_tab_context.h (right):

https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/brow...
chrome/browser/ui/browser_live_tab_context.h:15: #include
"ui/gfx/geometry/rect.h"
forward declare rect.

https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
File components/sessions/core/live_tab_context.h (right):

https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
components/sessions/core/live_tab_context.h:14: #include
"ui/gfx/geometry/rect.h"
forward declare rect.

https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:511: WindowPayload2
payload;
Doesn't this need to be updated?

https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
File components/sessions/core/tab_restore_service_client.h (right):

https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
components/sessions/core/tab_restore_service_client.h:15: #include
"ui/gfx/geometry/rect.h"
forward declare.

https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
File components/sessions/core/tab_restore_service_helper.cc (right):

https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
components/sessions/core/tab_restore_service_helper.cc:470:
ui::SHOW_STATE_NORMAL, "");
std::string()

[sdefresne, 2017-05-15 15:59:55]

ios lgtm

https://codereview.chromium.org/2868983003/diff/130001/ios/chrome/browser/ses...
File ios/chrome/browser/sessions/tab_restore_service_delegate_impl_ios.mm
(right):

https://codereview.chromium.org/2868983003/diff/130001/ios/chrome/browser/ses...
ios/chrome/browser/sessions/tab_restore_service_delegate_impl_ios.mm:86: return
"";
return std::string();

[sdefresne, 2017-06-15 09:41:58]

chrisha: ping?

[chrisha, 2017-06-15 19:34:34]

On 2017/06/15 09:41:58, sdefresne wrote:
> chrisha: ping?

Was traveling 3 of last 4 weeks, just getting back to this. Expect movement next
week :)

[chrisha, 2017-06-28 18:30:42]

*Finally* another round on this. Working on unittests.

sky@, can you recommend another reviewer while you're out on vacation? (If not,
this is not high priority and can wait.)

https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/andr...
File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right):

https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/andr...
chrome/browser/ui/android/tab_model/android_live_tab_context.cc:62: // Not
supported by android.
On 2017/05/12 02:42:06, sky wrote:
> supported->applicable to?

I was copy-pasting similar comments from in this file, but agreed "not
applicable to" is more precise. Changed this and the other comments.

https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/andr...
chrome/browser/ui/android/tab_model/android_live_tab_context.cc:74: return "";
On 2017/05/12 02:42:06, sky wrote:
> "" -> std::string()

Done.

https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_live_tab_context.h (right):

https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/brow...
chrome/browser/ui/browser_live_tab_context.h:15: #include
"ui/gfx/geometry/rect.h"
On 2017/05/12 02:42:06, sky wrote:
> forward declare rect.

Done.

https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
File components/sessions/core/live_tab_context.h (right):

https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
components/sessions/core/live_tab_context.h:14: #include
"ui/gfx/geometry/rect.h"
On 2017/05/12 02:42:06, sky wrote:
> forward declare rect.

Done.

https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:511: WindowPayload2
payload;
On 2017/05/12 02:42:06, sky wrote:
> Doesn't this need to be updated?

Indeed it does.

https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
File components/sessions/core/tab_restore_service_client.h (right):

https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
components/sessions/core/tab_restore_service_client.h:15: #include
"ui/gfx/geometry/rect.h"
On 2017/05/12 02:42:06, sky wrote:
> forward declare.

Done.

https://codereview.chromium.org/2868983003/diff/130001/ios/chrome/browser/ses...
File ios/chrome/browser/sessions/tab_restore_service_delegate_impl_ios.mm
(right):

https://codereview.chromium.org/2868983003/diff/130001/ios/chrome/browser/ses...
ios/chrome/browser/sessions/tab_restore_service_delegate_impl_ios.mm:86: return
"";
On 2017/05/15 15:59:54, sdefresne wrote:
> return std::string();

Done.

[sky, 2017-06-28 20:06:56]

On 2017/06/28 18:30:42, chrisha wrote:
> *Finally* another round on this. Working on unittests.
> 
> sky@, can you recommend another reviewer while you're out on vacation? (If
not,
> this is not high priority and can wait.)
> 
>
https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/andr...
> File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right):
> 
>
https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/andr...
> chrome/browser/ui/android/tab_model/android_live_tab_context.cc:62: // Not
> supported by android.
> On 2017/05/12 02:42:06, sky wrote:
> > supported->applicable to?
> 
> I was copy-pasting similar comments from in this file, but agreed "not
> applicable to" is more precise. Changed this and the other comments.
> 
>
https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/andr...
> chrome/browser/ui/android/tab_model/android_live_tab_context.cc:74: return "";
> On 2017/05/12 02:42:06, sky wrote:
> > "" -> std::string()
> 
> Done.
> 
>
https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/brow...
> File chrome/browser/ui/browser_live_tab_context.h (right):
> 
>
https://codereview.chromium.org/2868983003/diff/130001/chrome/browser/ui/brow...
> chrome/browser/ui/browser_live_tab_context.h:15: #include
> "ui/gfx/geometry/rect.h"
> On 2017/05/12 02:42:06, sky wrote:
> > forward declare rect.
> 
> Done.
> 
>
https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
> File components/sessions/core/live_tab_context.h (right):
> 
>
https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
> components/sessions/core/live_tab_context.h:14: #include
> "ui/gfx/geometry/rect.h"
> On 2017/05/12 02:42:06, sky wrote:
> > forward declare rect.
> 
> Done.
> 
>
https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
> File components/sessions/core/persistent_tab_restore_service.cc (right):
> 
>
https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
> components/sessions/core/persistent_tab_restore_service.cc:511: WindowPayload2
> payload;
> On 2017/05/12 02:42:06, sky wrote:
> > Doesn't this need to be updated?
> 
> Indeed it does.
> 
>
https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
> File components/sessions/core/tab_restore_service_client.h (right):
> 
>
https://codereview.chromium.org/2868983003/diff/130001/components/sessions/co...
> components/sessions/core/tab_restore_service_client.h:15: #include
> "ui/gfx/geometry/rect.h"
> On 2017/05/12 02:42:06, sky wrote:
> > forward declare.
> 
> Done.
> 
>
https://codereview.chromium.org/2868983003/diff/130001/ios/chrome/browser/ses...
> File ios/chrome/browser/sessions/tab_restore_service_delegate_impl_ios.mm
> (right):
> 
>
https://codereview.chromium.org/2868983003/diff/130001/ios/chrome/browser/ses...
> ios/chrome/browser/sessions/tab_restore_service_delegate_impl_ios.mm:86:
return
> "";
> On 2017/05/15 15:59:54, sdefresne wrote:
> > return std::string();
> 
> Done.

In a pinch you could go with brettw@.

[chrisha, 2017-06-28 21:25:28]

Now with a unittest, working on a browsertest.

sky: Feel free to take a look if you've got a chance before leaving.

[sky, 2017-06-28 22:38:01]

https://codereview.chromium.org/2868983003/diff/230001/chrome/browser/session...
File chrome/browser/sessions/chrome_tab_restore_service_client.h (right):

https://codereview.chromium.org/2868983003/diff/230001/chrome/browser/session...
chrome/browser/sessions/chrome_tab_restore_service_client.h:10: #include
"ui/base/ui_base_types.h"
These includes (or forward declarations) should be in TabRestoreServiceClient
and not necessary here.

https://codereview.chromium.org/2868983003/diff/230001/chrome/browser/session...
File chrome/browser/sessions/persistent_tab_restore_service_unittest.cc (right):

https://codereview.chromium.org/2868983003/diff/230001/chrome/browser/session...
chrome/browser/sessions/persistent_tab_restore_service_unittest.cc:81: SessionID
tab_id() { return tab_id_; }
const on both these. That is:
SessionID tab_id() const

https://codereview.chromium.org/2868983003/diff/230001/chrome/browser/session...
chrome/browser/sessions/persistent_tab_restore_service_unittest.cc:496: static
constexpr gfx::Rect kBounds(10, 20, 640, 480);
Remove static from these.

https://codereview.chromium.org/2868983003/diff/230001/chrome/browser/ui/andr...
File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right):

https://codereview.chromium.org/2868983003/diff/230001/chrome/browser/ui/andr...
chrome/browser/ui/android/tab_model/android_live_tab_context.cc:57: // Not
applicable to android.
optional: move comments inside implementation.

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
File components/sessions/core/live_tab_context.h (right):

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/live_tab_context.h:39: virtual const gfx::Rect
GetBounds() const = 0;
I favor making these functions match the functions chrome code calls to. By that
I mean GetRestoredBounds and GetRestoredState.

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (left):

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:776: // Unknown type,
usually indicates corruption of file. Ignore it.
Why are you removing this comment?

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:57: struct
WindowPayload2 : WindowPayload {
How about adding Obsolete as a suffix to this and WindowPayload?

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:174:
WindowCommandFields f;
Please use a more readable name then 'f'.

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:233:
static_cast<ui::WindowShowState>(fields.window_show_state);
You need to validate the enum.

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:623:
pickle.WriteInt(window_id);
You need to make sure the writing, reading and representation of the types line
up. For example, here you use int, but window_id is an int32_t, not an int.

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:631:
pickle.WriteInt(static_cast<int32_t>(show_state));
I would prefer you mirror WindowShowState locally. That way if the values of the
WindowShowState enum change this code isn't impacted.

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:635: if
(workspace.size() <= 128) {
no {}

[chrisha, 2017-06-30 15:42:10]

chrisha@chromium.org changed reviewers:
+ brettw@chromium.org

[chrisha, 2017-06-30 15:42:13]

Addressed all of sky's nits, and added a new browser test.

brettw: sky mentioned you as a possible alternative reviewer given that he's now
on vacation. You up for it?

https://codereview.chromium.org/2868983003/diff/230001/chrome/browser/session...
File chrome/browser/sessions/chrome_tab_restore_service_client.h (right):

https://codereview.chromium.org/2868983003/diff/230001/chrome/browser/session...
chrome/browser/sessions/chrome_tab_restore_service_client.h:10: #include
"ui/base/ui_base_types.h"
On 2017/06/28 22:38:00, sky wrote:
> These includes (or forward declarations) should be in TabRestoreServiceClient
> and not necessary here.

Ah, good point.

Done.

https://codereview.chromium.org/2868983003/diff/230001/chrome/browser/session...
File chrome/browser/sessions/persistent_tab_restore_service_unittest.cc (right):

https://codereview.chromium.org/2868983003/diff/230001/chrome/browser/session...
chrome/browser/sessions/persistent_tab_restore_service_unittest.cc:81: SessionID
tab_id() { return tab_id_; }
On 2017/06/28 22:38:00, sky OOO wrote:
> const on both these. That is:
> SessionID tab_id() const

Done.

https://codereview.chromium.org/2868983003/diff/230001/chrome/browser/session...
chrome/browser/sessions/persistent_tab_restore_service_unittest.cc:496: static
constexpr gfx::Rect kBounds(10, 20, 640, 480);
On 2017/06/28 22:38:00, sky OOO wrote:
> Remove static from these.

Done.

https://codereview.chromium.org/2868983003/diff/230001/chrome/browser/ui/andr...
File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right):

https://codereview.chromium.org/2868983003/diff/230001/chrome/browser/ui/andr...
chrome/browser/ui/android/tab_model/android_live_tab_context.cc:57: // Not
applicable to android.
On 2017/06/28 22:38:00, sky OOO wrote:
> optional: move comments inside implementation.

Done.

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
File components/sessions/core/live_tab_context.h (right):

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/live_tab_context.h:39: virtual const gfx::Rect
GetBounds() const = 0;
On 2017/06/28 22:38:00, sky OOO wrote:
> I favor making these functions match the functions chrome code calls to. By
that
> I mean GetRestoredBounds and GetRestoredState.

Done.

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (left):

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:776: // Unknown type,
usually indicates corruption of file. Ignore it.
On 2017/06/28 22:38:00, sky OOO wrote:
> Why are you removing this comment?

Oops, stray line removal. Restored.

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:57: struct
WindowPayload2 : WindowPayload {
On 2017/06/28 22:38:01, sky OOO wrote:
> How about adding Obsolete as a suffix to this and WindowPayload?

Done.

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:174:
WindowCommandFields f;
On 2017/06/28 22:38:00, sky OOO wrote:
> Please use a more readable name then 'f'.

Done.

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:233:
static_cast<ui::WindowShowState>(fields.window_show_state);
On 2017/06/28 22:38:00, sky OOO wrote:
> You need to validate the enum.

Done.

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:623:
pickle.WriteInt(window_id);
On 2017/06/28 22:38:00, sky OOO wrote:
> You need to make sure the writing, reading and representation of the types
line
> up. For example, here you use int, but window_id is an int32_t, not an int.

Done.

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:631:
pickle.WriteInt(static_cast<int32_t>(show_state));
On 2017/06/28 22:38:00, sky OOO wrote:
> I would prefer you mirror WindowShowState locally. That way if the values of
the
> WindowShowState enum change this code isn't impacted.

Done.

https://codereview.chromium.org/2868983003/diff/230001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:635: if
(workspace.size() <= 128) {
On 2017/06/28 22:38:00, sky OOO wrote:
> no {}

Done.

[sky, 2017-07-11 20:28:09]

https://codereview.chromium.org/2868983003/diff/270001/chrome/browser/ui/andr...
File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right):

https://codereview.chromium.org/2868983003/diff/270001/chrome/browser/ui/andr...
chrome/browser/ui/android/tab_model/android_live_tab_context.cc:58: // Not
applicable to android.
I like this better that you have the comment inside the function. Add
NOTREACHED() where applicable.

https://codereview.chromium.org/2868983003/diff/270001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/2868983003/diff/270001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:141: int
WindowShowStateToInt(ui::WindowShowState show_state) {
optional: Maybe WindowShowStateToSerializedValue and DeserializeWindowShowState?

https://codereview.chromium.org/2868983003/diff/270001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:144: return 0;
Use a local enum with explicit values and a warning about values needing to line
up.

https://codereview.chromium.org/2868983003/diff/270001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:223:
std::unique_ptr<base::Pickle> pickle(command->PayloadAsPickle());
I think this code would be less error prone if you use a different id for the
new pickle variant. That way you only try to extract the pickle if it should be
a pickle. (my concern is having to worry about the size of the pickle variant
potentially lining up with WindowPayloadObsolete*).

https://codereview.chromium.org/2868983003/diff/270001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:236:
it.ReadInt(&parsed_fields.window_width) &&
Make sure width/height are non-negative.

https://codereview.chromium.org/2868983003/diff/270001/ios/chrome/browser/ses...
File ios/chrome/browser/sessions/ios_chrome_tab_restore_service_client.mm
(right):

https://codereview.chromium.org/2868983003/diff/270001/ios/chrome/browser/ses...
ios/chrome/browser/sessions/ios_chrome_tab_restore_service_client.mm:74: const
std::string& unused_app_name,
Parameters should match that of the header.

https://codereview.chromium.org/2868983003/diff/270001/ios/chrome/browser/ses...
File ios/chrome/browser/sessions/tab_restore_service_delegate_impl_ios.mm
(right):

https://codereview.chromium.org/2868983003/diff/270001/ios/chrome/browser/ses...
ios/chrome/browser/sessions/tab_restore_service_delegate_impl_ios.mm:69: // Not
supported by iOS.
I think this comment would be better inside the function right a NOTREACHED(). I
would also make it clear these functions aren't *called* either (which the
NOTREACHED() implies, but the comment doesn't exactly indicate that).

[chrisha, 2017-08-14 14:43:20]

chrisha@chromium.org changed reviewers:
- brettw@chromium.org

[chrisha, 2017-08-14 14:43:21]

Yet another round. I think this is now officially the "longest" CL I've had! :)

(Much of the noise in the review is due to rebasing.)

https://codereview.chromium.org/2868983003/diff/270001/chrome/browser/ui/andr...
File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right):

https://codereview.chromium.org/2868983003/diff/270001/chrome/browser/ui/andr...
chrome/browser/ui/android/tab_model/android_live_tab_context.cc:58: // Not
applicable to android.
On 2017/07/11 20:28:09, sky wrote:
> I like this better that you have the comment inside the function. Add
> NOTREACHED() where applicable.

Done.

https://codereview.chromium.org/2868983003/diff/270001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/2868983003/diff/270001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:141: int
WindowShowStateToInt(ui::WindowShowState show_state) {
On 2017/07/11 20:28:09, sky wrote:
> optional: Maybe WindowShowStateToSerializedValue and
DeserializeWindowShowState?

Done.

https://codereview.chromium.org/2868983003/diff/270001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:144: return 0;
On 2017/07/11 20:28:09, sky wrote:
> Use a local enum with explicit values and a warning about values needing to
line
> up.

Done.

https://codereview.chromium.org/2868983003/diff/270001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:223:
std::unique_ptr<base::Pickle> pickle(command->PayloadAsPickle());
On 2017/07/11 20:28:09, sky wrote:
> I think this code would be less error prone if you use a different id for the
> new pickle variant. That way you only try to extract the pickle if it should
be
> a pickle. (my concern is having to worry about the size of the pickle variant
> potentially lining up with WindowPayloadObsolete*).

That's a good point. Done.

https://codereview.chromium.org/2868983003/diff/270001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:236:
it.ReadInt(&parsed_fields.window_width) &&
On 2017/07/11 20:28:09, sky wrote:
> Make sure width/height are non-negative.

Done.

https://codereview.chromium.org/2868983003/diff/270001/ios/chrome/browser/ses...
File ios/chrome/browser/sessions/ios_chrome_tab_restore_service_client.mm
(right):

https://codereview.chromium.org/2868983003/diff/270001/ios/chrome/browser/ses...
ios/chrome/browser/sessions/ios_chrome_tab_restore_service_client.mm:74: const
std::string& unused_app_name,
On 2017/07/11 20:28:09, sky wrote:
> Parameters should match that of the header.

I'm generally in favor of that, but I suppose I see it as clear documentation
that the indicated parameters are being ignored. From the style guide they say
to use the same parameter names, but to comment them out:

https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...

https://codereview.chromium.org/2868983003/diff/270001/ios/chrome/browser/ses...
File ios/chrome/browser/sessions/tab_restore_service_delegate_impl_ios.mm
(right):

https://codereview.chromium.org/2868983003/diff/270001/ios/chrome/browser/ses...
ios/chrome/browser/sessions/tab_restore_service_delegate_impl_ios.mm:69: // Not
supported by iOS.
On 2017/07/11 20:28:09, sky wrote:
> I think this comment would be better inside the function right a NOTREACHED().
I
> would also make it clear these functions aren't *called* either (which the
> NOTREACHED() implies, but the comment doesn't exactly indicate that).

Done.

[sky, 2017-08-14 16:43:52]

LGTM

https://codereview.chromium.org/2868983003/diff/290001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/2868983003/diff/290001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:145:
SERIALIZED_SHOW_STATE_INVALID = -1,
Latest style is kCamelCase.

https://codereview.chromium.org/2868983003/diff/290001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:174: ;
remove this.

[chrisha, 2017-08-14 17:22:10]

Description was changed from

==========
Ensure History > Recent Tabs restore preserves window disposition.

This plumbs the window bounds, show state and workspace through the tab restore
stack.
This ensure that "Window" entries are restored with their previous window
parameters.

BUG=718042
==========

to

==========
Ensure History > Recent Tabs restore preserves window disposition.

This plumbs the window bounds, show state and workspace through the tab restore
stack.
This ensure that "Window" entries are restored with their previous window
parameters.

This is being submitted with NOPRESUBMIT, because the code uses a deprecated API
(Time::To/FromInternal). Unfortunately, it can't be updated to not use it as
their exists user data with these values. The code isn't "new", simply
refactored.

BUG=718042
NOPRESUBMIT=true
==========

[chrisha, 2017-08-14 17:22:34]

Thanks for the thorough reviews!

https://codereview.chromium.org/2868983003/diff/290001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/2868983003/diff/290001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:145:
SERIALIZED_SHOW_STATE_INVALID = -1,
On 2017/08/14 16:43:52, sky wrote:
> Latest style is kCamelCase.

Done.

https://codereview.chromium.org/2868983003/diff/290001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:174: ;
On 2017/08/14 16:43:52, sky wrote:
> remove this.

Done.

[chrisha, 2017-08-14 17:22:45]

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

[commit-bot: I haz the power, 2017-08-14 17:23:01]

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, 2017-08-14 17:46:47]

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

[commit-bot: I haz the power, 2017-08-14 17:46:48]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[chrisha, 2017-08-14 20:13:18]

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

[commit-bot: I haz the power, 2017-08-14 20:13:33]

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, 2017-08-14 21:10:04]

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

[commit-bot: I haz the power, 2017-08-14 21:10:05]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chrisha, 2017-08-14 21:48:00]

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

[commit-bot: I haz the power, 2017-08-14 21:48:06]

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, 2017-08-14 23:25:35]

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

[commit-bot: I haz the power, 2017-08-14 23:25:36]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chrisha, 2017-08-15 13:57:40]

The CQ bit was checked by chrisha@chromium.org

[chrisha, 2017-08-15 13:57:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2868983003/#ps370001
(title: "Fix unittest.")

[commit-bot: I haz the power, 2017-08-15 13:57:49]

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-08-15 15:20:39]

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

[commit-bot: I haz the power, 2017-08-15 15:20:40]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chrisha, 2017-08-17 21:18:22]

The CQ bit was checked by chrisha@chromium.org

[commit-bot: I haz the power, 2017-08-17 21:18:32]

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-08-17 22:48:41]

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

[commit-bot: I haz the power, 2017-08-17 22:48:43]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chrisha, 2017-08-18 20:58:17]

The CQ bit was checked by chrisha@chromium.org

[chrisha, 2017-08-18 20:58:17]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2868983003/#ps390001
(title: "Remove NOTREACHED().")

[commit-bot: I haz the power, 2017-08-18 20:58:21]

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-08-18 22:00:52]

CQ is committing da patch.

Bot data: {"patchset_id": 390001, "attempt_start_ts": 1503089897430080,
"parent_rev": "2084d0274e99a6a5de25148fca67ded219682a2a", "commit_rev":
"b1ba090f0f5fa59c98bd4d8b5c17d6ae7192c749"}

[commit-bot: I haz the power, 2017-08-18 22:01:17]

Description was changed from

==========
Ensure History > Recent Tabs restore preserves window disposition.

This plumbs the window bounds, show state and workspace through the tab restore
stack.
This ensure that "Window" entries are restored with their previous window
parameters.

This is being submitted with NOPRESUBMIT, because the code uses a deprecated API
(Time::To/FromInternal). Unfortunately, it can't be updated to not use it as
their exists user data with these values. The code isn't "new", simply
refactored.

BUG=718042
NOPRESUBMIT=true
==========

to

==========
Ensure History > Recent Tabs restore preserves window disposition.

This plumbs the window bounds, show state and workspace through the tab restore
stack.
This ensure that "Window" entries are restored with their previous window
parameters.

This is being submitted with NOPRESUBMIT, because the code uses a deprecated API
(Time::To/FromInternal). Unfortunately, it can't be updated to not use it as
their exists user data with these values. The code isn't "new", simply
refactored.

BUG=718042
NOPRESUBMIT=true

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

[commit-bot: I haz the power, 2017-08-18 22:01:18]

Committed patchset #21 (id:390001) as
https://chromium.googlesource.com/chromium/src/+/b1ba090f0f5fa59c98bd4d8b5c17...