Update Physical Web WebUI to show new results automatically

Update Physical Web WebUI to show new results automatically

Before users need to manually refresh the page to see new nearby urls.
With this patch, new urls will be automatically appended to WebUI result.

BUG=692111
Review-Url: https://codereview.chromium.org/2741823002
Cr-Commit-Position: refs/heads/master@{#460431}
Committed: https://chromium.googlesource.com/chromium/src/+/e1fba6f591fe7c8ecfdb1f4e52f37928f648b108

[Ran, 2017-03-09 19:44:14]

Description was changed from

==========
workaround for jstemplate

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

to

==========
Update Physical Web WebUI to show new results automatically

This patch is a workaround for jstemplate.
Jstemplate seems cannot re-render things correctly.
In order for it to work, we have to manually remove the previous rendered
result.


BUG=692111
==========

[Ran, 2017-03-09 19:44:29]

Description was changed from

==========
Update Physical Web WebUI to show new results automatically

This patch is a workaround for jstemplate.
Jstemplate seems cannot re-render things correctly.
In order for it to work, we have to manually remove the previous rendered
result.


BUG=692111
==========

to

==========
Update Physical Web WebUI to show new results automatically

This patch is a workaround for jstemplate.
Jstemplate seems cannot re-render things correctly.
In order for it to work, we have to manually remove the previous rendered
result.


BUG=692111
==========

[Ran, 2017-03-09 19:46:01]

Description was changed from

==========
Update Physical Web WebUI to show new results automatically

This patch is a workaround for jstemplate.
Jstemplate seems cannot re-render things correctly.
In order for it to work, we have to manually remove the previous rendered
result.


BUG=692111
==========

to

==========
Update Physical Web WebUI to show new results automatically

This patch is a workaround for jstemplate.
Jstemplate seems cannot re-render things correctly.
In order for it to work, we have to manually remove the previous
rendered result.


BUG=692111
==========

[Ran, 2017-03-09 19:47:59]

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

[Ran, 2017-03-09 19:48:48]

Description was changed from

==========
Update Physical Web WebUI to show new results automatically

This patch is a workaround for jstemplate.
Jstemplate seems cannot re-render things correctly.
In order for it to work, we have to manually remove the previous
rendered result.


BUG=692111
==========

to

==========
Update Physical Web WebUI to show new results automatically.

This patch is a workaround for jstemplate.
Jstemplate seems cannot re-render things correctly.
In order for it to work, we have to manually remove the previous
rendered result.


BUG=692111
==========

[Ran, 2017-03-09 19:49:15]

Description was changed from

==========
Update Physical Web WebUI to show new results automatically.

This patch is a workaround for jstemplate.
Jstemplate seems cannot re-render things correctly.
In order for it to work, we have to manually remove the previous
rendered result.


BUG=692111
==========

to

==========
Update Physical Web WebUI to show new results automatically

This patch is a workaround for jstemplate.
Jstemplate seems cannot re-render things correctly.
In order for it to work, we have to manually remove the previous
rendered result.


BUG=692111
==========

[Ran, 2017-03-09 20:29:26]

Matt, could you try to see if this patch works when you do scanning for new
result in backend?

[Ran, 2017-03-10 15:34:05]

Description was changed from

==========
Update Physical Web WebUI to show new results automatically

This patch is a workaround for jstemplate.
Jstemplate seems cannot re-render things correctly.
In order for it to work, we have to manually remove the previous
rendered result.


BUG=692111
==========

to

==========
Update Physical Web WebUI to show new results automatically

The JavaScript function is a workaround for jstemplate.
Jstemplate seems cannot re-render things correctly.
In order for it to work, we have to manually remove the previous
rendered result.


BUG=692111
==========

[Ran, 2017-03-10 15:35:25]

Description was changed from

==========
Update Physical Web WebUI to show new results automatically

The JavaScript function is a workaround for jstemplate.
Jstemplate seems cannot re-render things correctly.
In order for it to work, we have to manually remove the previous
rendered result.


BUG=692111
==========

to

==========
Update Physical Web WebUI to show new results automatically

Before user need to manually refresh the page to see new result.
With this patch, the new result will automatically append to WebUI.

BUG=692111
==========

[Ran, 2017-03-10 15:35:35]

Description was changed from

==========
Update Physical Web WebUI to show new results automatically

Before user need to manually refresh the page to see new result.
With this patch, the new result will automatically append to WebUI.

BUG=692111
==========

to

==========
Update Physical Web WebUI to show new results automatically

Before users need to manually refresh the page to see new result.
With this patch, the new result will automatically append to WebUI.

BUG=692111
==========

[Ran, 2017-03-10 15:35:49]

Description was changed from

==========
Update Physical Web WebUI to show new results automatically

Before users need to manually refresh the page to see new result.
With this patch, the new result will automatically append to WebUI.

BUG=692111
==========

to

==========
Update Physical Web WebUI to show new results automatically

Before users need to manually refresh the page to see new nearby urls.
With this patch, the new result will automatically append to WebUI.

BUG=692111
==========

[Ran, 2017-03-10 15:36:10]

Description was changed from

==========
Update Physical Web WebUI to show new results automatically

Before users need to manually refresh the page to see new nearby urls.
With this patch, the new result will automatically append to WebUI.

BUG=692111
==========

to

==========
Update Physical Web WebUI to show new results automatically

Before users need to manually refresh the page to see new nearby urls.
With this patch, new urls will automatically be appended to WebUI.

BUG=692111
==========

[Ran, 2017-03-10 15:36:25]

Description was changed from

==========
Update Physical Web WebUI to show new results automatically

Before users need to manually refresh the page to see new nearby urls.
With this patch, new urls will automatically be appended to WebUI.

BUG=692111
==========

to

==========
Update Physical Web WebUI to show new results automatically

Before users need to manually refresh the page to see new nearby urls.
With this patch, new urls will be automatically appended to WebUI.

BUG=692111
==========

[Ran, 2017-03-10 15:36:51]

Description was changed from

==========
Update Physical Web WebUI to show new results automatically

Before users need to manually refresh the page to see new nearby urls.
With this patch, new urls will be automatically appended to WebUI.

BUG=692111
==========

to

==========
Update Physical Web WebUI to show new results automatically

Before users need to manually refresh the page to see new nearby urls.
With this patch, new urls will be automatically appended to WebUI result.

BUG=692111
==========

[Ran, 2017-03-10 15:37:08]

Patchset #1 (id:1) has been deleted

[Ran, 2017-03-10 15:37:14]

Patchset #1 (id:20001) has been deleted

[Ran, 2017-03-10 15:37:19]

Patchset #1 (id:40001) has been deleted

[Ran, 2017-03-10 16:05:20]

Tested on device.

[mmocny, 2017-03-10 17:38:41]

This is starting to look good!  Small issues but I think this is close.

https://codereview.chromium.org/2741823002/diff/60001/components/physical_web...
File components/physical_web/webui/physical_web_base_message_handler.cc (right):

https://codereview.chromium.org/2741823002/diff/60001/components/physical_web...
components/physical_web/webui/physical_web_base_message_handler.cc:54: const
base::ListValue* args) {
What is args used for?  Should we change the signature?

https://codereview.chromium.org/2741823002/diff/60001/components/physical_web...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2741823002/diff/60001/components/physical_web...
components/physical_web/webui/resources/physical_web.js:5: var Render_ID =
'RenderId';
Is this the right ID?

Also, why set a constant for this ID when all the rest are inline.  I'm happy to
define them up here but can we do it for all.  In that light, I think we need a
RenderSourceID and RenderTargetID.

Also nit: I think you can use 'let' and 'const' in JavaScript in place of 'var'
which is probably good to get used to.

https://codereview.chromium.org/2741823002/diff/60001/components/physical_web...
components/physical_web/webui/resources/physical_web.js:36: function
resetTemplate() {
Did replaceChild not work?

[Ran, 2017-03-10 18:15:31]

Patchset #2 (id:80001) has been deleted

[Ran, 2017-03-10 18:19:10]

Fix issues.

Hi Matt & Conley, what is the init purpose for having the args in
HandleRequestNearbyURLs? Any plan for using it at later point?

https://cs.corp.google.com/clankium/src/components/physical_web/webui/physica...

https://codereview.chromium.org/2741823002/diff/60001/components/physical_web...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2741823002/diff/60001/components/physical_web...
components/physical_web/webui/resources/physical_web.js:5: var Render_ID =
'RenderId';
On 2017/03/10 17:38:41, mmocny wrote:
> Is this the right ID?
> 
> Also, why set a constant for this ID when all the rest are inline.  I'm happy
to
> define them up here but can we do it for all.  In that light, I think we need
a
> RenderSourceID and RenderTargetID.
> 
> Also nit: I think you can use 'let' and 'const' in JavaScript in place of
'var'
> which is probably good to get used to.

Done.

https://codereview.chromium.org/2741823002/diff/60001/components/physical_web...
components/physical_web/webui/resources/physical_web.js:5: var Render_ID =
'RenderId';
On 2017/03/10 17:38:41, mmocny wrote:
> Is this the right ID?
> 
> Also, why set a constant for this ID when all the rest are inline.  I'm happy
to
> define them up here but can we do it for all.  In that light, I think we need
a
> RenderSourceID and RenderTargetID.
> 
> Also nit: I think you can use 'let' and 'const' in JavaScript in place of
'var'
> which is probably good to get used to.

This id can be any string. It's only used in this js so set it to a variable.

Other id/class names are used in html too, inline would make more sense in this
case.

https://codereview.chromium.org/2741823002/diff/60001/components/physical_web...
components/physical_web/webui/resources/physical_web.js:36: function
resetTemplate() {
On 2017/03/10 17:38:41, mmocny wrote:
> Did replaceChild not work?

With the new implementation (make a hidden template), I think it makes more
sense to append the template to body instead of replacing the whole
body-container.

[mmocny, 2017-03-10 18:32:36]

https://codereview.chromium.org/2741823002/diff/60001/components/physical_web...
File components/physical_web/webui/resources/physical_web.js (right):

https://codereview.chromium.org/2741823002/diff/60001/components/physical_web...
components/physical_web/webui/resources/physical_web.js:5: var Render_ID =
'RenderId';
On 2017/03/10 18:19:10, Ran wrote:
> On 2017/03/10 17:38:41, mmocny wrote:
> > Is this the right ID?
> > 
> > Also, why set a constant for this ID when all the rest are inline.  I'm
happy
> to
> > define them up here but can we do it for all.  In that light, I think we
need
> a
> > RenderSourceID and RenderTargetID.
> > 
> > Also nit: I think you can use 'let' and 'const' in JavaScript in place of
> 'var'
> > which is probably good to get used to.
> 
> Done.

I understand now, its used to help get a handle to the cloned Node.

I think you can just omit lookup by name.  Move the "resetTemplate" code into
renderTemplate and then use the "templateFormat" variable and don't bother with
ID's.

https://codereview.chromium.org/2741823002/diff/60001/components/physical_web...
components/physical_web/webui/resources/physical_web.js:36: function
resetTemplate() {
On 2017/03/10 18:19:10, Ran wrote:
> On 2017/03/10 17:38:41, mmocny wrote:
> > Did replaceChild not work?
> 
> With the new implementation (make a hidden template), I think it makes more
> sense to append the template to body instead of replacing the whole
> body-container.

Okay I understand now, this is fine.  Possibly we could be fancy, but its better
like this.

[Ran, 2017-03-10 19:43:54]

Patchset #3 (id:120001) has been deleted

[mattreynolds, 2017-03-10 19:59:44]

https://codereview.chromium.org/2741823002/diff/60001/components/physical_web...
File components/physical_web/webui/physical_web_base_message_handler.cc (right):

https://codereview.chromium.org/2741823002/diff/60001/components/physical_web...
components/physical_web/webui/physical_web_base_message_handler.cc:54: const
base::ListValue* args) {
On 2017/03/10 17:38:41, mmocny wrote:
> What is args used for?  Should we change the signature?

We need to match the MessageCallback signature to register it with
RegisterMessageCallback:

https://chromium.googlesource.com/chromium/src.git/+/master/content/public/br...

args holds the arguments passed to requestNearbyURLs.

https://chromium.googlesource.com/chromium/src.git/+/master/components/physic...

We don't need to pass anything, so the args are unused.

[mmocny, 2017-03-17 19:11:32]

lgtm

[mmocny, 2017-03-17 19:17:48]

-lgtm

I think we need to adjust UMA now that we can update results.

https://codereview.chromium.org/2741823002/diff/110005/components/physical_we...
File components/physical_web/webui/physical_web_base_message_handler.cc (right):

https://codereview.chromium.org/2741823002/diff/110005/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:91:
UMA_HISTOGRAM_EXACT_LINEAR("PhysicalWeb.TotalUrls.OnInitialDisplay",
Just noticed, this is a bug now that we update results multiple times.

[Ran, 2017-03-21 21:58:06]

Patchset #4 (id:150001) has been deleted

[Ran, 2017-03-21 21:59:43]

Fix uma logger, append new urls to the end.

[mmocny, 2017-03-22 16:03:50]

not lgtm.  (I misused -lgtm last time)

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
File components/physical_web/webui/physical_web_base_message_handler.cc (right):

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:40:
base::Bind(
Looks like you switched to using base::Bind, which states is deprecated.  Did
you mean to use BindRepeating or BindOnce?

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:50: this);
It looks like you switched to use raw `this`, instead of
`base::Unretained(this)` -- was this intended?

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:51:
RegisterMessageCallback(kRequestNearbyUrls, RequestNearbyUrlsCb);
Could you explain what this is doing?

It looks like you wrapped the HandleRequestNearbyURLs function with a lambda
which calls the UMA.

It still looks like OnInitialDisplay will be called every time this message is
handled, unless I misunderstand, so I don't see how this is resolved?

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:82: for
(const auto& group_id : ordered_group_ids_) {
Ran, as you can see, this is attempting to produce values one-per-groupid.

I wonder, is the group-id just not being set correctly?  Perhaps we should just
fix this code if we are seeing duplicates.

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

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:53: function
reorderURLs(nearbyUrlsData) {
This seems like an error prone hand rolled solution which concerns me.

I think its just a version of stable_partition, but since it won't be super
convenient to import an algorithms library here, I think we should try to find a
less clever solution, and I think we can.

Hows this:

1. Change `reorderURLs` to `findNewUrls` and have it return `newUrlsMetadata`
directly (doesn't modify any global state).
2. Instead of storing `ShownUrls` as a list of resolvedUrl, store the full
`nearbyUrlsData` array from last rendering (initial load will be empty array). 
(This is global state, but makes sense to store).
3. Implement `findNewUrls` by just doing a nested linear scan comparing
`newNearbyUrlsData` to `curentNearbyUrlsData`.  (This should be a very simple
implementation, the data sizes are trivial so its OK)
4. Before calling renderTemplate, do something like  `currentNearbyUrlsData =
currentNearbyUrlsData.concat(findNewUrls(currentNearbyUrlsData, nearbyUrlsData)`

Does this make sense?

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:69: function
dedupURLs(nearbyUrlsData) {
As I mentioned in person, I think URLs are (or should be) deduped in C++, and I
think it should already be doing this from the code Matt gave you (maybe there
is a small bug?).

In a future version of the PW DataSource provider API, C++ will give you deduped
data so we should write the WebUI assuming such.

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:80: function
resetUrlIndex(nearbyUrlsData) {
Do you use .index for anything?

[Ran, 2017-03-22 18:31:23]

Patchset #5 (id:190001) has been deleted

[mattreynolds, 2017-03-22 18:33:10]

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

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:80: function
resetUrlIndex(nearbyUrlsData) {
On 2017/03/22 16:03:50, mmocny wrote:
> Do you use .index for anything?

Only for UMA. If we can remove it that would be great, we would just need some
other way to determine our index in physicalWebItemClicked.

[Ran, 2017-03-22 18:44:32]

Patchset #5 (id:210001) has been deleted

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

Patchset #5 (id:230001) has been deleted

[Ran, 2017-03-22 19:10:47]

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
File components/physical_web/webui/physical_web_base_message_handler.cc (right):

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:40:
base::Bind(
On 2017/03/22 16:03:50, mmocny wrote:
> Looks like you switched to using base::Bind, which states is deprecated.  Did
> you mean to use BindRepeating or BindOnce?

Done.

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:50: this);
On 2017/03/22 16:03:50, mmocny wrote:
> It looks like you switched to use raw `this`, instead of
> `base::Unretained(this)` -- was this intended?

It is intended. Can't bind base::Unretained(this) to a lambda function. (even
can't capture this, Bind can only convert a captureless lambda to callback)

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:51:
RegisterMessageCallback(kRequestNearbyUrls, RequestNearbyUrlsCb);
On 2017/03/22 16:03:50, mmocny wrote:
> Could you explain what this is doing?
> 
> It looks like you wrapped the HandleRequestNearbyURLs function with a lambda
> which calls the UMA.
> 
> It still looks like OnInitialDisplay will be called every time this message is
> handled, unless I misunderstand, so I don't see how this is resolved?

I separate the UMA from the HandleRequestNearbyURLs function. The UMA will be
called once the page is loaded, other time it will call HandleRequestNearbyURLs,
which does not include the UMA.

Renamed it to PhysicalWebPageLoaded to avoid confusing.

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:82: for
(const auto& group_id : ordered_group_ids_) {
On 2017/03/22 16:03:50, mmocny wrote:
> Ran, as you can see, this is attempting to produce values one-per-groupid.
> 
> I wonder, is the group-id just not being set correctly?  Perhaps we should
just
> fix this code if we are seeing duplicates.

Done.

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

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:53: function
reorderURLs(nearbyUrlsData) {
On 2017/03/22 16:03:50, mmocny wrote:
> This seems like an error prone hand rolled solution which concerns me.
> 
> I think its just a version of stable_partition, but since it won't be super
> convenient to import an algorithms library here, I think we should try to find
a
> less clever solution, and I think we can.
> 
> Hows this:
> 
> 1. Change `reorderURLs` to `findNewUrls` and have it return `newUrlsMetadata`
> directly (doesn't modify any global state).
> 2. Instead of storing `ShownUrls` as a list of resolvedUrl, store the full
> `nearbyUrlsData` array from last rendering (initial load will be empty array).

> (This is global state, but makes sense to store).
> 3. Implement `findNewUrls` by just doing a nested linear scan comparing
> `newNearbyUrlsData` to `curentNearbyUrlsData`.  (This should be a very simple
> implementation, the data sizes are trivial so its OK)
> 4. Before calling renderTemplate, do something like  `currentNearbyUrlsData =
> currentNearbyUrlsData.concat(findNewUrls(currentNearbyUrlsData,
nearbyUrlsData)`
> 
> Does this make sense?

Done.

https://codereview.chromium.org/2741823002/diff/170001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:69: function
dedupURLs(nearbyUrlsData) {
On 2017/03/22 16:03:50, mmocny wrote:
> As I mentioned in person, I think URLs are (or should be) deduped in C++, and
I
> think it should already be doing this from the code Matt gave you (maybe there
> is a small bug?).
> 
> In a future version of the PW DataSource provider API, C++ will give you
deduped
> data so we should write the WebUI assuming such.

Done.

[Ran, 2017-03-24 17:14:03]

Patchset #6 (id:270001) has been deleted

[mmocny, 2017-03-24 17:15:13]

https://codereview.chromium.org/2741823002/diff/250001/components/physical_we...
File components/physical_web/webui/physical_web_base_message_handler.cc (right):

https://codereview.chromium.org/2741823002/diff/250001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:51:
RegisterMessageCallback(kPhysicalWebPageLoaded, PhysicalWebPageLoadedCb);
I think you should create a member function called HandlePhysicalWebPageLoaded
since I think you will now want to track Load and Reload differently, plus the
lambda is getting ugly.

Also, I think you should not call GetMetadataList()->size() here, you should use
the actual number of results that are shown, same as your other CL
https://codereview.chromium.org/2775653002

https://codereview.chromium.org/2741823002/diff/250001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:61:
data_source_->RegisterListener(this);
Should we perhaps do this in the constructor instead?  This isn't related to
registering messages.

 I know we cannot push results to webview until after OnPageLoaded, so
alternatively we can also move this registration there.  However I think we may
already have to do some more work given that on page reloads we don't unregister
this listener, and probably already need a way to track when page is
loaded/unloaded to prevent OnFound causing an issue if timing is bad.

Also, if we subscribed in the constructor we could start a foreground scan + PWS
resolve in parallel while the webui page loads.  I don't expect this is a big
deal, but it may be a good idea for code-cleanliness anyway.

https://codereview.chromium.org/2741823002/diff/250001/components/physical_we...
File components/physical_web/webui/physical_web_base_message_handler.h (right):

https://codereview.chromium.org/2741823002/diff/250001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.h:41: void
ReturnNearbyURLs(const base::ListValue* args);
I think we don't need this ListValue* parameter any more.  It was used when we
registered it as a handler from JS, but its not used for that any more.

We should also rename this now to "PushNearbyResults" or something like that.

[mmocny, 2017-03-24 17:19:11]

Getting close.  This is starting to look much cleaner.

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

https://codereview.chromium.org/2741823002/diff/290001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:37: function
returnNearbyURLs(nearbyUrlsData) {
Nit: Also rename this function if you rename the C++ function to
"pushNearbyResults"

https://codereview.chromium.org/2741823002/diff/290001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:40: bodyContainer.hidden
= false;
Nit: I think you should also move this .hidden=false out of this function now.

[Ran, 2017-03-24 18:45:21]

https://codereview.chromium.org/2741823002/diff/250001/components/physical_we...
File components/physical_web/webui/physical_web_base_message_handler.cc (right):

https://codereview.chromium.org/2741823002/diff/250001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:51:
RegisterMessageCallback(kPhysicalWebPageLoaded, PhysicalWebPageLoadedCb);
On 2017/03/24 17:15:12, mmocny wrote:
> I think you should create a member function called HandlePhysicalWebPageLoaded
> since I think you will now want to track Load and Reload differently, plus the
> lambda is getting ugly.
> 
> Also, I think you should not call GetMetadataList()->size() here, you should
use
> the actual number of results that are shown, same as your other CL
> https://codereview.chromium.org/2775653002

Done.

https://codereview.chromium.org/2741823002/diff/250001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:61:
data_source_->RegisterListener(this);
On 2017/03/24 17:15:12, mmocny wrote:
> Should we perhaps do this in the constructor instead?  This isn't related to
> registering messages.
> 
>  I know we cannot push results to webview until after OnPageLoaded, so
> alternatively we can also move this registration there.  However I think we
may
> already have to do some more work given that on page reloads we don't
unregister
> this listener, and probably already need a way to track when page is
> loaded/unloaded to prevent OnFound causing an issue if timing is bad.
> 
> Also, if we subscribed in the constructor we could start a foreground scan +
PWS
> resolve in parallel while the webui page loads.  I don't expect this is a big
> deal, but it may be a good idea for code-cleanliness anyway.

Can't call it in constructor as GetPhysicalWebDataSource is a pure virtual
function..

https://codereview.chromium.org/2741823002/diff/250001/components/physical_we...
File components/physical_web/webui/physical_web_base_message_handler.h (right):

https://codereview.chromium.org/2741823002/diff/250001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.h:41: void
ReturnNearbyURLs(const base::ListValue* args);
On 2017/03/24 17:15:12, mmocny wrote:
> I think we don't need this ListValue* parameter any more.  It was used when we
> registered it as a handler from JS, but its not used for that any more.
> 
> We should also rename this now to "PushNearbyResults" or something like that.

Done.

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

https://codereview.chromium.org/2741823002/diff/290001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:37: function
returnNearbyURLs(nearbyUrlsData) {
On 2017/03/24 17:19:11, mmocny wrote:
> Nit: Also rename this function if you rename the C++ function to
> "pushNearbyResults"

Done.

https://codereview.chromium.org/2741823002/diff/290001/components/physical_we...
components/physical_web/webui/resources/physical_web.js:40: bodyContainer.hidden
= false;
On 2017/03/24 17:19:11, mmocny wrote:
> Nit: I think you should also move this .hidden=false out of this function now.

Done.

[mmocny, 2017-03-24 18:54:14]

LGTM much better!

I think we can now work to fire OnReload UMA as well.

https://codereview.chromium.org/2741823002/diff/250001/components/physical_we...
File components/physical_web/webui/physical_web_base_message_handler.cc (right):

https://codereview.chromium.org/2741823002/diff/250001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:61:
data_source_->RegisterListener(this);
On 2017/03/24 18:45:19, Ran wrote:
> On 2017/03/24 17:15:12, mmocny wrote:
> > Should we perhaps do this in the constructor instead?  This isn't related to
> > registering messages.
> > 
> >  I know we cannot push results to webview until after OnPageLoaded, so
> > alternatively we can also move this registration there.  However I think we
> may
> > already have to do some more work given that on page reloads we don't
> unregister
> > this listener, and probably already need a way to track when page is
> > loaded/unloaded to prevent OnFound causing an issue if timing is bad.
> > 
> > Also, if we subscribed in the constructor we could start a foreground scan +
> PWS
> > resolve in parallel while the webui page loads.  I don't expect this is a
big
> > deal, but it may be a good idea for code-cleanliness anyway.
> 
> Can't call it in constructor as GetPhysicalWebDataSource is a pure virtual
> function..

Okay fair enough.  We could create a "start" method, but at that point I think
thats what RegisterMessages is.

Just to review: who named this "RegisterMessages"?  Is this standard naming
scheme or should we rename it?

[mattreynolds, 2017-03-27 21:46:59]

lgtm

https://codereview.chromium.org/2741823002/diff/250001/components/physical_we...
File components/physical_web/webui/physical_web_base_message_handler.cc (right):

https://codereview.chromium.org/2741823002/diff/250001/components/physical_we...
components/physical_web/webui/physical_web_base_message_handler.cc:61:
data_source_->RegisterListener(this);
> Just to review: who named this "RegisterMessages"?  Is this standard naming
> scheme or should we rename it?

The name comes from the web_ui message handler which calls it RegisterMethods in
both the iOS and non-iOS implementations:

content/public/browser/web_ui_message_handler.h
ios/web/public/webui/web_ui_ios_message_handler.h

Both implementations have the same documentation suggesting it's fine to put
other initialization tasks in RegisterMethods:

  // This is where subclasses specify which messages they'd like to handle and
  // perform any additional initialization. At this point web_ui() will return
  // the associated WebUI[IOS] object.

[Ran, 2017-03-28 18:08:33]

The CQ bit was checked by ranj@chromium.org

[Ran, 2017-03-28 18:08:34]

The CQ bit was unchecked by ranj@chromium.org

[cco3, 2017-03-28 18:08:47]

lgtm

[Ran, 2017-03-28 18:09:33]

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

[commit-bot: I haz the power, 2017-03-28 18:09:54]

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-28 18:18:14]

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

[commit-bot: I haz the power, 2017-03-28 18:18:15]

Dry run: Try jobs failed on following builders:
  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-03-29 04:49:35]

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

[commit-bot: I haz the power, 2017-03-29 04:50:07]

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-29 05:01:38]

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

[commit-bot: I haz the power, 2017-03-29 05:01:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ran, 2017-03-29 05:06:09]

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

[commit-bot: I haz the power, 2017-03-29 05:06:32]

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-29 05:15:01]

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

[commit-bot: I haz the power, 2017-03-29 05:15:02]

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

[Ran, 2017-03-29 06:24:12]

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

[Ran, 2017-03-29 06:24:33]

Patchset #9 (id:350001) has been deleted

[commit-bot: I haz the power, 2017-03-29 06:24: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-03-29 06:35:50]

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

[commit-bot: I haz the power, 2017-03-29 06:35:51]

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-03-29 07:05:37]

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

[commit-bot: I haz the power, 2017-03-29 07:06:09]

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-29 07:38:38]

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

[commit-bot: I haz the power, 2017-03-29 07:38:39]

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-03-29 15:30:19]

The CQ bit was checked by ranj@chromium.org

[Ran, 2017-03-29 15:30:19]

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

[commit-bot: I haz the power, 2017-03-29 15:31:07]

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-29 16:14:36]

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

[commit-bot: I haz the power, 2017-03-29 16:14:37]

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-03-29 16:24:21]

The CQ bit was checked by ranj@chromium.org

[Ran, 2017-03-29 16:24:21]

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

[commit-bot: I haz the power, 2017-03-29 16:24:49]

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-29 17:13:05]

CQ is committing da patch.

Bot data: {"patchset_id": 410001, "attempt_start_ts": 1490804661532620,
"parent_rev": "75dd50b325a709221dd971e61eb3e6f53de1a8aa", "commit_rev":
"e1fba6f591fe7c8ecfdb1f4e52f37928f648b108"}

[commit-bot: I haz the power, 2017-03-29 17:13:53]

Description was changed from

==========
Update Physical Web WebUI to show new results automatically

Before users need to manually refresh the page to see new nearby urls.
With this patch, new urls will be automatically appended to WebUI result.

BUG=692111
==========

to

==========
Update Physical Web WebUI to show new results automatically

Before users need to manually refresh the page to see new nearby urls.
With this patch, new urls will be automatically appended to WebUI result.

BUG=692111

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

[commit-bot: I haz the power, 2017-03-29 17:13:56]

Committed patchset #11 (id:410001) as
https://chromium.googlesource.com/chromium/src/+/e1fba6f591fe7c8ecfdb1f4e52f3...

[Ran, 2017-04-03 18:45:38]

Patchset #2 (id:100001) has been deleted

[Ran, 2017-04-03 18:46:02]

Patchset #8 (id:370001) has been deleted

[Ran, 2017-04-03 18:46:18]

Patchset #8 (id:390001) has been deleted