|
|
DescriptionWebUI: Add helper FireWebUIListener method on WebUIMessageHandler.
This will eliminate repetition of "cr.webUIListenerCallback" string literal as follows:
Before:
CallJavascriptFunction("cr.webUIListenerCallback",
base::Value("some-event-name"),
*arg1, *arg2);
After:
FireWebUIListener("some-event-name", *arg1, *arg2);
BUG=None
Review-Url: https://codereview.chromium.org/2890523003
Cr-Commit-Position: refs/heads/master@{#473260}
Committed: https://chromium.googlesource.com/chromium/src/+/9d8cb77dbeb6a4f437ff50b6d0c3f8f40672a0a9
Patch Set 1 #Patch Set 2 : Nit #Patch Set 3 : Add example conversion. #
Total comments: 2
Patch Set 4 : Rename #
Total comments: 5
Patch Set 5 : Address comment. #
Messages
Total messages: 41 (22 generated)
The CQ bit was checked by dpapad@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...
Description was changed from ========== WebUI: Add helper FireJavascript event method, on WebUIMessageHandler. BUG=None ========== to ========== WebUI: Add helper FireJavascript event method, on WebUIMessageHandler. This helper will allow reducing duplication of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireJavascriptEvent("some-event-name", *arg1, *arg2); BUG=None ==========
Description was changed from ========== WebUI: Add helper FireJavascript event method, on WebUIMessageHandler. This helper will allow reducing duplication of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireJavascriptEvent("some-event-name", *arg1, *arg2); BUG=None ========== to ========== WebUI: Add helper FireJavascript event method, on WebUIMessageHandler. This helper will allow reducing duplication of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireJavascriptEvent("some-event-name", *arg1, *arg2); BUG=None ==========
Description was changed from ========== WebUI: Add helper FireJavascript event method, on WebUIMessageHandler. This helper will allow reducing duplication of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireJavascriptEvent("some-event-name", *arg1, *arg2); BUG=None ========== to ========== WebUI: Add helper FireJavascript event method, on WebUIMessageHandler. This helper will eliminate duplication of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireJavascriptEvent("some-event-name", *arg1, *arg2); BUG=None ==========
Description was changed from ========== WebUI: Add helper FireJavascript event method, on WebUIMessageHandler. This helper will eliminate duplication of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireJavascriptEvent("some-event-name", *arg1, *arg2); BUG=None ========== to ========== WebUI: Add helper FireJavascriptEvent method, on WebUIMessageHandler. This helper will eliminate duplication of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireJavascriptEvent("some-event-name", *arg1, *arg2); BUG=None ==========
The CQ bit was checked by dpapad@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 checked by dpapad@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...
Description was changed from ========== WebUI: Add helper FireJavascriptEvent method, on WebUIMessageHandler. This helper will eliminate duplication of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireJavascriptEvent("some-event-name", *arg1, *arg2); BUG=None ========== to ========== WebUI: Add helper FireJavascriptEvent method on WebUIMessageHandler. This helper will eliminate duplication of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireJavascriptEvent("some-event-name", *arg1, *arg2); BUG=None ==========
Description was changed from ========== WebUI: Add helper FireJavascriptEvent method on WebUIMessageHandler. This helper will eliminate duplication of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireJavascriptEvent("some-event-name", *arg1, *arg2); BUG=None ========== to ========== WebUI: Add helper FireJavascriptEvent method on WebUIMessageHandler. This will eliminate repetition of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireJavascriptEvent("some-event-name", *arg1, *arg2); BUG=None ==========
dpapad@chromium.org changed reviewers: + dbeam@chromium.org, sky@chromium.org
I only converted one usage in this CL as an example, see [1] for a list of places where the new helper would be used. [1] https://cs.chromium.org/search/?q=cr.webUIListenerCallback+file:%5Esrc/chrome...
https://codereview.chromium.org/2890523003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/search_engines_handler.cc (right): https://codereview.chromium.org/2890523003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/search_engines_handler.cc:199: FireJavascriptEvent("search-engines-changed", *GetSearchEnginesList()); what about ListenerCallback("search-engines-changed", *GetSearchEnginesList()); ?
otherwise I think this is a grand idea!
https://codereview.chromium.org/2890523003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/search_engines_handler.cc (right): https://codereview.chromium.org/2890523003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/search_engines_handler.cc:199: FireJavascriptEvent("search-engines-changed", *GetSearchEnginesList()); On 2017/05/16 at 22:33:38, Dan Beam wrote: > what about > > ListenerCallback("search-engines-changed", *GetSearchEnginesList()); > > ? I attempted to keep the naming in the same spirit as existing naming. Call-Javascript-Function Fire-Javascript-Event I think the presence of "Javascript" in the name makes it clearer that this function ends up invoking an action on the JS side. Also |ListenerCallback| sounds more like a member variable than a member method (there is no verb in the name to indicate what action is being taken). How about FireWebUIListener(), which matches the other side of this equation, cr.addWebUIListener() ?
On 2017/05/16 22:42:14, dpapad wrote: > https://codereview.chromium.org/2890523003/diff/40001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/settings/search_engines_handler.cc (right): > > https://codereview.chromium.org/2890523003/diff/40001/chrome/browser/ui/webui... > chrome/browser/ui/webui/settings/search_engines_handler.cc:199: > FireJavascriptEvent("search-engines-changed", *GetSearchEnginesList()); > On 2017/05/16 at 22:33:38, Dan Beam wrote: > > what about > > > > ListenerCallback("search-engines-changed", *GetSearchEnginesList()); > > > > ? > > I attempted to keep the naming in the same spirit as existing naming. > Call-Javascript-Function > Fire-Javascript-Event > > I think the presence of "Javascript" in the name makes it clearer that this > function ends up invoking an action on the JS side. Also |ListenerCallback| > sounds more like a member variable than a member method (there is no verb in the > name to indicate what action is being taken). > > How about FireWebUIListener(), which matches the other side of this equation, > cr.addWebUIListener() ? FireWebUIListener sgtm
Description was changed from ========== WebUI: Add helper FireJavascriptEvent method on WebUIMessageHandler. This will eliminate repetition of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireJavascriptEvent("some-event-name", *arg1, *arg2); BUG=None ========== to ========== WebUI: Add helper FireWebUIListener method on WebUIMessageHandler. This will eliminate repetition of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireWebUIListener("some-event-name", *arg1, *arg2); BUG=None ==========
> FireWebUIListener sgtm Done.
On 2017/05/16 at 23:42:58, dpapad wrote: > > FireWebUIListener sgtm > > Done. FYI, follow up https://codereview.chromium.org/2886083002 replaces all usages of CallJavascriptFunction("cr.webUIListenerCallback", ...). Net savings: 84 LOC of duplicated code.
On 2017/05/17 at 01:00:31, dpapad wrote: > On 2017/05/16 at 23:42:58, dpapad wrote: > > > FireWebUIListener sgtm > > > > Done. > > FYI, follow up https://codereview.chromium.org/2886083002 replaces all usages of CallJavascriptFunction("cr.webUIListenerCallback", ...). Net savings: 84 LOC of duplicated code. Friendly ping.
https://codereview.chromium.org/2890523003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/search_engines_handler.cc (right): https://codereview.chromium.org/2890523003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/search_engines_handler.cc:519: } no curlies https://codereview.chromium.org/2890523003/diff/60001/content/public/browser/... File content/public/browser/web_ui_message_handler.h (right): https://codereview.chromium.org/2890523003/diff/60001/content/public/browser/... content/public/browser/web_ui_message_handler.h:97: void FireWebUIListener(const std::string& event_name, why do you have to implement this in the .h file?
https://codereview.chromium.org/2890523003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/search_engines_handler.cc (right): https://codereview.chromium.org/2890523003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/search_engines_handler.cc:519: } On 2017/05/18 at 00:08:44, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/2890523003/diff/60001/content/public/browser/... File content/public/browser/web_ui_message_handler.h (right): https://codereview.chromium.org/2890523003/diff/60001/content/public/browser/... content/public/browser/web_ui_message_handler.h:97: void FireWebUIListener(const std::string& event_name, On 2017/05/18 at 00:08:44, Dan Beam wrote: > why do you have to implement this in the .h file? I tried moving the implementation to the .cc file, but did not succeed, throws a linker error (I am either doing something wrong, or it is not possible), here is the diff I tried https://pastebin.com/cKcvQZje. Also looked for examples at https://cs.chromium.org/search/?q=%22typename...%22&p=3&type=cs, and almost all search results are implementing such function within the header files (with the exceptions of some test.cc files which don't have a corresponding header file).
https://codereview.chromium.org/2890523003/diff/60001/content/public/browser/... File content/public/browser/web_ui_message_handler.h (right): https://codereview.chromium.org/2890523003/diff/60001/content/public/browser/... content/public/browser/web_ui_message_handler.h:97: void FireWebUIListener(const std::string& event_name, On 2017/05/18 00:41:32, dpapad wrote: > On 2017/05/18 at 00:08:44, Dan Beam wrote: > > why do you have to implement this in the .h file? > > I tried moving the implementation to the .cc file, but did not succeed, throws a > linker error (I am either doing something wrong, or it is not possible), here is > the diff I tried https://pastebin.com/cKcvQZje. > > Also looked for examples at > https://cs.chromium.org/search/?q=%22typename...%22&p=3&type=cs, and almost all > search results are implementing such function within the header files (with the > exceptions of some test.cc files which don't have a corresponding header file). aight, lgtm until a wizard tells us how to do this
sky@chromium.org changed reviewers: + jam@chromium.org - sky@chromium.org
I'm not an owner here. sky->jam (content change)
lgtm
The CQ bit was checked by dpapad@chromium.org
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dpapad@chromium.org
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dpapad@chromium.org
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": 80001, "attempt_start_ts": 1495213101605650, "parent_rev": "c39c757894f7a9a68a4ed4bf9942f94eece2b6a0", "commit_rev": "9d8cb77dbeb6a4f437ff50b6d0c3f8f40672a0a9"}
Message was sent while issue was closed.
Description was changed from ========== WebUI: Add helper FireWebUIListener method on WebUIMessageHandler. This will eliminate repetition of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireWebUIListener("some-event-name", *arg1, *arg2); BUG=None ========== to ========== WebUI: Add helper FireWebUIListener method on WebUIMessageHandler. This will eliminate repetition of "cr.webUIListenerCallback" string literal as follows: Before: CallJavascriptFunction("cr.webUIListenerCallback", base::Value("some-event-name"), *arg1, *arg2); After: FireWebUIListener("some-event-name", *arg1, *arg2); BUG=None Review-Url: https://codereview.chromium.org/2890523003 Cr-Commit-Position: refs/heads/master@{#473260} Committed: https://chromium.googlesource.com/chromium/src/+/9d8cb77dbeb6a4f437ff50b6d0c3... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/9d8cb77dbeb6a4f437ff50b6d0c3... |