Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2231983002:
Expose Physical Web data (Closed)
|
Created:
4 years, 4 months ago by hayesjordan Modified:
4 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpose Physical Web data
The goal is to create an API for pushing changes of Physical Web data,
such as new URLs found, so that other parties can react to the change
without constantly pulling the data. To do this we are providing an API
that allows others to register a listener for push updates when data
changes.
BUG=636490
Committed: https://crrev.com/1098aecd3818cf9eef26cfa77aed4ae51f6621c6
Cr-Commit-Position: refs/heads/master@{#413948}
Patch Set 1 #
Total comments: 4
Patch Set 2 : cco3 comment fixes #
Total comments: 2
Patch Set 3 : Address mattreynolds comments #Patch Set 4 : Address mattreynolds comments #
Total comments: 2
Patch Set 5 : Address cco3 comments #
Total comments: 4
Patch Set 6 : Address mattreynolds comments #
Total comments: 14
Patch Set 7 : Address Olivier's comments #Patch Set 8 : Squash changes and sync #Patch Set 9 : Fix build errors #
Total comments: 2
Patch Set 10 : Address mattreynolds comments #
Total comments: 1
Patch Set 11 : Reordered forward declarations #
Total comments: 2
Patch Set 12 : Address Mark's comments #Patch Set 13 : Fix build failure #Patch Set 14 : Fix build errors #
Messages
Total messages: 61 (21 generated)
hayesjordan@google.com changed reviewers: + cco3@chromium.org
hayesjordan@google.com changed reviewers: + mattreynolds@chromium.org
https://codereview.chromium.org/2231983002/diff/1/components/physical_web/dat... File components/physical_web/data_source/physical_web_listener.h (right): https://codereview.chromium.org/2231983002/diff/1/components/physical_web/dat... components/physical_web/data_source/physical_web_listener.h:12: // On update will be called when the physical web data source updates. Use the actual method name here, i.e. "OnUpdate() will be..." https://codereview.chromium.org/2231983002/diff/1/components/physical_web/dat... components/physical_web/data_source/physical_web_listener.h:13: virtual void onUpdate() = 0; OnUpdate
https://codereview.chromium.org/2231983002/diff/1/components/physical_web/dat... File components/physical_web/data_source/physical_web_listener.h (right): https://codereview.chromium.org/2231983002/diff/1/components/physical_web/dat... components/physical_web/data_source/physical_web_listener.h:12: // On update will be called when the physical web data source updates. On 2016/08/12 17:23:47, cco3 wrote: > Use the actual method name here, i.e. "OnUpdate() will be..." Done. https://codereview.chromium.org/2231983002/diff/1/components/physical_web/dat... components/physical_web/data_source/physical_web_listener.h:13: virtual void onUpdate() = 0; On 2016/08/12 17:23:47, cco3 wrote: > OnUpdate Done.
Matt, does this look good to you or do you want a more granular listener?
Let's split OnUpdate into OnFound, OnLost, and OnDistanceChanged to mirror Nearby's API. https://codereview.chromium.org/2231983002/diff/20001/components/physical_web... File components/physical_web/data_source/physical_web_data_source.h (right): https://codereview.chromium.org/2231983002/diff/20001/components/physical_web... components/physical_web/data_source/physical_web_data_source.h:42: virtual void RegisterListener(PhysicalWebListener physical_web_listner) = 0; physical_web_listner -> physical_web_listener Capitalize Physical Web
https://codereview.chromium.org/2231983002/diff/20001/components/physical_web... File components/physical_web/data_source/physical_web_data_source.h (right): https://codereview.chromium.org/2231983002/diff/20001/components/physical_web... components/physical_web/data_source/physical_web_data_source.h:42: virtual void RegisterListener(PhysicalWebListener physical_web_listner) = 0; On 2016/08/12 23:07:30, mattreynolds wrote: > physical_web_listner -> physical_web_listener > > Capitalize Physical Web Done.
https://codereview.chromium.org/2231983002/diff/60001/components/physical_web... File components/physical_web/data_source/physical_web_listener.h (right): https://codereview.chromium.org/2231983002/diff/60001/components/physical_web... components/physical_web/data_source/physical_web_listener.h:15: virtual void OnFound(string& url) = 0; const (same below)
https://codereview.chromium.org/2231983002/diff/60001/components/physical_web... File components/physical_web/data_source/physical_web_listener.h (right): https://codereview.chromium.org/2231983002/diff/60001/components/physical_web... components/physical_web/data_source/physical_web_listener.h:15: virtual void OnFound(string& url) = 0; On 2016/08/15 19:12:24, cco3 wrote: > const (same below) Done.
https://codereview.chromium.org/2231983002/diff/80001/components/physical_web... File components/physical_web/data_source/physical_web_data_source.h (right): https://codereview.chromium.org/2231983002/diff/80001/components/physical_web... components/physical_web/data_source/physical_web_data_source.h:42: virtual void RegisterListener(PhysicalWebListener physical_web_listener) = 0; Pass the listener as a pointer (PhysicalWebListener* physical_web_listener) so we don't register a copy https://codereview.chromium.org/2231983002/diff/80001/components/physical_web... File components/physical_web/data_source/physical_web_listener.h (right): https://codereview.chromium.org/2231983002/diff/80001/components/physical_web... components/physical_web/data_source/physical_web_listener.h:15: virtual void OnFound(const string& url) = 0; Use std::string
https://codereview.chromium.org/2231983002/diff/80001/components/physical_web... File components/physical_web/data_source/physical_web_data_source.h (right): https://codereview.chromium.org/2231983002/diff/80001/components/physical_web... components/physical_web/data_source/physical_web_data_source.h:42: virtual void RegisterListener(PhysicalWebListener physical_web_listener) = 0; On 2016/08/16 17:35:46, mattreynolds wrote: > Pass the listener as a pointer (PhysicalWebListener* physical_web_listener) so > we don't register a copy Done. https://codereview.chromium.org/2231983002/diff/80001/components/physical_web... File components/physical_web/data_source/physical_web_listener.h (right): https://codereview.chromium.org/2231983002/diff/80001/components/physical_web... components/physical_web/data_source/physical_web_listener.h:15: virtual void OnFound(const string& url) = 0; On 2016/08/16 17:35:46, mattreynolds wrote: > Use std::string Done.
lgtm
Description was changed from ========== Expose Physical Web data The goal is to create an API for pushing changes of Physical Web data, such as new URLs found, so that other parties can react to the change without constantly pulling the data. To do this we are providing an API that allows others to register a listener for push updates when data changes. BUG=636490 ========== to ========== Expose Physical Web data The goal is to create an API for pushing changes of Physical Web data, such as new URLs found, so that other parties can react to the change without constantly pulling the data. To do this we are providing an API that allows others to register a listener for push updates when data changes. BUG=636490 ==========
hayesjordan@google.com changed reviewers: + olivierrobin@chromium.org
Vitalii, this the API for getting push notifications for changes in the Physical Web data.
On 2016/08/16 20:45:50, hayesjordan wrote: > Vitalii, this the API for getting push notifications for changes in the Physical > Web data. Jordan, the API looks good assuming that you plan to extend it. Namely, so far we need to fetch all URLs metadata when a new URL is found. This is fine for now, but we may go to incremental model at some point.
On 2016/08/17 14:28:41, vitaliii wrote: > On 2016/08/16 20:45:50, hayesjordan wrote: > > Vitalii, this the API for getting push notifications for changes in the > Physical > > Web data. > > Jordan, the API looks good assuming that you plan to extend it. > Namely, so far we need to fetch all URLs metadata when a new URL is found. > This is fine for now, but we may go to incremental model at some point. The idea is that this will let you know a URL has changed. Eventually, PhysicalWebDataSource will give you a way to query it for metadata pertaining to specific URLs.
Hi Olivier, would you be able to take a look at this change?
On 2016/08/17 17:22:20, cco3 wrote: > The idea is that this will let you know a URL has changed. Eventually, > PhysicalWebDataSource will give you a way to query it for metadata pertaining to > specific URLs. Then both the plan and the presented API look good!
https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... File components/physical_web/data_source/physical_web_data_source.h (right): https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... components/physical_web/data_source/physical_web_data_source.h:10: #include physical_web_listener.h All include path must be full path starting from src. Here #include "components/physical_web/data_source/physical_web_listener.h" We also tend to include the minimal amount of files in headers (and in source file too). You don't need the actual header here and can just forward declare the PhysicalWebListener class. https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... components/physical_web/data_source/physical_web_data_source.h:42: virtual void RegisterListener(PhysicalWebListener* physical_web_listener) = 0; Please add UnregisterListener (or similar). https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... File components/physical_web/data_source/physical_web_listener.h (right): https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... components/physical_web/data_source/physical_web_listener.h:11: class PhysicalWebListener { Question: Is it possible that a beacon change URL? In that case, do we have a Lost/Found notification? https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... components/physical_web/data_source/physical_web_listener.h:14: // OnFound(url) will be called when a new url has been found. NIT: when a new URL has https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... components/physical_web/data_source/physical_web_listener.h:17: // OnLost(url) will be called when a url can no longer be seen. (for my personnal knowledge: is this timeout based?) https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... components/physical_web/data_source/physical_web_listener.h:17: // OnLost(url) will be called when a url can no longer be seen. NIT: when a URL can no longer... https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... components/physical_web/data_source/physical_web_listener.h:21: // estimate is changed for the url. nit: for the URL.
https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... File components/physical_web/data_source/physical_web_listener.h (right): https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... components/physical_web/data_source/physical_web_listener.h:11: class PhysicalWebListener { On 2016/08/22 07:45:09, Olivier Robin wrote: > Question: Is it possible that a beacon change URL? > In that case, do we have a Lost/Found notification? Yes, that is how we decided we would signal client code if the beacon changes its URL. https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... components/physical_web/data_source/physical_web_listener.h:17: // OnLost(url) will be called when a url can no longer be seen. On 2016/08/22 07:45:09, Olivier Robin wrote: > (for my personnal knowledge: is this timeout based?) Yes. We don't have a particular timeout chosen for iOS. On Android, it will be hidden to us, and we will just pass on onLost events that we get from Nearby.
https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... File components/physical_web/data_source/physical_web_data_source.h (right): https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... components/physical_web/data_source/physical_web_data_source.h:10: #include physical_web_listener.h On 2016/08/22 07:45:09, Olivier Robin wrote: > All include path must be full path starting from src. > Here > #include "components/physical_web/data_source/physical_web_listener.h" > > We also tend to include the minimal amount of files in headers (and in source > file too). > You don't need the actual header here and can just forward declare the > PhysicalWebListener class. Done. https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... components/physical_web/data_source/physical_web_data_source.h:42: virtual void RegisterListener(PhysicalWebListener* physical_web_listener) = 0; On 2016/08/22 07:45:09, Olivier Robin wrote: > Please add UnregisterListener (or similar). Done. https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... File components/physical_web/data_source/physical_web_listener.h (right): https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... components/physical_web/data_source/physical_web_listener.h:14: // OnFound(url) will be called when a new url has been found. On 2016/08/22 07:45:09, Olivier Robin wrote: > NIT: when a new URL has Done. https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... components/physical_web/data_source/physical_web_listener.h:17: // OnLost(url) will be called when a url can no longer be seen. On 2016/08/22 07:45:09, Olivier Robin wrote: > NIT: when a URL can no longer... Done. https://codereview.chromium.org/2231983002/diff/100001/components/physical_we... components/physical_web/data_source/physical_web_listener.h:21: // estimate is changed for the url. On 2016/08/22 07:45:09, Olivier Robin wrote: > nit: for the URL. Done.
lgtm
The CQ bit was checked by hayesjordan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org Link to the patchset: https://codereview.chromium.org/2231983002/#ps120001 (title: "Address Olivier's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by hayesjordan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org, olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/2231983002/#ps140001 (title: "Squash changes and sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Matt could you take a look at the updates?
https://codereview.chromium.org/2231983002/diff/160001/ios/chrome/common/phys... File ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h (right): https://codereview.chromium.org/2231983002/diff/160001/ios/chrome/common/phys... ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h:17: @class PhysicalWebListener; This forward declaration should just be "class PhysicalWebListener;" PhysicalWebScanner is an Objective-C class which is why it uses slightly different syntax.
https://codereview.chromium.org/2231983002/diff/160001/ios/chrome/common/phys... File ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h (right): https://codereview.chromium.org/2231983002/diff/160001/ios/chrome/common/phys... ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h:17: @class PhysicalWebListener; On 2016/08/23 19:33:42, mattreynolds wrote: > This forward declaration should just be "class PhysicalWebListener;" > PhysicalWebScanner is an Objective-C class which is why it uses slightly > different syntax. Done.
lgtm https://codereview.chromium.org/2231983002/diff/180001/ios/chrome/common/phys... File ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h (right): https://codereview.chromium.org/2231983002/diff/180001/ios/chrome/common/phys... ios/chrome/common/physical_web/ios_chrome_physical_web_data_source.h:17: class PhysicalWebListener; Let's move this up with the ListValue declaration (but not inside the namespace) to keep the C++ declarations grouped together.
hayesjordan@google.com changed reviewers: + mpearson@chromium.org
mpearson@chromium.org: Please review changes in physical_web_provider_unittest.cc
https://codereview.chromium.org/2231983002/diff/200001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2231983002/diff/200001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:45: void RegisterListener(PhysicalWebListener* physical_web_listener) {} Shouldn't you have the override keyword here?
https://codereview.chromium.org/2231983002/diff/200001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider_unittest.cc (right): https://codereview.chromium.org/2231983002/diff/200001/components/omnibox/bro... components/omnibox/browser/physical_web_provider_unittest.cc:45: void RegisterListener(PhysicalWebListener* physical_web_listener) {} On 2016/08/23 21:47:48, Mark P wrote: > Shouldn't you have the override keyword here? Done.
components/omnibox/browser/physical_web_provider_unittest.cc lgtm
The CQ bit was checked by hayesjordan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from olivierrobin@chromium.org, mattreynolds@chromium.org Link to the patchset: https://codereview.chromium.org/2231983002/#ps220001 (title: "Address Mark's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by hayesjordan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org, mpearson@chromium.org, olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/2231983002/#ps240001 (title: "Fix build failure")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by hayesjordan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org, mpearson@chromium.org, olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/2231983002/#ps260001 (title: "Fix build errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Expose Physical Web data The goal is to create an API for pushing changes of Physical Web data, such as new URLs found, so that other parties can react to the change without constantly pulling the data. To do this we are providing an API that allows others to register a listener for push updates when data changes. BUG=636490 ========== to ========== Expose Physical Web data The goal is to create an API for pushing changes of Physical Web data, such as new URLs found, so that other parties can react to the change without constantly pulling the data. To do this we are providing an API that allows others to register a listener for push updates when data changes. BUG=636490 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Expose Physical Web data The goal is to create an API for pushing changes of Physical Web data, such as new URLs found, so that other parties can react to the change without constantly pulling the data. To do this we are providing an API that allows others to register a listener for push updates when data changes. BUG=636490 ========== to ========== Expose Physical Web data The goal is to create an API for pushing changes of Physical Web data, such as new URLs found, so that other parties can react to the change without constantly pulling the data. To do this we are providing an API that allows others to register a listener for push updates when data changes. BUG=636490 Committed: https://crrev.com/1098aecd3818cf9eef26cfa77aed4ae51f6621c6 Cr-Commit-Position: refs/heads/master@{#413948} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/1098aecd3818cf9eef26cfa77aed4ae51f6621c6 Cr-Commit-Position: refs/heads/master@{#413948} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
