Add empty list indicator for Physical Web WebUI

Add empty list indicator for Physical Web WebUI.

Before if there is no Physical Web url, WebUI will give an empty page.
Add a indicator message to let the user know there is no result.

BUG=692789
Review-Url: https://codereview.chromium.org/2698503004
Cr-Commit-Position: refs/heads/master@{#456091}
Committed: https://chromium.googlesource.com/chromium/src/+/ffaccf2fc7b8ff03cb3e4a8aa496aae6ccf03317

[Ran, 2017-02-15 22:22:15]

Description was changed from

==========
Make a notification message when doing scanning and automaticlly disappear after
6s, show a message if there is no result instead of empty page.

BUG=
==========

to

==========
When doing scanning, make a notification message and automatically disappear
after 6s, show a message if there is no result instead of empty page.

BUG=
==========

[Ran, 2017-02-15 22:22:16]

ranj@chromium.org changed reviewers:
+ cco3@chromium.org, mattreynolds@chromium.org, mmocny@chromium.org

[Ran, 2017-02-15 22:23:56]

The CQ bit was checked by ranj@chromium.org to run a CQ dry run

[Ran, 2017-02-15 22:24:01]

The CQ bit was unchecked by ranj@chromium.org

[Ran, 2017-02-15 22:24:33]

The CQ bit was checked by ranj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 22:24:38]

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

[Ran, 2017-02-15 22:40:49]

Description was changed from

==========
When doing scanning, make a notification message and automatically disappear
after 6s, show a message if there is no result instead of empty page.

BUG=
==========

to

==========
When doing scanning, make a notification message and automatically disappear
after 6s, show a message if there is no result instead of empty page.

BUG=692789
==========

[commit-bot: I haz the power, 2017-02-15 22:42:34]

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

[commit-bot: I haz the power, 2017-02-15 22:42:34]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Ran, 2017-02-15 22:44:05]

The CQ bit was checked by ranj@chromium.org to run a CQ dry run

[Ran, 2017-02-15 22:44:15]

The CQ bit was unchecked by ranj@chromium.org

[Ran, 2017-02-15 22:46:39]

The CQ bit was checked by ranj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 22:47:28]

Dry run: 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, 2017-02-15 23:43:53]

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

[commit-bot: I haz the power, 2017-02-15 23:43:54]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ran, 2017-02-16 00:36:20]

The CQ bit was checked by ranj@chromium.org to run a CQ dry run

[Ran, 2017-02-16 00:36:34]

The CQ bit was unchecked by ranj@chromium.org

[Ran, 2017-02-16 17:19:06]

The CQ bit was checked by ranj@chromium.org to run a CQ dry run

[Ran, 2017-02-16 17:19:20]

The CQ bit was unchecked by ranj@chromium.org

[Ran, 2017-02-16 17:21:23]

The CQ bit was checked by ranj@chromium.org to run a CQ dry run

[Ran, 2017-02-16 17:21:29]

The CQ bit was unchecked by ranj@chromium.org

[Ran, 2017-02-16 19:03:10]

The CQ bit was checked by ranj@chromium.org to run a CQ dry run

[Ran, 2017-02-16 19:03:19]

The CQ bit was unchecked by ranj@chromium.org

[Ran, 2017-02-16 19:04:21]

The CQ bit was checked by ranj@chromium.org to run a CQ dry run

[Ran, 2017-02-16 19:05:29]

The CQ bit was unchecked by ranj@chromium.org

[commit-bot: I haz the power, 2017-02-16 19:05:43]

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

[Ran, 2017-02-16 19:06:25]

The CQ bit was checked by ranj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 19:07:38]

Dry run: 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, 2017-02-16 20:05:17]

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

[commit-bot: I haz the power, 2017-02-16 20:05:18]

Dry run: This issue passed the CQ dry run.

[Ran, 2017-02-22 19:43:24]

Description was changed from

==========
When doing scanning, make a notification message and automatically disappear
after 6s, show a message if there is no result instead of empty page.

BUG=692789
==========

to

==========
Add notification messages for scanning and empty result page.

