Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
139053003:
Chrome OS: avoid suspending on lid close when casting is active. (Closed)
|
Created:
6 years, 11 months ago by hshi1 Modified:
6 years, 11 months ago CC:
chromium-reviews, fischman+watch_chromium.org, stevenjb+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, oshima+watch_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChrome OS: avoid suspending on lid close when casting is active.
Treat casting (both tab casting and screen casting) the same as
external display to avoid suspending on lid close.
BUG=268615
TEST=verify that device is not suspended on lid close with casting is active.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245659
Patch Set 1 #Patch Set 2 : Oshima's suggestion. #
Total comments: 6
Patch Set 3 : Address comments and add unit test. #
Total comments: 1
Patch Set 4 : Remove dependencies of chrome in chromeos. #
Total comments: 2
Patch Set 5 : Remove dependencies of chrome in ash. #
Total comments: 10
Patch Set 6 : Fix nits. #Patch Set 7 : Restrict keep-awake logic to casting only (based on security origin). #Patch Set 8 : Fix compile warning of unused functions. #
Total comments: 1
Patch Set 9 : Pass security origin to MediaObserver. #
Total comments: 6
Patch Set 10 : Address nits from sergeyu, derat. #
Total comments: 4
Patch Set 11 : More nits. #Patch Set 12 : Another nit. #Messages
Total messages: 39 (0 generated)
PTAL. This CL intends to prevent Chrome OS from suspending on lid close when screen sharing (either tab capture or desktop capture) is active. +oshima@ and derat@ (OWNER of chromeos/display) +sergeyu@ (OWNER of chrome/browser/media)
can you also add a unittest for output configurator change? https://codereview.chromium.org/139053003/diff/30001/chromeos/display/output_... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/139053003/diff/30001/chromeos/display/output_... chromeos/display/output_configurator.cc:257: bool OutputConfigurator::IsProjecting() const { Since IsProjecting is always used with delegate how about just adding void SendProjectingStateToPowerManager() { } instead?
Does this need to be chromecast specific feature or do we need it for all apps using desktop capture API? I think it makes sense only for chromecast (unless we want to expose some API to enable/disable this feature). https://codereview.chromium.org/139053003/diff/30001/chrome/browser/media/med... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/139053003/diff/30001/chrome/browser/media/med... chrome/browser/media/media_capture_devices_dispatcher.cc:749: chromeos::OutputConfigurator* configurator = Can OutputConfigurator register to observe media streams using MediaCaptureDevicesDispatcher::AddObserver()? Then you wouldn't need chromeos-specific code here.
please test your new code in output_configurator_unittest.cc https://codereview.chromium.org/139053003/diff/30001/chrome/browser/media/med... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/139053003/diff/30001/chrome/browser/media/med... chrome/browser/media/media_capture_devices_dispatcher.cc:751: if (configurator) nit (maybe made obsolete by sergey's comment above): this line and the next should be indented two more spaces
PTAL. I had to touch the DEPS and gypi file so that the include dirs and rules are fixed. https://codereview.chromium.org/139053003/diff/30001/chrome/browser/media/med... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/139053003/diff/30001/chrome/browser/media/med... chrome/browser/media/media_capture_devices_dispatcher.cc:749: chromeos::OutputConfigurator* configurator = On 2014/01/15 02:51:20, Sergey Ulanov wrote: > Can OutputConfigurator register to observe media streams using > MediaCaptureDevicesDispatcher::AddObserver()? Then you wouldn't need > chromeos-specific code here. Done. https://codereview.chromium.org/139053003/diff/30001/chrome/browser/media/med... chrome/browser/media/media_capture_devices_dispatcher.cc:751: if (configurator) On 2014/01/15 15:08:55, Daniel Erat wrote: > nit (maybe made obsolete by sergey's comment above): this line and the next > should be indented two more spaces (this no longer applies) https://codereview.chromium.org/139053003/diff/30001/chromeos/display/output_... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/139053003/diff/30001/chromeos/display/output_... chromeos/display/output_configurator.cc:257: bool OutputConfigurator::IsProjecting() const { On 2014/01/15 02:26:37, oshima wrote: > Since IsProjecting is always used with delegate how about just adding > > void SendProjectingStateToPowerManager() { > } > > instead? Done.
https://codereview.chromium.org/139053003/diff/190001/chromeos/DEPS File chromeos/DEPS (right): https://codereview.chromium.org/139053003/diff/190001/chromeos/DEPS#newcode2 chromeos/DEPS:2: "+chrome/browser/media", no dependency to chrome (and subdirs) in chromeos please. can you do media stuff in ash instead of chromeos?
On 2014/01/15 19:49:05, oshima wrote: > https://codereview.chromium.org/139053003/diff/190001/chromeos/DEPS > File chromeos/DEPS (right): > > https://codereview.chromium.org/139053003/diff/190001/chromeos/DEPS#newcode2 > chromeos/DEPS:2: "+chrome/browser/media", > no dependency to chrome (and subdirs) in chromeos please. > > can you do media stuff in ash instead of chromeos? Uh, I added dependency to chrome in chromeos as a result of removing dependency to chromeos in chrome/browser (per sergeyu@'s review). The dependency is so that OutputConfigurator can add itself as an observer on the MediaCaptureDevicesDispatcher. Are you suggesting that I add ash::Shell as an observer of MediaCaptureDevicesDispatcher and then define another Observer interface to pass the event to OutputConfigurator?
On 2014/01/15 19:52:14, hshi1 wrote: > On 2014/01/15 19:49:05, oshima wrote: > > https://codereview.chromium.org/139053003/diff/190001/chromeos/DEPS > > File chromeos/DEPS (right): > > > > https://codereview.chromium.org/139053003/diff/190001/chromeos/DEPS#newcode2 > > chromeos/DEPS:2: "+chrome/browser/media", > > no dependency to chrome (and subdirs) in chromeos please. > > > > can you do media stuff in ash instead of chromeos? > > Uh, I added dependency to chrome in chromeos as a result of removing dependency > to chromeos in chrome/browser (per sergeyu@'s review). We already have chromeos dependency in chrome (https://code.google.com/p/chromium/codesearch#search/&q=%5C%2Bchromeos%20file...) and it's ok, so I don't understand what was the issue. sergeyu@ what was your concern? > The dependency is so that OutputConfigurator can add itself as an observer on > the MediaCaptureDevicesDispatcher. > > Are you suggesting that I add ash::Shell as an observer of > MediaCaptureDevicesDispatcher and then define another Observer interface to pass > the event to OutputConfigurator? You can implement MediaCaptureDevicesDispatcher::Observer somewhere in ash and call output configurator, although I think it's ok to use chromeos from chrome/browser/media.
PTAL. I have removed the dependencies to chrome in chromeos as well as chromeos in chrome. However I still need a dependency to chrome/browser/media/media_capture_devices_dispatcher.h in shell.h (see ash/DEPS), I hope that is ok with you.
https://codereview.chromium.org/139053003/diff/310001/ash/DEPS File ash/DEPS (right): https://codereview.chromium.org/139053003/diff/310001/ash/DEPS#newcode21 ash/DEPS:21: "+chrome/browser/media/media_capture_devices_dispatcher.h" ash shouldn't depend on chrome. - chromeos/ shouldn't depend on anything - ash/ can depend on chromeos/ (and apparently parts of content/?) - chrome can depend on chromeos/, ash/, and content/
On 2014/01/15 20:47:08, Daniel Erat wrote: > https://codereview.chromium.org/139053003/diff/310001/ash/DEPS > File ash/DEPS (right): > > https://codereview.chromium.org/139053003/diff/310001/ash/DEPS#newcode21 > ash/DEPS:21: "+chrome/browser/media/media_capture_devices_dispatcher.h" > ash shouldn't depend on chrome. > > - chromeos/ shouldn't depend on anything > - ash/ can depend on chromeos/ (and apparently parts of content/?) > - chrome can depend on chromeos/, ash/, and content/ Yeah, sorry I somehow missed that the this is in chrome but content. Given that media_capture_devices_dispatcher already depends on ash, you can call ash from there, and ash can call output configurator.
https://codereview.chromium.org/139053003/diff/310001/ash/DEPS File ash/DEPS (right): https://codereview.chromium.org/139053003/diff/310001/ash/DEPS#newcode21 ash/DEPS:21: "+chrome/browser/media/media_capture_devices_dispatcher.h" On 2014/01/15 20:47:08, Daniel Erat wrote: > ash shouldn't depend on chrome. > > - chromeos/ shouldn't depend on anything > - ash/ can depend on chromeos/ (and apparently parts of content/?) > - chrome can depend on chromeos/, ash/, and content/ PTAL new patch set, thanks!
I still think that this feature should only apply to Chromecast. It doesn't make sense for all screen/tab capture streams. Not LGTM. https://codereview.chromium.org/139053003/diff/370001/chrome/browser/media/me... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/139053003/diff/370001/chrome/browser/media/me... chrome/browser/media/media_capture_devices_dispatcher.cc:763: ash::Shell::GetInstance()->OnScreenSharingStateChanged(true); can ash::Shell register itself as an observer?
On 2014/01/15 22:23:44, Sergey Ulanov wrote: > I still think that this feature should only apply to Chromecast. It doesn't make > sense for all screen/tab capture streams. Not LGTM. > > https://codereview.chromium.org/139053003/diff/370001/chrome/browser/media/me... > File chrome/browser/media/media_capture_devices_dispatcher.cc (right): > > https://codereview.chromium.org/139053003/diff/370001/chrome/browser/media/me... > chrome/browser/media/media_capture_devices_dispatcher.cc:763: > ash::Shell::GetInstance()->OnScreenSharingStateChanged(true); > can ash::Shell register itself as an observer? Sergey - no ash::Shell can't register itself as observer, see Patch Set #4 which was my attempt to do so and that got rejected by derat@ because ash cannot depend on chrome.
On Wed, Jan 15, 2014 at 2:24 PM, <hshi@chromium.org> wrote: > > https://codereview.chromium.org/139053003/diff/370001/ > chrome/browser/media/media_capture_devices_dispatcher.cc#newcode763 > >> chrome/browser/media/media_capture_devices_dispatcher.cc:763: >> ash::Shell::GetInstance()->OnScreenSharingStateChanged(true); >> can ash::Shell register itself as an observer? >> > > Sergey - no ash::Shell can't register itself as observer, see Patch Set #4 > which > was my attempt to do so and that got rejected by derat@ because ash cannot > depend on chrome. > SGTM, but this is not my main concern about this CL. > > https://codereview.chromium.org/139053003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/139053003/diff/370001/chrome/browser/media/me... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/139053003/diff/370001/chrome/browser/media/me... chrome/browser/media/media_capture_devices_dispatcher.cc:763: ash::Shell::GetInstance()->OnScreenSharingStateChanged(true); On 2014/01/15 22:23:45, Sergey Ulanov wrote: > can ash::Shell register itself as an observer? like haixia said, this would only be possible if this code e.g. lived in content/ instead of chrome/ https://codereview.chromium.org/139053003/diff/370001/chromeos/display/output... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/139053003/diff/370001/chromeos/display/output... chromeos/display/output_configurator.cc:257: void OutputConfigurator::SendProjectingStateToPowerManager() { the order of methods in the .cc file should match that of the .h file https://codereview.chromium.org/139053003/diff/370001/chromeos/display/output... chromeos/display/output_configurator.cc:261: has_internal_output |= cached_outputs_[i].type == OUTPUT_TYPE_INTERNAL; nit: just do: if (cached_outputs_[i].type == OUTPUT_TYPE_INTERNAL) { has_internal_output = true; break; } https://codereview.chromium.org/139053003/diff/370001/chromeos/display/output... chromeos/display/output_configurator.cc:1138: void OutputConfigurator::OnScreenSharingStateChanged(bool started) { same here https://codereview.chromium.org/139053003/diff/370001/chromeos/display/output... chromeos/display/output_configurator.cc:1140: ++screen_sharing_count_; indent two spaces beyond the previous line, not four
It is still debated whether this is the right path to go (see bug report) but here's the updated patch with nits fixed. https://codereview.chromium.org/139053003/diff/370001/chromeos/display/output... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/139053003/diff/370001/chromeos/display/output... chromeos/display/output_configurator.cc:257: void OutputConfigurator::SendProjectingStateToPowerManager() { On 2014/01/15 22:42:59, Daniel Erat wrote: > the order of methods in the .cc file should match that of the .h file Done. https://codereview.chromium.org/139053003/diff/370001/chromeos/display/output... chromeos/display/output_configurator.cc:261: has_internal_output |= cached_outputs_[i].type == OUTPUT_TYPE_INTERNAL; On 2014/01/15 22:42:59, Daniel Erat wrote: > nit: just do: > > if (cached_outputs_[i].type == OUTPUT_TYPE_INTERNAL) { > has_internal_output = true; > break; > } Done. https://codereview.chromium.org/139053003/diff/370001/chromeos/display/output... chromeos/display/output_configurator.cc:1138: void OutputConfigurator::OnScreenSharingStateChanged(bool started) { On 2014/01/15 22:42:59, Daniel Erat wrote: > same here Done. https://codereview.chromium.org/139053003/diff/370001/chromeos/display/output... chromeos/display/output_configurator.cc:1140: ++screen_sharing_count_; On 2014/01/15 22:42:59, Daniel Erat wrote: > indent two spaces beyond the previous line, not four Done.
sergeyu: PTAL patch set #7. I have added code to pass the security origin of the MediaStreamRequest to MediaCaptureDevicesDispatcher::OnMediaRequestStateChanged() and use the URL to check for casting. Thanks!
https://codereview.chromium.org/139053003/diff/690001/content/browser/rendere... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/139053003/diff/690001/content/browser/rendere... content/browser/renderer_host/media/media_stream_manager.cc:222: ui_request_->security_origin), Note: alternatively I can just pass the GURL |security_origin| (or the entire request) in OnMediaRequestStateChanged() as a separate argument. This way I don't have to re-define MediaStreamDevice structure. Please let me know your preference, thanks!
lgtm with one nit https://codereview.chromium.org/139053003/diff/840001/chrome/browser/media/me... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/139053003/diff/840001/chrome/browser/media/me... chrome/browser/media/media_capture_devices_dispatcher.cc:124: hexencoded_origin_hash == "3C2705BC432E7C51CA8553FDC5BEE873FF2468EE" || These do not need to be hashed anymore.
LGTM with a few nits https://codereview.chromium.org/139053003/diff/840001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/139053003/diff/840001/ash/shell.h#newcode289 ash/shell.h:289: // Called when a casting session is started or stopped. nit: it looks from the rest of the code like there can be multiple concurrent casting sessions. i'm wondering if this could be made more clear through a change to the function name -- the current OnCastingStateChanged() name made me (as someone who doesn't know anything about casting) assume that there was just a single global state. maybe a name like: void OnCastingSessionStartedOrStopped(bool started); would be clearer. any opinions about this? https://codereview.chromium.org/139053003/diff/840001/chromeos/display/output... File chromeos/display/output_configurator_unittest.cc (right): https://codereview.chromium.org/139053003/diff/840001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:795: EXPECT_EQ(JoinActions(kProjectingOn, NULL), delegate_->GetActionsAndClear()); nit: after this code, maybe add: configurator_.OnCastingStateChanged(true); EXPECT_EQ(kProjectingOn, delegate_->GetActionsAndClear()); configurator_.OnCastingStateChanged(false); EXPECT_EQ(kProjectingOn, delegate_->GetActionsAndClear()); to verify that the configurator keeps a count of the number of active sessions instead of treating it as a single global state. (at least, it looks to me like each StateChanged() call with result in another projecting message being sent. if it doesn't, since the state didn't change, kNoActions is fine too) :-)
still need one more OWNERS bits for content/public +piman@ PTAL https://codereview.chromium.org/139053003/diff/840001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/139053003/diff/840001/ash/shell.h#newcode289 ash/shell.h:289: // Called when a casting session is started or stopped. On 2014/01/17 19:13:51, Daniel Erat wrote: > nit: it looks from the rest of the code like there can be multiple concurrent > casting sessions. i'm wondering if this could be made more clear through a > change to the function name -- the current OnCastingStateChanged() name made me > (as someone who doesn't know anything about casting) assume that there was just > a single global state. maybe a name like: > > void OnCastingSessionStartedOrStopped(bool started); > > would be clearer. any opinions about this? Done. https://codereview.chromium.org/139053003/diff/840001/chrome/browser/media/me... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/139053003/diff/840001/chrome/browser/media/me... chrome/browser/media/media_capture_devices_dispatcher.cc:124: hexencoded_origin_hash == "3C2705BC432E7C51CA8553FDC5BEE873FF2468EE" || On 2014/01/17 18:39:32, Sergey Ulanov wrote: > These do not need to be hashed anymore. Done (removed hash for public release ID; left a TODO for the trusted tester ID) https://codereview.chromium.org/139053003/diff/840001/chromeos/display/output... File chromeos/display/output_configurator_unittest.cc (right): https://codereview.chromium.org/139053003/diff/840001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:795: EXPECT_EQ(JoinActions(kProjectingOn, NULL), delegate_->GetActionsAndClear()); On 2014/01/17 19:13:51, Daniel Erat wrote: > nit: after this code, maybe add: > > configurator_.OnCastingStateChanged(true); > EXPECT_EQ(kProjectingOn, delegate_->GetActionsAndClear()); > configurator_.OnCastingStateChanged(false); > EXPECT_EQ(kProjectingOn, delegate_->GetActionsAndClear()); > > to verify that the configurator keeps a count of the number of active sessions > instead of treating it as a single global state. > > (at least, it looks to me like each StateChanged() call with result in another > projecting message being sent. if it doesn't, since the state didn't change, > kNoActions is fine too) :-) Done.
https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output... File chromeos/display/output_configurator_unittest.cc (right): https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:795: EXPECT_EQ(JoinActions(kProjectingOn, NULL), delegate_->GetActionsAndClear()); nit: you should be able to just do: EXPECT_EQ(kProjectingOn, delegate_->GetActionsAndClear()); when there's a single action (JoinActions() is just used to comma-join multiple actions)
https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output... File chromeos/display/output_configurator_unittest.cc (right): https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:795: EXPECT_EQ(JoinActions(kProjectingOn, NULL), delegate_->GetActionsAndClear()); On 2014/01/17 20:21:15, Daniel Erat wrote: > nit: you should be able to just do: > > EXPECT_EQ(kProjectingOn, delegate_->GetActionsAndClear()); > > when there's a single action (JoinActions() is just used to comma-join multiple > actions) Done, thanks!
lgtm with a nit https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output... chromeos/display/output_configurator.cc:610: --casting_session_count_; nit: you may want to reset to 0 if it goes blow 0 as a fallback. (just for the safety as it messes up power state).
https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output... chromeos/display/output_configurator.cc:610: --casting_session_count_; On 2014/01/17 20:57:36, oshima wrote: > nit: you may want to reset to 0 if it goes blow 0 as a fallback. (just for the > safety as it messes up power state). Done.
LGTM for content, but, rather than whitelisting extensions, shouldn't this be an extension API?
On 2014/01/17 21:06:18, piman wrote: > LGTM for content, but, rather than whitelisting extensions, shouldn't this be an > extension API? Yes sergeyu@ proposed to add an extension API to keep awake with lid closed (see http://crbug.com/268615#c32) but derat@ expressed concern that a public API may be abused to harm the system (see http://crbug.com/268615#c34).
On Fri, Jan 17, 2014 at 1:08 PM, <hshi@chromium.org> wrote: > On 2014/01/17 21:06:18, piman wrote: > >> LGTM for content, but, rather than whitelisting extensions, shouldn't >> this be >> > an > >> extension API? >> > > Yes sergeyu@ proposed to add an extension API to keep awake with lid > closed (see > http://crbug.com/268615#c32) but derat@ expressed concern that a public > API may > be abused to harm the system (see http://crbug.com/268615#c34). > We should obviously ask for permissions. But I think it's much better for the ecosystem to not give magic implicit capabilities to Google-authored extensions. > https://codereview.chromium.org/139053003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> But I think it's much better for the ecosystem to not give magic implicit > capabilities to Google-authored extensions. Whitelisting is a short-term solution but still necessary for the time being. We already have whitelisting in MediaCaptureDevicesDispatcher to grant camera/microphone and screen capture access to specific extensions. Per markdavidscott@ cast will switch to use the new chrome.cast.streaming API, which hopefully would eliminate the need of whitelisting cast extensions.
On 2014/01/17 21:14:10, piman wrote: > On Fri, Jan 17, 2014 at 1:08 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=hshi@chromium.org> wrote: > > > On 2014/01/17 21:06:18, piman wrote: > > > >> LGTM for content, but, rather than whitelisting extensions, shouldn't > >> this be > >> > > an > > > >> extension API? > >> > > > > Yes sergeyu@ proposed to add an extension API to keep awake with lid > > closed (see > > http://crbug.com/268615#c32) but derat@ expressed concern that a public > > API may > > be abused to harm the system (see http://crbug.com/268615#c34). > > > > We should obviously ask for permissions. > > But I think it's much better for the ecosystem to not give magic implicit > capabilities to Google-authored extensions. It's not clear to me that preventing the system from suspending while the lid is closed is something that we want to allow apps to do. "Docked mode" is a built-in feature where we stay awake while the lid is closed with an external display connected so the user can effectively use their Chromebook as a desktop machine. This change is a continuation of that feature: http://crbug.com/268615 was filed to request similar behavior when casting from a Chromebook (i.e. it's dumb for users to need to keep their Chromebook's lid slightly open while they're tab-casting). Unless there are other cases where an app needs to send video data to external displays that aren't already covered by DP/HDMI or tab-casting, I don't know that we want to permit overriding the lid-closed action in general. > > > https://codereview.chromium.org/139053003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....
On Fri, Jan 17, 2014 at 1:26 PM, <derat@chromium.org> wrote: > On 2014/01/17 21:14:10, piman wrote: > >> On Fri, Jan 17, 2014 at 1:08 PM, >> > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=hshi@chromium.org> > wrote: > > > On 2014/01/17 21:06:18, piman wrote: >> > >> >> LGTM for content, but, rather than whitelisting extensions, shouldn't >> >> this be >> >> >> > an >> > >> >> extension API? >> >> >> > >> > Yes sergeyu@ proposed to add an extension API to keep awake with lid >> > closed (see >> > http://crbug.com/268615#c32) but derat@ expressed concern that a public >> > API may >> > be abused to harm the system (see http://crbug.com/268615#c34). >> > >> > > We should obviously ask for permissions. >> > > But I think it's much better for the ecosystem to not give magic implicit >> capabilities to Google-authored extensions. >> > > It's not clear to me that preventing the system from suspending while the > lid is > closed is something that we want to allow apps to do. > If so, what makes ChromeCast special? > > "Docked mode" is a built-in feature where we stay awake while the lid is > closed > with an external display connected so the user can effectively use their > Chromebook as a desktop machine. This change is a continuation of that > feature: > http://crbug.com/268615 was filed to request similar behavior when > casting from > a Chromebook (i.e. it's dumb for users to need to keep their Chromebook's > lid > slightly open while they're tab-casting). Unless there are other cases > where an > app needs to send video data to external displays that aren't already > covered by > DP/HDMI or tab-casting, I don't know that we want to permit overriding the > lid-closed action in general. > > > > https://codereview.chromium.org/139053003/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to >> > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium- > reviews+unsubscribe@chromium.org. > > > https://codereview.chromium.org/139053003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/01/17 21:33:20, piman wrote: > On Fri, Jan 17, 2014 at 1:26 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=derat@chromium.org> wrote: > > > On 2014/01/17 21:14:10, piman wrote: > > > >> On Fri, Jan 17, 2014 at 1:08 PM, > >> > > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=hshi%40chromium.org> > > wrote: > > > > > On 2014/01/17 21:06:18, piman wrote: > >> > > >> >> LGTM for content, but, rather than whitelisting extensions, shouldn't > >> >> this be > >> >> > >> > an > >> > > >> >> extension API? > >> >> > >> > > >> > Yes sergeyu@ proposed to add an extension API to keep awake with lid > >> > closed (see > >> > http://crbug.com/268615#c32) but derat@ expressed concern that a public > >> > API may > >> > be abused to harm the system (see http://crbug.com/268615#c34). > >> > > >> > > > > We should obviously ask for permissions. > >> > > > > But I think it's much better for the ecosystem to not give magic implicit > >> capabilities to Google-authored extensions. > >> > > > > It's not clear to me that preventing the system from suspending while the > > lid is > > closed is something that we want to allow apps to do. > > > > If so, what makes ChromeCast special? It is effectively an external display. If/when there are other apps that also send video, there would not be anything special about the Chromecast app. > > > > > "Docked mode" is a built-in feature where we stay awake while the lid is > > closed > > with an external display connected so the user can effectively use their > > Chromebook as a desktop machine. This change is a continuation of that > > feature: > > http://crbug.com/268615 was filed to request similar behavior when > > casting from > > a Chromebook (i.e. it's dumb for users to need to keep their Chromebook's > > lid > > slightly open while they're tab-casting). Unless there are other cases > > where an > > app needs to send video data to external displays that aren't already > > covered by > > DP/HDMI or tab-casting, I don't know that we want to permit overriding the > > lid-closed action in general. > > > > > > > https://codereview.chromium.org/139053003/ > >> > > >> > > > > To unsubscribe from this group and stop receiving emails from it, send an > >> > > email > > > >> to > >> > > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium- > > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=reviews+unsubscribe@chromi.... > > > > > > https://codereview.chromium.org/139053003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/139053003/1230001
On Fri, Jan 17, 2014 at 1:40 PM, <derat@chromium.org> wrote: > On 2014/01/17 21:33:20, piman wrote: > >> On Fri, Jan 17, 2014 at 1:26 PM, >> > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=derat@chromium.org> > wrote: > > > On 2014/01/17 21:14:10, piman wrote: >> > >> >> On Fri, Jan 17, 2014 at 1:08 PM, >> >> >> > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=hshi%40chromium.org >> > >> >> > wrote: >> > >> > > On 2014/01/17 21:06:18, piman wrote: >> >> > >> >> >> LGTM for content, but, rather than whitelisting extensions, >> shouldn't >> >> >> this be >> >> >> >> >> > an >> >> > >> >> >> extension API? >> >> >> >> >> > >> >> > Yes sergeyu@ proposed to add an extension API to keep awake with lid >> >> > closed (see >> >> > http://crbug.com/268615#c32) but derat@ expressed concern that a >> public >> >> > API may >> >> > be abused to harm the system (see http://crbug.com/268615#c34). >> >> > >> >> >> > >> > We should obviously ask for permissions. >> >> >> > >> > But I think it's much better for the ecosystem to not give magic >> implicit >> >> capabilities to Google-authored extensions. >> >> >> > >> > It's not clear to me that preventing the system from suspending while >> the >> > lid is >> > closed is something that we want to allow apps to do. >> > >> > > If so, what makes ChromeCast special? >> > > It is effectively an external display. It's an extension... > If/when there are other apps that also > send video, there would not be anything special about the Chromecast app. Well, with this change, one cannot write an extension that has an equivalent behavior to the ChromeCast extension, but talks to a different device or application, and have the same capabilities. > > > > > >> > "Docked mode" is a built-in feature where we stay awake while the lid is >> > closed >> > with an external display connected so the user can effectively use their >> > Chromebook as a desktop machine. This change is a continuation of that >> > feature: >> > http://crbug.com/268615 was filed to request similar behavior when >> > casting from >> > a Chromebook (i.e. it's dumb for users to need to keep their >> Chromebook's >> > lid >> > slightly open while they're tab-casting). Unless there are other cases >> > where an >> > app needs to send video data to external displays that aren't already >> > covered by >> > DP/HDMI or tab-casting, I don't know that we want to permit overriding >> the >> > lid-closed action in general. >> > >> > >> > > https://codereview.chromium.org/139053003/ >> >> > >> >> >> > >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> >> >> > email >> > >> >> to >> >> >> > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium- >> > >> > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=reviews+ > unsubscribe@chromium.org. > > > >> > >> > https://codereview.chromium.org/139053003/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to >> > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium- > reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/139053003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> Well, with this change, one cannot write an extension that has an > equivalent behavior to the ChromeCast extension, but talks to a different > device or application, and have the same capabilities. One cannot do that before this CL either. As I mentioned, there's already whitelists that grants cast extensions privileged access to desktop capture. The general casting capability will become available when chrome.cast.streaming API is available (currently shooting for M35 but may be later)
Retried try job too often on linux_aura for step(s) compositor_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/139053003/1230001
Message was sent while issue was closed.
Change committed as 245659 |
