InstantExtended: don't preload the local NTP.

InstantExtended: don't preload the local NTP.

In local-only mode, don't bother preloading the Instant NTP. The speed savings
of preloading a local page are minimal and this will help reduce the memory
usage.

BUG=none

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

[samarth, 2013-04-17 16:37:44]

Anecdotally, this seems to also the fix the "infinite reload" on ctrl-C issue.

I'll add a browser test and post another patchset later today.

Thanks,
Samarth

[samarth, 2013-04-17 17:26:30]

Updated tests and of course discovered a bug in the process :)

PTAL.

Thanks,
Samarth

[samarth, 2013-04-17 22:00:24]

sreeram -> jered to load balance
(Sreeram: of course, feel free to kibitz if you have any comments here)

Thanks!
Samarth

[Jered, 2013-04-17 22:42:06]

Mostly looks good, just some nits.

Do we have some other test which tests that we really do get a local ntp on
demand (i.e. not preloaded)?

https://codereview.chromium.org/14232007/diff/3001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14232007/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:508: // Don't preload the local
NTP. Override the blacklist on an explicit user
How about:
// Preload a new Instant NTP for the next new tab, unless using the local NTP
which is not preloaded to conserve memory.

https://codereview.chromium.org/14232007/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:508: // Don't preload the local
NTP. Override the blacklist on an explicit user
Update the comment to explain why the blacklist is overridden here (I can see
that's done by reading the code, but not why).

https://codereview.chromium.org/14232007/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:912: // Preload the Instant NTP
unless we are using the local NTP.
Why not? Why do we want to reset it otherwise?

[samarth, 2013-04-17 23:28:15]

On 2013/04/17 22:42:06, Jered wrote:
> Do we have some other test which tests that we really do get a local ntp on
> demand (i.e. not preloaded)?

No tests for that yet I think. Jeremy's changing that logic, so I'm asking him
to write some tests :)

https://codereview.chromium.org/14232007/diff/3001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14232007/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:508: // Don't preload the local
NTP. Override the blacklist on an explicit user
On 2013/04/17 22:42:06, Jered wrote:
> How about:
> // Preload a new Instant NTP for the next new tab, unless using the local NTP
> which is not preloaded to conserve memory.

Done.

https://codereview.chromium.org/14232007/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:508: // Don't preload the local
NTP. Override the blacklist on an explicit user
On 2013/04/17 22:42:06, Jered wrote:
> Update the comment to explain why the blacklist is overridden here (I can see
> that's done by reading the code, but not why).

Done. How does this sound?

https://codereview.chromium.org/14232007/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:912: // Preload the Instant NTP
unless we are using the local NTP.
On 2013/04/17 22:42:06, Jered wrote:
> Why not? Why do we want to reset it otherwise?

Done.

[Jered, 2013-04-17 23:45:33]

lgtm

Thanks that's much clearer.

https://codereview.chromium.org/14232007/diff/4/chrome/browser/ui/search/inst...
File chrome/browser/ui/search/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/14232007/diff/4/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_extended_browsertest.cc:1487: ASSERT_EQ(NULL,
instant()->ntp());
EXPECT_EQ

[sreeram, 2013-04-18 00:00:34]

https://codereview.chromium.org/14232007/diff/9002/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14232007/diff/9002/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1498: ResetNTP(false, false);
You should add a similar sort of "if (!use_local_page_only_)" check here, I
think. I suspect that at shutdown we might get a RenderViewGone(), which might
lead to this place trying to reload the page.

[samarth, 2013-04-18 00:31:19]

https://codereview.chromium.org/14232007/diff/9002/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14232007/diff/9002/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1498: ResetNTP(false, false);
On 2013/04/18 00:00:34, sreeram wrote:
> You should add a similar sort of "if (!use_local_page_only_)" check here, I
> think. I suspect that at shutdown we might get a RenderViewGone(), which might
> lead to this place trying to reload the page.

The only situation I can think of where we get here with use_local_page_only_ is
if we load the local NTP in ReleaseNTPContents and it crashes before we release
it (is that even possible?).  And in that case, reloading sounds right to me.

During regular shutdown, ntp_ would be NULL since we never preload ntp_ if
|use_local_page_only_|

[sreeram, 2013-04-18 13:38:26]

LGTM

You're right about not needing to do the check in BlacklistAndResetNTP.

This CL is a step forward, hence we should submit this right away, but the
underlying problem (repeatedly loading the page at shutdown) still has other
manifestations:
1. When use_local_page_only_ is true, we'll still reload the local overlay over
and over.
2. When use_local_page_only_ is false, we'll reload both the server overlay and
the server NTP 10 times, then we reload the local overlay and local NTP over and
over.

We should fix those cases too.

[commit-bot: I haz the power, 2013-04-18 15:43:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/14232007/9002

[commit-bot: I haz the power, 2013-04-18 19:21:44]

Change committed as 194982