Add a static Google Doodle to the experimental app list start page.

Add a static Google Doodle to the experimental app list start page.

This CL adds a static Google Doodle to the experimental app list start
page by downloading a JSON blob and using the contained doodle URL as an
image source in the app list start page WebContents.

BUG=453205
Committed: https://crrev.com/c3fe75df567e02f12c89db4d41b1617dd44b871c
Cr-Commit-Position: refs/heads/master@{#314442}

[calamity, 2015-01-29 05:51:02]

Patchset #1 (id:1) has been deleted

[calamity, 2015-01-29 05:54:04]

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

[Matt Giuca, 2015-01-29 08:00:53]

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/resources...
File chrome/browser/resources/app_list/start_page.html (right):

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/resources...
chrome/browser/resources/app_list/start_page.html:18: <div id="logo"></div>
Can you rename this to default_logo (to match the JS variable name, and to be
more descriptive).

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/resources...
File chrome/browser/resources/app_list/start_page.js (right):

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/resources...
chrome/browser/resources/app_list/start_page.js:53: * @param {Object} data The
data object representing the current doodle.
Any reason you pass the whole JSON in? Why not grab transparent_large_image in
the C++ code and pass it in as a string.

Then also you won't need to do base_url + transparent_large_image (you can do
that in the C++ as well).

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

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:63: const int
kDefaultDoodleRecheckDelayMs = 30 * 60 * 1000;  // 30 minutes.
Not a fan of these hard-coded constants (even if they are reasonably fixed in
the scientific community). Yeah maybe make tihs RecheckDelayMins and then use
FromMinutes later.

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:524: // |replacements| is, so a
std::string is needed, instead of a char*.
But kDoodleJsonPath has global lifetime and thus surely outlives |replacements|.
Why was this necessary?

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:544: json_data.erase(0,
json_start_index);
I think the preferred strategy these days is to use a StringPiece instead of
creating a substring.

(This won't actually remove the copy because JSONStringValueSerializer takes a
string so it will just be delaying the copy. But this is the better style.)

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:550:
deserializer.Deserialize(&error_code, NULL));
nullptrs (everywhere)

https://codereview.chromium.org/871193006/diff/20001/ui/app_list/views/start_...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/871193006/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:38: const int kWebViewWidth = 700;
How does this affect the normal doodle?

Is it just that the HTML box is much bigger (always), but the CSS ensures it's
centered?

[calamity, 2015-01-30 05:09:14]

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/resources...
File chrome/browser/resources/app_list/start_page.html (right):

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/resources...
chrome/browser/resources/app_list/start_page.html:18: <div id="logo"></div>
On 2015/01/29 08:00:53, Matt Giuca wrote:
> Can you rename this to default_logo (to match the JS variable name, and to be
> more descriptive).

Done.

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/resources...
File chrome/browser/resources/app_list/start_page.js (right):

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/resources...
chrome/browser/resources/app_list/start_page.js:53: * @param {Object} data The
data object representing the current doodle.
On 2015/01/29 08:00:53, Matt Giuca wrote:
> Any reason you pass the whole JSON in? Why not grab transparent_large_image in
> the C++ code and pass it in as a string.
> 
> Then also you won't need to do base_url + transparent_large_image (you can do
> that in the C++ as well).

There are other fields we're interested in such as alt text and target url. I'll
add them in a follow up.

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

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:63: const int
kDefaultDoodleRecheckDelayMs = 30 * 60 * 1000;  // 30 minutes.
On 2015/01/29 08:00:53, Matt Giuca wrote:
> Not a fan of these hard-coded constants (even if they are reasonably fixed in
> the scientific community). Yeah maybe make tihs RecheckDelayMins and then use
> FromMinutes later.

Done.

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:524: // |replacements| is, so a
std::string is needed, instead of a char*.
On 2015/01/29 08:00:53, Matt Giuca wrote:
> But kDoodleJsonPath has global lifetime and thus surely outlives
|replacements|.
> Why was this necessary?

