Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
948243005:
Merge custom bindings for context menus (Closed)
|
Created:
5 years, 10 months ago by robwu Modified:
5 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMerge custom bindings for context menus
De-duplicated the logic for contextMenus custom bindings.
And changed the contextMenus.update method, so that calling it with
"onclick: null" will remove the onclick handler.
BUG=461869
TEST=browser_tests ExtensionContextMenuBrowserTest.UpdateOnclick
Committed: https://crrev.com/6e3c0446a84339e95996e9cafd6e5e72c0698827
Cr-Commit-Position: refs/heads/master@{#323880}
Patch Set 1 #Patch Set 2 : $Array.unshift -> $Array.concat #
Total comments: 27
Patch Set 3 : Nits and tests #
Total comments: 8
Patch Set 4 : Address all comments up to #9 #
Total comments: 2
Patch Set 5 : isWebview=true -> false #Patch Set 6 : rebase #Patch Set 7 : Fix ExtensionContextMenuBrowserTest.UpdateOnclick test #Messages
Total messages: 29 (9 generated)
rob@robwu.nl changed reviewers: + lazyboy@chromium.org
lazyboy@chromium.org changed reviewers: + kalman@chromium.org
Hi robwu, thanks for your patch.
I've partially looked at it and got stuck on the newly added
file, I have one question about the arbitrary js object
return:
return {
requestHandlers: requestHandlers,
callbacks: callbacks
};
in extensions/renderer/resources/context_menus_handlers.js
kalman@ can you take a quick look at my last comment in that
file? Thanks.
https://chromiumcodereview.appspot.com/948243005/diff/20001/chrome/renderer/r...
File
chrome/renderer/resources/extensions/chrome_web_view_internal_custom_bindings.js
(right):
https://chromiumcodereview.appspot.com/948243005/diff/20001/chrome/renderer/r...
chrome/renderer/resources/extensions/chrome_web_view_internal_custom_bindings.js:11:
var impl = createContextMenusHandlers(/* isWebview = */ true);
nit: the most followed pattern here is:
createContextMenusHandlers(true /* isWebView */);
https://chromiumcodereview.appspot.com/948243005/diff/20001/extensions/render...
File extensions/renderer/resources/context_menus_handlers.js (right):
https://chromiumcodereview.appspot.com/948243005/diff/20001/extensions/render...
extensions/renderer/resources/context_menus_handlers.js:5: // Implementation of
custom bindings for the contextMenus API.
A little bit detail here would help.
Note that we use this for chrome contextMenus API and
<webview> contextMenus API.
Note that webviewInstanceId in below would be unset/set to 0/-1 for
the former.
https://chromiumcodereview.appspot.com/948243005/diff/20001/extensions/render...
extensions/renderer/resources/context_menus_handlers.js:16: var
INSTANCEID_NONWEBVIEW = -1;
Most places in the code base refers to this as kInstanceIDNone, e.g:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
webview's contextmenus api uses "viewInstanceID" generated from
IdGeneratorCustomBindings, so 0 is invalid id as well, so change it to:
var WEBVIEW_INSTANCE_ID_NONE = 0;
https://chromiumcodereview.appspot.com/948243005/diff/20001/extensions/render...
extensions/renderer/resources/context_menus_handlers.js:120: // When onclick is
explicitly set to null, remove the event listener.
Great, can you file a bug for this and add it to to "BUG=" in CL description.
Also this probably makes sense to fix on a CL on it's own.
https://chromiumcodereview.appspot.com/948243005/diff/20001/extensions/render...
extensions/renderer/resources/context_menus_handlers.js:129: return {
This is a bit brittle, the json schema compiler does not
enforce any check on this returned object, which might be a bit
worrying. This is one reason I didn't refactor this code
like this (I thought about it) in the beginning, maybe it's
OK. kalman@ can probably chime in here.
A few replies to your review. I'll make the code changes after a few more cycles of feedback. https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... File extensions/renderer/resources/context_menus_handlers.js (right): https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:16: var INSTANCEID_NONWEBVIEW = -1; On 2015/02/25 17:58:42, lazyboy wrote: > Most places in the code base refers to this as kInstanceIDNone, e.g: > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... > > webview's contextmenus api uses "viewInstanceID" generated from > IdGeneratorCustomBindings, so 0 is invalid id as well, so change it to: > var WEBVIEW_INSTANCE_ID_NONE = 0; I've picked -1 because "0" could be a value associated with the webview API. I based this assumption on https://cs.chromium.org/file:chrome_web_view_experimental.js%20%22opt_webView.... I really want to convey that the key is not related to webviews at all, including "invalid webview" (which is different from "not a webview"). https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:120: // When onclick is explicitly set to null, remove the event listener. On 2015/02/25 17:58:42, lazyboy wrote: > Great, can you file a bug for this and add it to to "BUG=" in CL description. > > Also this probably makes sense to fix on a CL on it's own. Done. https://crbug.com/461869 https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:129: return { On 2015/02/25 17:58:42, lazyboy wrote: > This is a bit brittle, the json schema compiler does not > enforce any check on this returned object, which might be a bit > worrying. This is one reason I didn't refactor this code > like this (I thought about it) in the beginning, maybe it's > OK. kalman@ can probably chime in here. This return value is not validated at all. This method is just a factory used by the other two files. Is the extension contextMenus API significantly different from the webview's? E.g. if I were to add a hypothethical tabId to show contextMenus in a certain tab only, what would be the effect on the implementation in JS/C++? The current implementation in webview looks complex (chrome_web_view_experimental.js). Why can't we create a contextMenus binding factory that takes an instanceId upon construction, and pass this instance ID to the C++ code (either via a parameter, or via "this" to allow the same JSON scheme to be used for the webview's and extension's API?
Two small comments, but I'm happy to give my stamp once lazyboy@ is happy. This is a great change that mostly affects webview. https://codereview.chromium.org/948243005/diff/20001/chrome/renderer/resource... File chrome/renderer/resources/extensions/chrome_web_view_internal_custom_bindings.js (right): https://codereview.chromium.org/948243005/diff/20001/chrome/renderer/resource... chrome/renderer/resources/extensions/chrome_web_view_internal_custom_bindings.js:6: var createContextMenusHandlers = require('contextMenusHandlers').create; I'd rather imports import the module, not a method from the module, unless you're importing a class. https://codereview.chromium.org/948243005/diff/20001/chrome/renderer/resource... chrome/renderer/resources/extensions/chrome_web_view_internal_custom_bindings.js:14: impl.requestHandlers.create); Can we call this "handlers" rather than "impl"?
Some more comments/answers. I'm OK /w the approach here. https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... File extensions/renderer/resources/context_menus_handlers.js (right): https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:29: // <webviews> have an extra item in front of the parameter list, which nit: <webview>s ... https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:30: // specifies the ID of the webview. This is used to hide context menu s/ID/viewInstanceId https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:52: contextMenus.handlersForId = function(instanceid, id) { here and in all other places: s/instanceid/instanceId https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:73: delete info.webviewInstanceId; Add a note here saying we don't expose "webviewInstanceId" in the event info. https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:120: // When onclick is explicitly set to null, remove the event listener. On 2015/02/25 18:25:30, robwu wrote: > On 2015/02/25 17:58:42, lazyboy wrote: > > Great, can you file a bug for this and add it to to "BUG=" in CL description. > > > > Also this probably makes sense to fix on a CL on it's own. > > Done. https://crbug.com/461869 Please write a test for this behavior :) https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:129: return { On 2015/02/25 18:25:30, robwu wrote: > On 2015/02/25 17:58:42, lazyboy wrote: > > This is a bit brittle, the json schema compiler does not > > enforce any check on this returned object, which might be a bit > > worrying. This is one reason I didn't refactor this code > > like this (I thought about it) in the beginning, maybe it's > > OK. kalman@ can probably chime in here. > > This return value is not validated at all. This method is just a factory used by > the other two files. > > Is the extension contextMenus API significantly different from the webview's? Not significantly different, but there are differences, e.g. extension menu items are saved in the extension storage where webview ones are transient. > E.g. if I were to add a hypothethical tabId to show contextMenus in a certain > tab only, what would be the effect on the implementation in JS/C++? If we can refactor stuffs then the JS effect wouldn't be much, but the C++ might or might not get involved. The current > implementation in webview looks complex (chrome_web_view_experimental.js). Why > can't we create a contextMenus binding factory that takes an instanceId upon > construction, and pass this instance ID to the C++ code (either via a parameter, > or via "this" to allow the same JSON scheme to be used for the webview's and > extension's API? I'm just giving one example where stuff differs: The current state of events require unique event names, and we want separate event objects per <webview>: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... so we use subEvent trick: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... Said that, it doesn't need to be this way, if we can generalize the code so adding a tabId becomes trivial, that would be ideal.
https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... File extensions/renderer/resources/context_menus_handlers.js (right): https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:5: // Implementation of custom bindings for the contextMenus API. On 2015/02/25 17:58:42, lazyboy wrote: > A little bit detail here would help. > Note that we use this for chrome contextMenus API and > <webview> contextMenus API. > Note that webviewInstanceId in below would be unset/set to 0/-1 for > the former. Done. https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:16: var INSTANCEID_NONWEBVIEW = -1; On 2015/02/25 18:25:30, robwu wrote: > On 2015/02/25 17:58:42, lazyboy wrote: > > Most places in the code base refers to this as kInstanceIDNone, e.g: > > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... > > > > webview's contextmenus api uses "viewInstanceID" generated from > > IdGeneratorCustomBindings, so 0 is invalid id as well, so change it to: > > var WEBVIEW_INSTANCE_ID_NONE = 0; > > I've picked -1 because "0" could be a value associated with the webview API. I > based this assumption on > https://cs.chromium.org/file:chrome_web_view_experimental.js%20%22opt_webView.... > I really want to convey that the key is not related to webviews at all, > including "invalid webview" (which is different from "not a webview"). I've added a comment to clarify my intention. https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:29: // <webviews> have an extra item in front of the parameter list, which On 2015/02/25 22:52:48, lazyboy wrote: > nit: <webview>s ... Done. https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:30: // specifies the ID of the webview. This is used to hide context menu On 2015/02/25 22:52:47, lazyboy wrote: > s/ID/viewInstanceId Done. https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:52: contextMenus.handlersForId = function(instanceid, id) { On 2015/02/25 22:52:47, lazyboy wrote: > here and in all other places: > s/instanceid/instanceId Done. https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:73: delete info.webviewInstanceId; On 2015/02/25 22:52:47, lazyboy wrote: > Add a note here saying we don't expose "webviewInstanceId" in the event info. Done. https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:120: // When onclick is explicitly set to null, remove the event listener. On 2015/02/25 22:52:47, lazyboy wrote: > On 2015/02/25 18:25:30, robwu wrote: > > On 2015/02/25 17:58:42, lazyboy wrote: > > > Great, can you file a bug for this and add it to to "BUG=" in CL > description. > > > > > > Also this probably makes sense to fix on a CL on it's own. > > > > Done. https://crbug.com/461869 > > Please write a test for this behavior :) Done. https://codereview.chromium.org/948243005/diff/20001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:129: return { On 2015/02/25 22:52:47, lazyboy wrote: > On 2015/02/25 18:25:30, robwu wrote: > > On 2015/02/25 17:58:42, lazyboy wrote: > > > This is a bit brittle, the json schema compiler does not > > > enforce any check on this returned object, which might be a bit > > > worrying. This is one reason I didn't refactor this code > > > like this (I thought about it) in the beginning, maybe it's > > > OK. kalman@ can probably chime in here. > > > > This return value is not validated at all. This method is just a factory used > by > > the other two files. > > > > Is the extension contextMenus API significantly different from the webview's? > Not significantly different, but there are differences, > e.g. extension menu items are saved in the extension storage where > webview ones are transient. If that's all, then merging would make maintenance so much easier. I glanced over the C++ implementation, and it seems to be a mingled (and incomplete) copy of context menus and webview. > > E.g. if I were to add a hypothethical tabId to show contextMenus in a certain > > tab only, what would be the effect on the implementation in JS/C++? > If we can refactor stuffs then the JS effect wouldn't be much, but > the C++ might or might not get involved. > > The current > > implementation in webview looks complex (chrome_web_view_experimental.js). Why > > can't we create a contextMenus binding factory that takes an instanceId upon > > construction, and pass this instance ID to the C++ code (either via a > parameter, > > or via "this" to allow the same JSON scheme to be used for the webview's and > > extension's API? > I'm just giving one example where stuff differs: > The current state of events require unique event names, > and we want separate event objects per <webview>: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... > so we use subEvent trick: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... The "webview contextmenu logic" could be generalized, and extensions would then be a specific implementation of this feature. > > Said that, it doesn't need to be this way, if we can generalize > the code so adding a tabId becomes trivial, that would be ideal.
Thanks for the test, I guess you missed addressing comments from patchset #2? https://codereview.chromium.org/948243005/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_browsertest.cc (right): https://codereview.chromium.org/948243005/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_browsertest.cc:218: // Tests that previous onclick is not fired after updating the menu's onclick. // This test also tests that setting onclick handler explicitly to null removes the handler. https://codereview.chromium.org/948243005/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_browsertest.cc:220: ExtensionTestMessageListener listener_error1("onclick-unexpected", false); "onclick1-unexpected", let's append 1 in all places where it refers to first menu item. id->id1 in js https://codereview.chromium.org/948243005/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/context_menus/onclick_null/test.js (right): https://codereview.chromium.org/948243005/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/context_menus/onclick_null/test.js:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: new style is to ommit "(c)" https://codereview.chromium.org/948243005/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/context_menus_handlers.js (right): https://codereview.chromium.org/948243005/diff/40001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:21: var INSTANCEID_NONWEBVIEW = -1; nit: INSTANCEID_NON_WEBVIEW
https://codereview.chromium.org/948243005/diff/20001/chrome/renderer/resource... File chrome/renderer/resources/extensions/chrome_web_view_internal_custom_bindings.js (right): https://codereview.chromium.org/948243005/diff/20001/chrome/renderer/resource... chrome/renderer/resources/extensions/chrome_web_view_internal_custom_bindings.js:6: var createContextMenusHandlers = require('contextMenusHandlers').create; On 2015/02/25 19:03:53, kalman wrote: > I'd rather imports import the module, not a method from the module, unless > you're importing a class. Done. https://codereview.chromium.org/948243005/diff/20001/chrome/renderer/resource... chrome/renderer/resources/extensions/chrome_web_view_internal_custom_bindings.js:11: var impl = createContextMenusHandlers(/* isWebview = */ true); On 2015/02/25 17:58:42, lazyboy wrote: > nit: the most followed pattern here is: > createContextMenusHandlers(true /* isWebView */); Done. https://codereview.chromium.org/948243005/diff/20001/chrome/renderer/resource... chrome/renderer/resources/extensions/chrome_web_view_internal_custom_bindings.js:14: impl.requestHandlers.create); On 2015/02/25 19:03:53, kalman wrote: > Can we call this "handlers" rather than "impl"? Done. https://codereview.chromium.org/948243005/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_browsertest.cc (right): https://codereview.chromium.org/948243005/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_browsertest.cc:218: // Tests that previous onclick is not fired after updating the menu's onclick. On 2015/02/26 00:39:47, lazyboy wrote: > // This test also tests that setting onclick handler explicitly to null removes > the handler. Done. https://codereview.chromium.org/948243005/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_browsertest.cc:220: ExtensionTestMessageListener listener_error1("onclick-unexpected", false); On 2015/02/26 00:39:47, lazyboy wrote: > "onclick1-unexpected", let's append 1 in all places where it refers to first > menu item. > id->id1 in js Done. https://codereview.chromium.org/948243005/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/context_menus/onclick_null/test.js (right): https://codereview.chromium.org/948243005/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/context_menus/onclick_null/test.js:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/02/26 00:39:47, lazyboy wrote: > nit: new style is to ommit "(c)" Done. https://codereview.chromium.org/948243005/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/context_menus_handlers.js (right): https://codereview.chromium.org/948243005/diff/40001/extensions/renderer/reso... extensions/renderer/resources/context_menus_handlers.js:21: var INSTANCEID_NONWEBVIEW = -1; On 2015/02/26 00:39:47, lazyboy wrote: > nit: INSTANCEID_NON_WEBVIEW Done.
https://codereview.chromium.org/948243005/diff/60001/extensions/renderer/reso... File extensions/renderer/resources/context_menus_custom_bindings.js (right): https://codereview.chromium.org/948243005/diff/60001/extensions/renderer/reso... extensions/renderer/resources/context_menus_custom_bindings.js:13: var handlers = contextMenusHandlers.create(true /* isWebview */); This should be false?
https://codereview.chromium.org/948243005/diff/60001/extensions/renderer/reso... File extensions/renderer/resources/context_menus_custom_bindings.js (right): https://codereview.chromium.org/948243005/diff/60001/extensions/renderer/reso... extensions/renderer/resources/context_menus_custom_bindings.js:13: var handlers = contextMenusHandlers.create(true /* isWebview */); On 2015/02/26 18:04:37, lazyboy wrote: > This should be false? Yes indeed, thanks for catching!
lgtm
kalman: lazyboy approved this patch, could you give your lg as well?
lgtm
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/948243005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by rob@robwu.nl
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/948243005/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/948243005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rob@robwu.nl
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/948243005/#ps120001 (title: "Fix ExtensionContextMenuBrowserTest.UpdateOnclick test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/948243005/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6e3c0446a84339e95996e9cafd6e5e72c0698827 Cr-Commit-Position: refs/heads/master@{#323880} |
