Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
503503003:
Documentation updates for chrome.tabCapture. (Closed)
|
Created:
6 years, 3 months ago by mark a. foltz Modified:
6 years, 3 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionDocumentation updates for chrome.tabCapture.
BUG=338449
Committed: https://crrev.com/93727f756a29c617d0f321386e09bf8a89eefc96
Cr-Commit-Position: refs/heads/master@{#292474}
Patch Set 1 #
Total comments: 7
Patch Set 2 : #Patch Set 3 : Clarify activeTab comment. #Messages
Total messages: 14 (0 generated)
mfoltz@chromium.org changed reviewers: + kalman@chromium.org, miu@chromium.org
lgtm, with one change for completeness: https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/tab_capture.idl (right): https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/tab_capture.idl:54: // across page navigations within the tab, and stops when the tab is closed. s/when the tab is closed/when the tab is closed by the user, or the media stream is closed by the extension/.
https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/tab_capture.idl (right): https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/tab_capture.idl:54: // across page navigations within the tab, and stops when the tab is closed. On 2014/08/28 02:37:26, miu wrote: > s/when the tab is closed/when the tab is closed by the user, or the media stream > is closed by the extension/. Are you sure it's maintained across page navigations? This isn't how activeTab works. https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/tab_capture.idl:56: // |callback| : Callback with either the tab capture stream or |null|. <code>null</code> looks better than |null| when rendered.
https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/tab_capture.idl (right): https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/tab_capture.idl:54: // across page navigations within the tab, and stops when the tab is closed. On 2014/08/28 02:37:26, miu wrote: > s/when the tab is closed/when the tab is closed by the user, or the media stream > is closed by the extension/. Done. https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/tab_capture.idl:54: // across page navigations within the tab, and stops when the tab is closed. On 2014/08/28 02:44:31, kalman wrote: > On 2014/08/28 02:37:26, miu wrote: > > s/when the tab is closed/when the tab is closed by the user, or the media > stream > > is closed by the extension/. > > Are you sure it's maintained across page navigations? This isn't how activeTab > works. Yes. Removed reference to ActiveTab. https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/tab_capture.idl:56: // |callback| : Callback with either the tab capture stream or |null|. On 2014/08/28 02:44:31, kalman wrote: > <code>null</code> looks better than |null| when rendered. Done.
https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/tab_capture.idl (right): https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/tab_capture.idl:54: // across page navigations within the tab, and stops when the tab is closed. On 2014/08/28 17:58:55, mark a. foltz wrote: > On 2014/08/28 02:37:26, miu wrote: > > s/when the tab is closed/when the tab is closed by the user, or the media > stream > > is closed by the extension/. > > Done. It seems to me from reading the code that tabCapture is lost as well on navigation: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... (it's enable for the tab by granting the tabCaptureForTab permission, line 57). Fun thing: just above there (line 104) I have an experiment where we *do* maintain these transient permissions for same-origin navigation.
On 2014/08/28 18:21:54, kalman wrote: > https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... > File chrome/common/extensions/api/tab_capture.idl (right): > > https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/tab_capture.idl:54: // across page navigations > within the tab, and stops when the tab is closed. > On 2014/08/28 17:58:55, mark a. foltz wrote: > > On 2014/08/28 02:37:26, miu wrote: > > > s/when the tab is closed/when the tab is closed by the user, or the media > > stream > > > is closed by the extension/. > > > > Done. > > It seems to me from reading the code that tabCapture is lost as well on > navigation: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > (it's enable for the tab by granting the tabCaptureForTab permission, line 57). > > Fun thing: just above there (line 104) I have an experiment where we *do* > maintain these transient permissions for same-origin navigation. Uh, if that were the case that would be a serious regression in our API. I just tested on 37 beta and it works as expected (both for in-origin and cross-origin navigations). The stream is *only* closed when the tab closes, the extension unloads or at client request.
On 2014/08/28 18:27:02, mark a. foltz wrote: > On 2014/08/28 18:21:54, kalman wrote: > > > https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... > > File chrome/common/extensions/api/tab_capture.idl (right): > > > > > https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... > > chrome/common/extensions/api/tab_capture.idl:54: // across page navigations > > within the tab, and stops when the tab is closed. > > On 2014/08/28 17:58:55, mark a. foltz wrote: > > > On 2014/08/28 02:37:26, miu wrote: > > > > s/when the tab is closed/when the tab is closed by the user, or the media > > > stream > > > > is closed by the extension/. > > > > > > Done. > > > > It seems to me from reading the code that tabCapture is lost as well on > > navigation: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > (it's enable for the tab by granting the tabCaptureForTab permission, line > 57). > > > > Fun thing: just above there (line 104) I have an experiment where we *do* > > maintain these transient permissions for same-origin navigation. > > Uh, if that were the case that would be a serious regression in our API. I just > tested on 37 beta and it works as expected (both for in-origin and cross-origin > navigations). The stream is *only* closed when the tab closes, the extension > unloads or at client request. Ah ok maybe this is due to however the tabCapture API is implemented - you might start capturing and just not stop it on navigation. I.e. not test the presence of that permission every time. That makes sense - lgtm.
On 2014/08/28 18:31:27, kalman wrote: > On 2014/08/28 18:27:02, mark a. foltz wrote: > > On 2014/08/28 18:21:54, kalman wrote: > > > > > > https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... > > > File chrome/common/extensions/api/tab_capture.idl (right): > > > > > > > > > https://codereview.chromium.org/503503003/diff/1/chrome/common/extensions/api... > > > chrome/common/extensions/api/tab_capture.idl:54: // across page navigations > > > within the tab, and stops when the tab is closed. > > > On 2014/08/28 17:58:55, mark a. foltz wrote: > > > > On 2014/08/28 02:37:26, miu wrote: > > > > > s/when the tab is closed/when the tab is closed by the user, or the > media > > > > stream > > > > > is closed by the extension/. > > > > > > > > Done. > > > > > > It seems to me from reading the code that tabCapture is lost as well on > > > navigation: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > > > (it's enable for the tab by granting the tabCaptureForTab permission, line > > 57). > > > > > > Fun thing: just above there (line 104) I have an experiment where we *do* > > > maintain these transient permissions for same-origin navigation. > > > > Uh, if that were the case that would be a serious regression in our API. I > just > > tested on 37 beta and it works as expected (both for in-origin and > cross-origin > > navigations). The stream is *only* closed when the tab closes, the extension > > unloads or at client request. > > Ah ok maybe this is due to however the tabCapture API is implemented - you might > start capturing and just not stop it on navigation. I.e. not test the presence > of that permission every time. That makes sense - lgtm. Ok, I see how that comment could be confusing. I clarified the wording bit to indicate we check permissions on start of capture.
The CQ bit was checked by mfoltz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfoltz@chromium.org/503503003/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as fea7a010f1e8b9a8f2097d1377ec19a6d15c8960
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/93727f756a29c617d0f321386e09bf8a89eefc96 Cr-Commit-Position: refs/heads/master@{#292474} |