When doing scanning, display a message, automatically disappear
after 6s if it's still doing scanning.
Display a notification message if there is no Physical Web result.

BUG=692789
==========

[Ran, 2017-02-22 19:44:49]

Description was changed from

==========
Add notification messages for scanning and empty result page.

When doing scanning, display a message, automatically disappear
after 6s if it's still doing scanning.
Display a notification message if there is no Physical Web result.

BUG=692789
==========

to

==========
Add notification messages for scanning and empty result page.

When doing scanning, display a message, automatically disappear
after 6s if it's still doing scanning.

Display a notification message if there is no Physical Web result.

BUG=692789
==========

[Ran, 2017-02-23 18:55:09]

The CQ bit was checked by ranj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 18:55:52]

Dry run: 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, 2017-02-23 19:53:08]

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

[commit-bot: I haz the power, 2017-02-23 19:53:09]

Dry run: This issue passed the CQ dry run.

[cco3, 2017-02-23 20:07:31]

Commit subjects should not have a full stop (period) and should be no more than
50 columns wide.

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
File components/physical_web/webui/resources/physical_web.html (right):

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
components/physical_web/webui/resources/physical_web.html:41: <div
id="empty-container" i18n-content="emptyMessage" style="visibility:hidden">
Is there a reason we should do visibility:hidden instead of display:none?  Same
with below for display:block

Also, I think the css should be in physical_web.css

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:24: function
clearScanNotification() {
I think the language of "notification" is a bit confusing.  Maybe we could call
this a scan indicator?

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:25: var
scanningContainer = $('scan-container');
This selector isn't going to work.   You need the #

If we are using jquery, just use hide() or css():
$('#scan-container').hide();

same below for .show()

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:43:
setTimeout(clearScanNotification, 6000);
Since we always return nearbyurls right away, this doesn't do anything yet,
right?  If not, we should remove it.

[mattreynolds, 2017-02-23 20:18:43]

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:34:
clearScanNotification();
How long does it normally take to receive the URLs? Will the user realistically
have a chance to read the scanning message before it's replaced with the URL
list or empty message?

https://codereview.chromium.org/2698503004/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/webui/physical_web_ui.cc (right):

https://codereview.chromium.org/2698503004/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/webui/physical_web_ui.cc:34:
IDS_PHYSICAL_WEB_UI_SCANNING_MESSAGE);
nit: please fix the line wrapping so the args are aligned

[Ran, 2017-02-23 20:59:46]

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
File components/physical_web/webui/resources/physical_web.html (right):

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
components/physical_web/webui/resources/physical_web.html:41: <div
id="empty-container" i18n-content="emptyMessage" style="visibility:hidden">
On 2017/02/23 20:07:31, cco3 wrote:
> Is there a reason we should do visibility:hidden instead of display:none? 
Same
> with below for display:block
> 
> Also, I think the css should be in physical_web.css

Moved css to physical_web.css.
For visibility over display, I was referring the "body-container" and try to
make it consistent.

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:24: function
clearScanNotification() {
On 2017/02/23 20:07:31, cco3 wrote:
> I think the language of "notification" is a bit confusing.  Maybe we could
call
> this a scan indicator?

Done.

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:25: var
scanningContainer = $('scan-container');
On 2017/02/23 20:07:31, cco3 wrote:
> This selector isn't going to work.   You need the #
> 
> If we are using jquery, just use hide() or css():
> $('#scan-container').hide();
> 
> same below for .show()

I was referring the "body-container". If I add the # it does not work. I don't
think it uses jquery here.

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:34:
clearScanNotification();
On 2017/02/23 20:18:42, mattreynolds wrote:
> How long does it normally take to receive the URLs? Will the user
realistically
> have a chance to read the scanning message before it's replaced with the URL
> list or empty message?

It's very fast, when I am doing test, I did't see the scanning message.

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:43:
setTimeout(clearScanNotification, 6000);
On 2017/02/23 20:07:31, cco3 wrote:
> Since we always return nearbyurls right away, this doesn't do anything yet,
> right?  If not, we should remove it.

Removed.
when I test, it gives the nearbyurls right way, just not sure if it is always
the case.

https://codereview.chromium.org/2698503004/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/webui/physical_web_ui.cc (right):

https://codereview.chromium.org/2698503004/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/webui/physical_web_ui.cc:34:
IDS_PHYSICAL_WEB_UI_SCANNING_MESSAGE);
On 2017/02/23 20:18:42, mattreynolds wrote:
> nit: please fix the line wrapping so the args are aligned

