Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(50)

Issue 139053003: Chrome OS: avoid suspending on lid close when casting is active. (Closed)

Created:
5 years, 10 months ago by hshi1
Modified:
5 years, 10 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
Visibility:
Public.

Description

Chrome 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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+141 lines, -24 lines) Patch
M ash/shell.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M ash/shell.cc View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/media/media_capture_devices_dispatcher.h View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/media/media_capture_devices_dispatcher.cc View 1 2 3 4 5 6 7 8 9 6 chunks +43 lines, -4 lines 0 comments Download
M chromeos/display/output_configurator.h View 1 2 3 4 5 6 7 8 9 3 chunks +9 lines, -0 lines 0 comments Download
M chromeos/display/output_configurator.cc View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +32 lines, -17 lines 0 comments Download
M chromeos/display/output_configurator_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +41 lines, -0 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_manager.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/media/mock_media_observer.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -2 lines 0 comments Download
M content/public/browser/media_observer.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 39 (0 generated)
hshi1
PTAL. This CL intends to prevent Chrome OS from suspending on lid close when screen ...
5 years, 10 months ago (2014-01-15 02:00:43 UTC) #1
oshima
can you also add a unittest for output configurator change? https://codereview.chromium.org/139053003/diff/30001/chromeos/display/output_configurator.cc File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/139053003/diff/30001/chromeos/display/output_configurator.cc#newcode257 ...
5 years, 10 months ago (2014-01-15 02:26:37 UTC) #2
Sergey Ulanov
Does this need to be chromecast specific feature or do we need it for all ...
5 years, 10 months ago (2014-01-15 02:51:20 UTC) #3
Daniel Erat
please test your new code in output_configurator_unittest.cc https://codereview.chromium.org/139053003/diff/30001/chrome/browser/media/media_capture_devices_dispatcher.cc File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/139053003/diff/30001/chrome/browser/media/media_capture_devices_dispatcher.cc#newcode751 chrome/browser/media/media_capture_devices_dispatcher.cc:751: if (configurator) ...
5 years, 10 months ago (2014-01-15 15:08:54 UTC) #4
hshi1
PTAL. I had to touch the DEPS and gypi file so that the include dirs ...
5 years, 10 months ago (2014-01-15 19:43:23 UTC) #5
oshima
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 ...
5 years, 10 months ago (2014-01-15 19:49:05 UTC) #6
hshi1
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 > ...
5 years, 10 months ago (2014-01-15 19:52:14 UTC) #7
oshima
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 > ...
5 years, 10 months ago (2014-01-15 20:09:21 UTC) #8
hshi1
PTAL. I have removed the dependencies to chrome in chromeos as well as chromeos in ...
5 years, 10 months ago (2014-01-15 20:32:38 UTC) #9
Daniel Erat
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 ...
5 years, 10 months ago (2014-01-15 20:47:08 UTC) #10
oshima
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 ...
5 years, 10 months ago (2014-01-15 20:55:33 UTC) #11
hshi1
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 ...
5 years, 10 months ago (2014-01-15 21:49:12 UTC) #12
Sergey Ulanov
I still think that this feature should only apply to Chromecast. It doesn't make sense ...
5 years, 10 months ago (2014-01-15 22:23:44 UTC) #13
hshi1
On 2014/01/15 22:23:44, Sergey Ulanov wrote: > I still think that this feature should only ...
5 years, 10 months ago (2014-01-15 22:24:40 UTC) #14
Sergey Ulanov
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 ...
5 years, 10 months ago (2014-01-15 22:33:58 UTC) #15
Daniel Erat
https://codereview.chromium.org/139053003/diff/370001/chrome/browser/media/media_capture_devices_dispatcher.cc File chrome/browser/media/media_capture_devices_dispatcher.cc (right): 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); On 2014/01/15 22:23:45, Sergey Ulanov wrote: > can ...
5 years, 10 months ago (2014-01-15 22:42:59 UTC) #16
hshi1
It is still debated whether this is the right path to go (see bug report) ...
5 years, 10 months ago (2014-01-16 02:01:44 UTC) #17
hshi1
sergeyu: PTAL patch set #7. I have added code to pass the security origin of ...
5 years, 10 months ago (2014-01-16 22:49:58 UTC) #18
hshi1
https://codereview.chromium.org/139053003/diff/690001/content/browser/renderer_host/media/media_stream_manager.cc File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/139053003/diff/690001/content/browser/renderer_host/media/media_stream_manager.cc#newcode222 content/browser/renderer_host/media/media_stream_manager.cc:222: ui_request_->security_origin), Note: alternatively I can just pass the GURL ...
5 years, 10 months ago (2014-01-17 02:21:25 UTC) #19
Sergey Ulanov
lgtm with one nit https://codereview.chromium.org/139053003/diff/840001/chrome/browser/media/media_capture_devices_dispatcher.cc File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/139053003/diff/840001/chrome/browser/media/media_capture_devices_dispatcher.cc#newcode124 chrome/browser/media/media_capture_devices_dispatcher.cc:124: hexencoded_origin_hash == "3C2705BC432E7C51CA8553FDC5BEE873FF2468EE" || These ...
5 years, 10 months ago (2014-01-17 18:39:32 UTC) #20
Daniel Erat
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 ...
5 years, 10 months ago (2014-01-17 19:13:50 UTC) #21
hshi1
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 ...
5 years, 10 months ago (2014-01-17 19:31:28 UTC) #22
Daniel Erat
https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output_configurator_unittest.cc File chromeos/display/output_configurator_unittest.cc (right): https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output_configurator_unittest.cc#newcode795 chromeos/display/output_configurator_unittest.cc:795: EXPECT_EQ(JoinActions(kProjectingOn, NULL), delegate_->GetActionsAndClear()); nit: you should be able to ...
5 years, 10 months ago (2014-01-17 20:21:14 UTC) #23
hshi1
https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output_configurator_unittest.cc File chromeos/display/output_configurator_unittest.cc (right): https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output_configurator_unittest.cc#newcode795 chromeos/display/output_configurator_unittest.cc:795: EXPECT_EQ(JoinActions(kProjectingOn, NULL), delegate_->GetActionsAndClear()); On 2014/01/17 20:21:15, Daniel Erat wrote: ...
5 years, 10 months ago (2014-01-17 20:52:52 UTC) #24
oshima
lgtm with a nit https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output_configurator.cc File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output_configurator.cc#newcode610 chromeos/display/output_configurator.cc:610: --casting_session_count_; nit: you may want ...
5 years, 10 months ago (2014-01-17 20:57:35 UTC) #25
hshi1
https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output_configurator.cc File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/139053003/diff/930002/chromeos/display/output_configurator.cc#newcode610 chromeos/display/output_configurator.cc:610: --casting_session_count_; On 2014/01/17 20:57:36, oshima wrote: > nit: you ...
5 years, 10 months ago (2014-01-17 21:01:36 UTC) #26
piman
LGTM for content, but, rather than whitelisting extensions, shouldn't this be an extension API?
5 years, 10 months ago (2014-01-17 21:06:18 UTC) #27
hshi1
On 2014/01/17 21:06:18, piman wrote: > LGTM for content, but, rather than whitelisting extensions, shouldn't ...
5 years, 10 months ago (2014-01-17 21:08:48 UTC) #28
piman
On Fri, Jan 17, 2014 at 1:08 PM, <hshi@chromium.org> wrote: > On 2014/01/17 21:06:18, piman ...
5 years, 10 months ago (2014-01-17 21:14:10 UTC) #29
hshi1
> But I think it's much better for the ecosystem to not give magic implicit ...
5 years, 10 months ago (2014-01-17 21:21:09 UTC) #30
Daniel Erat
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> ...
5 years, 10 months ago (2014-01-17 21:26:56 UTC) #31
piman
On Fri, Jan 17, 2014 at 1:26 PM, <derat@chromium.org> wrote: > On 2014/01/17 21:14:10, piman ...
5 years, 10 months ago (2014-01-17 21:33:20 UTC) #32
Daniel Erat
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> ...
5 years, 10 months ago (2014-01-17 21:40:26 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/139053003/1230001
5 years, 10 months ago (2014-01-17 21:56:00 UTC) #34
piman
On Fri, Jan 17, 2014 at 1:40 PM, <derat@chromium.org> wrote: > On 2014/01/17 21:33:20, piman ...
5 years, 10 months ago (2014-01-17 22:11:55 UTC) #35
hshi1
> Well, with this change, one cannot write an extension that has an > equivalent ...
5 years, 10 months ago (2014-01-17 22:16:02 UTC) #36
commit-bot: I haz the power
Retried try job too often on linux_aura for step(s) compositor_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=113467
5 years, 10 months ago (2014-01-17 23:00:03 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/139053003/1230001
5 years, 10 months ago (2014-01-17 23:03:05 UTC) #38
commit-bot: I haz the power
5 years, 10 months ago (2014-01-17 23:45:36 UTC) #39
Message was sent while issue was closed.
Change committed as 245659

Powered by Google App Engine
This is Rietveld 408576698