Deprecate chrome.extension.sendRequest in favor of sendMessage, with a safer

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

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=130213

[Matt Perry, 2012-03-30 04:34:55]

http://codereview.chromium.org/9965005/diff/1/chrome/renderer/resources/exten...
File chrome/renderer/resources/extensions/extension_custom_bindings.js (right):

http://codereview.chromium.org/9965005/diff/1/chrome/renderer/resources/exten...
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.

[Matt Perry, 2012-03-30 04:55:40]

Hmm.. of course as soon as I sent this, I noticed a flaw:
  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.

[Matt Perry, 2012-03-30 05:41:39]

OK, this version implements comment 2 from bug 120531. It seems to work pretty
well.

http://codereview.chromium.org/9965005/diff/8001/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/9965005/diff/8001/chrome/renderer/resources/ex...
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.

[Aaron Boodman, 2012-03-30 21:27:59]

http://codereview.chromium.org/9965005/diff/12/chrome/common/extensions/api/e...
File chrome/common/extensions/api/extension.json (right):

http://codereview.chromium.org/9965005/diff/12/chrome/common/extensions/api/e...
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/e...
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/exte...
File chrome/renderer/resources/extensions/extension_custom_bindings.js (right):

http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/exte...
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/exte...
File chrome/renderer/resources/extensions/miscellaneous_bindings.js (right):

http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/miscellaneous_bindings.js:149: var
requestEvent = (isSendMessage ?
O_o

http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/exte...
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/exte...
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/exte...
chrome/renderer/resources/extensions/miscellaneous_bindings.js:258: if
(!responseCallback)
Maybe add a comment that this is used by the older kRequestChannel?

[Matt Perry, 2012-03-30 23:02:48]

PTAL

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/e...
File chrome/common/extensions/api/extension.json (right):

http://codereview.chromium.org/9965005/diff/12/chrome/common/extensions/api/e...
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/exte...
File chrome/renderer/resources/extensions/extension_custom_bindings.js (right):

http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/exte...
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/exte...
File chrome/renderer/resources/extensions/miscellaneous_bindings.js (right):

http://codereview.chromium.org/9965005/diff/12/chrome/renderer/resources/exte...
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/exte...
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/exte...
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/exte...
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.

[Aaron Boodman, 2012-03-30 23:11:10]

LGTM

[bjornstar, 2012-07-09 22:32:26]

On 2012/03/30 23:11:10, Aaron Boodman wrote:
> LGTM

Can we get some documentation regarding the minimum version required for:
chrome.extension.sendMessage, chrome.extension.onMessage,
chrome.tabs.sendMessage, and chrome.tabs.onMessage?

The documentation for chrome.extension.isAllowedIncognitoAccess clearly states:
"This function was added in version 12.0.706.0. If you require this function,
the manifest key minimum_chrome_version can ensure that your extension won't be
run in an earlier browser version."

[Matt Perry, 2012-07-09 23:29:52]

On 2012/07/09 22:32:26, bjornstar wrote:
> On 2012/03/30 23:11:10, Aaron Boodman wrote:
> > LGTM
> 
> Can we get some documentation regarding the minimum version required for:
> chrome.extension.sendMessage, chrome.extension.onMessage,
> chrome.tabs.sendMessage, and chrome.tabs.onMessage?
> 
> The documentation for chrome.extension.isAllowedIncognitoAccess clearly
states:
> "This function was added in version 12.0.706.0. If you require this function,
> the manifest key minimum_chrome_version can ensure that your extension won't
be
> run in an earlier browser version."

These methods are available in all current versions of Chrome (stable is
currently Chrome 20). It may have been in earlier versions as well, but those
users should be upgraded anyway.