Merge custom bindings for context menus

extensions/renderer/resources/context_menus_handlers.js

Index: extensions/renderer/resources/context_menus_handlers.js
diff --git a/extensions/renderer/resources/context_menus_handlers.js b/extensions/renderer/resources/context_menus_handlers.js
new file mode 100644
index 0000000000000000000000000000000000000000..863d378200dbbecc4998b56faf172225e3675fee
--- /dev/null
+++ b/extensions/renderer/resources/context_menus_handlers.js
@@ -0,0 +1,135 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Implementation of custom bindings for the contextMenus API.

[lazyboy, 2015/02/25 17:58:42]

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.

[robwu, 2015/02/26 00:15:11]

Done.

+
+var contextMenuNatives = requireNative('context_menus');
+var sendRequest = require('sendRequest').sendRequest;
+var Event = require('event_bindings').Event;
+var lastError = require('lastError');
+
+// Add the bindings to the contextMenus API.
+function createContextMenusHandlers(isWebview) {
+  var eventName = isWebview ? 'webViewInternal.contextMenus' : 'contextMenus';
+  // Some dummy value for chrome.contextMenus instances.
+  var INSTANCEID_NONWEBVIEW = -1;

[lazyboy, 2015/02/25 17:58:42]

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;

[robwu, 2015/02/25 18:25:30]

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").

[robwu, 2015/02/26 00:15:11]

I've added a comment to clarify my intention.

+
+  // Generates a customCallback for a given method. |handleCallback| will be
+  // invoked with |request.args| as parameters.
+  function createCustomCallback(handleCallback) {
+    return function(name, request, callback) {
+      if (lastError.hasError(chrome)) {
+        if (callback)
+          callback();
+        return;
+      }
+      var args = request.args;
+      if (!isWebview) {
+        // <webviews> have an extra item in front of the parameter list, which

[lazyboy, 2015/02/25 22:52:48]

nit: <webview>s ...

[robwu, 2015/02/26 00:15:11]

Done.

+        // specifies the ID of the webview. This is used to hide context menu

[lazyboy, 2015/02/25 22:52:47]

s/ID/viewInstanceId

[robwu, 2015/02/26 00:15:11]

Done.

+        // events in one webview from another.
+        // The non-webview chrome.contextMenus API is not called with such an
+        // ID, so we prepend an ID to match the function signature.
+        args = $Array.concat([INSTANCEID_NONWEBVIEW], args);
+      }
+      $Function.apply(handleCallback, null, args);
+      if (callback)
+        callback();
+    };
+  }
+
+  var contextMenus = {};
+  contextMenus.handlers = {};
+  contextMenus.event = new Event(eventName);
+
+  contextMenus.getIdFromCreateProperties = function(createProperties) {
+    if (typeof createProperties.id !== 'undefined')
+      return createProperties.id;
+    return createProperties.generatedId;
+  };
+
+  contextMenus.handlersForId = function(instanceid, id) {

[lazyboy, 2015/02/25 22:52:47]

here and in all other places:
s/instanceid/instanceId

[robwu, 2015/02/26 00:15:11]

Done.

+    if (!contextMenus.handlers[instanceid]) {
+      contextMenus.handlers[instanceid] = {
+        generated: {},
+        string: {}
+      };
+    }
+    if (typeof id === 'number')
+      return contextMenus.handlers[instanceid].generated;
+    return contextMenus.handlers[instanceid].string;
+  };
+
+  contextMenus.ensureListenerSetup = function() {
+    if (contextMenus.listening) {
+      return;
+    }
+    contextMenus.listening = true;
+    contextMenus.event.addListener(function(info) {
+      var instanceid = INSTANCEID_NONWEBVIEW;
+      if (isWebview) {
+        instanceid = info.webviewInstanceId;
+        delete info.webviewInstanceId;

[lazyboy, 2015/02/25 22:52:47]

Add a note here saying we don't expose "webviewInstanceId" in the event info.

[robwu, 2015/02/26 00:15:11]

Done.

+      }
+
+      var id = info.menuItemId;
+      var onclick = contextMenus.handlersForId(instanceid, id)[id];
+      if (onclick) {
+        $Function.apply(onclick, null, arguments);
+      }
+    });
+  };
+
+  // To be used with apiFunctions.setHandleRequest
+  var requestHandlers = {};
+  // To be used with apiFunctions.setCustomCallback
+  var callbacks = {};
+
+  requestHandlers.create = function() {
+    var createProperties = isWebview ? arguments[1] : arguments[0];
+    createProperties.generatedId = contextMenuNatives.GetNextContextMenuId();
+    var optArgs = {
+      customCallback: this.customCallback,
+    };
+    sendRequest(this.name, arguments, this.definition.parameters, optArgs);
+    return contextMenus.getIdFromCreateProperties(createProperties);
+  };
+
+  callbacks.create =
+      createCustomCallback(function(instanceid, createProperties) {
+    var id = contextMenus.getIdFromCreateProperties(createProperties);
+    var onclick = createProperties.onclick;
+    if (onclick) {
+      contextMenus.ensureListenerSetup();
+      contextMenus.handlersForId(instanceid, id)[id] = onclick;
+    }
+  });
+
+  callbacks.remove = createCustomCallback(function(instanceid, id) {
+    delete contextMenus.handlersForId(instanceid, id)[id];
+  });
+
+  callbacks.update =
+      createCustomCallback(function(instanceid, id, updateProperties) {
+    var onclick = updateProperties.onclick;
+    if (onclick) {
+      contextMenus.ensureListenerSetup();
+      contextMenus.handlersForId(instanceid, id)[id] = onclick;
+    } else if (onclick === null) {
+      // When onclick is explicitly set to null, remove the event listener.

[lazyboy, 2015/02/25 17:58:42]

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.

[robwu, 2015/02/25 18:25:30]

Done. https://crbug.com/461869

[lazyboy, 2015/02/25 22:52:47]

Please write a test for this behavior :)

[robwu, 2015/02/26 00:15:11]

Done.

+      delete contextMenus.handlersForId(instanceid, id)[id];
+    }
+  });
+
+  callbacks.removeAll = createCustomCallback(function(instanceid) {
+    delete contextMenus.handlers[instanceid];
+  });
+
+  return {

[lazyboy, 2015/02/25 17:58:42]

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.

[robwu, 2015/02/25 18:25:30]

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?

[lazyboy, 2015/02/25 22:52:47]

Not significantly different, but there are differences,
e.g. extension menu items are saved in the extension storage where
webview ones are transient.
If we can refactor stuffs then the JS effect wouldn't be much, but
the C++ might or might not get involved.
The current
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.

[robwu, 2015/02/26 00:15:11]

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.
The "webview contextmenu logic" could be generalized, and extensions would then
be a specific implementation of this feature.

+    requestHandlers: requestHandlers,
+    callbacks: callbacks
+  };
+}
+
+exports.create = createContextMenusHandlers;