Done.

[mattreynolds, 2017-02-23 22:58:19]

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:34:
clearScanNotification();
On 2017/02/23 20:59:46, Ran wrote:
> On 2017/02/23 20:18:42, mattreynolds wrote:
> > How long does it normally take to receive the URLs? Will the user
> realistically
> > have a chance to read the scanning message before it's replaced with the URL
> > list or empty message?
> 
> It's very fast, when I am doing test, I did't see the scanning message.

With the current behavior it should always be instant since we're just pulling
the current values from the PhysicalWebDataSource and don't need to wait for any
scanning or URL resolution. I'd consider it a bug if this took long enough for
the user to read the scanning message. I think we shouldn't try to show a
message in this case since it will only be frustrating to the user, if they
notice it at all.

[Ran, 2017-02-23 23:06:11]

Description was changed from

==========
Add notification messages for scanning and empty result page.

When doing scanning, display a message, automatically disappear
after 6s if it's still doing scanning.

Display a notification message if there is no Physical Web result.

BUG=692789
==========

to

==========
Display a notification message if there is no Physical Web pages to show.

BUG=692789
==========

[Ran, 2017-02-23 23:09:47]

On 2017/02/23 22:58:19, mattreynolds wrote:
>
https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
> File components/physical_web/webui/resources/physical_web.js (right):
> 
>
https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
> components/physical_web/webui/resources/physical_web.js:34:
> clearScanNotification();
> On 2017/02/23 20:59:46, Ran wrote:
> > On 2017/02/23 20:18:42, mattreynolds wrote:
> > > How long does it normally take to receive the URLs? Will the user
> > realistically
> > > have a chance to read the scanning message before it's replaced with the
URL
> > > list or empty message?
> > 
> > It's very fast, when I am doing test, I did't see the scanning message.
> 
> With the current behavior it should always be instant since we're just pulling
> the current values from the PhysicalWebDataSource and don't need to wait for
any
> scanning or URL resolution. I'd consider it a bug if this took long enough for
> the user to read the scanning message. I think we shouldn't try to show a
> message in this case since it will only be frustrating to the user, if they
> notice it at all.

Done.
Removed the scanning indicator.

[cco3, 2017-02-23 23:36:55]

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
File components/physical_web/webui/resources/physical_web.css (right):

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
components/physical_web/webui/resources/physical_web.css:55: visibility:visible;
This is default, so I don't think it's needed.

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
components/physical_web/webui/resources/physical_web.css:59: visibility:hidden;
I think we should be using display:none, because otherwise this will take up
space you shouldn't be taking up.

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
File components/physical_web/webui/resources/physical_web.html (right):

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
components/physical_web/webui/resources/physical_web.html:41: <div
id="empty-container" i18n-content="emptyMessage">
We should probably call this something other than empty-container (because it's
not an empty container).  Let's call it something like empty-list-message

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:27:
bodyContainer.style.visibility = 'visible';
Just use the appropriate jquery method:
$('#body-container').show()

[cco3, 2017-02-23 23:39:33]

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:27:
bodyContainer.style.visibility = 'visible';
On 2017/02/23 23:36:54, cco3 wrote:
> Just use the appropriate jquery method:
> $('#body-container').show()

