Implementation of chrome://cast page.

Implementation of chrome://cast WebUI.
Spec: go/cast-setup-chrome-webui

BUG=611758
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/c16319aaa8a36102549c89e7ab188528dcba813a
Cr-Commit-Position: refs/heads/master@{#396501}

[sheretov, 2016-05-14 02:13:24]

Description was changed from

==========
Implementation of chrome://cast page.
==========

to

==========
Implementation of chrome://cast page.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[sheretov, 2016-05-14 02:37:35]

Description was changed from

==========
Implementation of chrome://cast page.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Implementation of chrome://cast page.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

BUG=611758
==========

[sheretov, 2016-05-14 05:55:03]

Description was changed from

==========
Implementation of chrome://cast page.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

BUG=611758
==========

to

==========
Please take a look at the chrome://cast WebUI implementation.
Spec: go/cast-setup-chrome-webui

BUG=611758
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[sheretov, 2016-05-14 05:55:03]

sheretov@chromium.org changed reviewers:
+ apacible@chromium.org, mfoltz@chromium.org

[sheretov, 2016-05-14 05:55:03]

sheretov@chromium.org changed required reviewers:
+ apacible@chromium.org, mfoltz@chromium.org

[apacible, 2016-05-16 19:04:07]

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/cast/cast.html (right):

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cast/cast.html:5: <title>Google Cast</title>
This should be translated.

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/cast/cast.js (right):

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cast/cast.js:36:
ev.load('chrome-extension://enhhojjnijigcajfphajepfemndkmdlo' +
For different channels, the extension ID will be different. We keep track of it
here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cast/cast.js:36:
ev.load('chrome-extension://enhhojjnijigcajfphajepfemndkmdlo' +
If this fails to load, do we expect the default "Page cannot be displayed"
error?

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/cast/cast_ui.cc (right):

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cast/cast_ui.cc:15: // Set up the chrome://hello-world
source.
chrome://cast

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/cast/cast_ui.h (right):

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cast/cast_ui.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
nit: 2016

https://codereview.chromium.org/1820023002/diff/100001/chrome/common/url_cons...
File chrome/common/url_constants.h (right):

https://codereview.chromium.org/1820023002/diff/100001/chrome/common/url_cons...
chrome/common/url_constants.h:143: 
nit: Remove extra newlines.

[sheretov, 2016-05-17 01:59:27]

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/cast/cast.html (right):

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cast/cast.html:5: <title>Google Cast</title>
On 2016/05/16 19:04:07, apacible wrote:
> This should be translated.

"Google Cast" by itself is never translated.  If we decide to change the title
to something less generic, I'll add an IDS_CAST_TITLE.

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/cast/cast.js (right):

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cast/cast.js:36:
ev.load('chrome-extension://enhhojjnijigcajfphajepfemndkmdlo' +
On 2016/05/16 19:04:07, apacible wrote:
> If this fails to load, do we expect the default "Page cannot be displayed"
> error?

We expect to get the default behavior when extensionView fails to load a URL,
which right now results in a promise rejection error message in the JS console,
while the page remains blank.  This shouldn't happen unless something is
seriously broken, however.

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cast/cast.js:36:
ev.load('chrome-extension://enhhojjnijigcajfphajepfemndkmdlo' +
On 2016/05/16 19:04:07, apacible wrote:
> For different channels, the extension ID will be different. We keep track of
it
> here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...

Done, but I'm not sure if this is the right mechanism in light of
crbug.com/597778.

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/cast/cast_ui.cc (right):

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cast/cast_ui.cc:15: // Set up the chrome://hello-world
source.
On 2016/05/16 19:04:07, apacible wrote:
> chrome://cast

Done :)

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/cast/cast_ui.h (right):

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cast/cast_ui.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/05/16 19:04:07, apacible wrote:
> nit: 2016

Done.

https://codereview.chromium.org/1820023002/diff/100001/chrome/common/url_cons...
File chrome/common/url_constants.h (right):

https://codereview.chromium.org/1820023002/diff/100001/chrome/common/url_cons...
chrome/common/url_constants.h:143: 
On 2016/05/16 19:04:07, apacible wrote:
> nit: Remove extra newlines.

Done.

[apacible, 2016-05-18 17:30:42]

lgtm if mfoltz is fine wrt method of obtaining the extension id.

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/cast/cast.js (right):

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cast/cast.js:36:
ev.load('chrome-extension://enhhojjnijigcajfphajepfemndkmdlo' +
On 2016/05/17 01:59:26, sheretov wrote:
> On 2016/05/16 19:04:07, apacible wrote:
> > If this fails to load, do we expect the default "Page cannot be displayed"
> > error?
> 
> We expect to get the default behavior when extensionView fails to load a URL,
> which right now results in a promise rejection error message in the JS
console,
> while the page remains blank.  This shouldn't happen unless something is
> seriously broken, however.

If the extensionview fails to load the URL, could we remove/hide the
extensionview element and show an error message? It likely won't happen unless
the 1) component extension wasn't installed 2) there are other issues, so I'm
fine with this being added later.

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cast/cast.js:36:
ev.load('chrome-extension://enhhojjnijigcajfphajepfemndkmdlo' +
On 2016/05/17 01:59:26, sheretov wrote:
> On 2016/05/16 19:04:07, apacible wrote:
> > For different channels, the extension ID will be different. We keep track of
> it
> > here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...
> 
> Done, but I'm not sure if this is the right mechanism in light of
> crbug.com/597778.

mfoltz: thoughts?

[sheretov, 2016-05-18 18:11:01]

Description was changed from

==========
Please take a look at the chrome://cast WebUI implementation.
Spec: go/cast-setup-chrome-webui

BUG=611758
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Implementation of chrome://cast WebUI.
Spec: go/cast-setup-chrome-webui

BUG=611758
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[sheretov, 2016-05-18 18:11:11]

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/cast/cast.js (right):

https://codereview.chromium.org/1820023002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cast/cast.js:36:
ev.load('chrome-extension://enhhojjnijigcajfphajepfemndkmdlo' +
On 2016/05/18 17:30:42, apacible wrote:
> On 2016/05/17 01:59:26, sheretov wrote:
> > On 2016/05/16 19:04:07, apacible wrote:
> > > If this fails to load, do we expect the default "Page cannot be displayed"
> > > error?
> > 
> > We expect to get the default behavior when extensionView fails to load a
URL,
> > which right now results in a promise rejection error message in the JS
> console,
> > while the page remains blank.  This shouldn't happen unless something is
> > seriously broken, however.
> 
> If the extensionview fails to load the URL, could we remove/hide the
> extensionview element and show an error message? It likely won't happen unless
> the 1) component extension wasn't installed 2) there are other issues, so I'm
> fine with this being added later.

Sounds good.  I'll put this in a subsequent CL.

[sheretov, 2016-05-18 18:46:44]

sheretov@chromium.org changed reviewers:
+ bauerb@chromium.org, finnur@chromium.org

[sheretov, 2016-05-18 18:47:42]

+ bauerb for OWNERS review of chrome/browser/...
+ finnur for OWNERS review of extensions/common/api/_api_features.json

[mark a. foltz, 2016-05-18 21:21:05]

One comment.

Here's a thought:  Is it required that Cast setup be in its own tab?  We already
have an <extensionview> in chrome://media-router that is populated with a URL
from Cast (for custom media controls).  Did you consider using that to host the
setup UX instead?

https://codereview.chromium.org/1820023002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/cast/cast_ui.cc (right):

https://codereview.chromium.org/1820023002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cast/cast_ui.cc:31: component_extension_id_ =
router->media_route_provider_extension_id();
Digging through the MR to find out the ID of the component is something we are
trying to avoid.

I assume this is so you can construct the right URL for the <extensionview> when
you have a debug component extension installed.

Please add a TODO here like the following so we go back to fix this when we have
a better solution:

// TODO(crbug.com/597778): remove reference to MediaRouterMojoImpl

[sheretov, 2016-05-18 21:58:32]

> One comment.
> 
> Here's a thought:  Is it required that Cast setup be in its own tab?  We
already
> have an <extensionview> in chrome://media-router that is populated with a URL
> from Cast (for custom media controls).  Did you consider using that to host
the
> setup UX instead?

Technically, merging the two pages is possible, although it would require some
careful refactoring and new logic on both sides.  There are also going to be
product implications that we should discuss offline.  In the meantime, I'd like
to get this in, so that we can solve the immediate problem of users running
setup on a strange-looking chrome-extension:// URL as outlined in the spec.

[sheretov, 2016-05-18 21:58:47]

https://codereview.chromium.org/1820023002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/cast/cast_ui.cc (right):

https://codereview.chromium.org/1820023002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cast/cast_ui.cc:31: component_extension_id_ =
router->media_route_provider_extension_id();
On 2016/05/18 21:21:05, mark a. foltz wrote:
> Digging through the MR to find out the ID of the component is something we are
> trying to avoid.
> 
> I assume this is so you can construct the right URL for the <extensionview>
when
> you have a debug component extension installed.
> 
> Please add a TODO here like the following so we go back to fix this when we
have
> a better solution:
> 
> // TODO(crbug.com/597778): remove reference to MediaRouterMojoImpl

Done.

[mark a. foltz, 2016-05-18 22:07:38]

lgtm

[Bernhard Bauer, 2016-05-19 14:34:07]

https://codereview.chromium.org/1820023002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/cast/cast.js (right):

https://codereview.chromium.org/1820023002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/cast/cast.js:11: var ev =
document.querySelector('extensionview');
Don't use abbreviations in variable names.

https://codereview.chromium.org/1820023002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/cast/cast_ui.cc (right):

https://codereview.chromium.org/1820023002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cast/cast_ui.cc:25: auto wc = web_ui->GetWebContents();
Use the full type. Also, don't use abbreviations in variable names.

https://codereview.chromium.org/1820023002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cast/cast_ui.cc:26: DCHECK(wc);
If this is null, you'll crash below in any case. (Also, the fact that you are
dereferencing |wc| serves as documentation that you don't expect it to be null,
so that aspect doesn't apply either).

https://codereview.chromium.org/1820023002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cast/cast_ui.cc:31: DCHECK(router);
Same here.

https://codereview.chromium.org/1820023002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cast/cast_ui.cc:32: component_extension_id_ =
router->media_route_provider_extension_id();
Is there a reason you're not just adding this as a string to the data source?

[sheretov, 2016-05-19 16:16:46]

PTAL

https://codereview.chromium.org/1820023002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/cast/cast.js (right):

https://codereview.chromium.org/1820023002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/cast/cast.js:11: var ev =
document.querySelector('extensionview');
On 2016/05/19 14:34:07, Bernhard Bauer wrote:
> Don't use abbreviations in variable names.

Done.

https://codereview.chromium.org/1820023002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/cast/cast_ui.cc (right):

https://codereview.chromium.org/1820023002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cast/cast_ui.cc:25: auto wc = web_ui->GetWebContents();
On 2016/05/19 14:34:07, Bernhard Bauer wrote:
> Use the full type. Also, don't use abbreviations in variable names.

Removed this temporary variable altogether.  Still using auto typing for
|router|, since it's assigned a result of a static cast where the type is
explicitly specified.

https://codereview.chromium.org/1820023002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cast/cast_ui.cc:26: DCHECK(wc);
On 2016/05/19 14:34:07, Bernhard Bauer wrote:
> If this is null, you'll crash below in any case. (Also, the fact that you are
> dereferencing |wc| serves as documentation that you don't expect it to be
null,
> so that aspect doesn't apply either).

Done.

https://codereview.chromium.org/1820023002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cast/cast_ui.cc:31: DCHECK(router);
On 2016/05/19 14:34:07, Bernhard Bauer wrote:
> Same here.

Done.

https://codereview.chromium.org/1820023002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cast/cast_ui.cc:32: component_extension_id_ =
router->media_route_provider_extension_id();
On 2016/05/19 14:34:07, Bernhard Bauer wrote:
> Is there a reason you're not just adding this as a string to the data source?

Done.

[Bernhard Bauer, 2016-05-19 17:07:32]

lgtm

[Finnur, 2016-05-23 11:32:46]

finnur@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Finnur, 2016-05-23 11:32:47]

Sorry for the late reply -- this fell through the cracks.

I think Devlin is a better candidate for blessing this as my work on extensions
predates this API.

[sheretov, 2016-05-24 18:51:26]

On 2016/05/23 11:32:47, Finnur wrote:
> Sorry for the late reply -- this fell through the cracks.
> 
> I think Devlin is a better candidate for blessing this as my work on
extensions
> predates this API.

ping

[Devlin, 2016-05-24 19:40:17]

I'm not an OWNER in resources/ or webui/, sorry.

[Devlin, 2016-05-24 19:40:46]

On 2016/05/24 19:40:17, Devlin wrote:
> I'm not an OWNER in resources/ or webui/, sorry.

Oh, I'm just here for extensions, nvm

[Devlin, 2016-05-24 19:41:44]

extensions lgtm

[sheretov, 2016-05-25 02:35:50]

Description was changed from

==========
Implementation of chrome://cast WebUI.
Spec: go/cast-setup-chrome-webui

BUG=611758
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Implementation of chrome://cast WebUI.
Spec: go/cast-setup-chrome-webui

BUG=611758
==========

[sheretov, 2016-05-27 15:48:36]

Description was changed from

==========
Implementation of chrome://cast WebUI.
Spec: go/cast-setup-chrome-webui

BUG=611758
==========

to

==========
Implementation of chrome://cast WebUI.
Spec: go/cast-setup-chrome-webui

BUG=611758
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[sheretov, 2016-05-27 15:59:54]

Description was changed from

==========
Implementation of chrome://cast WebUI.
Spec: go/cast-setup-chrome-webui

BUG=611758
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Implementation of chrome://cast WebUI.
Spec: go/cast-setup-chrome-webui

BUG=611758
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[sheretov, 2016-05-27 17:21:31]

The CQ bit was checked by sheretov@chromium.org

[sheretov, 2016-05-27 17:21:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
apacible@chromium.org, mfoltz@chromium.org, bauerb@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1820023002/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-27 17:21:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1820023002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1820023002/200001

[commit-bot: I haz the power, 2016-05-27 17:30:02]

Description was changed from

==========
Implementation of chrome://cast WebUI.
Spec: go/cast-setup-chrome-webui

BUG=611758
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Implementation of chrome://cast WebUI.
Spec: go/cast-setup-chrome-webui

BUG=611758
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-05-27 17:30:04]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-05-27 17:31:46]

Description was changed from

==========
Implementation of chrome://cast WebUI.
Spec: go/cast-setup-chrome-webui

BUG=611758
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Implementation of chrome://cast WebUI.
Spec: go/cast-setup-chrome-webui

BUG=611758
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/c16319aaa8a36102549c89e7ab188528dcba813a
Cr-Commit-Position: refs/heads/master@{#396501}
==========

[commit-bot: I haz the power, 2016-05-27 17:31:48]

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