Don't show the apps page on the NTP if the app launcher is installed.

Reland: Don't show the apps page on the NTP if the app launcher is installed.

R=benwells@chromium.org,estade@chromium.org
BUG=170526
Originally committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178769

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

[Evan Stade, 2013-01-22 18:58:14]

It's not ok to delay the startup of the NTP. You should store whether the app
launcher is installed in a pref, and assume that pref is still correct when
launching the NTP the first time (you could also run the synchronous checks).
You can also kick off a task to see if the install state changed, and update the
NTP accordingly when the callback runs.

[koz (OOO until 15th September), 2013-01-23 06:30:15]

Code looks good, but leaving for estade to review at a higher level.

[Evan Stade, 2013-01-23 23:34:23]

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/new_tab.js (right):

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/new_tab.js:658: chrome.send('getShouldShowApps');
this should probably be a part of onLoad

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:807: true,
shouldn't this default to NewTabUI::ShouldShowApps()?

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:34: class
AppsPagePresenceHandler : public content::WebUIMessageHandler {
why is this separate from AppLauncherHandler?

[jeremya, 2013-01-24 02:40:04]

Moved the pref into app_launcher.

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/new_tab.js (right):

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/new_tab.js:658: chrome.send('getShouldShowApps');
On 2013/01/23 23:34:24, Evan Stade wrote:
> this should probably be a part of onLoad

Why's that?

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:807: true,
On 2013/01/23 23:34:24, Evan Stade wrote:
> shouldn't this default to NewTabUI::ShouldShowApps()?

It should probably default to the result of just the synchronous checks in
extensions::GetIsAppLauncherEnabled, and NewTabUI::ShouldShowApps() should be
removed.

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:34: class
AppsPagePresenceHandler : public content::WebUIMessageHandler {
On 2013/01/23 23:34:24, Evan Stade wrote:
> why is this separate from AppLauncherHandler?

Because AppLauncherHandler isn't always present. Previously this was required
because if the NTP didn't receive the gotShouldShowApps message it wouldn't
load, but since it loads opportunistically now, this can probably be rolled into
AppLauncherHandler.

[Evan Stade, 2013-01-24 18:40:16]

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/new_tab.js (right):

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/new_tab.js:658: chrome.send('getShouldShowApps');
On 2013/01/24 02:40:04, jeremya wrote:
> On 2013/01/23 23:34:24, Evan Stade wrote:
> > this should probably be a part of onLoad
> 
> Why's that?

well, my thinking was that if gotShouldShowApps were called before onLoad, then
you'd have a hard time updating the DOM (your TODO). But I suppose in that case
you could just build the DOM correctly in the first place instead of needing to
update it.

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/extensions...
File chrome/browser/extensions/app_launcher.cc (right):

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/app_launcher.cc:51: #if defined(OS_CHROMEOS)
should this be USE_ASH?

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/app_launcher.cc:63: }
nit: no curlies

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/app_launcher.cc:94: namespace app_launcher {
\n

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/app_launcher.cc:100:
pref_service->RegisterBooleanPref(prefs::kAppLauncherIsEnabled, is_enabled);
would be nice to explain a bit more why this is necessary (i.e. this is just a
cache of the value from the last run of chrome)

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/app_launcher.cc:101: }
\n

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:249:
web_ui()->CallJavascriptFunction("ntp.appNotificationChanged",
now that you're always including the app launcher handler, won't calls like this
wind up causing errors on the NTP?

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:400:
base::Unretained(this)));
this doesn't seem right. You need to pass a weak pointer.

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:295: #if defined(USE_ASH) ||
defined(OS_ANDROID)
seems the ASH part of this should be handled in app_launcher.cc

[jeremya, 2013-01-25 00:06:19]

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/new_tab.js (right):