I saw your other comment.  This is definitely using some kind of jquery; `$` is
the jquery function.  You should be able to call show or .css('display, 'none').

I'm not sure why it would work without the #, but not with the hash.

[cco3, 2017-02-23 23:41:33]

Also, I would suggest changing the commit subject to: "Add empty list indicator
for Physical Web WebUI"

[mmocny, 2017-02-24 14:16:14]

On 2017/02/23 22:58:19, mattreynolds wrote:
>
https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
> File components/physical_web/webui/resources/physical_web.js (right):
> 
>
https://codereview.chromium.org/2698503004/diff/100001/components/physical_we...
> components/physical_web/webui/resources/physical_web.js:34:
> clearScanNotification();
> On 2017/02/23 20:59:46, Ran wrote:
> > On 2017/02/23 20:18:42, mattreynolds wrote:
> > > How long does it normally take to receive the URLs? Will the user
> > realistically
> > > have a chance to read the scanning message before it's replaced with the
URL
> > > list or empty message?
> > 
> > It's very fast, when I am doing test, I did't see the scanning message.
> 
> With the current behavior it should always be instant since we're just pulling
> the current values from the PhysicalWebDataSource and don't need to wait for
any
> scanning or URL resolution. I'd consider it a bug if this took long enough for
> the user to read the scanning message. I think we shouldn't try to show a
> message in this case since it will only be frustrating to the user, if they
> notice it at all.

It can show up if you open PW list explicitly from the PW settings page, or if
you turn on your phone immediately to chrome and Nearby still hasn't found
anything.

Indeed there are situations where we will claim we are scanning when indeed we
are not, but as we move to foreground subscribe, those should be remedied.

[mmocny, 2017-02-24 14:19:15]

On 2017/02/23 23:39:33, cco3 wrote:
>
https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
> File components/physical_web/webui/resources/physical_web.js (right):
> 
>
https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
> components/physical_web/webui/resources/physical_web.js:27:
> bodyContainer.style.visibility = 'visible';
> On 2017/02/23 23:36:54, cco3 wrote:
> > Just use the appropriate jquery method:
> > $('#body-container').show()
> 
> I saw your other comment.  This is definitely using some kind of jquery; `$`
is
> the jquery function.  You should be able to call show or .css('display,
'none').
> 
> I'm not sure why it would work without the #, but not with the hash.

$ can be assigned to any variable, just happens to be jQuery convention.  Chrome
uses $ often inside inspector etc just as an alias for document.querySelector. 
It could also be some lightweight library -- I'm not sure, but we certainly
aren't importing jQuery specifically and seems unlikely we do so automatically
for all webui pages.

[Ran, 2017-02-24 18:39:10]

Description was changed from

==========
Display a notification message if there is no Physical Web pages to show.

BUG=692789
==========

to

==========
Display a indicator message if there is no Physical Web pages to show.

BUG=692789
==========

[Ran, 2017-02-24 18:52:59]

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
File components/physical_web/webui/resources/physical_web.css (right):

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
components/physical_web/webui/resources/physical_web.css:55: visibility:visible;
On 2017/02/23 23:36:54, cco3 wrote:
> This is default, so I don't think it's needed.

Done.

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
components/physical_web/webui/resources/physical_web.css:59: visibility:hidden;
On 2017/02/23 23:36:54, cco3 wrote:
> I think we should be using display:none, because otherwise this will take up
> space you shouldn't be taking up.

Done.

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
File components/physical_web/webui/resources/physical_web.html (right):

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
components/physical_web/webui/resources/physical_web.html:41: <div
id="empty-container" i18n-content="emptyMessage">
On 2017/02/23 23:36:54, cco3 wrote:
> We should probably call this something other than empty-container (because
it's
> not an empty container).  Let's call it something like empty-list-message

Done.

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2698503004/diff/160001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:27:
bodyContainer.style.visibility = 'visible';
On 2017/02/23 23:36:54, cco3 wrote:
> Just use the appropriate jquery method:
> $('#body-container').show()

The show and hide() function is not available, I guess it's not jQuery here.

[cco3, 2017-02-25 00:01:40]

lgtm

[mattreynolds, 2017-02-27 18:52:38]

lgtm

[Ran, 2017-03-02 17:45:07]

ranj@chromium.org changed reviewers:
+ nyquist@chromium.org

[Ran, 2017-03-02 17:46:43]

Description was changed from

==========
Display a indicator message if there is no Physical Web pages to show.

BUG=692789
==========

to

==========
Before if there is no Physical Web pages, WebUI will give an empty page.

Add a indicator message if there is no Physical Web pages to show.

BUG=692789
==========

[Ran, 2017-03-02 17:46:50]

Description was changed from

==========
Before if there is no Physical Web pages, WebUI will give an empty page.

Add a indicator message if there is no Physical Web pages to show.

BUG=692789
==========

to

==========
Before if there is no Physical Web page, WebUI will give an empty page.

Add a indicator message if there is no Physical Web pages to show.

BUG=692789
==========

[Ran, 2017-03-02 17:47:30]

Description was changed from

==========
Before if there is no Physical Web page, WebUI will give an empty page.

Add a indicator message if there is no Physical Web pages to show.

BUG=692789
==========

to

==========
Add empty list indicator for Physical Web WebUI

Before if there is no Physical Web page, WebUI will give an empty page.
Add a indicator message if there is no Physical Web pages to show.

BUG=692789
==========

[Ran, 2017-03-02 17:47:55]

Description was changed from

==========
Add empty list indicator for Physical Web WebUI

Before if there is no Physical Web page, WebUI will give an empty page.
Add a indicator message if there is no Physical Web pages to show.

BUG=692789
==========

to

==========
Add empty list indicator for Physical Web WebUI

Before if there is no Physical Web page, WebUI will give an empty page.
Add a indicator message to let the user know there is no result.

BUG=692789
==========

[Ran, 2017-03-02 17:48:03]

Description was changed from

==========
Add empty list indicator for Physical Web WebUI

Before if there is no Physical Web page, WebUI will give an empty page.
Add a indicator message to let the user know there is no result.

BUG=692789
==========

to

==========
Add empty list indicator for Physical Web WebUI.

Before if there is no Physical Web page, WebUI will give an empty page.
Add a indicator message to let the user know there is no result.

BUG=692789
==========

[Ran, 2017-03-02 17:48:40]

Description was changed from

==========
Add empty list indicator for Physical Web WebUI.

Before if there is no Physical Web page, WebUI will give an empty page.
Add a indicator message to let the user know there is no result.

BUG=692789
==========

to

==========
Add empty list indicator for Physical Web WebUI.

Before if there is no Physical Web url, WebUI will give an empty page.
Add a indicator message to let the user know there is no result.

BUG=692789
==========

[Ran, 2017-03-02 18:29:13]

+nyquist

[nyquist, 2017-03-02 19:01:28]

https://codereview.chromium.org/2698503004/diff/180001/components/physical_we...
File components/physical_web/webui/resources/physical_web.css (right):

https://codereview.chromium.org/2698503004/diff/180001/components/physical_we...
components/physical_web/webui/resources/physical_web.css:55: display:none;
Optional nit: Sometimes we write 'display:none' and other times 'display: none'
in our code base. This file in general though seems to prefer the latter. Would
you want to update this to match the style of the rest of the file? Totally up
to you. I won't judge.

https://codereview.chromium.org/2698503004/diff/180001/components/physical_we...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2698503004/diff/180001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:31:
emptyMessage.style.display = 'block';
If this page stays open for a bit, does it automatically refresh?
If so, I guess this should be hidden again when length != 0?

If all of these things are one-off though, you should be good to go with this.

[Ran, 2017-03-02 19:36:36]

https://codereview.chromium.org/2698503004/diff/180001/components/physical_we...
File components/physical_web/webui/resources/physical_web.css (right):

https://codereview.chromium.org/2698503004/diff/180001/components/physical_we...
components/physical_web/webui/resources/physical_web.css:55: display:none;
On 2017/03/02 19:01:27, nyquist wrote:
> Optional nit: Sometimes we write 'display:none' and other times 'display:
none'
> in our code base. This file in general though seems to prefer the latter.
Would
> you want to update this to match the style of the rest of the file? Totally up
> to you. I won't judge.

Done.

https://codereview.chromium.org/2698503004/diff/180001/components/physical_we...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2698503004/diff/180001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:31:
emptyMessage.style.display = 'block';
On 2017/03/02 19:01:28, nyquist wrote:
> If this page stays open for a bit, does it automatically refresh?
> If so, I guess this should be hidden again when length != 0?
> 
> If all of these things are one-off though, you should be good to go with this.

The function only gets called when the page loaded, it won't automatically
refresh (although we hope we can make it do that someday).

[nyquist, 2017-03-02 19:37:16]

lgtm

[Ran, 2017-03-02 19:37:41]

The CQ bit was checked by ranj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 19:38:46]

