Add a Physical Web WebUI

Add a Physical Web WebUI

Adds a WebUI page that receives a list of URLs broadcast by
nearby devices along with associated page metadata and displays
them as a list of clickable items.

BUG=630769
Committed: https://crrev.com/364ddcfc06dd2961111c9cbd390ed22323a54284
Cr-Commit-Position: refs/heads/master@{#416393}

[mattreynolds, 2016-08-16 22:31:26]

mattreynolds@chromium.org changed reviewers:
+ rohitrao@chromium.org

[mattreynolds, 2016-08-16 22:31:27]

Hi Rohit, could you take a look at the ios/chrome changes for the Physical Web
WebUI?

[mattreynolds, 2016-08-25 17:53:09]

friendly ping

[rohitrao (ping after 24h), 2016-08-30 13:58:33]

rohitrao@chromium.org changed reviewers:
+ jyquinn@chromium.org

[rohitrao (ping after 24h), 2016-08-30 13:58:34]

ios changes lgtm

I've added jyquinn as a reviewer who would be the most familiar with ios webui
code.

I can't approve changes in components/, so you will likely need another reviewer
for that.

[mattreynolds, 2016-08-30 21:45:38]

mattreynolds@chromium.org changed reviewers:
+ jochen@chromium.org

[mattreynolds, 2016-08-30 21:45:38]

Hi Jochen, please review changes in components/ and chrome/browser/ui, thanks!

[Jackie Quinn, 2016-08-30 22:37:13]

On 2016/08/30 21:45:38, mattreynolds wrote:
> Hi Jochen, please review changes in components/ and chrome/browser/ui, thanks!

ios lgtm, so long as it renders as you expect on iOS. Some API is not available
on iOS, and I'm not sure whether what you've used falls under that category. If
it works, great! If it doesn't feel free to ping me if you want help working out
why :-) 

Also, is this meant to be a user facing feature, or a debugging page? Based on
the bug, it seems like it's debug, but if it's user-facing, could you make sure
to coordinate with our PM mardini@ to check on whether this may be something we
want implemented natively rather than as WebUI on iOS? Thanks!

[Jackie Quinn, 2016-08-30 22:37:24]

https://codereview.chromium.org/2248913007/diff/1/components/physical_web/web...
File components/physical_web/webui/physical_web_ui_constants.h (right):

https://codereview.chromium.org/2248913007/diff/1/components/physical_web/web...
components/physical_web/webui/physical_web_ui_constants.h:28: extern const char
kReturnNearbyURLs[];
Nit: Is there a reason why URL is all caps here (and in kRequestNearbyURLs) but
not above? I assume it's because that's the constant in the resource files...
would it be possible to make them consistent one way or another?

https://codereview.chromium.org/2248913007/diff/1/components/physical_web/web...
File components/physical_web/webui/resources/physical_web.html (right):

https://codereview.chromium.org/2248913007/diff/1/components/physical_web/web...
components/physical_web/webui/resources/physical_web.html:29: <img
jsvalues="src:icon" />
I'm not familiar with jsvalues as an attribute... and I'm not sure whether this
will work with iOS WebUI. Have you tested this on iOS?

[mattreynolds, 2016-08-30 23:33:24]

Thanks Jackie!

https://codereview.chromium.org/2248913007/diff/1/components/physical_web/web...
File components/physical_web/webui/physical_web_ui_constants.h (right):

https://codereview.chromium.org/2248913007/diff/1/components/physical_web/web...
components/physical_web/webui/physical_web_ui_constants.h:28: extern const char
kReturnNearbyURLs[];
On 2016/08/30 22:37:24, Jackie Quinn wrote:
> Nit: Is there a reason why URL is all caps here (and in kRequestNearbyURLs)
but
> not above? I assume it's because that's the constant in the resource files...
> would it be possible to make them consistent one way or another?

Done.

https://codereview.chromium.org/2248913007/diff/1/components/physical_web/web...
File components/physical_web/webui/resources/physical_web.html (right):

https://codereview.chromium.org/2248913007/diff/1/components/physical_web/web...
components/physical_web/webui/resources/physical_web.html:29: <img
jsvalues="src:icon" />
On 2016/08/30 22:37:24, Jackie Quinn wrote:
> I'm not familiar with jsvalues as an attribute... and I'm not sure whether
this
> will work with iOS WebUI. Have you tested this on iOS?

Yeah, it works fine for me on iOS.

[jochen (gone - plz use gerrit), 2016-08-31 14:32:25]

it looks like you only implemented this for ios? why the changes to chrome/
then?

is it possible to add some tests?

https://codereview.chromium.org/2248913007/diff/20001/components/physical_web...
File components/physical_web.gypi (right):

https://codereview.chromium.org/2248913007/diff/20001/components/physical_web...
components/physical_web.gypi:27: '../base/base.gyp:base',
why is this needed?

https://codereview.chromium.org/2248913007/diff/20001/components/physical_web...
File components/physical_web/webui/DEPS (right):

https://codereview.chromium.org/2248913007/diff/20001/components/physical_web...
components/physical_web/webui/DEPS:3: "+ui/base",
none of these includes seems to be used?

[mattreynolds, 2016-09-01 02:25:20]

Not sure why I had the chrome/ changes, they were unnecessary so I removed them.

I considered adding unit tests but there really isn't much testable logic in
this CL, and I didn't find any other webui pages with tests to use as a
reference.  Any suggestions on suitable tests for this component?

https://codereview.chromium.org/2248913007/diff/20001/components/physical_web...
File components/physical_web.gypi (right):