const char[] != std::string. The autoconverted string doesn't live past the
method call.

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:544: json_data.erase(0,
json_start_index);
On 2015/01/29 08:00:53, Matt Giuca wrote:
> I think the preferred strategy these days is to use a StringPiece instead of
> creating a substring.
> 
> (This won't actually remove the copy because JSONStringValueSerializer takes a
> string so it will just be delaying the copy. But this is the better style.)

As discussed, this is more complicated that it's worth.

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:550:
deserializer.Deserialize(&error_code, NULL));
On 2015/01/29 08:00:53, Matt Giuca wrote:
> nullptrs (everywhere)

Done.

https://codereview.chromium.org/871193006/diff/20001/ui/app_list/views/start_...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/871193006/diff/20001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:38: const int kWebViewWidth = 700;
On 2015/01/29 08:00:53, Matt Giuca wrote:
> How does this affect the normal doodle?
> 
> Is it just that the HTML box is much bigger (always), but the CSS ensures
it's
> centered?

Correct. |CSS is awe|some.

[Matt Giuca, 2015-02-02 06:23:14]

lgtm

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/resources...
File chrome/browser/resources/app_list/start_page.js (right):

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/resources...
chrome/browser/resources/app_list/start_page.js:53: * @param {Object} data The
data object representing the current doodle.
On 2015/01/30 05:09:14, calamity wrote:
> On 2015/01/29 08:00:53, Matt Giuca wrote:
> > Any reason you pass the whole JSON in? Why not grab transparent_large_image
in
> > the C++ code and pass it in as a string.
> > 
> > Then also you won't need to do base_url + transparent_large_image (you can
do
> > that in the C++ as well).
> 
> There are other fields we're interested in such as alt text and target url.
I'll
> add them in a follow up.

Acknowledged.

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

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:524: // |replacements| is, so a
std::string is needed, instead of a char*.
On 2015/01/30 05:09:14, calamity wrote:
> On 2015/01/29 08:00:53, Matt Giuca wrote:
> > But kDoodleJsonPath has global lifetime and thus surely outlives
> |replacements|.
> > Why was this necessary?
> 
> const char[] != std::string. The autoconverted string doesn't live past the
> method call.

Acknowledged.

(Lol... it's funny that GURL::Replacements::SetPath, as opposed to SetPathStr,
actually takes a char*, which would work fine here, except it also needs some
other low-level gumbo so it's not appropriate here. Yet another case where if it
just took a StringPiece everything would be fine.)

...

2 hours later ...

And fixed GURL::Replacements to take a StringPiece:
https://codereview.chromium.org/889463003. Don't wait for me to commit this.
Land this CL first and I will update it along with the other changes in that CL.

https://codereview.chromium.org/871193006/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:544: json_data.erase(0,
json_start_index);
On 2015/01/30 05:09:14, calamity wrote:
> On 2015/01/29 08:00:53, Matt Giuca wrote:
> > I think the preferred strategy these days is to use a StringPiece instead of
> > creating a substring.
> > 
> > (This won't actually remove the copy because JSONStringValueSerializer takes
a
> > string so it will just be delaying the copy. But this is the better style.)
> 
> As discussed, this is more complicated that it's worth.

Acknowledged.

[calamity, 2015-02-03 01:48:04]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2015-02-03 01:48:40]

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

[commit-bot: I haz the power, 2015-02-03 01:55:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-03 01:55:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[calamity, 2015-02-03 02:46:37]

+xiyuan for c/b/ui/webui/app_list OWNERS

[calamity, 2015-02-03 07:36:30]

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

[calamity, 2015-02-03 07:37:06]

+xiyuan for c/b/ui/webui/app_list OWNERS.

[xiyuan, 2015-02-03 15:58:16]

webui/app_list LGTM

[Matt Giuca, 2015-02-03 23:47:40]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2015-02-03 23:48:47]

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

[commit-bot: I haz the power, 2015-02-03 23:52:48]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-02-03 23:54:50]

Patchset 2 (id:??) landed as
https://crrev.com/c3fe75df567e02f12c89db4d41b1617dd44b871c
Cr-Commit-Position: refs/heads/master@{#314442}