Dry run: 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, 2017-03-02 20:45:09]

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

[commit-bot: I haz the power, 2017-03-02 20:45:10]

Dry run: This issue passed the CQ dry run.

[Ran, 2017-03-02 20:47:21]

The CQ bit was checked by ranj@chromium.org

[Ran, 2017-03-02 20:47:22]

The patchset sent to the CQ was uploaded after l-g-t-m from cco3@chromium.org,
mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/2698503004/#ps200001
(title: "fix style")

[commit-bot: I haz the power, 2017-03-02 20:48:04]

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, 2017-03-02 21:00:07]

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

[commit-bot: I haz the power, 2017-03-02 21:00:09]

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

[Ran, 2017-03-02 21:18:25]

ranj@chromium.org changed reviewers:
+ jam@chromium.org

[Ran, 2017-03-02 21:18:26]

+jam for

chrome/browser/ui/webui/physical_web/physical_web_ui.cc
ios/chrome/browser/ui/webui/physical_web_ui.cc

[jam, 2017-03-03 17:15:52]

On 2017/03/02 21:18:26, Ran wrote:
> +jam for
> 
> chrome/browser/ui/webui/physical_web/physical_web_ui.cc
> ios/chrome/browser/ui/webui/physical_web_ui.cc

why is chromium-reviews mailing list removed?
please pick a specific owner when possible, i.e. chrome/browser/ui/webui/owners

