Avoid the long merge session throttle interstitial page for NTP on Chrome OS

Avoid the long merge session throttle interstitial page for NTP on Chrome OS

While the session is being merged, opening the NTP while cause the throttling
interstitial page to kick in, which can cause a delay in displaying the page for
5+ seconds. Instead we should force loading the local NTP until the merge session
is complete.

BUG=591530
Committed: https://crrev.com/96379036a48ce6ee82040597d5e9b930c21a7a3d
Cr-Commit-Position: refs/heads/master@{#389514}

[afakhry, 2016-04-21 00:23:11]

afakhry@chromium.org changed reviewers:
+ treib@chromium.org, xiyuan@chromium.org

[afakhry, 2016-04-21 00:23:11]

Can you please take a look at this suggested fix? Thanks!

[Marc Treib, 2016-04-21 08:32:11]

treib@chromium.org changed reviewers:
+ kmadhusu@chromium.org

[Marc Treib, 2016-04-21 08:32:12]

This looks generally fine to me, but I'd like another pair of eyes to make sure.
Kausalya, could you take a look? Thanks!

https://codereview.chromium.org/1910693002/diff/1/chrome/browser/search/searc...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/1910693002/diff/1/chrome/browser/search/searc...
chrome/browser/search/search.cc:16: #include
"chrome/browser/chromeos/login/signin/merge_session_throttling_utils.h"
This should go into an #if defined(OS_CHROMEOS) block.

https://codereview.chromium.org/1910693002/diff/1/chrome/browser/search/searc...
chrome/browser/search/search.cc:250: if
(!merge_session_throttling_utils::ShouldDelayUrl(url) ||
I'd move the "#if defined(OS_CHROMEOS)" here so that it covers this "if" only,
and rename the function to ShouldShowLocalNewTab. Then you won't need another
OS-specific block below.

[afakhry, 2016-04-21 17:04:09]

https://codereview.chromium.org/1910693002/diff/1/chrome/browser/search/searc...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/1910693002/diff/1/chrome/browser/search/searc...
chrome/browser/search/search.cc:16: #include
"chrome/browser/chromeos/login/signin/merge_session_throttling_utils.h"
On 2016/04/21 08:32:12, Marc Treib wrote:
> This should go into an #if defined(OS_CHROMEOS) block.

Done.

https://codereview.chromium.org/1910693002/diff/1/chrome/browser/search/searc...
chrome/browser/search/search.cc:250: if
(!merge_session_throttling_utils::ShouldDelayUrl(url) ||
On 2016/04/21 08:32:12, Marc Treib wrote:
> I'd move the "#if defined(OS_CHROMEOS)" here so that it covers this "if" only,
> and rename the function to ShouldShowLocalNewTab. Then you won't need another
> OS-specific block below.

Done.

[Marc Treib, 2016-04-21 19:53:38]

Thanks! LGTM with one more nit (but please wait for kmadhusu@ to take a look
too)

https://codereview.chromium.org/1910693002/diff/20001/chrome/browser/search/s...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/1910693002/diff/20001/chrome/browser/search/s...
chrome/browser/search/search.cc:252:
!merge_session_throttling_utils::ShouldDelayRequest(profile)) {
Can you please swap the if around, to
if (ShouldDelayUrl(...) && ShouldDelayRequest(...)) return true;

Less negation makes for easier reading, and that way the "default case" for CrOS
and other platforms matches up.

Now that I look at it like this: Is it correct that you only want to show the
local NTP if *both* ShouldDelay functions return true? If so, would it make
sense to combine them into one ShouldDelay(profile, url) function?

[afakhry, 2016-04-22 00:41:14]

afakhry@chromium.org changed reviewers:
+ piman@chromium.org

[afakhry, 2016-04-22 00:41:15]

+piman@chromium.org: Could you please review:
  merge_session_throttling_utils.cc
  merge_session_throttling_utils.h
? Thanks!

https://codereview.chromium.org/1910693002/diff/20001/chrome/browser/search/s...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/1910693002/diff/20001/chrome/browser/search/s...
chrome/browser/search/search.cc:252:
!merge_session_throttling_utils::ShouldDelayRequest(profile)) {
On 2016/04/21 19:53:38, Marc Treib wrote:
> Can you please swap the if around, to
> if (ShouldDelayUrl(...) && ShouldDelayRequest(...)) return true;
> 
> Less negation makes for easier reading, and that way the "default case" for
CrOS
> and other platforms matches up.
> 
> Now that I look at it like this: Is it correct that you only want to show the
> local NTP if *both* ShouldDelay functions return true? If so, would it make
> sense to combine them into one ShouldDelay(profile, url) function?

Done.

Regarding combining them into one function, I see both are used separately in a
couple of places. So I don't think we should combine them.

[piman, 2016-04-22 00:53:20]

LGTM+nit

https://codereview.chromium.org/1910693002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/signin/merge_session_throttling_utils.h
(right):

https://codereview.chromium.org/1910693002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/merge_session_throttling_utils.h:38: bool
ShouldDelayRequest(Profile* profile);
nit: avoid overloading. Maybe use ShouldDelayRequestForProfile vs
ShouldDelayRequestForContents?

[afakhry, 2016-04-22 02:27:46]

https://codereview.chromium.org/1910693002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/signin/merge_session_throttling_utils.h
(right):

https://codereview.chromium.org/1910693002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/merge_session_throttling_utils.h:38: bool
ShouldDelayRequest(Profile* profile);
On 2016/04/22 00:53:20, piman wrote:
> nit: avoid overloading. Maybe use ShouldDelayRequestForProfile vs
> ShouldDelayRequestForContents?

Done.

[piman, 2016-04-22 04:06:41]

lgtm

[xiyuan, 2016-04-22 19:56:18]

lgtm

[afakhry, 2016-04-25 16:46:27]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2016-04-25 16:46:28]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/1910693002/#ps60001
(title: "piman's nit")

[commit-bot: I haz the power, 2016-04-25 16:47:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1910693002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1910693002/60001

[commit-bot: I haz the power, 2016-04-25 18:24:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-25 18:26:19]

Description was changed from

==========
Avoid the long merge session throttle interstitial page for NTP on Chrome OS

While the session is being merged, opening the NTP while cause the throttling
interstitial page to kick in, which can cause a delay in displaying the page for
5+ seconds. Instead we should force loading the local NTP until the merge
session
is complete.

BUG=591530
==========

to

==========
Avoid the long merge session throttle interstitial page for NTP on Chrome OS

While the session is being merged, opening the NTP while cause the throttling
interstitial page to kick in, which can cause a delay in displaying the page for
5+ seconds. Instead we should force loading the local NTP until the merge
session
is complete.

BUG=591530

Committed: https://crrev.com/96379036a48ce6ee82040597d5e9b930c21a7a3d
Cr-Commit-Position: refs/heads/master@{#389514}
==========

[commit-bot: I haz the power, 2016-04-25 18:26:20]

Patchset 4 (id:??) landed as
https://crrev.com/96379036a48ce6ee82040597d5e9b930c21a7a3d
Cr-Commit-Position: refs/heads/master@{#389514}