Make app list start page doodle clickable and have alt text.

Make app list start page doodle clickable and have alt text.

This CL makes the app list start page doodle clickable by implementing
the necessary methods on the StartPageWebContentsDelegate and adding
the appropriate JS in the start page WebUI.

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

[calamity, 2015-02-02 01:45:28]

Patchset #1 (id:1) has been deleted

[calamity, 2015-02-02 02:21:02]

Patchset #1 (id:20001) has been deleted

[calamity, 2015-02-02 02:21:17]

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

[Matt Giuca, 2015-02-03 07:08:50]

nice

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

https://codereview.chromium.org/887853003/diff/40001/chrome/browser/resources...
chrome/browser/resources/app_list/start_page.js:58: function
setDoodleVisible(visible) {
So if $('doodle') doesn't exist and visible == true, we go back to default logo?

Seems like this should never happen (following the logic where you only set an
image source after you create a doodle). So I think it's appropriate to just
crash or throw an error (whatever is appropriate JS equivalent of a DCHECK) in
that case.

Then you can structure this a bit more nicely:
if (visible) {
  // dcheck equivalent (doodle)
  // assume doodle && visible
} else {
  // only happens if user is explicitly hiding
}

https://codereview.chromium.org/887853003/diff/40001/chrome/browser/resources...
chrome/browser/resources/app_list/start_page.js:111: var doodleLink =
document.createElement('a');
Curious: In my experience, when you have <a><img /></a>, the image has a blue
border around it by default, unless you mute it in the CSS. Are you doing that?
Do you know why it isn't an issue here?

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

https://codereview.chromium.org/887853003/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:144: NULL,
nullptr

https://codereview.chromium.org/887853003/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:158:
static_cast<Browser*>(NULL), params.url, params.transition);
nullptr

https://codereview.chromium.org/887853003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/app_list/start_page_browsertest.js (right):

https://codereview.chromium.org/887853003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/app_list/start_page_browsertest.js:141:
'http://example.com/');
Can you make this http://example.com/x/.

Then you test relative vs absolute paths. (src should be
http://example.com/x/doodle.png, whereas href should be
http://example.com/target.html.)

[calamity, 2015-02-04 03:16:22]

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

https://codereview.chromium.org/887853003/diff/40001/chrome/browser/resources...
chrome/browser/resources/app_list/start_page.js:58: function
setDoodleVisible(visible) {
On 2015/02/03 07:08:50, Matt Giuca wrote:
> So if $('doodle') doesn't exist and visible == true, we go back to default
logo?
> 
> Seems like this should never happen (following the logic where you only set an
> image source after you create a doodle). So I think it's appropriate to just
> crash or throw an error (whatever is appropriate JS equivalent of a DCHECK) in
> that case.
> 
> Then you can structure this a bit more nicely:
> if (visible) {
>   // dcheck equivalent (doodle)
>   // assume doodle && visible
> } else {
>   // only happens if user is explicitly hiding
> }

I don't think there's an assert in JS so.....

Done?

https://codereview.chromium.org/887853003/diff/40001/chrome/browser/resources...
chrome/browser/resources/app_list/start_page.js:111: var doodleLink =
document.createElement('a');
On 2015/02/03 07:08:50, Matt Giuca wrote:
> Curious: In my experience, when you have <a><img /></a>, the image has a blue
> border around it by default, unless you mute it in the CSS. Are you doing
that?
> Do you know why it isn't an issue here?

I am not seeing this happen. I have no idea why. I think it's just like that?

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

https://codereview.chromium.org/887853003/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:144: NULL,
On 2015/02/03 07:08:50, Matt Giuca wrote:
> nullptr

Done.

https://codereview.chromium.org/887853003/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:158:
static_cast<Browser*>(NULL), params.url, params.transition);
On 2015/02/03 07:08:50, Matt Giuca wrote:
> nullptr

Done.

https://codereview.chromium.org/887853003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/app_list/start_page_browsertest.js (right):

https://codereview.chromium.org/887853003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/app_list/start_page_browsertest.js:141:
'http://example.com/');
On 2015/02/03 07:08:50, Matt Giuca wrote:
> Can you make this http://example.com/x/.
> 
> Then you test relative vs absolute paths. (src should be
> http://example.com/x/doodle.png, whereas href should be
> http://example.com/target.html.)

Done.

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

https://codereview.chromium.org/887853003/diff/60001/chrome/browser/resources...
chrome/browser/resources/app_list/start_page.js:148:
document.addEventListener('contextmenu', function(e) { e.preventDefault(); });
Oops, this got lost.

[Matt Giuca, 2015-02-04 04:36:10]

lgtm

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

https://codereview.chromium.org/887853003/diff/40001/chrome/browser/resources...
chrome/browser/resources/app_list/start_page.js:58: function
setDoodleVisible(visible) {
On 2015/02/04 03:16:22, calamity wrote:
> On 2015/02/03 07:08:50, Matt Giuca wrote:
> > So if $('doodle') doesn't exist and visible == true, we go back to default
> logo?
> > 
> > Seems like this should never happen (following the logic where you only set
an
> > image source after you create a doodle). So I think it's appropriate to just
> > crash or throw an error (whatever is appropriate JS equivalent of a DCHECK)
in
> > that case.
> > 
> > Then you can structure this a bit more nicely:
> > if (visible) {
> >   // dcheck equivalent (doodle)
> >   // assume doodle && visible
> > } else {
> >   // only happens if user is explicitly hiding
> > }
> 
> I don't think there's an assert in JS so.....
> 
> Done?

Acknowledged.

https://codereview.chromium.org/887853003/diff/40001/chrome/browser/resources...
chrome/browser/resources/app_list/start_page.js:111: var doodleLink =
document.createElement('a');
Yeah, I'm not seeing it even on a totally fresh page. Maybe it's something old
browsers used to do.

[calamity, 2015-02-04 04:36:44]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2015-02-04 04:37:04]

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

[commit-bot: I haz the power, 2015-02-04 04:40:27]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-02-04 04:41:21]

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