[Ran, 2017-03-06 17:40:15]

ranj@chromium.org changed reviewers:
+ dbeam@chromium.org, eugenebut@chromium.org
- jam@chromium.org

[Ran, 2017-03-06 17:41:08]

+eugenebut for ios/chrome/browser/ui/webui/physical_web_ui.cc
+dbeam for chrome/browser/ui/webui/physical_web/physical_web_ui.cc

[Eugene But (OOO till 7-30), 2017-03-06 19:37:43]

ios/chrome/browser/ui/webui/physical_web_ui.cc lgtm

[Ran, 2017-03-08 20:01:23]

ranj@chromium.org changed reviewers:
+ bauerb@chromium.org
- dbeam@chromium.org

[Ran, 2017-03-08 20:02:08]

+bauerb for
chrome/browser/ui/webui/physical_web/physical_web_ui.cc

[Bernhard Bauer, 2017-03-09 09:25:13]

https://codereview.chromium.org/2698503004/diff/200001/components/physical_we...
File components/physical_web/webui/resources/physical_web.html (right):

https://codereview.chromium.org/2698503004/diff/200001/components/physical_we...
components/physical_web/webui/resources/physical_web.html:41: <div
id="empty-list-container" i18n-content="emptyMessage">
I think I would set a `hidden` attribute on this div and setting it to false
from Javascript as required, instead of adjusting the style.

[mmocny, 2017-03-09 15:17:40]

https://codereview.chromium.org/2698503004/diff/200001/components/physical_we...
File components/physical_web/webui/resources/physical_web.html (right):

https://codereview.chromium.org/2698503004/diff/200001/components/physical_we...
components/physical_web/webui/resources/physical_web.html:41: <div
id="empty-list-container" i18n-content="emptyMessage">
On 2017/03/09 09:25:13, Bernhard Bauer wrote:
> I think I would set a `hidden` attribute on this div and setting it to false
> from Javascript as required, instead of adjusting the style.

I was not previously aware of hidden attribute even though it seems ancient.

