Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1471243002:
chrome.displaySource custom bindings (Closed)
Descriptionchrome.displaySource custom bindings
This patch introduces custom bindings for 'chrome.displaySource' API.
These are bindings for 'startSession', 'terminateSession' methods
and for the 'onSessionStarted', 'onSessionTerminated',
'onSessionErrorOccured' events.
The bindings should belong to render process (i.e. be custom) as:
1) they accept dom objects arguments (MediaStreamTrack)
2) for security reasons: to keep all protocols handling within sandbox
The abstract 'DisplaySourceSession' class is added to be implemented
by the 'chrome.displaySource' API backends.
BUG=242107
Committed: https://crrev.com/79486f1c04861a577489fa63b1f82342aaee095b
Cr-Commit-Position: refs/heads/master@{#364045}
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Comments from Antony #
Total comments: 1
Patch Set 3 : Report errors via 'chrome.runtime.lastError' #
Total comments: 10
Patch Set 4 : Fixing nits #Messages
Total messages: 40 (19 generated)
Description was changed from ========== chrome.displaySource custom bindings This patch introduces custom bindings for 'chrome.displaySource' API. These are bindings for 'startSession', 'terminateSession' methods and for the 'onSessionStarted', 'onSessionTerminated', 'onSessionErrorOccured' events. The bindings should belong to render process (i.e. be custom) as: 1) they accept dom objects arguments (MediaStreamTrack) 2) for security reasons: to keep all protocols handling within sandbox The abstract 'DisplaySourceSession' class is added to be implemented by the 'chrome.displaySource' API backends. BUG=242107 ========== to ========== chrome.displaySource custom bindings This patch introduces custom bindings for 'chrome.displaySource' API. These are bindings for 'startSession', 'terminateSession' methods and for the 'onSessionStarted', 'onSessionTerminated', 'onSessionErrorOccured' events. The bindings should belong to render process (i.e. be custom) as: 1) they accept dom objects arguments (MediaStreamTrack) 2) for security reasons: to keep all protocols handling within sandbox The abstract 'DisplaySourceSession' class is added to be implemented by the 'chrome.displaySource' API backends. BUG=242107 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
mikhail.pozdnyakov@intel.com changed reviewers: + asargent@chromium.org
PTAL
https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/api... File extensions/renderer/api/display_source/display_source_session.h (right): https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/api... extensions/renderer/api/display_source/display_source_session.h:38: // Starts the session. optional nit: consider adding an additional whitespace line before start of comment block (elsewhere in this file as well) https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/dis... File extensions/renderer/display_source_custom_bindings.cc (right): https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/dis... extensions/renderer/display_source_custom_bindings.cc:33: base::Bind(&DisplaySourceCustomBindings::CreateSession, nit: any reason for the name mismatch between the js function "StartSession" and the c++ function "CreateSession"? https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/dis... extensions/renderer/display_source_custom_bindings.cc:59: v8::TryCatch try_catch; nit: according to the comment at https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&l=... it looks like they prefer callers switch to the version of the TryCatch constructor that takes an isolate argument https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/dis... extensions/renderer/display_source_custom_bindings.cc:84: v8::String::NewFromUtf8(isolate, kInvalidSinkIdArg))); The typical pattern for returning errors from extension functions is to dispatch the callback with chrome.runtime.lastError set, possibly with a developer-readable error message in chrome.runtime.lastError.message. Is there a reason to deviate from that here (and elsewhere below)? https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/dis... extensions/renderer/display_source_custom_bindings.cc:140: if (GetDisplaySession(sink_id)) { optional: would it make sense to move this check up to line 88 just after we get the sink_id?
Thanks for review! https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/api... File extensions/renderer/api/display_source/display_source_session.h (right): https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/api... extensions/renderer/api/display_source/display_source_session.h:38: // Starts the session. On 2015/11/26 00:50:07, Antony Sargent wrote: > optional nit: consider adding an additional whitespace line before start of > comment block (elsewhere in this file as well) Done. https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/dis... File extensions/renderer/display_source_custom_bindings.cc (right): https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/dis... extensions/renderer/display_source_custom_bindings.cc:33: base::Bind(&DisplaySourceCustomBindings::CreateSession, On 2015/11/26 00:50:07, Antony Sargent wrote: > nit: any reason for the name mismatch between the js function "StartSession" and > the c++ function "CreateSession"? Not really. Renamed. https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/dis... extensions/renderer/display_source_custom_bindings.cc:59: v8::TryCatch try_catch; On 2015/11/26 00:50:07, Antony Sargent wrote: > nit: according to the comment at > https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&l=... > it looks like they prefer callers switch to the version of the TryCatch > constructor that takes an isolate argument Done. https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/dis... extensions/renderer/display_source_custom_bindings.cc:84: v8::String::NewFromUtf8(isolate, kInvalidSinkIdArg))); On 2015/11/26 00:50:07, Antony Sargent wrote: > The typical pattern for returning errors from extension functions is to dispatch > the callback with chrome.runtime.lastError set, possibly with a > developer-readable error message in chrome.runtime.lastError.message. Is there a > reason to deviate from that here (and elsewhere below)? These are just the arguments sanity checks that can be done right away -- no need to make them asynchronous, other custom bindings throw exceptions the same way (e.g. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex... ) https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/dis... extensions/renderer/display_source_custom_bindings.cc:140: if (GetDisplaySession(sink_id)) { On 2015/11/26 00:50:07, Antony Sargent wrote: > optional: would it make sense to move this check up to line 88 just after we get > the sink_id? Makes sense, thus we'll be able to avoid unnecessary checks of the rest of the arguments
asargent@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+rdevlin.cronin to get his opinion on the question of error handling in display_source_custom_bindings.cc https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/dis... File extensions/renderer/display_source_custom_bindings.cc (right): https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/dis... extensions/renderer/display_source_custom_bindings.cc:84: v8::String::NewFromUtf8(isolate, kInvalidSinkIdArg))); On 2015/11/26 13:00:47, Mikhail wrote: > On 2015/11/26 00:50:07, Antony Sargent wrote: > > The typical pattern for returning errors from extension functions is to > dispatch > > the callback with chrome.runtime.lastError set, possibly with a > > developer-readable error message in chrome.runtime.lastError.message. Is there > a > > reason to deviate from that here (and elsewhere below)? > > These are just the arguments sanity checks that can be done right away -- no > need to make them asynchronous, > other custom bindings throw exceptions the same way (e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex... > ) > If some errors are reported as exceptions and some via runtime.lastError in the callback, developers have to write more verbose code. E.g. function handleError(error) { console.error("oops " + error); } try { chrome.foo.bar(args, function(result) { if (chrome.runtime.lastError) { handleError(chrome.runtime.lastError.message); return; } ... }); } catch (e) { handleError(e.message); } vs. chrome.foo.bar(args, function(result) { if (chrome.runtime.lastError) { console.error("oops " + chrome.runtime.lastError.message); return; } ... }); Devlin, do you have a preference here?
On 2015/12/01 01:54:23, Antony Sargent wrote: > +rdevlin.cronin to get his opinion on the question of error handling in > display_source_custom_bindings.cc > > > If some errors are reported as exceptions and some via runtime.lastError in the > callback, developers have to write more verbose code. E.g. > I would like to add that exception throwing is already automatically provided (for arguments type checking) in all the '[nocompile]' functions (so that in the bindings code it's enough to have just 'CHECK()'). I think that 'runtime.lastError' mechanism is mostly targeted for browser-side extensions (which have to be asynchronous), on the other hand the render-located synchronous functions can just throw exceptions.
https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/dis... File extensions/renderer/display_source_custom_bindings.cc (right): https://codereview.chromium.org/1471243002/diff/40001/extensions/renderer/dis... extensions/renderer/display_source_custom_bindings.cc:84: v8::String::NewFromUtf8(isolate, kInvalidSinkIdArg))); On 2015/12/01 01:54:23, Antony Sargent wrote: > On 2015/11/26 13:00:47, Mikhail wrote: > > On 2015/11/26 00:50:07, Antony Sargent wrote: > > > The typical pattern for returning errors from extension functions is to > > dispatch > > > the callback with chrome.runtime.lastError set, possibly with a > > > developer-readable error message in chrome.runtime.lastError.message. Is > there > > a > > > reason to deviate from that here (and elsewhere below)? > > > > These are just the arguments sanity checks that can be done right away -- no > > need to make them asynchronous, > > other custom bindings throw exceptions the same way (e.g. > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex... > > ) > > > > If some errors are reported as exceptions and some via runtime.lastError in the > callback, developers have to write more verbose code. E.g. > > function handleError(error) { > console.error("oops " + error); > } > > try { > chrome.foo.bar(args, function(result) { > if (chrome.runtime.lastError) { > handleError(chrome.runtime.lastError.message); > return; > } > ... > }); > } catch (e) { > handleError(e.message); > } > > > vs. > > > chrome.foo.bar(args, function(result) { > if (chrome.runtime.lastError) { > console.error("oops " + chrome.runtime.lastError.message); > return; > } > ... > }); > > > Devlin, do you have a preference here? The standard we should be using is chrome.runtime.lastError. It has its flaws, but we should stick with it rather than using multiple error idioms. https://codereview.chromium.org/1471243002/diff/60001/extensions/renderer/res... File extensions/renderer/resources/display_source_custom_bindings.js (right): https://codereview.chromium.org/1471243002/diff/60001/extensions/renderer/res... extensions/renderer/resources/display_source_custom_bindings.js:22: exports.binding = binding.generate(); Drive-by. This is a security bug. Don't use exports.foo = foo, rather exports.$set() (all other custom bindings should also be using this).
On 2015/12/01 17:48:37, Devlin wrote: > > Devlin, do you have a preference here? > > The standard we should be using is chrome.runtime.lastError. It has its flaws, > but we should stick with it rather than using multiple error idioms. Fixed with the latest patch. I also added completion callback to the 'startSession' method so that user knows when s/he needs to check the 'lastError' value. > > https://codereview.chromium.org/1471243002/diff/60001/extensions/renderer/res... > File extensions/renderer/resources/display_source_custom_bindings.js (right): > > https://codereview.chromium.org/1471243002/diff/60001/extensions/renderer/res... > extensions/renderer/resources/display_source_custom_bindings.js:22: > exports.binding = binding.generate(); > Drive-by. This is a security bug. Don't use exports.foo = foo, rather > exports.$set() (all other custom bindings should also be using this). Fixed.
On 2015/12/02 19:49:58, Mikhail wrote: > On 2015/12/01 17:48:37, Devlin wrote: > > > Devlin, do you have a preference here? > > > > The standard we should be using is chrome.runtime.lastError. It has its > flaws, > > but we should stick with it rather than using multiple error idioms. > > Fixed with the latest patch. > I also added completion callback to the 'startSession' method so that user knows > when s/he needs to check the 'lastError' value. > > > > > > https://codereview.chromium.org/1471243002/diff/60001/extensions/renderer/res... > > File extensions/renderer/resources/display_source_custom_bindings.js (right): > > > > > https://codereview.chromium.org/1471243002/diff/60001/extensions/renderer/res... > > extensions/renderer/resources/display_source_custom_bindings.js:22: > > exports.binding = binding.generate(); > > Drive-by. This is a security bug. Don't use exports.foo = foo, rather > > exports.$set() (all other custom bindings should also be using this). > > Fixed. Any further comments pls.?
lgtm with a couple of minor things to fix before checking in https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/api... File extensions/renderer/api/display_source/display_source_session.h (right): https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/api... extensions/renderer/api/display_source/display_source_session.h:23: base::Callback<void(int, DisplaySourceErrorType, const std::string&)>; nit: please add comments or variable names so that it's clear what the meaning of each parameter is in SinkIdCallback and ErrorCallback, e.g. using Foo = base::Callback<void(int requestId)>; or // Callback for passing back a request id. using Foo = base::Callback<void(int)>; https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/api... extensions/renderer/api/display_source/display_source_session.h:52: // Sets the callbacks invoked to inform about the session's state changes suggestion: It might be helpful to document somewhere whether it's required or optional to call SetCallbacks before calling Start(). https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/dis... File extensions/renderer/display_source_custom_bindings.cc (right): https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/dis... extensions/renderer/display_source_custom_bindings.cc:181: session->Terminate(); I'm assuming we rely on the concrete implementations of DisplaySourceSession::Terminate to dispatch the termination callback, so that we eventually should end up in OnSessionTerminated below. It might be worth just having a comment here to direct the reader, something like "The session will get removed from session_map_ in OnSessionTerminated" or something like that. https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/res... File extensions/renderer/resources/display_source_custom_bindings.js (right): https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/res... extensions/renderer/resources/display_source_custom_bindings.js:19: lastError.set('displaySource.startSession', e.message, null, chrome); Looks like you're missing a matching call to lastError.clear. You can probably either add that in the bottom of the finally block below, or just use lastError.run instead of a matched pair of lastError.set/lastError.clear. https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/res... extensions/renderer/resources/display_source_custom_bindings.js:30: lastError.set( same thing here
thanks! https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/api... File extensions/renderer/api/display_source/display_source_session.h (right): https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/api... extensions/renderer/api/display_source/display_source_session.h:23: base::Callback<void(int, DisplaySourceErrorType, const std::string&)>; On 2015/12/07 21:47:49, Antony Sargent wrote: > nit: please add comments or variable names so that it's clear what the meaning > of each parameter is in SinkIdCallback and ErrorCallback, e.g. > > > using Foo = base::Callback<void(int requestId)>; > > or > > // Callback for passing back a request id. > using Foo = base::Callback<void(int)>; > Done. https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/api... extensions/renderer/api/display_source/display_source_session.h:52: // Sets the callbacks invoked to inform about the session's state changes On 2015/12/07 21:47:49, Antony Sargent wrote: > suggestion: It might be helpful to document somewhere whether it's required or > optional to call SetCallbacks before calling Start(). Done. https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/dis... File extensions/renderer/display_source_custom_bindings.cc (right): https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/dis... extensions/renderer/display_source_custom_bindings.cc:181: session->Terminate(); On 2015/12/07 21:47:49, Antony Sargent wrote: > I'm assuming we rely on the concrete implementations of > DisplaySourceSession::Terminate to dispatch the termination callback, so that we > eventually should end up in OnSessionTerminated below. It might be worth just > having a comment here to direct the reader, something like "The session will get > removed from session_map_ in OnSessionTerminated" or something like that. Done. https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/res... File extensions/renderer/resources/display_source_custom_bindings.js (right): https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/res... extensions/renderer/resources/display_source_custom_bindings.js:19: lastError.set('displaySource.startSession', e.message, null, chrome); On 2015/12/07 21:47:49, Antony Sargent wrote: > Looks like you're missing a matching call to lastError.clear. You can probably > either add that in the bottom of the finally block below, or just use > lastError.run instead of a matched pair of lastError.set/lastError.clear. Done. https://codereview.chromium.org/1471243002/diff/80001/extensions/renderer/res... extensions/renderer/resources/display_source_custom_bindings.js:30: lastError.set( On 2015/12/07 21:47:49, Antony Sargent wrote: > same thing here Done.
The CQ bit was checked by mikhail.pozdnyakov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from asargent@chromium.org Link to the patchset: https://codereview.chromium.org/1471243002/#ps100001 (title: "Fixing nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1471243002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471243002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by mikhail.pozdnyakov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from asargent@chromium.org Link to the patchset: https://codereview.chromium.org/1471243002/#ps120001 (title: "Fixing nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1471243002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471243002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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 mikhail.pozdnyakov@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1471243002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471243002/120001
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_...)
The CQ bit was checked by mikhail.pozdnyakov@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1471243002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471243002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 mikhail.pozdnyakov@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1471243002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471243002/120001
Message was sent while issue was closed.
Description was changed from ========== chrome.displaySource custom bindings This patch introduces custom bindings for 'chrome.displaySource' API. These are bindings for 'startSession', 'terminateSession' methods and for the 'onSessionStarted', 'onSessionTerminated', 'onSessionErrorOccured' events. The bindings should belong to render process (i.e. be custom) as: 1) they accept dom objects arguments (MediaStreamTrack) 2) for security reasons: to keep all protocols handling within sandbox The abstract 'DisplaySourceSession' class is added to be implemented by the 'chrome.displaySource' API backends. BUG=242107 ========== to ========== chrome.displaySource custom bindings This patch introduces custom bindings for 'chrome.displaySource' API. These are bindings for 'startSession', 'terminateSession' methods and for the 'onSessionStarted', 'onSessionTerminated', 'onSessionErrorOccured' events. The bindings should belong to render process (i.e. be custom) as: 1) they accept dom objects arguments (MediaStreamTrack) 2) for security reasons: to keep all protocols handling within sandbox The abstract 'DisplaySourceSession' class is added to be implemented by the 'chrome.displaySource' API backends. BUG=242107 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== chrome.displaySource custom bindings This patch introduces custom bindings for 'chrome.displaySource' API. These are bindings for 'startSession', 'terminateSession' methods and for the 'onSessionStarted', 'onSessionTerminated', 'onSessionErrorOccured' events. The bindings should belong to render process (i.e. be custom) as: 1) they accept dom objects arguments (MediaStreamTrack) 2) for security reasons: to keep all protocols handling within sandbox The abstract 'DisplaySourceSession' class is added to be implemented by the 'chrome.displaySource' API backends. BUG=242107 ========== to ========== chrome.displaySource custom bindings This patch introduces custom bindings for 'chrome.displaySource' API. These are bindings for 'startSession', 'terminateSession' methods and for the 'onSessionStarted', 'onSessionTerminated', 'onSessionErrorOccured' events. The bindings should belong to render process (i.e. be custom) as: 1) they accept dom objects arguments (MediaStreamTrack) 2) for security reasons: to keep all protocols handling within sandbox The abstract 'DisplaySourceSession' class is added to be implemented by the 'chrome.displaySource' API backends. BUG=242107 Committed: https://crrev.com/79486f1c04861a577489fa63b1f82342aaee095b Cr-Commit-Position: refs/heads/master@{#364045} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/79486f1c04861a577489fa63b1f82342aaee095b Cr-Commit-Position: refs/heads/master@{#364045} |
