Added interstitial page for merge session.

Added interstitial page for merge session. This page will let us delay loading of Google properties during ChromeOS cookie session restore at login (aka /MergeSession).

The interstitial page will show 3 seconds of no UI (white tab) followed with up to 7 seconds of UI with only "Loading..." message showing in tabs. For huge majority of cases, that secondary UI won't show since merge session will probably complete by then.

BUG=171980
TEST=added merge_session_load_page_unittest.cc
TBR=sky

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182894

[zel, 2013-02-15 19:09:30]

added tests, PTAL

[oshima, 2013-02-15 22:40:52]

+achuith who worked on offline page more recently than I was.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/merge_session_load_page.cc (right):

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page.cc:94:
strings.SetString("display_icon", "block");
are these two values used?

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page.cc:128: }
nuke {}

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/merge_session_load_page.h (right):

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page.h:41: typedef
base::Callback<void(bool /*proceed*/)> CompletionCallback;
optional nit: it'd be more readable wit enum rather than bool

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/merge_session_load_page_unittest.cc (right):

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page_unittest.cc:20: static
const char* kURL2 = "http://mail.google.com/";
put this in anonymous namespace. you can remove static although it's not strict
requirement.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page_unittest.cc:27: class
TestMergeSessionLoadPage :  public chromeos::MergeSessionLoadPage {
you don't need chromeos:: namespace as you're already
in the namespace. same or other places.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page_unittest.cc:54:
ChromeRenderViewHostTestHarness::SetUp();
don't you have to call ChromeRenderViewHostTestHarness::TearDown?

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/merge_session_throttle.cc (right):

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_throttle.cc:62: int
render_process_id,
nit: is it possible to align arguments to (?
I think it's more readable if they fit 80 chars.

[zel, 2013-02-15 23:16:57]

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/merge_session_load_page.cc (right):

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page.cc:94:
strings.SetString("display_icon", "block");
On 2013/02/15 22:40:52, oshima wrote:
> are these two values used?

Done.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page.cc:128: }
On 2013/02/15 22:40:52, oshima wrote:
> nuke {}

Done.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/merge_session_load_page.h (right):

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page.h:41: typedef
base::Callback<void(bool /*proceed*/)> CompletionCallback;
On 2013/02/15 22:40:52, oshima wrote:
> optional nit: it'd be more readable wit enum rather than bool

don't need bool either... removed

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/merge_session_load_page_unittest.cc (right):

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page_unittest.cc:20: static
const char* kURL2 = "http://mail.google.com/";
On 2013/02/15 22:40:52, oshima wrote:
> put this in anonymous namespace. you can remove static although it's not
strict
> requirement.

Done.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page_unittest.cc:27: class
TestMergeSessionLoadPage :  public chromeos::MergeSessionLoadPage {
On 2013/02/15 22:40:52, oshima wrote:
> you don't need chromeos:: namespace as you're already
> in the namespace. same or other places.

Done.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page_unittest.cc:54:
ChromeRenderViewHostTestHarness::SetUp();
On 2013/02/15 22:40:52, oshima wrote:
> don't you have to call ChromeRenderViewHostTestHarness::TearDown?

Actually, I don't need to override either of these two methods. Removed.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/merge_session_throttle.cc (right):

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_throttle.cc:62: int
render_process_id,
On 2013/02/15 22:40:52, oshima wrote:
> nit: is it possible to align arguments to (?
> I think it's more readable if they fit 80 chars.

Done.

[xiyuan, 2013-02-15 23:43:07]

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/merge_session_load_page.h (right):

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page.h:55: virtual void
NotifyBlockingPageComplete(bool proceed) OVERRIDE;
Test seems no longer uses it.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/merge_session_load_page_unittest.cc (right):

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page_unittest.cc:20: static
const char* kURL2 = "http://mail.google.com/";
nit: const char* kURL1 -> const char kURL1[]

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/merge_session_load.html (right):

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:2: <html
id="template_root" i18n-values="dir:textdirection">
nit: Get rid of id="template_root" since it seems not used.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:20: -webkit-box-align:
stretch;
The above two probably have no effect when body has only one block child.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:22:
-webkit-user-select: none;
nit: alpha sort the list and -webkit-.. rules goes before other ones.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:25: .header {
seems not used. You need either a #header here or change the div to have
class=header.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:26: float: left;
why we need float here?

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:27: height: 5;
5px or 5%?

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:30: padding: 3px;
nit: alpha sort the list

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:62: <div id="header"
i18n-content="heading">
class="header"? Otherwise, you need to change your css selector above to
"#header".

[zel, 2013-02-15 23:51:05]

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/merge_session_load_page.h (right):

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page.h:55: virtual void
NotifyBlockingPageComplete(bool proceed) OVERRIDE;
On 2013/02/15 23:43:07, xiyuan wrote:
> Test seems no longer uses it.

Done.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/merge_session_load_page_unittest.cc (right):

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/merge_session_load_page_unittest.cc:20: static
const char* kURL2 = "http://mail.google.com/";
On 2013/02/15 23:43:07, xiyuan wrote:
> nit: const char* kURL1 -> const char kURL1[]

Done.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/merge_session_load.html (right):

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:2: <html
id="template_root" i18n-values="dir:textdirection">
On 2013/02/15 23:43:07, xiyuan wrote:
> nit: Get rid of id="template_root" since it seems not used.

Done.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:20: -webkit-box-align:
stretch;
On 2013/02/15 23:43:07, xiyuan wrote:
> The above two probably have no effect when body has only one block child.

Done.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:22:
-webkit-user-select: none;
On 2013/02/15 23:43:07, xiyuan wrote:
> nit: alpha sort the list and -webkit-.. rules goes before other ones.

Done.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:25: .header {
On 2013/02/15 23:43:07, xiyuan wrote:
> seems not used. You need either a #header here or change the div to have
> class=header.

Done.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:26: float: left;
On 2013/02/15 23:43:07, xiyuan wrote:
> why we need float here?

Done.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:27: height: 5;
On 2013/02/15 23:43:07, xiyuan wrote:
> 5px or 5%?

Done.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:30: padding: 3px;
On 2013/02/15 23:43:07, xiyuan wrote:
> nit: alpha sort the list

Done.

https://codereview.chromium.org/12256046/diff/8004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/merge_session_load.html:62: <div id="header"
i18n-content="heading">
On 2013/02/15 23:43:07, xiyuan wrote:
> class="header"? Otherwise, you need to change your css selector above to
> "#header".

Done.

[xiyuan, 2013-02-15 23:55:20]

LGTM

[oshima, 2013-02-16 00:05:23]

lgtm with nits

https://codereview.chromium.org/12256046/diff/14002/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/merge_session_throttle.cc (right):

https://codereview.chromium.org/12256046/diff/14002/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/merge_session_throttle.cc:7: #include <vector>
nit: this isn't necessary?

https://codereview.chromium.org/12256046/diff/14002/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/merge_session_throttle.h (right):

https://codereview.chromium.org/12256046/diff/14002/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/merge_session_throttle.h:8: #include <string>
nit: this isn't necessary?

https://codereview.chromium.org/12256046/diff/14002/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth_login_manager.h (right):

https://codereview.chromium.org/12256046/diff/14002/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:8: #include <string>
ditto

[zel, 2013-02-16 00:56:27]

https://codereview.chromium.org/12256046/diff/14002/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/merge_session_throttle.cc (right):

https://codereview.chromium.org/12256046/diff/14002/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/merge_session_throttle.cc:7: #include <vector>
On 2013/02/16 00:05:23, oshima wrote:
> nit: this isn't necessary?

Done.

https://codereview.chromium.org/12256046/diff/14002/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/merge_session_throttle.h (right):

https://codereview.chromium.org/12256046/diff/14002/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/merge_session_throttle.h:8: #include <string>
On 2013/02/16 00:05:23, oshima wrote:
> nit: this isn't necessary?

Done.

https://codereview.chromium.org/12256046/diff/14002/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth_login_manager.h (right):

https://codereview.chromium.org/12256046/diff/14002/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:8: #include <string>
On 2013/02/16 00:05:23, oshima wrote:
> ditto

Done.

[commit-bot: I haz the power, 2013-02-16 01:10:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/12256046/15023