Quick SO article
(http://stackoverflow.com/questions/6708247/what-is-the-difference-between-the...)
search shows there may be advantage to hidden attribute over CSS style,
specifically impact on accessiblity features like screenreaders.

Interesting!

[Bernhard Bauer, 2017-03-09 22:10:04]

lgtm

https://codereview.chromium.org/2698503004/diff/220001/components/physical_we...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2698503004/diff/220001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:27:
bodyContainer.removeAttribute("hidden");
You can actually directly access the .hidden attribute on the element.

[Ran, 2017-03-09 22:28:12]

+mmocny

https://codereview.chromium.org/2698503004/diff/220001/components/physical_we...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2698503004/diff/220001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:27:
bodyContainer.removeAttribute("hidden");
On 2017/03/09 22:10:04, Bernhard Bauer wrote:
> You can actually directly access the .hidden attribute on the element.

Done.

[mmocny, 2017-03-09 23:22:32]

Much better, lgtm with one Nit.

https://codereview.chromium.org/2698503004/diff/240001/components/physical_we...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2698503004/diff/240001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:27: bodyContainer.hidden
= false;
Nit: I know that if the result length is 0 that this body will be empty, but
should we really be showing it?

If yes, why did we make it hidden in the first place?

if no, could we make this hidden = false inside an else block below?

[Ran, 2017-03-10 01:00:52]

On 2017/03/09 23:22:32, mmocny wrote:
> Much better, lgtm with one Nit.
> 
>
https://codereview.chromium.org/2698503004/diff/240001/components/physical_we...
> File components/physical_web/webui/resources/physical_web.js (right):
> 
>
https://codereview.chromium.org/2698503004/diff/240001/components/physical_we...
> components/physical_web/webui/resources/physical_web.js:27:
bodyContainer.hidden
> = false;
> Nit: I know that if the result length is 0 that this body will be empty, but
> should we really be showing it?
> 
> If yes, why did we make it hidden in the first place?
> 
> if no, could we make this hidden = false inside an else block below?

The body is only showed when renderTemplate() is done. I think the reason to
make it hidden is we don't want the users see the render process, so show it
after we done rendering.

The original code is confused, in html it set to 'visible' then in js it set to
'visible' again... I guess the first 'visible' should be 'hidden'.
https://cs.corp.google.com/clankium/src/components/physical_web/webui/resourc...
https://cs.corp.google.com/clankium/src/components/physical_web/webui/resourc...

[Ran, 2017-03-10 16:07:47]

The CQ bit was checked by ranj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 16:08:02]

Dry run: 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, 2017-03-10 17:23:54]

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

[commit-bot: I haz the power, 2017-03-10 17:23:55]

Dry run: This issue passed the CQ dry run.

[Ran, 2017-03-10 17:27:33]

The CQ bit was checked by ranj@chromium.org

[Ran, 2017-03-10 17:27:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, mattreynolds@chromium.org, nyquist@chromium.org,
cco3@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2698503004/#ps240001
(title: "access hidden attr directly")

[commit-bot: I haz the power, 2017-03-10 17:28:10]

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, 2017-03-10 17:33:37]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1489166853000650,
"parent_rev": "64862913dee380c55d7b6b37f48219c77838ff86", "commit_rev":
"ffaccf2fc7b8ff03cb3e4a8aa496aae6ccf03317"}

[commit-bot: I haz the power, 2017-03-10 17:34:19]

Description was changed from

==========
Add empty list indicator for Physical Web WebUI.

Before if there is no Physical Web url, WebUI will give an empty page.
Add a indicator message to let the user know there is no result.

BUG=692789
==========

to

==========
Add empty list indicator for Physical Web WebUI.

Before if there is no Physical Web url, WebUI will give an empty page.
Add a indicator message to let the user know there is no result.

BUG=692789

Review-Url: https://codereview.chromium.org/2698503004
Cr-Commit-Position: refs/heads/master@{#456091}
Committed:
https://chromium.googlesource.com/chromium/src/+/ffaccf2fc7b8ff03cb3e4a8aa496...
==========

[commit-bot: I haz the power, 2017-03-10 17:34:23]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/ffaccf2fc7b8ff03cb3e4a8aa496...