Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2571853003:
Move PW message handler logic to components (Closed)
DescriptionMove PW message handler logic to components
We need a Physical Web WebUI on both iOS and Android. In order to
support both and share code, we need to place all core message handler
logic in components.
BUG=663842
Committed: https://crrev.com/c07a45d17ce723ac166020b5c108448baf765035
Cr-Commit-Position: refs/heads/master@{#440989}
Patch Set 1 #Patch Set 2 : Restore url_constants.h include #
Total comments: 10
Patch Set 3 : Limit ourselves to the sadness that is single inheritance #
Total comments: 31
Patch Set 4 : Address style/syntax comments #
Total comments: 16
Patch Set 5 : More style/syntax comments #Patch Set 6 : Fix a handful of compilation errors #Patch Set 7 : Use default destructor #Patch Set 8 : Set WebUI at handler Registration time #
Messages
Total messages: 46 (28 generated)
cco3@chromium.org changed reviewers: + mattreynolds@chromium.org
Description was changed from ========== Move PW message handler logic to components We need a Physical Web WebUI on both iOS and Android. In order to support both and share code, we need to place that logic in components. BUG=663842 ========== to ========== Move PW message handler logic to components We need a Physical Web WebUI on both iOS and Android. In order to support both and share code, we need to place all core message handler logic in components. BUG=663842 ==========
https://codereview.chromium.org/2571853003/diff/20001/components/physical_web... File components/physical_web/webui/physical_web_base_message_handler.cc (right): https://codereview.chromium.org/2571853003/diff/20001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.cc:29: base::Bind(&PhysicalWebBaseMessageHandler::HandleRequestNearbyURLs, src/docs/callback.md suggests we should use base::BindRepeating since base::Bind is due to be deprecated. https://codereview.chromium.org/2571853003/diff/20001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.cc:68: if (index > kSelectedPositionMaxValue) { Turns out this is incorrect, UMA_HISTOGRAM_EXACT_LINEAR does have overflow and underflow buckets so this check is unnecessary. Let's remove it. https://codereview.chromium.org/2571853003/diff/20001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.cc:72: kSelectedPositionMaxValue); Let's remove the kSelectedPositionMaxValue constant and just put the max value inline. https://codereview.chromium.org/2571853003/diff/20001/ios/chrome/browser/ui/w... File ios/chrome/browser/ui/webui/physical_web_ui.cc (right): https://codereview.chromium.org/2571853003/diff/20001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:48: public physical_web_ui::PhysicalWebBaseMessageHandler { Multiple inheritance is allowed only when all superclasses, with the possible exception of the first one, are pure interfaces. (go/cppstyle) https://codereview.chromium.org/2571853003/diff/20001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:63: web_ui()->CallJavascriptFunctionUnsafe(function, arg); Why does this need to call the Unsafe version?
https://codereview.chromium.org/2571853003/diff/20001/components/physical_web... File components/physical_web/webui/physical_web_base_message_handler.cc (right): https://codereview.chromium.org/2571853003/diff/20001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.cc:29: base::Bind(&PhysicalWebBaseMessageHandler::HandleRequestNearbyURLs, On 2016/12/13 22:40:13, mattreynolds wrote: > src/docs/callback.md suggests we should use base::BindRepeating since base::Bind > is due to be deprecated. Done. https://codereview.chromium.org/2571853003/diff/20001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.cc:68: if (index > kSelectedPositionMaxValue) { On 2016/12/13 22:40:13, mattreynolds wrote: > Turns out this is incorrect, UMA_HISTOGRAM_EXACT_LINEAR does have overflow and > underflow buckets so this check is unnecessary. Let's remove it. Done. https://codereview.chromium.org/2571853003/diff/20001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.cc:72: kSelectedPositionMaxValue); On 2016/12/13 22:40:13, mattreynolds wrote: > Let's remove the kSelectedPositionMaxValue constant and just put the max value > inline. Done. https://codereview.chromium.org/2571853003/diff/20001/ios/chrome/browser/ui/w... File ios/chrome/browser/ui/webui/physical_web_ui.cc (right): https://codereview.chromium.org/2571853003/diff/20001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:48: public physical_web_ui::PhysicalWebBaseMessageHandler { On 2016/12/13 22:40:13, mattreynolds wrote: > Multiple inheritance is allowed only when all superclasses, with the possible > exception of the first one, are pure interfaces. (go/cppstyle) Done. https://codereview.chromium.org/2571853003/diff/20001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:63: web_ui()->CallJavascriptFunctionUnsafe(function, arg); On 2016/12/13 22:40:13, mattreynolds wrote: > Why does this need to call the Unsafe version? Done.
lgtm
cco3@chromium.org changed reviewers: + jyquinn@chromium.org
Hi jyquinn@, would you be able to review this ios/chrome/browser/ui/webui/ file?
jyquinn@chromium.org changed reviewers: + eugenebut@chromium.org
I'm no longer an owner, so I'm not the review you're looking for. Added eugenebut
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This change does not compile on iOS https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... File ios/chrome/browser/ui/webui/physical_web_ui.cc (right): https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:39: class MessageHandlerImpl : What is the reason for creating this class here? It just a thin wrapper around WebUIIOS which does not add any functionality. If this is a part of refactoring it would be nice if I could read somewhere about it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
On 2016/12/22 16:09:23, Eugene But wrote: > This change does not compile on iOS Sorry, Matt will be able to take a look at that when he gets back. > > https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... > File ios/chrome/browser/ui/webui/physical_web_ui.cc (right): > > https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... > ios/chrome/browser/ui/webui/physical_web_ui.cc:39: class MessageHandlerImpl : > What is the reason for creating this class here? It just a thin wrapper around > WebUIIOS which does not add any functionality. If this is a part of refactoring > it would be nice if I could read somewhere about it. It's so that we can share our core message handler logic with our Android code (physical web specific).
On 2016/12/22 17:52:39, cco3 wrote: > On 2016/12/22 16:09:23, Eugene But wrote: > > This change does not compile on iOS > > Sorry, Matt will be able to take a look at that when he gets back. > > > > > > https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... > > File ios/chrome/browser/ui/webui/physical_web_ui.cc (right): > > > > > https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... > > ios/chrome/browser/ui/webui/physical_web_ui.cc:39: class MessageHandlerImpl : > > What is the reason for creating this class here? It just a thin wrapper around > > WebUIIOS which does not add any functionality. If this is a part of > refactoring > > it would be nice if I could read somewhere about it. > > It's so that we can share our core message handler logic with our Android code > (physical web specific). Do you have a doc where I can read about sharing approach? MessageHandlerImpl has dependency on iOS only class and I don't know how this class could help us with code sharing.
On 2016/12/22 18:42:05, Eugene But wrote: > On 2016/12/22 17:52:39, cco3 wrote: > > On 2016/12/22 16:09:23, Eugene But wrote: > > > This change does not compile on iOS > > > > Sorry, Matt will be able to take a look at that when he gets back. > > > > > > > > > > > https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... > > > File ios/chrome/browser/ui/webui/physical_web_ui.cc (right): > > > > > > > > > https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... > > > ios/chrome/browser/ui/webui/physical_web_ui.cc:39: class MessageHandlerImpl > : > > > What is the reason for creating this class here? It just a thin wrapper > around > > > WebUIIOS which does not add any functionality. If this is a part of > > refactoring > > > it would be nice if I could read somewhere about it. > > > > It's so that we can share our core message handler logic with our Android code > > (physical web specific). > Do you have a doc where I can read about sharing approach? MessageHandlerImpl > has dependency on iOS only class and I don't know how this class could help us > with code sharing. Sorry, there's no doc. The idea is that Android would have its own MessageHandlerImpl that extends PhysicalWebBaseMessageHandler (that is the shared code).
Thanks for explanation. https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... File components/physical_web/webui/physical_web_base_message_handler.cc (right): https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.cc:41: base::DictionaryValue* metadata_item = NULL; nit: s/NULL/nullptr https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.cc:55: int index; Please initialize https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... File components/physical_web/webui/physical_web_base_message_handler.h (right): https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:5: #ifndef COMPONENTS_PHYSICAL_WEB_WEBUI_PHYSICAL_WEB_BASE_MESSAGE_HANDLER COMPONENTS_PHYSICAL_WEB_WEBUI_PHYSICAL_WEB_BASE_MESSAGE_HANDLER_H_ https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:24: // (Does not implement WebUIMessageHandler or register its methods) nit: Please finish comment with full stop https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:30: void HandleRequestNearbyURLs(const base::ListValue* args); Please add comments to the methods. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:34: void RegisterMessagesImpl(); nit: Do you need an Impl suffix here? https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:34: void RegisterMessagesImpl(); Should this function be protected or private? https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:40: virtual physical_web::PhysicalWebDataSource* GetPhysicalWebDataSource() = 0; Should this be a const method? https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... File ios/chrome/browser/ui/webui/physical_web_ui.cc (right): https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:39: class MessageHandlerImpl : Please add comment to this class. https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:42: MessageHandlerImpl(WebUIIOS* web_ui) { explicit https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:43: web_ui_ = web_ui; Please use initialization list. https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:47: private private: https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:72: class PhysicalWebDOMHandler : public web::WebUIIOSMessageHandler { s/{/, https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:75: PhysicalWebDOMHandler() : impl(web_ui()) {} You need an ivar for impl
On 2016/12/22 19:04:47, Eugene But wrote: > Thanks for explanation. > > https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... > File components/physical_web/webui/physical_web_base_message_handler.cc (right): > > https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... > components/physical_web/webui/physical_web_base_message_handler.cc:41: > base::DictionaryValue* metadata_item = NULL; > nit: s/NULL/nullptr > > https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... > components/physical_web/webui/physical_web_base_message_handler.cc:55: int > index; > Please initialize > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... > File components/physical_web/webui/physical_web_base_message_handler.h (right): > > https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... > components/physical_web/webui/physical_web_base_message_handler.h:5: #ifndef > COMPONENTS_PHYSICAL_WEB_WEBUI_PHYSICAL_WEB_BASE_MESSAGE_HANDLER > COMPONENTS_PHYSICAL_WEB_WEBUI_PHYSICAL_WEB_BASE_MESSAGE_HANDLER_H_ > > https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... > components/physical_web/webui/physical_web_base_message_handler.h:24: // (Does > not implement WebUIMessageHandler or register its methods) > nit: Please finish comment with full stop > > https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... > components/physical_web/webui/physical_web_base_message_handler.h:30: void > HandleRequestNearbyURLs(const base::ListValue* args); > Please add comments to the methods. > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... > components/physical_web/webui/physical_web_base_message_handler.h:34: void > RegisterMessagesImpl(); > nit: Do you need an Impl suffix here? > > https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... > components/physical_web/webui/physical_web_base_message_handler.h:34: void > RegisterMessagesImpl(); > Should this function be protected or private? > > https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... > components/physical_web/webui/physical_web_base_message_handler.h:40: virtual > physical_web::PhysicalWebDataSource* GetPhysicalWebDataSource() = 0; > Should this be a const method? > > https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... > File ios/chrome/browser/ui/webui/physical_web_ui.cc (right): > > https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... > ios/chrome/browser/ui/webui/physical_web_ui.cc:39: class MessageHandlerImpl : > Please add comment to this class. > > https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... > ios/chrome/browser/ui/webui/physical_web_ui.cc:42: MessageHandlerImpl(WebUIIOS* > web_ui) { > explicit > > https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... > ios/chrome/browser/ui/webui/physical_web_ui.cc:43: web_ui_ = web_ui; > Please use initialization list. > > https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... > ios/chrome/browser/ui/webui/physical_web_ui.cc:47: private > private: > > https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... > ios/chrome/browser/ui/webui/physical_web_ui.cc:72: class PhysicalWebDOMHandler : > public web::WebUIIOSMessageHandler { > s/{/, > > https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... > ios/chrome/browser/ui/webui/physical_web_ui.cc:75: PhysicalWebDOMHandler() : > impl(web_ui()) {} > You need an ivar for impl I'm very sorry, I didn't mean for you to have to give basic syntax pointers. I meant to have someone with an iOS development setup try this first before sending it off to you. Anyway, thank you, I've made the changes you've suggested.
https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... File components/physical_web/webui/physical_web_base_message_handler.cc (right): https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.cc:41: base::DictionaryValue* metadata_item = NULL; On 2016/12/22 19:04:47, Eugene But wrote: > nit: s/NULL/nullptr Done. https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.cc:55: int index; On 2016/12/22 19:04:47, Eugene But wrote: > Please initialize > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Done. https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... File components/physical_web/webui/physical_web_base_message_handler.h (right): https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:5: #ifndef COMPONENTS_PHYSICAL_WEB_WEBUI_PHYSICAL_WEB_BASE_MESSAGE_HANDLER On 2016/12/22 19:04:47, Eugene But wrote: > COMPONENTS_PHYSICAL_WEB_WEBUI_PHYSICAL_WEB_BASE_MESSAGE_HANDLER_H_ Done. https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:24: // (Does not implement WebUIMessageHandler or register its methods) On 2016/12/22 19:04:47, Eugene But wrote: > nit: Please finish comment with full stop Done. https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:30: void HandleRequestNearbyURLs(const base::ListValue* args); On 2016/12/22 19:04:47, Eugene But wrote: > Please add comments to the methods. > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Done. https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:34: void RegisterMessagesImpl(); On 2016/12/22 19:04:47, Eugene But wrote: > nit: Do you need an Impl suffix here? Not any more. https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:34: void RegisterMessagesImpl(); On 2016/12/22 19:04:47, Eugene But wrote: > Should this function be protected or private? Done. https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:40: virtual physical_web::PhysicalWebDataSource* GetPhysicalWebDataSource() = 0; On 2016/12/22 19:04:47, Eugene But wrote: > Should this be a const method? Done. https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... File ios/chrome/browser/ui/webui/physical_web_ui.cc (right): https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:39: class MessageHandlerImpl : On 2016/12/22 16:09:22, Eugene But wrote: > What is the reason for creating this class here? It just a thin wrapper around > WebUIIOS which does not add any functionality. If this is a part of refactoring > it would be nice if I could read somewhere about it. Acknowledged. https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:39: class MessageHandlerImpl : On 2016/12/22 19:04:47, Eugene But wrote: > Please add comment to this class. Done. https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:42: MessageHandlerImpl(WebUIIOS* web_ui) { On 2016/12/22 19:04:47, Eugene But wrote: > explicit Done. https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:43: web_ui_ = web_ui; On 2016/12/22 19:04:47, Eugene But wrote: > Please use initialization list. Done. https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:47: private On 2016/12/22 19:04:47, Eugene But wrote: > private: Done. https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:72: class PhysicalWebDOMHandler : public web::WebUIIOSMessageHandler { On 2016/12/22 19:04:47, Eugene But wrote: > s/{/, Done. https://codereview.chromium.org/2571853003/diff/40001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:75: PhysicalWebDOMHandler() : impl(web_ui()) {} On 2016/12/22 19:04:47, Eugene But wrote: > You need an ivar for impl Done.
No worries. lgtm with changes :) https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... File components/physical_web/webui/physical_web_base_message_handler.h (right): https://codereview.chromium.org/2571853003/diff/40001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:40: virtual physical_web::PhysicalWebDataSource* GetPhysicalWebDataSource() = 0; On 2016/12/22 20:14:45, cco3 wrote: > On 2016/12/22 19:04:47, Eugene But wrote: > > Should this be a const method? > > Done. Sorry, I meant "const method", which is: virtual physical_web::PhysicalWebDataSource* GetPhysicalWebDataSource() const = 0; https://codereview.chromium.org/2571853003/diff/60001/components/physical_web... File components/physical_web/webui/physical_web_base_message_handler.cc (right): https://codereview.chromium.org/2571853003/diff/60001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.cc:19: void PhysicalWebBaseMessageHandler::RegisterMessagesImpl() { RegisterMessages? https://codereview.chromium.org/2571853003/diff/60001/components/physical_web... File components/physical_web/webui/physical_web_base_message_handler.h (right): https://codereview.chromium.org/2571853003/diff/60001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:30: // This method handles the RequestNearbyURLs message, returning URLs that are Optional nit: How about s/This method handles/Handles https://codereview.chromium.org/2571853003/diff/60001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:34: // This method handles a click on a Physical Web URL, recording the click and ditto https://codereview.chromium.org/2571853003/diff/60001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:38: // Register the messages that this MessageHandler can handle. s/Register/Registers https://codereview.chromium.org/2571853003/diff/60001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:42: virtual void RegisterMessageCallback( Do you want to add comments for protected methods as well and explain what do you expect from subclasses? https://codereview.chromium.org/2571853003/diff/60001/ios/chrome/browser/ui/w... File ios/chrome/browser/ui/webui/physical_web_ui.cc (left): https://codereview.chromium.org/2571853003/diff/60001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:107: // Record the index of the selected item. The index must be strictly less than This code was removed. Is it ok? https://codereview.chromium.org/2571853003/diff/60001/ios/chrome/browser/ui/w... File ios/chrome/browser/ui/webui/physical_web_ui.cc (right): https://codereview.chromium.org/2571853003/diff/60001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:39: // The mixin for the MessageHandler that implements all MessageHandler nit: Is this actually mix-in? C++ mix-ins are usually template classes which inherit from their template class, so they can mix any functionality into a class. This class does not add any functionality, it's just a an implementation of physical_web_ui::PhysicalWebBaseMessageHandler abstract class. https://codereview.chromium.org/2571853003/diff/60001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:41: // implements functions to manipulate an iOS-specific WebUi object. nit: s/WebUi/WebUI
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: 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
Dry run: 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...)
https://codereview.chromium.org/2571853003/diff/60001/components/physical_web... File components/physical_web/webui/physical_web_base_message_handler.cc (right): https://codereview.chromium.org/2571853003/diff/60001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.cc:19: void PhysicalWebBaseMessageHandler::RegisterMessagesImpl() { On 2016/12/22 21:26:32, Eugene But wrote: > RegisterMessages? Done. https://codereview.chromium.org/2571853003/diff/60001/components/physical_web... File components/physical_web/webui/physical_web_base_message_handler.h (right): https://codereview.chromium.org/2571853003/diff/60001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:30: // This method handles the RequestNearbyURLs message, returning URLs that are On 2016/12/22 21:26:32, Eugene But wrote: > Optional nit: How about s/This method handles/Handles Done. https://codereview.chromium.org/2571853003/diff/60001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:34: // This method handles a click on a Physical Web URL, recording the click and On 2016/12/22 21:26:33, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2571853003/diff/60001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:38: // Register the messages that this MessageHandler can handle. On 2016/12/22 21:26:32, Eugene But wrote: > s/Register/Registers Done. https://codereview.chromium.org/2571853003/diff/60001/components/physical_web... components/physical_web/webui/physical_web_base_message_handler.h:42: virtual void RegisterMessageCallback( On 2016/12/22 21:26:32, Eugene But wrote: > Do you want to add comments for protected methods as well and explain what do > you expect from subclasses? Done. https://codereview.chromium.org/2571853003/diff/60001/ios/chrome/browser/ui/w... File ios/chrome/browser/ui/webui/physical_web_ui.cc (left): https://codereview.chromium.org/2571853003/diff/60001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:107: // Record the index of the selected item. The index must be strictly less than On 2016/12/22 21:26:33, Eugene But wrote: > This code was removed. Is it ok? Good catch, but yes. Matt made a comment on an earlier patchset, explaining that the test is not needed. https://codereview.chromium.org/2571853003/diff/60001/ios/chrome/browser/ui/w... File ios/chrome/browser/ui/webui/physical_web_ui.cc (right): https://codereview.chromium.org/2571853003/diff/60001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:39: // The mixin for the MessageHandler that implements all MessageHandler On 2016/12/22 21:26:33, Eugene But wrote: > nit: Is this actually mix-in? C++ mix-ins are usually template classes which > inherit from their template class, so they can mix any functionality into a > class. This class does not add any functionality, it's just a an implementation > of physical_web_ui::PhysicalWebBaseMessageHandler abstract class. Sorry, I wasn't sure if that term translated over to C++ (where there is multiple inheritance). Originally PhysicalWebBaseMessageHandler *was* a class to be used as a second base class with multiple inheritance, but I was told that was frowned upon. https://codereview.chromium.org/2571853003/diff/60001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/physical_web_ui.cc:41: // implements functions to manipulate an iOS-specific WebUi object. On 2016/12/22 21:26:33, Eugene But wrote: > nit: s/WebUi/WebUI Done.
The CQ bit was checked by cco3@chromium.org to run a CQ dry run
Dry run: 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
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...)
The CQ bit was checked by cco3@chromium.org to run a CQ dry run
Dry run: 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
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...)
The CQ bit was checked by cco3@chromium.org to run a CQ dry run
Dry run: 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 cco3@chromium.org
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2571853003/#ps140001 (title: "Set WebUI at handler Registration time")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483040779882400,
"parent_rev": "483075d9d079f66c957dc09ea0152cafec853cb7", "commit_rev":
"4ab6fb63014583bff8d9e2c5dc37233d7f0a2945"}
Message was sent while issue was closed.
Description was changed from ========== Move PW message handler logic to components We need a Physical Web WebUI on both iOS and Android. In order to support both and share code, we need to place all core message handler logic in components. BUG=663842 ========== to ========== Move PW message handler logic to components We need a Physical Web WebUI on both iOS and Android. In order to support both and share code, we need to place all core message handler logic in components. BUG=663842 Review-Url: https://codereview.chromium.org/2571853003 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Move PW message handler logic to components We need a Physical Web WebUI on both iOS and Android. In order to support both and share code, we need to place all core message handler logic in components. BUG=663842 Review-Url: https://codereview.chromium.org/2571853003 ========== to ========== Move PW message handler logic to components We need a Physical Web WebUI on both iOS and Android. In order to support both and share code, we need to place all core message handler logic in components. BUG=663842 Committed: https://crrev.com/c07a45d17ce723ac166020b5c108448baf765035 Cr-Commit-Position: refs/heads/master@{#440989} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c07a45d17ce723ac166020b5c108448baf765035 Cr-Commit-Position: refs/heads/master@{#440989} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
