Hide experimental app list Google logo and custom launcher page when search engine is not Google.

Hide Google logo and custom launcher page in app list for non-Google search engines.

This CL hides the Google logo and the custom launcher page in the experimental
app list when the search is not set to Google. This also affects the app list
start page doodle.

BUG=453224
Committed: https://crrev.com/9382ab44c8438b90cb0afe4de1832189661cc3df
Cr-Commit-Position: refs/heads/master@{#316763}

[calamity, 2015-02-09 06:09:28]

calamity@chromium.org changed reviewers:
+ mgiuca@chromium.org

[Matt Giuca, 2015-02-10 08:27:05]

https://codereview.chromium.org/907833002/diff/1/chrome/browser/resources/app...
File chrome/browser/resources/app_list/start_page.js (right):

https://codereview.chromium.org/907833002/diff/1/chrome/browser/resources/app...
chrome/browser/resources/app_list/start_page.js:138:
$('logo_container').style.display = is_google ? 'block' : 'none';
Can't we just hide the whole WebContents in this case, rather than sending a
message down to the JS and having it behave differently?

[Matt Giuca, 2015-02-12 02:24:16]

Did you decide not to do this or are you still going to?

[calamity, 2015-02-12 05:53:51]

Patchset #2 (id:20001) has been deleted

[calamity, 2015-02-13 02:55:24]

https://codereview.chromium.org/907833002/diff/1/chrome/browser/resources/app...
File chrome/browser/resources/app_list/start_page.js (right):

https://codereview.chromium.org/907833002/diff/1/chrome/browser/resources/app...
chrome/browser/resources/app_list/start_page.js:138:
$('logo_container').style.display = is_google ? 'block' : 'none';
On 2015/02/10 08:27:05, Matt Giuca wrote:
> Can't we just hide the whole WebContents in this case, rather than sending a
> message down to the JS and having it behave differently?

That's a good point. Done.

[Matt Giuca, 2015-02-17 00:23:34]

https://codereview.chromium.org/907833002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/907833002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:807: if
(start_page_service)
Do you know when the SPS gets created? I'm worried about lifetime in the
following scenario:

1. ALVD gets created. ALVD::OTUSC gets called. StartPageService::Get() returns
nullptr. We do not call set_doodle_enabled(false).
2. SPS gets created, defaulting to having the doodle enabled.

Do we need to have SPS's constructor also check whether Google is the search
engine, and appropriately set its doodle enabled flag?

https://codereview.chromium.org/907833002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/start_page_service.h (right):

https://codereview.chromium.org/907833002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.h:170: bool doodle_enabled_;
Do you think it would be better to call this |search_engine_is_google_|? This
way, the client isn't telling SPS which of its own features are enabled, but
rather, simply telling it a fact about the configuration, which SPS can use to
make decisions.

https://codereview.chromium.org/907833002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/907833002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:321: model_->search_engine_is_google();
You're adding this behaviour here, right? (Not just refactoring it.) If so, you
should update the CL description to reflect that you are disabling the CLP as
well as the doodle.

[calamity, 2015-02-17 06:16:37]

https://codereview.chromium.org/907833002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/907833002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:807: if
(start_page_service)
On 2015/02/17 00:23:34, Matt Giuca wrote:
> Do you know when the SPS gets created? I'm worried about lifetime in the
> following scenario:
> 
> 1. ALVD gets created. ALVD::OTUSC gets called. StartPageService::Get() returns
> nullptr. We do not call set_doodle_enabled(false).
> 2. SPS gets created, defaulting to having the doodle enabled.
> 
> Do we need to have SPS's constructor also check whether Google is the search
> engine, and appropriately set its doodle enabled flag?

Hmm. I guess that's the most resilient thing to do. Note though, that even
though the doodle is 'enabled', the WebView is hidden so nothing will be shown.
It saves on the fetches though.

Done.

https://codereview.chromium.org/907833002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/start_page_service.h (right):

https://codereview.chromium.org/907833002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.h:170: bool doodle_enabled_;
On 2015/02/17 00:23:34, Matt Giuca wrote:
> Do you think it would be better to call this |search_engine_is_google_|? This
> way, the client isn't telling SPS which of its own features are enabled, but
> rather, simply telling it a fact about the configuration, which SPS can use to
> make decisions.

Done.

https://codereview.chromium.org/907833002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/907833002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:321: model_->search_engine_is_google();
On 2015/02/17 00:23:34, Matt Giuca wrote:
> You're adding this behaviour here, right? (Not just refactoring it.) If so,
you
> should update the CL description to reflect that you are disabling the CLP as
> well as the doodle.

Done.

[Matt Giuca, 2015-02-17 08:07:45]

lgtm

https://codereview.chromium.org/907833002/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/start_page_service.cc (right):

https://codereview.chromium.org/907833002/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:274:
search_engine_is_google_(true),
nit: Maybe init this to false? (Doesn't really matter since it is definitely
assigned later, so you may as well say false because that's the default bool
value.)

[calamity, 2015-02-18 01:02:17]

New patchsets have been uploaded after l-g-t-m from mgiuca@chromium.org

[calamity, 2015-02-18 01:42:34]

https://codereview.chromium.org/907833002/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/start_page_service.cc (right):

https://codereview.chromium.org/907833002/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:274:
search_engine_is_google_(true),
On 2015/02/17 08:07:45, Matt Giuca wrote:
> nit: Maybe init this to false? (Doesn't really matter since it is definitely
> assigned later, so you may as well say false because that's the default bool
> value.)

Done.

[calamity, 2015-02-18 01:42:39]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2015-02-18 01:44:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/907833002/100001

[commit-bot: I haz the power, 2015-02-18 03:43:12]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-02-18 03:43:42]

Patchset 5 (id:??) landed as
https://crrev.com/9382ab44c8438b90cb0afe4de1832189661cc3df
Cr-Commit-Position: refs/heads/master@{#316763}