Use TabLoader in TabRestoreService.

Use TabLoader in TabRestoreService.

SessionService uses TabLoader to load tabs gradually.
The same behaviour is expected from TabRestoreService.

BUG=697510
Review-Url: https://codereview.chromium.org/2727663004
Cr-Commit-Position: refs/heads/master@{#454919}
Committed: https://chromium.googlesource.com/chromium/src/+/80aa799df5f54fadb8ee1343ee7cdca79e3490eb

[Michael K. (Yandex Team), 2017-03-01 18:00:19]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 18:00:40]

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-03-01 18:00:41]

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

[commit-bot: I haz the power, 2017-03-01 18:00:42]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[Michael K. (Yandex Team), 2017-03-01 18:02:36]

Description was changed from

==========
Use TabLoader in TabRestoreService.

SessionService uses TabLoader to load tabs gradually.
The same behaviour is expected from TabRestoreService.

BUG=697510
==========

to

==========
Use TabLoader in TabRestoreService.

SessionService uses TabLoader to load tabs gradually.
The same behaviour is expected from TabRestoreService.

BUG=697510
==========

[Michael K. (Yandex Team), 2017-03-01 18:02:36]

mkolom@yandex-team.ru changed reviewers:
+ sky@chromium.org

[Michael K. (Yandex Team), 2017-03-02 12:48:16]

PTAL

[sky, 2017-03-02 19:12:05]

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/sessions/tab...
File chrome/browser/sessions/tab_loader.h (right):

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_loader.h:67: static void
SetMaxLoadedTabCountForTest(size_t value);
I prefer keeping the public api clean. Instead of this, make the test a friend.
If you need multiple tests to access it, friend a TabLoaderTestApi class that is
only built in tests.

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_loader.h:152: // The number of tabs started to load.
Document this value never decreases.

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

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_browsertest.cc:765: #if
BUILDFLAG(ENABLE_SESSION_SERVICE)
Why does this require session service?

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_browsertest.cc:776: EXPECT_EQ(4,
browser2->tab_strip_model()->count());
Use a constant for this so to make it clear the restored tab should have the
same number of tabs created here. Also use that constant in the previous line.

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_browsertest.cc:783: // Limit the number of
restored tabs that should be loaded.
'should be' -> 'are'

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_browsertest.cc:799: // It isn't necessary
but just to be sure there is no any async task that
I don't understand this comment. Can you elaborate on why this is needed?

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_live_tab_context.cc (right):

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_live_tab_context.cc:90:
std::vector<TabLoader::RestoredTab> restoredTabs;
restored_tabs

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/ui/browser_t...
File chrome/browser/ui/browser_tabrestore_browsertest.cc (right):

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/ui/browser_t...
chrome/browser/ui/browser_tabrestore_browsertest.cc:100: const auto* about_blank
= browser->tab_strip_model()->GetWebContentsAt(0);
about_blank_contents?
optional: mild preference for no auto* here.

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/ui/browser_t...
chrome/browser/ui/browser_tabrestore_browsertest.cc:102:
content::WindowedNotificationObserver load_stop_observer(
Move this into if?

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/ui/browser_t...
chrome/browser/ui/browser_tabrestore_browsertest.cc:154: const auto* about_blank
= browser->tab_strip_model()->GetWebContentsAt(0);
Similar comments as above.

[Michael K. (Yandex Team), 2017-03-03 08:26:37]

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/sessions/tab...
File chrome/browser/sessions/tab_loader.h (right):

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_loader.h:67: static void
SetMaxLoadedTabCountForTest(size_t value);
On 2017/03/02 19:12:05, sky wrote:
> I prefer keeping the public api clean. Instead of this, make the test a
friend.
> If you need multiple tests to access it, friend a TabLoaderTestApi class that
is
> only built in tests.

Done.

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_loader.h:152: // The number of tabs started to load.
On 2017/03/02 19:12:05, sky wrote:
> Document this value never decreases.

Done.

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

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_browsertest.cc:765: #if
BUILDFLAG(ENABLE_SESSION_SERVICE)
Because of TabLoader: it depends on this buildflag.
See chrome/browser/BUILD.gn ex.

I added a cooment here in the next patchset.

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_browsertest.cc:776: EXPECT_EQ(4,
browser2->tab_strip_model()->count());
On 2017/03/02 19:12:05, sky wrote:
> Use a constant for this so to make it clear the restored tab should have the
> same number of tabs created here. Also use that constant in the previous line.

Done.

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_browsertest.cc:783: // Limit the number of
restored tabs that should be loaded.
On 2017/03/02 19:12:05, sky wrote:
> 'should be' -> 'are'

Done.

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_browsertest.cc:799: // It isn't necessary
but just to be sure there is no any async task that
I made small fix here in the next patchset.

What I want to say: The last block (for-cycle) and its expectations can be ok if
we check it immediately. But what if there is an async/pending task that can
break these expectations?
RunAllPendingInMessageLoop adds a bit more reliability that there is no such a
task. That nothing can break these expectations.

Of course the test works without this RunAllPendingInMessageLoop.

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_live_tab_context.cc (right):

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_live_tab_context.cc:90:
std::vector<TabLoader::RestoredTab> restoredTabs;
On 2017/03/02 19:12:05, sky wrote:
> restored_tabs

Done.

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/ui/browser_t...
File chrome/browser/ui/browser_tabrestore_browsertest.cc (right):

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/ui/browser_t...
chrome/browser/ui/browser_tabrestore_browsertest.cc:100: const auto* about_blank
= browser->tab_strip_model()->GetWebContentsAt(0);
On 2017/03/02 19:12:05, sky wrote:
> about_blank_contents?
> optional: mild preference for no auto* here.

Done.

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/ui/browser_t...
chrome/browser/ui/browser_tabrestore_browsertest.cc:102:
content::WindowedNotificationObserver load_stop_observer(
On 2017/03/02 19:12:05, sky wrote:
> Move this into if?

Done.

https://codereview.chromium.org/2727663004/diff/1/chrome/browser/ui/browser_t...
chrome/browser/ui/browser_tabrestore_browsertest.cc:154: const auto* about_blank
= browser->tab_strip_model()->GetWebContentsAt(0);
On 2017/03/02 19:12:05, sky wrote:
> Similar comments as above.

Done.

[Michael K. (Yandex Team), 2017-03-03 08:27:11]

PTAL

[sky, 2017-03-06 17:48:28]

LGTM

[Michael K. (Yandex Team), 2017-03-06 18:41:00]

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

[commit-bot: I haz the power, 2017-03-06 18:41:23]

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-06 19:38:30]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488825660311850,
"parent_rev": "7fa89a1107a81e961053b4a7e9f12fd8de6ca030", "commit_rev":
"80aa799df5f54fadb8ee1343ee7cdca79e3490eb"}

[commit-bot: I haz the power, 2017-03-06 19:39:51]

Description was changed from

==========
Use TabLoader in TabRestoreService.

SessionService uses TabLoader to load tabs gradually.
The same behaviour is expected from TabRestoreService.

BUG=697510
==========

to

==========
Use TabLoader in TabRestoreService.

SessionService uses TabLoader to load tabs gradually.
The same behaviour is expected from TabRestoreService.

BUG=697510

Review-Url: https://codereview.chromium.org/2727663004
Cr-Commit-Position: refs/heads/master@{#454919}
Committed:
https://chromium.googlesource.com/chromium/src/+/80aa799df5f54fadb8ee1343ee7c...
==========

[commit-bot: I haz the power, 2017-03-06 19:39:52]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/80aa799df5f54fadb8ee1343ee7c...