https://codereview.chromium.org/2248913007/diff/20001/components/physical_web...
components/physical_web.gypi:27: '../base/base.gyp:base',
On 2016/08/31 14:32:25, jochen wrote:
> why is this needed?

It's not, removed.

https://codereview.chromium.org/2248913007/diff/20001/components/physical_web...
File components/physical_web/webui/DEPS (right):

https://codereview.chromium.org/2248913007/diff/20001/components/physical_web...
components/physical_web/webui/DEPS:3: "+ui/base",
On 2016/08/31 14:32:25, jochen wrote:
> none of these includes seems to be used?

Removed

[jochen (gone - plz use gerrit), 2016-09-01 12:09:08]

if this is ios only, why can't it be completely under ios?

what about a test that fires up the webui and checks that it contains some
expected strings?

[blundell, 2016-09-01 12:13:42]

On 2016/09/01 12:09:08, jochen wrote:
> if this is ios only, why can't it be completely under ios?

This feature is intended to be expanded to Android later on (I had a similar
question on an earlier CL).

> 
> what about a test that fires up the webui and checks that it contains some
> expected strings?

[mattreynolds, 2016-09-01 17:38:52]

On 2016/09/01 12:09:08, jochen wrote:
> if this is ios only, why can't it be completely under ios?
> 
> what about a test that fires up the webui and checks that it contains some
> expected strings?

As Colin mentioned, we're hoping to eventually bring this WebUI to Chrome for
Android if the experiment is successful.

There's an ios_internal change that needs to go in first before we'll be able to
actually launch the WebUI. I'm not sure how to implement a test that needs to
inspect a loaded web page, any suggestions on where to start?

[Jackie Quinn, 2016-09-01 22:43:59]

On 2016/09/01 17:38:52, mattreynolds wrote:
> On 2016/09/01 12:09:08, jochen wrote:
> > if this is ios only, why can't it be completely under ios?
> > 
> > what about a test that fires up the webui and checks that it contains some
> > expected strings?
> 
> As Colin mentioned, we're hoping to eventually bring this WebUI to Chrome for
> Android if the experiment is successful.
> 
> There's an ios_internal change that needs to go in first before we'll be able
to
> actually launch the WebUI. I'm not sure how to implement a test that needs to
> inspect a loaded web page, any suggestions on where to start?

ios/web/public/test/web_test and js_test_util. I'd probably inject JS to get the
body's innerHTML and go from there. This also may fit better as an EarlGrey
integration test than as a unittest.

eugenebut@ is a good person to reach out to.

[jochen (gone - plz use gerrit), 2016-09-02 12:32:16]

ok, lgtm for components/ then

I'd still recommend a test. You'd be surprised by how many webui pages break
regularly

https://codereview.chromium.org/2248913007/diff/40001/components/physical_web...
File components/physical_web_ui_strings.grdp (right):

https://codereview.chromium.org/2248913007/diff/40001/components/physical_web...
components/physical_web_ui_strings.grdp:3: <message
name="IDS_PHYSICAL_WEB_UI_TITLE" desc="Title of a page that displays a list of
URLs broadcast by nearby devices.">
nit. keep in mind that the translators only see this description. you could add
(so-called "Physical Web") to "list of URLs broadcast by nearby devices" and
clarify that "page" is web page that ships with chrome.

[mattreynolds, 2016-09-02 17:29:59]

Thanks Jochen, I'm looking into adding a test.  There's an existing EarlGrey
test for the chrome://version webui that seems like it will be easy to adapt.

https://codereview.chromium.org/2248913007/diff/40001/components/physical_web...
File components/physical_web_ui_strings.grdp (right):

https://codereview.chromium.org/2248913007/diff/40001/components/physical_web...
components/physical_web_ui_strings.grdp:3: <message
name="IDS_PHYSICAL_WEB_UI_TITLE" desc="Title of a page that displays a list of
URLs broadcast by nearby devices.">
On 2016/09/02 12:32:16, jochen wrote:
> nit. keep in mind that the translators only see this description. you could
add
> (so-called "Physical Web") to "list of URLs broadcast by nearby devices" and
> clarify that "page" is web page that ships with chrome.

Done.

[mattreynolds, 2016-09-02 17:31:01]

mattreynolds@chromium.org changed reviewers:
+ mpearson@google.com

[mattreynolds, 2016-09-02 17:31:02]

Hi Mark, could you review the change in /tools/metrics?

[Mark P, 2016-09-02 18:30:58]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2016-09-02 18:30:59]

actions.xml lgtm

[mattreynolds, 2016-09-02 21:31:16]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-09-02 21:31:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyquinn@chromium.org, rohitrao@chromium.org, jochen@chromium.org,
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2248913007/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-02 21:32:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 22:36:47]

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

[commit-bot: I haz the power, 2016-09-02 22:36:49]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mattreynolds, 2016-09-02 22:41:04]

The CQ bit was checked by mattreynolds@chromium.org

[commit-bot: I haz the power, 2016-09-02 22:41:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 23:46:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-02 23:48:46]

Description was changed from

==========
Add a Physical Web WebUI

Adds a WebUI page that receives a list of URLs broadcast by
nearby devices along with associated page metadata and displays
them as a list of clickable items.

BUG=630769
==========

to

==========
Add a Physical Web WebUI

Adds a WebUI page that receives a list of URLs broadcast by
nearby devices along with associated page metadata and displays
them as a list of clickable items.

BUG=630769

Committed: https://crrev.com/364ddcfc06dd2961111c9cbd390ed22323a54284
Cr-Commit-Position: refs/heads/master@{#416393}
==========

[commit-bot: I haz the power, 2016-09-02 23:48:47]

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