Deprecate chrome.extension.sendRequest in favor of sendMessage, with a safer (issue 9965005)
mpcom...@chromium.org
http://codereview.chromium.org/9965005/diff/1/chrome/renderer/resources/extensions/extension_custom_bindings.js
File chrome/renderer/resources/extensions/extension_custom_bindings.js
(right):
http://codereview.chromium.org/9965005/diff/1/chrome/renderer/resources/extensions/extension_custom_bindings.js#newcode92
chrome/renderer/resources/extensions/extension_custom_bindings.js:92: if
(portName == chromeHidden.kMessageChannel && !responseCallback) {
It might be safe to do this for sendRequest as well (instead of
restricting it to sendMessage), but I'm being paranoid. I might change
this in a followup CL if I feel brave.
Description:
Deprecate chrome.extension.sendRequest in favor of sendMessage, with a safer
API that allows us to close the request port when the receiver doesn't send
a
response.
BUG=120531,114738
TEST=no
Please review this at http://codereview.chromium.org/9965005/
SVN Base: svn://svn.chromium.org/chrome/trunk/src
Affected files:
M chrome/common/extensions/api/extension.json
M chrome/common/extensions/api/tabs.json
M chrome/common/extensions/docs/extension.html
M chrome/common/extensions/docs/samples.json
M chrome/common/extensions/docs/tabs.html
M chrome/renderer/resources/extensions/extension_custom_bindings.js
M chrome/renderer/resources/extensions/miscellaneous_bindings.js
M chrome/renderer/resources/extensions/tabs_custom_bindings.js
mpcom...@chromium.org
chrome.extension.onMessage.addListener(function(args) {
window.setTimeout(function() {
args.sendResponse("won't actually send because sendResponse wasn't
accessed before the event listener returned.");
}, 100);
});
Back to the drawing board... I think comment 2 on bug 120531 is the best
bet so
far.
mpcom...@chromium.org
pretty
well.
http://codereview.chromium.org/9965005/diff/8001/chrome/renderer/resources/extensions/event.js
File chrome/renderer/resources/extensions/event.js (right):
http://codereview.chromium.org/9965005/diff/8001/chrome/renderer/resources/extensions/event.js#newcode210
chrome/renderer/resources/extensions/event.js:210: return retvals;
This conflicts with 'return validationErrors' above. If I knew which
caller needed it, I could return an object that contains either
validationErrors or retvals so that callers can distinguish the two.
a...@chromium.org
http://codereview.chromium.org/9965005/diff/12/chrome/common/extensions/api/extension.json
File chrome/common/extensions/api/extension.json (right):
http://codereview.chromium.org/9965005/diff/12/chrome/common/extensions/api/extension.json#newcode94
chrome/common/extensions/api/extension.json:94: "description":
"Deprecated: Please use sendMessage.",
Do you want to just 'nodoc' this instead?
http://codereview.chromium.org/9965005/diff/12/chrome/common/extensions/api/extension.json#newcode115
chrome/common/extensions/api/extension.json:115:
"allowAmbiguousOptionalArguments": true,
Why not fix this as long as we're changing things?
http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/extensions/extension_custom_bindings.js
File chrome/renderer/resources/extensions/extension_custom_bindings.js
(right):
http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/extensions/extension_custom_bindings.js#newcode80
chrome/renderer/resources/extensions/extension_custom_bindings.js:80:
sendMessageUpdateArguments);
You could use sendMessageUpdateArguments.bind(null, 'sendRequest') to
pass in the name for the error. You'd have to update the argument
munging code slightly inside the function.
http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/extensions/miscellaneous_bindings.js
File chrome/renderer/resources/extensions/miscellaneous_bindings.js
(right):
http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/extensions/miscellaneous_bindings.js#newcode149
chrome/renderer/resources/extensions/miscellaneous_bindings.js:149: var
requestEvent = (isSendMessage ?
O_o
http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/extensions/miscellaneous_bindings.js#newcode186
chrome/renderer/resources/extensions/miscellaneous_bindings.js:186:
responseCallbackAccessed = true;
Were you originally trying to do some craziness where you detect whether
they use the callback variable? That is an interesting idea, but now I
think the name is out of date?
http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/extensions/miscellaneous_bindings.js#newcode186
chrome/renderer/resources/extensions/miscellaneous_bindings.js:186:
responseCallbackAccessed = true;
responseCallback = retvals.indexOf(true) > -1;
http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/extensions/miscellaneous_bindings.js#newcode258
chrome/renderer/resources/extensions/miscellaneous_bindings.js:258: if
(!responseCallback)
Maybe add a comment that this is used by the older kRequestChannel?
mpcom...@chromium.org
I also found who was using Event.dispatch's validationErrors, and fixed
that up.
http://codereview.chromium.org/9965005/diff/12/chrome/common/extensions/api/extension.json
File chrome/common/extensions/api/extension.json (right):
http://codereview.chromium.org/9965005/diff/12/chrome/common/extensions/api/extension.json#newcode94
chrome/common/extensions/api/extension.json:94: "description":
"Deprecated: Please use sendMessage.",
On 2012/03/30 21:28:00, Aaron Boodman wrote:
> Do you want to just 'nodoc' this instead?
Done.
http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/extensions/extension_custom_bindings.js
File chrome/renderer/resources/extensions/extension_custom_bindings.js
(right):
http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/extensions/extension_custom_bindings.js#newcode80
chrome/renderer/resources/extensions/extension_custom_bindings.js:80:
sendMessageUpdateArguments);
On 2012/03/30 21:28:00, Aaron Boodman wrote:
> You could use sendMessageUpdateArguments.bind(null, 'sendRequest') to
pass in
> the name for the error. You'd have to update the argument munging code
slightly
> inside the function.
Done.
http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/extensions/miscellaneous_bindings.js
File chrome/renderer/resources/extensions/miscellaneous_bindings.js
(right):
http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/extensions/miscellaneous_bindings.js#newcode149
chrome/renderer/resources/extensions/miscellaneous_bindings.js:149: var
requestEvent = (isSendMessage ?
On 2012/03/30 21:28:00, Aaron Boodman wrote:
> O_o
Yeah I went there.
http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/extensions/miscellaneous_bindings.js#newcode186
chrome/renderer/resources/extensions/miscellaneous_bindings.js:186:
responseCallbackAccessed = true;
On 2012/03/30 21:28:00, Aaron Boodman wrote:
> Were you originally trying to do some craziness where you detect
whether they
> use the callback variable? That is an interesting idea, but now I
think the name
> is out of date?
Yep. New name.
http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/extensions/miscellaneous_bindings.js#newcode186
chrome/renderer/resources/extensions/miscellaneous_bindings.js:186:
responseCallbackAccessed = true;
On 2012/03/30 21:28:00, Aaron Boodman wrote:
> responseCallback = retvals.indexOf(true) > -1;
Sweet, done.
http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/extensions/miscellaneous_bindings.js#newcode258
chrome/renderer/resources/extensions/miscellaneous_bindings.js:258: if
(!responseCallback)
On 2012/03/30 21:28:00, Aaron Boodman wrote:
> Maybe add a comment that this is used by the older kRequestChannel?
Done.