https://codereview.chromium.org/11953021/diff/6001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/new_tab.js:658: chrome.send('getShouldShowApps');
On 2013/01/24 18:40:16, Evan Stade wrote:
> On 2013/01/24 02:40:04, jeremya wrote:
> > On 2013/01/23 23:34:24, Evan Stade wrote:
> > > this should probably be a part of onLoad
> > 
> > Why's that?
> 
> well, my thinking was that if gotShouldShowApps were called before onLoad,
then
> you'd have a hard time updating the DOM (your TODO). But I suppose in that
case
> you could just build the DOM correctly in the first place instead of needing
to
> update it.

Right, that makes sense -- if we get the gotShouldShowApps message before
onLoad, we could just update the load time data and carry on as normal, without
having to reload. I'll save that for a follow-up patch though.

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/extensions...
File chrome/browser/extensions/app_launcher.cc (right):

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/app_launcher.cc:51: #if defined(OS_CHROMEOS)
On 2013/01/24 18:40:17, Evan Stade wrote:
> should this be USE_ASH?

Yeah, I think that makes sense. Changed.

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/app_launcher.cc:63: }
On 2013/01/24 18:40:17, Evan Stade wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/app_launcher.cc:100:
pref_service->RegisterBooleanPref(prefs::kAppLauncherIsEnabled, is_enabled);
On 2013/01/24 18:40:17, Evan Stade wrote:
> would be nice to explain a bit more why this is necessary (i.e. this is just a
> cache of the value from the last run of chrome)

Done.

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:249:
web_ui()->CallJavascriptFunction("ntp.appNotificationChanged",
On 2013/01/24 18:40:17, Evan Stade wrote:
> now that you're always including the app launcher handler, won't calls like
this
> wind up causing errors on the NTP?

Good catch. Moved to new_tab_page_handler.

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:400:
base::Unretained(this)));
On 2013/01/24 18:40:17, Evan Stade wrote:
> this doesn't seem right. You need to pass a weak pointer.

Done.

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):

https://codereview.chromium.org/11953021/diff/10002/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:295: #if defined(USE_ASH) ||
defined(OS_ANDROID)
On 2013/01/24 18:40:17, Evan Stade wrote:
> seems the ASH part of this should be handled in app_launcher.cc

Done.

[Evan Stade, 2013-01-25 00:11:02]

lgtm

https://codereview.chromium.org/11953021/diff/10003/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

https://codereview.chromium.org/11953021/diff/10003/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:184: 
^H

https://codereview.chromium.org/11953021/diff/10003/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (left):

https://codereview.chromium.org/11953021/diff/10003/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:121: if (ShouldShowApps()) {
revert this

[Bernhard Bauer, 2013-01-25 00:18:13]

lgtm

[jeremya, 2013-01-25 00:20:42]

https://codereview.chromium.org/11953021/diff/10003/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (left):

https://codereview.chromium.org/11953021/diff/10003/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:121: if (ShouldShowApps()) {
On 2013/01/25 00:11:02, Evan Stade wrote:
> revert this

Done.

[commit-bot: I haz the power, 2013-01-25 00:22:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11953021/14003

[commit-bot: I haz the power, 2013-01-25 00:40:24]

Failed to trigger a try job on linux_aura
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-01-25 00:40:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11953021/13003

[commit-bot: I haz the power, 2013-01-25 01:30:32]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-01-25 02:20:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11953021/2034

[commit-bot: I haz the power, 2013-01-25 02:47:30]

Failed to trigger a try job on mac_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-01-25 02:47:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11953021/6007

[commit-bot: I haz the power, 2013-01-25 03:35:59]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-01-25 04:45:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11953021/14022

[commit-bot: I haz the power, 2013-01-25 06:54:14]

Change committed as 178769

[jeremya, 2013-01-27 07:39:56]

I think this failed intermittently because sometimes gotShouldShowApps would be
fired before the loadTimeData structure was filled in. I've changed the code to
only fire getShouldShowApps once onLoad is called, fingers crossed that's the
issue (I couldn't get the failure to reproduce locally.)