Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1874133002:
Improve documentation of extension messaging (Closed)
|
Created:
4 years, 8 months ago by robwu Modified:
4 years, 8 months ago Reviewers:
Devlin 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. |
DescriptionImprove documentation of extension messaging
- Document all methods and events of Port.
- Document the fact that messages have to be JSON-ifiable.
- Emphasize that "return true" have to be used (in the API docs, and
in the messaging tutorial, because this question keeps popping up
every week on Stack Overflow, so apparently the previous note was
not sufficiently obvious).
- Document the lifetime characteristics of ports.
BUG=87758, 126486, 328459, 592478
Committed: https://crrev.com/2ee8e354ade9b0ad6c030c94420e61d2ca3f743f
Cr-Commit-Position: refs/heads/master@{#387157}
Patch Set 1 #Patch Set 2 : Add comma #Patch Set 3 : Fix refs, clarify API parameters #
Total comments: 16
Patch Set 4 : #
Total comments: 8
Patch Set 5 : #
Messages
Total messages: 12 (3 generated)
rob@robwu.nl changed reviewers: + rdevlin.cronin@chromium.org
Unfortunately the preview server is dead (https://crbug.com/371402), but locally I verified that the docs is rendered as expected. (the documentation server seems also stale, hopefully that gets fixed soon-ish - https://crbug.com/513780)
Nice! Thanks for filling in some of the gaps. :) https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... chrome/common/extensions/api/tabs.json:253: "description": "The message to send. This message should be JSON-ifiable object." at the very least, we should say say "a JSON-ifiable object" instead of just "JSON-ifiable object". I'd also be inclined to rephrase this to something like: This message should be serializable by JSON.stringify()" or something. https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... File chrome/common/extensions/docs/templates/articles/messaging.html (right): https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:78: In the above example, <code>sendResponse</code> was called synchronously. Why is this important? https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:86: <b>Note:</b> If none of the handlers call <code>sendResponse</code>, then the I don't know that we should add this to the documentation. It strikes me as an implementation detail, and, if we do, it means that we're more locked into that behavior. I think I'd rather not document the cases in which sendResponse is called synchronously vs asynchronously - I don't think it's something that developers should be relying on. WDYT? https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:171: This port is immediately usable and messages can be used to send messages to rephrase this - "... usable and messages can be used to send messages" doesn't quite make sense. :) https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:178: Similarly, if $(ref:runtime.connect) is used, then the onConnect may be fired onConnect *event*? Also maybe linkify? https://codereview.chromium.org/1874133002/diff/40001/extensions/common/api/r... File extensions/common/api/runtime.json (right): https://codereview.chromium.org/1874133002/diff/40001/extensions/common/api/r... extensions/common/api/runtime.json:25: "description": "Immediately disconnect the port. Calling <code>disconnect()</code> on an already-disconnected port has no effect. When a port is disconnected, no <em>new</em> events are generated any more on this port. If <code>disconnect()</code> was called while dispatching an $(ref:Port.onMessage onMessage) event, then the existing handlers still continue to be fired for that event." remove "any more" https://codereview.chromium.org/1874133002/diff/40001/extensions/common/api/r... extensions/common/api/runtime.json:25: "description": "Immediately disconnect the port. Calling <code>disconnect()</code> on an already-disconnected port has no effect. When a port is disconnected, no <em>new</em> events are generated any more on this port. If <code>disconnect()</code> was called while dispatching an $(ref:Port.onMessage onMessage) event, then the existing handlers still continue to be fired for that event." Here, too, I worry that we're getting too much into the implementation. I'd rather not specify what happens to events in-flight.
https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... chrome/common/extensions/api/tabs.json:253: "description": "The message to send. This message should be JSON-ifiable object." On 2016/04/13 20:15:33, Devlin wrote: > at the very least, we should say say "a JSON-ifiable object" instead of just > "JSON-ifiable object". > > I'd also be inclined to rephrase this to something like: > This message should be serializable by JSON.stringify()" > or something. I'd personally say JSON-serializable, but the term "JSON-ifiable" was already used in the documentation (chrome.runtime), so I sticked to that. JSON.stringify is a specific implementation of the JSON serialization algorithm, so I prefer to name the serialization algorithm instead of the specific method. https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... File chrome/common/extensions/docs/templates/articles/messaging.html (right): https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:78: In the above example, <code>sendResponse</code> was called synchronously. On 2016/04/13 20:15:33, Devlin wrote: > Why is this important? I've added another sentence; see also my reply below. https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:86: <b>Note:</b> If none of the handlers call <code>sendResponse</code>, then the On 2016/04/13 20:15:33, Devlin wrote: > I don't know that we should add this to the documentation. It strikes me as an > implementation detail, and, if we do, it means that we're more locked into that > behavior. Sending messages within an extension is quite important, so we should commit to a stable API that behaves in a sane way. Whether and when the response callback is invoked is quite important in practice, so even if we don't document it, people will do trial-and-error and assume that that is the intended API. > I think I'd rather not document the cases in which sendResponse is > called synchronously vs asynchronously - I don't think it's something that > developers should be relying on. > > WDYT? Synchronous vs asynchronous invocation of sendResponse is highly relevant for the users of chrome.runtime.onMessage. It is the most frequently asked question about Chrome Extensions on Stack Overflow (https://stackoverflow.com/questions/20077487/chrome-extension-message-passing...). https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:171: This port is immediately usable and messages can be used to send messages to On 2016/04/13 20:15:33, Devlin wrote: > rephrase this - "... usable and messages can be used to send messages" doesn't > quite make sense. :) Done. https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:178: Similarly, if $(ref:runtime.connect) is used, then the onConnect may be fired On 2016/04/13 20:15:33, Devlin wrote: > onConnect *event*? Also maybe linkify? Done. onConnect was already linkified in the previous sentence. Linking it again is a bit too much. https://codereview.chromium.org/1874133002/diff/40001/extensions/common/api/r... File extensions/common/api/runtime.json (right): https://codereview.chromium.org/1874133002/diff/40001/extensions/common/api/r... extensions/common/api/runtime.json:25: "description": "Immediately disconnect the port. Calling <code>disconnect()</code> on an already-disconnected port has no effect. When a port is disconnected, no <em>new</em> events are generated any more on this port. If <code>disconnect()</code> was called while dispatching an $(ref:Port.onMessage onMessage) event, then the existing handlers still continue to be fired for that event." On 2016/04/13 20:15:33, Devlin wrote: > remove "any more" Done. https://codereview.chromium.org/1874133002/diff/40001/extensions/common/api/r... extensions/common/api/runtime.json:25: "description": "Immediately disconnect the port. Calling <code>disconnect()</code> on an already-disconnected port has no effect. When a port is disconnected, no <em>new</em> events are generated any more on this port. If <code>disconnect()</code> was called while dispatching an $(ref:Port.onMessage onMessage) event, then the existing handlers still continue to be fired for that event." On 2016/04/13 20:15:33, Devlin wrote: > Here, too, I worry that we're getting too much into the implementation. I'd > rather not specify what happens to events in-flight. Done.
https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... File chrome/common/extensions/docs/templates/articles/messaging.html (right): https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:86: <b>Note:</b> If none of the handlers call <code>sendResponse</code>, then the On 2016/04/13 21:03:05, robwu wrote: > On 2016/04/13 20:15:33, Devlin wrote: > > I don't know that we should add this to the documentation. It strikes me as > an > > implementation detail, and, if we do, it means that we're more locked into > that > > behavior. > > Sending messages within an extension is quite important, so we should commit to > a stable API that behaves in a sane way. Whether and when the response callback > is invoked is quite important in practice, so even if we don't document it, > people will do trial-and-error and assume that that is the intended API. > > > I think I'd rather not document the cases in which sendResponse is > > called synchronously vs asynchronously - I don't think it's something that > > developers should be relying on. > > > > WDYT? > > Synchronous vs asynchronous invocation of sendResponse is highly relevant for > the users of chrome.runtime.onMessage. It is the most frequently asked question > about Chrome Extensions on Stack Overflow > (https://stackoverflow.com/questions/20077487/chrome-extension-message-passing...). Ah, I see what you're trying to say with this comment (I thought you were trying to document whether or not sendResponse would be called synchronously by chrome if a developer didn't return true). Okay, this makes more sense now. I'd still like to tweak it a bit to make it more clear what the intent is. What about something like: The sendResponse callback is only valid if used synchronously, or if the event handler returns <code>true</code> to indicate that it will respond asynchronously. The <code>sendMessage</code> function's callback will be invoked automatically if no handlers return true or if the <code>sendResponse</code> callback is garbage-collected. https://codereview.chromium.org/1874133002/diff/60001/chrome/common/extension... File chrome/common/extensions/docs/templates/articles/messaging.html (right): https://codereview.chromium.org/1874133002/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:169: the extension (where a (top-level) frame is viewed as the smallest part). nit: remove double-nested parens https://codereview.chromium.org/1874133002/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:173: This port is immediately usable can be used for sending messages to nit: usable *and* can https://codereview.chromium.org/1874133002/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:179: (once for each frame in the tab). "each *connected* frame", maybe? (Ditto for below.) https://codereview.chromium.org/1874133002/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:187: event. This event is fired either when there are no valid ports at the other remove "either"
https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... File chrome/common/extensions/docs/templates/articles/messaging.html (right): https://codereview.chromium.org/1874133002/diff/40001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:86: <b>Note:</b> If none of the handlers call <code>sendResponse</code>, then the On 2016/04/13 21:41:40, Devlin wrote: > On 2016/04/13 21:03:05, robwu wrote: > > On 2016/04/13 20:15:33, Devlin wrote: > > > I don't know that we should add this to the documentation. It strikes me as > > an > > > implementation detail, and, if we do, it means that we're more locked into > > that > > > behavior. > > > > Sending messages within an extension is quite important, so we should commit > to > > a stable API that behaves in a sane way. Whether and when the response > callback > > is invoked is quite important in practice, so even if we don't document it, > > people will do trial-and-error and assume that that is the intended API. > > > > > I think I'd rather not document the cases in which sendResponse is > > > called synchronously vs asynchronously - I don't think it's something that > > > developers should be relying on. > > > > > > WDYT? > > > > Synchronous vs asynchronous invocation of sendResponse is highly relevant for > > the users of chrome.runtime.onMessage. It is the most frequently asked > question > > about Chrome Extensions on Stack Overflow > > > (https://stackoverflow.com/questions/20077487/chrome-extension-message-passing...). > > Ah, I see what you're trying to say with this comment (I thought you were trying > to document whether or not sendResponse would be called synchronously by chrome > if a developer didn't return true). Okay, this makes more sense now. I'd still > like to tweak it a bit to make it more clear what the intent is. What about > something like: > > The sendResponse callback is only valid if used synchronously, or if the event > handler returns <code>true</code> to indicate that it will respond > asynchronously. The <code>sendMessage</code> function's callback will be invoked > automatically if no handlers return true or if the <code>sendResponse</code> > callback is garbage-collected. Nice wording, I've copied it verbatim. https://codereview.chromium.org/1874133002/diff/60001/chrome/common/extension... File chrome/common/extensions/docs/templates/articles/messaging.html (right): https://codereview.chromium.org/1874133002/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:169: the extension (where a (top-level) frame is viewed as the smallest part). On 2016/04/13 21:41:40, Devlin wrote: > nit: remove double-nested parens Done. https://codereview.chromium.org/1874133002/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:173: This port is immediately usable can be used for sending messages to On 2016/04/13 21:41:40, Devlin wrote: > nit: usable *and* can Done. s/is immediately usable can be used/can immediately be used/. https://codereview.chromium.org/1874133002/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:179: (once for each frame in the tab). On 2016/04/13 21:41:40, Devlin wrote: > "each *connected* frame", maybe? (Ditto for below.) This is about onConnect, not onMessage, so "each frame" is fine. https://codereview.chromium.org/1874133002/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/messaging.html:187: event. This event is fired either when there are no valid ports at the other On 2016/04/13 21:41:40, Devlin wrote: > remove "either" Done.
lgtm, thanks for doing this!
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/1874133002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1874133002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Improve documentation of extension messaging - Document all methods and events of Port. - Document the fact that messages have to be JSON-ifiable. - Emphasize that "return true" have to be used (in the API docs, and in the messaging tutorial, because this question keeps popping up every week on Stack Overflow, so apparently the previous note was not sufficiently obvious). - Document the lifetime characteristics of ports. BUG=87758,126486,328459,592478 ========== to ========== Improve documentation of extension messaging - Document all methods and events of Port. - Document the fact that messages have to be JSON-ifiable. - Emphasize that "return true" have to be used (in the API docs, and in the messaging tutorial, because this question keeps popping up every week on Stack Overflow, so apparently the previous note was not sufficiently obvious). - Document the lifetime characteristics of ports. BUG=87758,126486,328459,592478 Committed: https://crrev.com/2ee8e354ade9b0ad6c030c94420e61d2ca3f743f Cr-Commit-Position: refs/heads/master@{#387157} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/2ee8e354ade9b0ad6c030c94420e61d2ca3f743f Cr-Commit-Position: refs/heads/master@{#387157} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
