Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1214883004:
Fixed the audio backgrounding bug (Closed)
|
Created:
5 years, 5 months ago by sebsg Modified:
5 years, 2 months ago CC:
chromium-reviews, creis+watch_chromium.org, posciak+watch_chromium.org, nasko+codewatch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org, Georges Khalil Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixed the audio backgrounding bug.
Fixed the fact that we don't background a tab that initially skipped backgrounding because audio was playing (at the moment the tab was hidden) when the audio stops.
We now make calls to UpdateProcessPriority when audio starts or stops.
BUG=491895
Committed: https://crrev.com/349188e9ed037b427815c9e5ad55223d043ab6fd
Cr-Commit-Position: refs/heads/master@{#352421}
Patch Set 1 #Patch Set 2 : Fixed a comment #
Total comments: 4
Patch Set 3 : Made changes as suggested by dalecurtis@chromium.org #
Total comments: 2
Patch Set 4 : Removed an unnecessary atomic operation #
Total comments: 18
Patch Set 5 : Adressed comments made by gab@chromium.org #Patch Set 6 : Changed method names #Patch Set 7 : Added a check to null before calling the render_process_host #
Total comments: 12
Patch Set 8 : Nits and fixes for the review #Patch Set 9 : Comment change #
Total comments: 8
Patch Set 10 : Implemented nick's suggestion #
Total comments: 30
Patch Set 11 : review comments + tests #Patch Set 12 : Fixed android bug #Patch Set 13 : Rebased and fixed the tests for macosx #
Total comments: 33
Patch Set 14 : Refactored tests #
Total comments: 2
Patch Set 15 : Forced the MACOSX include #Patch Set 16 : Added new line at eof #
Total comments: 13
Patch Set 17 : Addressed gab's comments #
Total comments: 8
Patch Set 18 : Addressed avi's comments #Patch Set 19 : Added method to get mach port #
Total comments: 20
Patch Set 20 : Rebased #Patch Set 21 : Addressed comments #Patch Set 22 : Rebased #Patch Set 23 : Refactored #Patch Set 24 : Temporary android fix #
Total comments: 8
Patch Set 25 : Addressed comments #
Total comments: 6
Patch Set 26 : Addressed comments #
Total comments: 5
Patch Set 27 : Rebase #
Total comments: 2
Patch Set 28 : Fixed bad rebase merge #Messages
Total messages: 168 (70 generated)
sebsg@chromium.org changed reviewers: + gab@chromium.org
Fixed for the bug for when the music stops from a backgrounded tab. Also added the code to un-background a tab if audio starts playing from it.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
https://codereview.chromium.org/1214883004/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1214883004/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:656: if (num_playing_streams_ == 1) { While thread safe given the current usage patterns, this should instead be "if (base::AtomicRefCountIsOne(...))" https://codereview.chromium.org/1214883004/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:672: if (num_playing_streams_ == 0) { Ditto, use "if (!base::AtomicRefCountDec(...))" for thread-safety.
Made changes as suggested by dalecurtis@chromium.org https://codereview.chromium.org/1214883004/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1214883004/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:656: if (num_playing_streams_ == 1) { On 2015/06/29 22:04:16, DaleCurtis wrote: > While thread safe given the current usage patterns, this should instead be "if > (base::AtomicRefCountIsOne(...))" Done. https://codereview.chromium.org/1214883004/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:672: if (num_playing_streams_ == 0) { On 2015/06/29 22:04:16, DaleCurtis wrote: > Ditto, use "if (!base::AtomicRefCountDec(...))" for thread-safety. Done.
https://codereview.chromium.org/1214883004/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1214883004/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:672: if (base::AtomicRefCountIsZero(&num_playing_streams_)) { You can use the result of AtomicRefCountDec instead of doing a whole new atomic operation.
Removed an unnecessary atomic operation https://codereview.chromium.org/1214883004/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1214883004/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:672: if (base::AtomicRefCountIsZero(&num_playing_streams_)) { On 2015/06/29 22:21:40, DaleCurtis wrote: > You can use the result of AtomicRefCountDec instead of doing a whole new atomic > operation. Done.
media/ lgtm % comment changes. https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:655: // If it's the first audio stream to start for this renderer. Comment doesn't add much value, explain why instead of just transliterating the conditional. https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:670: // If it was the last audio stream playing for this renderer. Ditto.
Awesome work, some comments below. Also, please expand the CL description to explain more precisely what it does and why. Cheers, Gab https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:663: base::Unretained(render_process_host))); base::Unretained() here is incorrect because there are no guarantees that |render_process_host| will still be around by the time this task is run (which could result in a use-after-free). For educational purposes, here are a couple ways to go about passing the |this| pointer: 1) Use Unretained if you have lifetime guarantees (you don't here). 2) Use refcounting (default if not using any bind helper): only an option if the object implements RefCounted (RenderProcessHost doesn't) AND it makes sense for your task to prevent the object from being deleted (in this case it doesn't either). 3) Use a WeakPtr: only an option if the object vends WeakPtrs (e.g. implements SupportsWeakPtr or has a WeakPtrFactory) -- not the case here either, but would have been a nice option (WeakPtrs will check that they are still valid before running the callback). None of the above actually work great here so here's what I suggest, instead of binding to the RenderProcessHost object right away, add a method in this task's anonymous namespace that runs on the UI thread and takes the |render_process_id_| as an argument, from it obtain the |render_process_host| pointer and call the RenderProcessHost's AudioFoo() methods directly. In fact, as I'm done writing this I had a thought that you probably shouldn't call RenderProcessHost::FromID() outside the UI thread. And indeed you shouldn't: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... So this is really the only option (and you should enable dcheck_always_on=1 in your build ASAP!). https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1098: // If the backgrounding was skipped because audio was playing, we background -we (in general we try to avoid "we" in comments), e.g. // If backgrounding was skipped because audio was playing, background the tab now. https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1107: // If audio starts from a backgrounded tab, we un-background it. // If audio starts playing from a backgrounded tab, unbackground it. https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1112: // it is backgrounded again when the video finishes. // Since it's not a visible tab, it needs to be marked for backgrounding again so that it is re-backgrounded if/when the audio stops. https://codereview.chromium.org/1214883004/diff/60001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/1214883004/diff/60001/content/public/browser/... content/public/browser/render_process_host.h:104: // called on the IO thread. s/IO/UI/ https://codereview.chromium.org/1214883004/diff/60001/content/public/browser/... content/public/browser/render_process_host.h:108: // Must be called on the IO thread. s/IO/UI/
https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:663: base::Unretained(render_process_host))); On 2015/06/30 13:41:40, gab wrote: > base::Unretained() here is incorrect because there are no guarantees that > |render_process_host| will still be around by the time this task is run (which > could result in a use-after-free). > > For educational purposes, here are a couple ways to go about passing the |this| > pointer: > > 1) Use Unretained if you have lifetime guarantees (you don't here). > > 2) Use refcounting (default if not using any bind helper): only an option if the > object implements RefCounted (RenderProcessHost doesn't) AND it makes sense for > your task to prevent the object from being deleted (in this case it doesn't > either). > > 3) Use a WeakPtr: only an option if the object vends WeakPtrs (e.g. implements > SupportsWeakPtr or has a WeakPtrFactory) -- not the case here either, but would > have been a nice option (WeakPtrs will check that they are still valid before > running the callback). > > None of the above actually work great here so here's what I suggest, instead of > binding to the RenderProcessHost object right away, add a method in this task's > anonymous namespace that runs on the UI thread and takes the > |render_process_id_| as an argument, from it obtain the |render_process_host| > pointer and call the RenderProcessHost's AudioFoo() methods directly. > > In fact, as I'm done writing this I had a thought that you probably shouldn't > call RenderProcessHost::FromID() outside the UI thread. And indeed you > shouldn't: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > So this is really the only option (and you should enable dcheck_always_on=1 in > your build ASAP!). Oops, thanks for catching this. The last method is in fact what should be done here. I had a brain fart and thought this was attaching to the browser process, so if it went poof everything went with it.
Last changes without the test https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:655: // If it's the first audio stream to start for this renderer. On 2015/06/29 22:44:23, DaleCurtis wrote: > Comment doesn't add much value, explain why instead of just transliterating the > conditional. Done. https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:663: base::Unretained(render_process_host))); On 2015/06/30 13:41:40, gab wrote: > base::Unretained() here is incorrect because there are no guarantees that > |render_process_host| will still be around by the time this task is run (which > could result in a use-after-free). > > For educational purposes, here are a couple ways to go about passing the |this| > pointer: > > 1) Use Unretained if you have lifetime guarantees (you don't here). > > 2) Use refcounting (default if not using any bind helper): only an option if the > object implements RefCounted (RenderProcessHost doesn't) AND it makes sense for > your task to prevent the object from being deleted (in this case it doesn't > either). > > 3) Use a WeakPtr: only an option if the object vends WeakPtrs (e.g. implements > SupportsWeakPtr or has a WeakPtrFactory) -- not the case here either, but would > have been a nice option (WeakPtrs will check that they are still valid before > running the callback). > > None of the above actually work great here so here's what I suggest, instead of > binding to the RenderProcessHost object right away, add a method in this task's > anonymous namespace that runs on the UI thread and takes the > |render_process_id_| as an argument, from it obtain the |render_process_host| > pointer and call the RenderProcessHost's AudioFoo() methods directly. > > In fact, as I'm done writing this I had a thought that you probably shouldn't > call RenderProcessHost::FromID() outside the UI thread. And indeed you > shouldn't: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > So this is really the only option (and you should enable dcheck_always_on=1 in > your build ASAP!). Done. https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:663: base::Unretained(render_process_host))); On 2015/06/30 16:34:35, DaleCurtis wrote: > On 2015/06/30 13:41:40, gab wrote: > > base::Unretained() here is incorrect because there are no guarantees that > > |render_process_host| will still be around by the time this task is run (which > > could result in a use-after-free). > > > > For educational purposes, here are a couple ways to go about passing the > |this| > > pointer: > > > > 1) Use Unretained if you have lifetime guarantees (you don't here). > > > > 2) Use refcounting (default if not using any bind helper): only an option if > the > > object implements RefCounted (RenderProcessHost doesn't) AND it makes sense > for > > your task to prevent the object from being deleted (in this case it doesn't > > either). > > > > 3) Use a WeakPtr: only an option if the object vends WeakPtrs (e.g. implements > > SupportsWeakPtr or has a WeakPtrFactory) -- not the case here either, but > would > > have been a nice option (WeakPtrs will check that they are still valid before > > running the callback). > > > > None of the above actually work great here so here's what I suggest, instead > of > > binding to the RenderProcessHost object right away, add a method in this > task's > > anonymous namespace that runs on the UI thread and takes the > > |render_process_id_| as an argument, from it obtain the |render_process_host| > > pointer and call the RenderProcessHost's AudioFoo() methods directly. > > > > In fact, as I'm done writing this I had a thought that you probably shouldn't > > call RenderProcessHost::FromID() outside the UI thread. And indeed you > > shouldn't: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > So this is really the only option (and you should enable dcheck_always_on=1 in > > your build ASAP!). > > Oops, thanks for catching this. The last method is in fact what should be done > here. I had a brain fart and thought this was attaching to the browser process, > so if it went poof everything went with it. Done. https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:670: // If it was the last audio stream playing for this renderer. On 2015/06/29 22:44:23, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1098: // If the backgrounding was skipped because audio was playing, we background On 2015/06/30 13:41:41, gab wrote: > -we (in general we try to avoid "we" in comments), e.g. > > // If backgrounding was skipped because audio was playing, background the tab > now. Done. https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1107: // If audio starts from a backgrounded tab, we un-background it. On 2015/06/30 13:41:41, gab wrote: > // If audio starts playing from a backgrounded tab, unbackground it. Done. https://codereview.chromium.org/1214883004/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1112: // it is backgrounded again when the video finishes. On 2015/06/30 13:41:41, gab wrote: > // Since it's not a visible tab, it needs to be marked for backgrounding again > so that it is re-backgrounded if/when the audio stops. Done. https://codereview.chromium.org/1214883004/diff/60001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/1214883004/diff/60001/content/public/browser/... content/public/browser/render_process_host.h:104: // called on the IO thread. On 2015/06/30 13:41:41, gab wrote: > s/IO/UI/ Done. https://codereview.chromium.org/1214883004/diff/60001/content/public/browser/... content/public/browser/render_process_host.h:108: // Must be called on the IO thread. On 2015/06/30 13:41:41, gab wrote: > s/IO/UI/ Done.
lgtm % a couple comment nits. I would still like to see a regression test (and I know you're working on one), but I'm okay with landing this first with the integration browser test as a follow-up if it doesn't look like the regression test will be done shortly. Thanks, Gab https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:675: // Inform the renderer that audio started playing for the first time so the Since this comment is outside the "if", I'd s/that audio started/when audio starts/ (i.e. outside the "if" it is not always the case "that audio started"). https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:675: // Inform the renderer that audio started playing for the first time so the s/renderer/renderer host/ https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:676: // process can be un-backgrounded if necessary. Remove " so the process can be...". What the RenderProcessHost does in response is its business and shouldn't be documented here. https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:688: // Inform the renderer that there is no more audio playing so the process s/that there is/when there is/ https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:689: // can be backgrounded if possible. Remove " so the process (...)".
In CL description: "Fixed the fact that we don't background a tab that initially skipped backgrounding because audio was playing when the audio stops." I think at the end you meant "when the tab was backgrounded".
One last thing: https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:677: if (base::AtomicRefCountIsOne(&num_playing_streams_)) { Note that this depends on this thread being the only writer (or at least on the writes being otherwise sequenced), otherwise you could go from two threads seeing 0 and then both seeing 2. It would also be possible for this thread to see 1 but for another thread to decrement immediately after, resulting in a race of start/stop notifications. This is not a problem in practice because these writes are sequenced on a single thread, but perhaps we should make a note on |num_playing_streams_|: // The number of streams in the playing state. Atomic read safe from any // thread, but should only be updated from the IO thread. base::AtomicRefCount num_playing_streams_;
sebsg@chromium.org changed reviewers: + nick@chromium.org
@nick as OWNER of src/content/* https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:675: // Inform the renderer that audio started playing for the first time so the On 2015/07/02 13:11:53, gab (OOO) wrote: > s/renderer/renderer host/ Done. https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:675: // Inform the renderer that audio started playing for the first time so the On 2015/07/02 13:11:53, gab (OOO) wrote: > Since this comment is outside the "if", I'd s/that audio started/when audio > starts/ (i.e. outside the "if" it is not always the case "that audio started"). Done. https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:676: // process can be un-backgrounded if necessary. On 2015/07/02 13:11:53, gab wrote: > Remove " so the process can be...". What the RenderProcessHost does in response > is its business and shouldn't be documented here. Done. https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:677: if (base::AtomicRefCountIsOne(&num_playing_streams_)) { On 2015/07/06 17:24:09, gab wrote: > Note that this depends on this thread being the only writer (or at least on the > writes being otherwise sequenced), otherwise you could go from two threads > seeing 0 and then both seeing 2. > > It would also be possible for this thread to see 1 but for another thread to > decrement immediately after, resulting in a race of start/stop notifications. > > This is not a problem in practice because these writes are sequenced on a single > thread, but perhaps we should make a note on |num_playing_streams_|: > > // The number of streams in the playing state. Atomic read safe from any > // thread, but should only be updated from the IO thread. > base::AtomicRefCount num_playing_streams_; Done. https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:688: // Inform the renderer that there is no more audio playing so the process On 2015/07/02 13:11:53, gab (OOO) wrote: > s/that there is/when there is/ Done. https://codereview.chromium.org/1214883004/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:689: // can be backgrounded if possible. On 2015/07/02 13:11:53, gab (OOO) wrote: > Remove " so the process (...)". Done.
https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1113: SetBackgrounded(true); I think we should fix things up so that this hack isn't necessary. Here's a sketch of what I'm thinking: https://codereview.chromium.org/1214393004 https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:335: // Must be called from the IO thread. I think you mean UI thread (since that's what the DCHECK says). In any case, RenderProcessHost is single-threaded and only exists on the UI thread. https://codereview.chromium.org/1214883004/diff/160001/content/public/browser... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/1214883004/diff/160001/content/public/browser... content/public/browser/render_process_host.h:104: // called on the UI thread. All the methods of this class must be called on the UI thread, so you don't need these comments.
Thanks Nick. https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1113: SetBackgrounded(true); On 2015/07/06 21:53:57, ncarter wrote: > I think we should fix things up so that this hack isn't necessary. Here's a > sketch of what I'm thinking: https://codereview.chromium.org/1214393004 Proposed skeleton SGTM, the AudioRendererHost could then either call UpdateProcessPriority() itself or, for potentially better abstraction, could take it as a Closure to be called when the audio-playing state changes (any preference on your end?). Dropped a few comments on https://codereview.chromium.org/1214393004 just in case Seb intended to copy it as-is.
On 2015/07/08 18:52:14, gab wrote: > Thanks Nick. > > https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... > File content/browser/renderer_host/render_process_host_impl.cc (right): > > https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... > content/browser/renderer_host/render_process_host_impl.cc:1113: > SetBackgrounded(true); > On 2015/07/06 21:53:57, ncarter wrote: > > I think we should fix things up so that this hack isn't necessary. Here's a > > sketch of what I'm thinking: https://codereview.chromium.org/1214393004 > > Proposed skeleton SGTM, the AudioRendererHost could then either call > UpdateProcessPriority() itself or, for potentially better abstraction, could > take it as a Closure to be called when the audio-playing state changes (any > preference on your end?). > > Dropped a few comments on https://codereview.chromium.org/1214393004 just in > case Seb intended to copy it as-is. Where are we with this patch? Apologies if my suggestion derailed the M45 train, but in a class as central to everything as render_process_host_impl, we really need to be careful and do things properly. I anticipate the next step here is to merge 1214393004 into this patch, and add the integration/regression tests?
A couple of drive-by comments/concerns: 1. Please make sure this doesn't conflict with the power save blockers in WebContentsImpl: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... 2. How does this interact with WebContentsImpl's calls to RenderWidgetHostView::Hide(), WasOccluded() and WasUnOccluded()? Can these conflict? Perhaps RenderProcessHostImpl should also be checking that all RWHostViews are in a occluded state before calling SetBackground(true)? 3. Please test this with tab capture to ensure non-active tabs can never be backgrounded, regardless of whether audio is playing (since the visual content could change at any time).
On 2015/07/17 23:53:40, miu wrote: > A couple of drive-by comments/concerns: > > 1. Please make sure this doesn't conflict with the power save blockers in > WebContentsImpl: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > 2. How does this interact with WebContentsImpl's calls to > RenderWidgetHostView::Hide(), WasOccluded() and WasUnOccluded()? Can these > conflict? Perhaps RenderProcessHostImpl should also be checking that all > RWHostViews are in a occluded state before calling SetBackground(true)? > > 3. Please test this with tab capture to ensure non-active tabs can never be > backgrounded, regardless of whether audio is playing (since the visual content > could change at any time). @miu: Interesting points, but I think this applies to SetBackground() overall, not only to this bug fix regarding backgrounding+audio specifically (i.e., if these are indeed issues, they are with and without this CL?). Also, FYI: backgrounding is live on Stable on Linux and Windows at the moment, so if anything, new features should make sure they play nicely with it, not the opposite. Re 2.: I don't think backgrounding observes occlusion at all, though perhaps it should. Thanks for your input, Gab
On 2015/07/21 16:11:29, gab wrote: > On 2015/07/17 23:53:40, miu wrote: > > A couple of drive-by comments/concerns: > > > > 1. Please make sure this doesn't conflict with the power save blockers in > > WebContentsImpl: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > > > 2. How does this interact with WebContentsImpl's calls to > > RenderWidgetHostView::Hide(), WasOccluded() and WasUnOccluded()? Can these > > conflict? Perhaps RenderProcessHostImpl should also be checking that all > > RWHostViews are in a occluded state before calling SetBackground(true)? > > > > 3. Please test this with tab capture to ensure non-active tabs can never be > > backgrounded, regardless of whether audio is playing (since the visual content > > could change at any time). > > @miu: Interesting points, but I think this applies to SetBackground() overall, > not only to this bug fix regarding backgrounding+audio specifically (i.e., if > these are indeed issues, they are with and without this CL?).0 I agree that if there's a bug, it likely exists with or without this CL, but ... > Also, FYI: backgrounding is live on Stable on Linux and Windows at the moment, > so if anything, new features should make sure they play nicely with it, not the > opposite. Tab capture is also live on stable, it's not a new feature either. Really it's a responsibility shared by everyone to make sure that fixing one bug doesn't expose (or enhance the severity) of some other latent bug. Ultimately what matters is things working properly for our users. miu is just suggesting you test out potential interactions with the code you're touching. I'm sure he's also happy to help you figure out how! > Re 2.: I don't think backgrounding observes occlusion at all, though perhaps it > should. Isn't it hooked up though? WebContentsImpl::WasHidden() handles capturer_count_, and calls RenderWidgetHostView::Hide(), which (on Windows, Linux and Cros) is RenderWidgetHostViewAura::Hide(), which calls RenderWidgetHostImpl::WasHidden(), which calls RenderProcessHost::WidgetHidden(), which drives the SetBackgrounded() mechanism. I am not really familiar with the parallel OnOccluded/OnUnOccluded mechanism, but it seems to be mac-only at the moment. > Thanks for your input, > Gab
On 2015/07/21 23:07:09, ncarter wrote: > On 2015/07/21 16:11:29, gab wrote: > > On 2015/07/17 23:53:40, miu wrote: > > > A couple of drive-by comments/concerns: > > > > > > 1. Please make sure this doesn't conflict with the power save blockers in > > > WebContentsImpl: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > > > > > 2. How does this interact with WebContentsImpl's calls to > > > RenderWidgetHostView::Hide(), WasOccluded() and WasUnOccluded()? Can these > > > conflict? Perhaps RenderProcessHostImpl should also be checking that all > > > RWHostViews are in a occluded state before calling SetBackground(true)? > > > > > > 3. Please test this with tab capture to ensure non-active tabs can never be > > > backgrounded, regardless of whether audio is playing (since the visual > content > > > could change at any time). > > > > @miu: Interesting points, but I think this applies to SetBackground() overall, > > not only to this bug fix regarding backgrounding+audio specifically (i.e., if > > these are indeed issues, they are with and without this CL?).0 > > I agree that if there's a bug, it likely exists with or without this CL, but ... > > > Also, FYI: backgrounding is live on Stable on Linux and Windows at the moment, > > so if anything, new features should make sure they play nicely with it, not > the > > opposite. > > Tab capture is also live on stable, it's not a new feature either. Really it's > a responsibility shared by everyone to make sure that fixing one bug doesn't > expose (or enhance the severity) of some other latent bug. Ultimately what > matters is things working properly for our users. miu is just suggesting > you test out potential interactions with the code you're touching. I'm sure > he's also happy to help you figure out how! Indeed, if renderers responsible for tab capture should never be backgrounded we should make sure they're not. I'm not familiar with tab capture, how do we get a renderer responsible for a tab capture content? > > > Re 2.: I don't think backgrounding observes occlusion at all, though perhaps > it > > should. > > Isn't it hooked up though? WebContentsImpl::WasHidden() handles capturer_count_, > and calls RenderWidgetHostView::Hide(), which (on Windows, Linux and Cros) is > RenderWidgetHostViewAura::Hide(), which calls RenderWidgetHostImpl::WasHidden(), > which calls RenderProcessHost::WidgetHidden(), which drives the > SetBackgrounded() mechanism. Very possible it's already hooked up, I'm not familiar with occlusion either, implied from miu's message that occlusion was not a strict superset of visibility. > > I am not really familiar with the parallel OnOccluded/OnUnOccluded mechanism, > but it seems to be mac-only at the moment. > > > Thanks for your input, > > Gab
Implemented Nick's suggestion. Implementing the tests now. https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1113: SetBackgrounded(true); On 2015/07/08 18:52:14, gab wrote: > On 2015/07/06 21:53:57, ncarter wrote: > > I think we should fix things up so that this hack isn't necessary. Here's a > > sketch of what I'm thinking: https://codereview.chromium.org/1214393004 > > Proposed skeleton SGTM, the AudioRendererHost could then either call > UpdateProcessPriority() itself or, for potentially better abstraction, could > take it as a Closure to be called when the audio-playing state changes (any > preference on your end?). > > Dropped a few comments on https://codereview.chromium.org/1214393004 just in > case Seb intended to copy it as-is. Done. https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1113: SetBackgrounded(true); On 2015/07/06 21:53:57, ncarter wrote: > I think we should fix things up so that this hack isn't necessary. Here's a > sketch of what I'm thinking: https://codereview.chromium.org/1214393004 Done. https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:335: // Must be called from the IO thread. On 2015/07/06 21:53:57, ncarter wrote: > I think you mean UI thread (since that's what the DCHECK says). > > In any case, RenderProcessHost is single-threaded and only exists on the UI > thread. Done. https://codereview.chromium.org/1214883004/diff/160001/content/public/browser... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/1214883004/diff/160001/content/public/browser... content/public/browser/render_process_host.h:104: // called on the UI thread. On 2015/07/06 21:53:57, ncarter wrote: > All the methods of this class must be called on the UI thread, so you don't need > these comments. Done.
On 2015/08/02 16:44:23, sebsg wrote: > Implemented Nick's suggestion. Implementing the tests now. > > https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... > File content/browser/renderer_host/render_process_host_impl.cc (right): > > https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... > content/browser/renderer_host/render_process_host_impl.cc:1113: > SetBackgrounded(true); > On 2015/07/08 18:52:14, gab wrote: > > On 2015/07/06 21:53:57, ncarter wrote: > > > I think we should fix things up so that this hack isn't necessary. Here's a > > > sketch of what I'm thinking: https://codereview.chromium.org/1214393004 > > > > Proposed skeleton SGTM, the AudioRendererHost could then either call > > UpdateProcessPriority() itself or, for potentially better abstraction, could > > take it as a Closure to be called when the audio-playing state changes (any > > preference on your end?). > > > > Dropped a few comments on https://codereview.chromium.org/1214393004 just in > > case Seb intended to copy it as-is. > > Done. > > https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... > content/browser/renderer_host/render_process_host_impl.cc:1113: > SetBackgrounded(true); > On 2015/07/06 21:53:57, ncarter wrote: > > I think we should fix things up so that this hack isn't necessary. Here's a > > sketch of what I'm thinking: https://codereview.chromium.org/1214393004 > > Done. > > https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... > File content/browser/renderer_host/render_process_host_impl.h (right): > > https://codereview.chromium.org/1214883004/diff/160001/content/browser/render... > content/browser/renderer_host/render_process_host_impl.h:335: // Must be called > from the IO thread. > On 2015/07/06 21:53:57, ncarter wrote: > > I think you mean UI thread (since that's what the DCHECK says). > > > > In any case, RenderProcessHost is single-threaded and only exists on the UI > > thread. > > Done. > > https://codereview.chromium.org/1214883004/diff/160001/content/public/browser... > File content/public/browser/render_process_host.h (right): > > https://codereview.chromium.org/1214883004/diff/160001/content/public/browser... > content/public/browser/render_process_host.h:104: // called on the UI thread. > On 2015/07/06 21:53:57, ncarter wrote: > > All the methods of this class must be called on the UI thread, so you don't > need > > these comments. > > Done. This looks right to me. Let me know when the tests are ready.
Looks great, a few things below, eager to see the tests :-). https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:665: // Inform the renderer host when audio starts playing for the first time. s/renderer host/RenderProcessHost/ https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:677: // Inform the renderer when there is no more audio playing. s/renderer/RenderProcessHost/ https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1066: // Verify we were properly backgrounded. This comment is now obsolete. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1073: // On startup, the browser will call Hide. We ignore these calls. s/these calls/this call/ https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1078: if (--visible_widgets_ == 0) { In Chromium we typically prefer not having inline self-operators inside conditionals. i.e., doing the decrement first then the check on its own would be the prefered style. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1089: DCHECK_CURRENTLY_ON(BrowserThread::UI); As Nick said, this class is not thread safe (i.e. all entry points should only be called on the UI thread and therefore it's overkill to document it here). (FYI, the way we'd check this typically in Chromium is that we'd have base::ThreadChecker thread_checker_; member and every method would call thread_checker_.CalledOnValidThread() to both document the lack of thread safety and enforce it -- not that you should adopt this paradigm in this CL, just letting you know how other non-thread-safe classes that do check for thread safety do it). https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2241: base::CommandLine::ForCurrentProcess(); inline this below since you only need it once https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2242: bool process_exists = const https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2249: bool should_background = const https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2257: } This is essentially: if (!process_exists) { is_process_backgrounded_ = false; return; } right? So shouldn't this check go right after initializing |process_exists| and then there is no need to check whether process_exists later in this method? In which case we should inline |process_exists| in the if and get rid of that variable?
lgtm % nits. FWICT, using the visible_widgets_ count mechanism should work well with tab capture since WebContentsImpl will call RWHView::Show() and Hide(), which in turn will ultimately call RWProcessHost::WidgetRestored() and WidgetHidden(). https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1077: DCHECK_GT(visible_widgets_, 0); Might as well remove this DCHECK since it can never trigger (due to the if-statement just above). https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2254: if (!child_process_launcher_.get() || child_process_launcher_->IsStarting()) { nit: This if-statement should be at the top of the method, since evaluating the booleans (i.e., |process_exists| and |should_background|) is unnecessary.
Patchset #11 (id:200001) has been deleted
Patchset #11 (id:220001) has been deleted
https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2257: } On 2015/08/04 19:40:24, gab wrote: > This is essentially: > > if (!process_exists) { > is_process_backgrounded_ = false; > return; > } > > right? > > So shouldn't this check go right after initializing |process_exists| and then > there is no need to check whether process_exists later in this method? > In which case we should inline |process_exists| in the if and get rid of that > variable? The only choice here is whether you want the TRACE_EVENT to log the false->true transition when the process crashes or not. If you want it to log (which was my preference, but it's kind of arbitrary) then you should just delete these four lines. If you don't want to log it, you should do what gab suggests.
https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2257: } On 2015/08/04 22:23:23, ncarter wrote: > On 2015/08/04 19:40:24, gab wrote: > > This is essentially: > > > > if (!process_exists) { > > is_process_backgrounded_ = false; > > return; > > } > > > > right? > > > > So shouldn't this check go right after initializing |process_exists| and then > > there is no need to check whether process_exists later in this method? > > In which case we should inline |process_exists| in the if and get rid of that > > variable? > > The only choice here is whether you want the TRACE_EVENT to log the false->true > transition when the process crashes or not. If you want it to log (which was my > preference, but it's kind of arbitrary) then you should just delete these four > lines. If you don't want to log it, you should do what gab suggests. Hmm, won't the TRACE_EVENT code below be hit in the exact same circumstances whether this check is here or at the top as suggested?
https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2257: } On 2015/08/06 16:35:02, gab wrote: > On 2015/08/04 22:23:23, ncarter wrote: > > On 2015/08/04 19:40:24, gab wrote: > > > This is essentially: > > > > > > if (!process_exists) { > > > is_process_backgrounded_ = false; > > > return; > > > } > > > > > > right? > > > > > > So shouldn't this check go right after initializing |process_exists| and > then > > > there is no need to check whether process_exists later in this method? > > > In which case we should inline |process_exists| in the if and get rid of > that > > > variable? > > > > The only choice here is whether you want the TRACE_EVENT to log the > false->true > > transition when the process crashes or not. If you want it to log (which was > my > > preference, but it's kind of arbitrary) then you should just delete these four > > lines. If you don't want to log it, you should do what gab suggests. > > Hmm, won't the TRACE_EVENT code below be hit in the exact same circumstances > whether this check is here or at the top as suggested? Yes, it will. Sorry my comment wasn't clearer. I was saying that the choice you wanted to make was whether, in the !process_exists case, to return before or after the TRACE_EVENT. It depends on what you want the TRACE_EVENT for. If you're using it to understand the state transitions of is_process_backgrounded_ over time, it's probably better to return after TRACE_EVENT. (Remember that process_exists can transition from true to false and then later back to true). If the TRACE_EVENT is to log the average runtime expense of GetModuleHandle/SetProcessBackgrounded(), then it makes sense to not include the !process_exists case. Having said that: Looking at the code history, I think this TRACE_EVENT was concerned with the latter, so the latest patch set is acceptable to me.
https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:665: // Inform the renderer host when audio starts playing for the first time. On 2015/08/04 19:40:24, gab wrote: > s/renderer host/RenderProcessHost/ Done. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:677: // Inform the renderer when there is no more audio playing. On 2015/08/04 19:40:24, gab wrote: > s/renderer/RenderProcessHost/ Done. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1066: // Verify we were properly backgrounded. On 2015/08/04 19:40:24, gab wrote: > This comment is now obsolete. Done. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1073: // On startup, the browser will call Hide. We ignore these calls. On 2015/08/04 19:40:24, gab wrote: > s/these calls/this call/ Done. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1077: DCHECK_GT(visible_widgets_, 0); On 2015/08/04 20:30:20, miu wrote: > Might as well remove this DCHECK since it can never trigger (due to the > if-statement just above). Done. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1078: if (--visible_widgets_ == 0) { On 2015/08/04 19:40:24, gab wrote: > In Chromium we typically prefer not having inline self-operators inside > conditionals. > > i.e., doing the decrement first then the check on its own would be the prefered > style. Done. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1089: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2015/08/04 19:40:24, gab wrote: > As Nick said, this class is not thread safe (i.e. all entry points should only > be called on the UI thread and therefore it's overkill to document it here). > > (FYI, the way we'd check this typically in Chromium is that we'd have > base::ThreadChecker thread_checker_; member and every method would call > thread_checker_.CalledOnValidThread() to both document the lack of thread safety > and enforce it -- not that you should adopt this paradigm in this CL, just > letting you know how other non-thread-safe classes that do check for thread > safety do it). Done. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2241: base::CommandLine::ForCurrentProcess(); On 2015/08/04 19:40:24, gab wrote: > inline this below since you only need it once Done. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2242: bool process_exists = On 2015/08/04 19:40:24, gab wrote: > const Done. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2249: bool should_background = On 2015/08/04 19:40:24, gab wrote: > const Done. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2254: if (!child_process_launcher_.get() || child_process_launcher_->IsStarting()) { On 2015/08/04 20:30:20, miu wrote: > nit: This if-statement should be at the top of the method, since evaluating the > booleans (i.e., |process_exists| and |should_background|) is unnecessary. Done. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2257: } On 2015/08/04 19:40:24, gab wrote: > This is essentially: > > if (!process_exists) { > is_process_backgrounded_ = false; > return; > } > > right? > > So shouldn't this check go right after initializing |process_exists| and then > there is no need to check whether process_exists later in this method? > In which case we should inline |process_exists| in the if and get rid of that > variable? Done. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2257: } On 2015/08/04 22:23:23, ncarter wrote: > On 2015/08/04 19:40:24, gab wrote: > > This is essentially: > > > > if (!process_exists) { > > is_process_backgrounded_ = false; > > return; > > } > > > > right? > > > > So shouldn't this check go right after initializing |process_exists| and then > > there is no need to check whether process_exists later in this method? > > In which case we should inline |process_exists| in the if and get rid of that > > variable? > > The only choice here is whether you want the TRACE_EVENT to log the false->true > transition when the process crashes or not. If you want it to log (which was my > preference, but it's kind of arbitrary) then you should just delete these four > lines. If you don't want to log it, you should do what gab suggests. Done. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2257: } On 2015/08/06 16:35:02, gab wrote: > On 2015/08/04 22:23:23, ncarter wrote: > > On 2015/08/04 19:40:24, gab wrote: > > > This is essentially: > > > > > > if (!process_exists) { > > > is_process_backgrounded_ = false; > > > return; > > > } > > > > > > right? > > > > > > So shouldn't this check go right after initializing |process_exists| and > then > > > there is no need to check whether process_exists later in this method? > > > In which case we should inline |process_exists| in the if and get rid of > that > > > variable? > > > > The only choice here is whether you want the TRACE_EVENT to log the > false->true > > transition when the process crashes or not. If you want it to log (which was > my > > preference, but it's kind of arbitrary) then you should just delete these four > > lines. If you don't want to log it, you should do what gab suggests. > > Hmm, won't the TRACE_EVENT code below be hit in the exact same circumstances > whether this check is here or at the top as suggested? Done. https://codereview.chromium.org/1214883004/diff/180001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2257: } On 2015/08/06 18:16:39, ncarter wrote: > On 2015/08/06 16:35:02, gab wrote: > > On 2015/08/04 22:23:23, ncarter wrote: > > > On 2015/08/04 19:40:24, gab wrote: > > > > This is essentially: > > > > > > > > if (!process_exists) { > > > > is_process_backgrounded_ = false; > > > > return; > > > > } > > > > > > > > right? > > > > > > > > So shouldn't this check go right after initializing |process_exists| and > > then > > > > there is no need to check whether process_exists later in this method? > > > > In which case we should inline |process_exists| in the if and get rid of > > that > > > > variable? > > > > > > The only choice here is whether you want the TRACE_EVENT to log the > > false->true > > > transition when the process crashes or not. If you want it to log (which was > > my > > > preference, but it's kind of arbitrary) then you should just delete these > four > > > lines. If you don't want to log it, you should do what gab suggests. > > > > Hmm, won't the TRACE_EVENT code below be hit in the exact same circumstances > > whether this check is here or at the top as suggested? > > Yes, it will. Sorry my comment wasn't clearer. I was saying that the choice you > wanted to make was whether, in the !process_exists case, to return before or > after the TRACE_EVENT. > > It depends on what you want the TRACE_EVENT for. If you're using it to > understand the state transitions of is_process_backgrounded_ over time, it's > probably better to return after TRACE_EVENT. (Remember that process_exists can > transition from true to false and then later back to true). If the TRACE_EVENT > is to log the average runtime expense of > GetModuleHandle/SetProcessBackgrounded(), then it makes sense to not include the > !process_exists case. > > Having said that: Looking at the code history, I think this TRACE_EVENT was > concerned with the latter, so the latest patch set is acceptable to me. Done.
Posting some partial feedback on the tests, because I'm nearly out of time for the today. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:575: BackgroundingLogicOfStoppedAudioAndImmediateTabSwitching) { ProcessPriorityAfterStoppedAudio https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:576: ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady()); Seems like you launch the embedded_test_server, but then load your content via the file URLs, instead of getting URLs via embedded_test_server()->GetURL("simple_page.html"). You only need one or the other. I'd prefer the embedded testserver here, since it's fewer lines of code and the embedded testserver is pretty fast and reliable (unlike the spawned testserver, which is a different beast). From a chrome/ browsertest, you could force the embedded_test_server to also serve files from content/test/data/ via the following code: base::FilePath test_data_dir; ASSERT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &test_data_dir)); embedded_test_server()->ServeFilesFromDirectory( test_data_dir.AppendASCII("content/test/data/")); That's allowed, and encouraged even for complex test data -- see this thread: https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/Go0F5PP2Vl8... However, I probably wouldn't bother with that just for simple_page.html -- you can use some other dummy page from chrome/test/data, maybe chrome/test/data/title1.html https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:606: EXPECT_FALSE(IsProcessBackgroundedHelper(&audio_process)); With the while loop above, these EXPECT's are kind of lame -- they can't possibly fail. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:617: base::RunLoop().RunUntilIdle(); What'll happen in practice here, is that RunUntilIdle will return immediately (since there's no work to do) and this functions as a spinloop, likely pegging a core and possibly stealing scheduler time from the threads we're actually waiting for. Also, it means that the test's only failure mode is a timeout, which becomes much harder to debug and more expensive to run. There are only see a handful of cases of this pattern in existing code: https://code.google.com/p/chromium/codesearch#search/&q=while(.*%5Cn)?(.*%5Cn... and I'm pretty sure it's generally discouraged. So, spins should be an absolute last resort. Is there any event/callback/observer method we can wait on (maybe from the audio code), to catch the point in time where the RPHImpl is told that the transition is done? Sometimes even just a single content::RunAllPendingInMessageLoop() (without a loop) does the trick.
Looking good, glad we got this test together :-), some comments below. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:87: no_audio_path = no_audio_path.Append(FILE_PATH_LITERAL("simple_page.html")); GetTestUrl("simple_page.html") from content_browser_test_utils.h gives you exactly this in one line :-) https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:104: bool IsProcessBackgroundedHelper(base::Process* process) { const base::Process& process (i.e. always use const& for params unless you need non-const -- (non-const & isn't allowed by style guide) https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:107: mach_port_t task_port = broker->TaskForPid(process.Pid()); I predict this won't compile because you're invoking the dot-operator on a pointer ;-). https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:572: // from it playing and there is an immediate tab switch. Remove "from it playing" it makes the sentence confusing and it's implicit (i.e. if you're stopping it it's because it was previously playing). https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:578: if (!base::Process::CanBackgroundProcesses()) { Make this the first line of the test with a comment that this test is invalid on platforms that can't background. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:579: LOG(ERROR) << "Can't background processes"; Avoid all types of logging in tests (and in general) if not on true errors (in which case in tests you should use asserts). Having everybody logging their thing is just as good as having no logs but noisier and there was a massive effort some years ago to get rid of all logs that happen in regular flow. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:582: base::CommandLine& parsed_command_line = non-const & is banned by the style-guide. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:584: parsed_command_line.AppendSwitch(switches::kProcessPerTab); Instead of doing this here, override SetUpCommandLine() in the test fixture. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:587: GURL audio_url = GetAudioPlayingPage(); Make both of these const https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:597: base::Process audio_process = ShowSingletonTab(audio_url); const https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:617: base::RunLoop().RunUntilIdle(); On 2015/08/06 22:15:58, ncarter wrote: > What'll happen in practice here, is that RunUntilIdle will return immediately > (since there's no work to do) and this functions as a spinloop, likely pegging a > core and possibly stealing scheduler time from the threads we're actually > waiting for. Also, it means that the test's only failure mode is a timeout, > which becomes much harder to debug and more expensive to run. > > There are only see a handful of cases of this pattern in existing code: > https://code.google.com/p/chromium/codesearch#search/&q=while(.*%5Cn)?(.*%5Cn... > and I'm pretty sure it's generally discouraged. > > So, spins should be an absolute last resort. Is there any > event/callback/observer method we can wait on (maybe from the audio code), to > catch the point in time where the RPHImpl is told that the transition is done? > > Sometimes even just a single content::RunAllPendingInMessageLoop() (without a > loop) does the trick. There is no event we can wait in the browser because this is waiting on the reaction of the OS/kernel for it to truly background the process (well, actually, I'm not exactly sure how the kernel handles it but I'd assume the OS's GetProcessPriority() immediately returns the last priority set by SetProcessPriority() even if it's not done handling it -- so maybe we don't even need to loop?! @seb, try that?). But I agree otherwise that this would result in an overly busy loop. If we have to loop, I'd suggest we add a base::PlatformThread::Sleep(TestTimeouts::tiny_timeout()); call here between each iteration (while also running the loop just in case it has something else to do I guess?). https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:654: EXPECT_NE(audio_process.Pid(), no_audio_process.Pid()); A lot of the test setup is the same for all 3 of these tests. I think it makes sense to extract the common code to a test fixture. That test fixture would be a subclass of ChromeRenderProcessHostTest and would override SetUpOnMainThread() to do the common work (something like starting up both tabs and storing their Process pointers in member variables). (it would also make sense for the helper methods specific to backgrounding to move to this backgrounding specific test fixture) https://codereview.chromium.org/1214883004/diff/280001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/1214883004/diff/280001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:361: // Whether this process currently has backgrounded priority. Tracked so that s/has backgrounded priority/is backgrounded/ https://codereview.chromium.org/1214883004/diff/280001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:362: // UpdateProcessPriority() can avoid redundantly setting the priority. Don't document "why" it's tracked, just state what it is (i.e. someone may re-use it for some other reason and it's what it is that matters not why it's stored; why it's stored can be inferred more reliably from the code than from a comment anyways).
Also please update the CL description to reflect the latest status. Thanks, Gab
Patchset #14 (id:300001) has been deleted
https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:87: no_audio_path = no_audio_path.Append(FILE_PATH_LITERAL("simple_page.html")); On 2015/08/07 19:55:06, gab wrote: > GetTestUrl("simple_page.html") from content_browser_test_utils.h gives you > exactly this in one line :-) Good to know! But, as Nick recommended I now use the embedded_test_server instead :) https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:104: bool IsProcessBackgroundedHelper(base::Process* process) { On 2015/08/07 19:55:06, gab wrote: > const base::Process& process > > (i.e. always use const& for params unless you need non-const -- (non-const & > isn't allowed by style guide) Done. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:107: mach_port_t task_port = broker->TaskForPid(process.Pid()); On 2015/08/07 19:55:06, gab wrote: > I predict this won't compile because you're invoking the dot-operator on a > pointer ;-). Done. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:572: // from it playing and there is an immediate tab switch. On 2015/08/07 19:55:06, gab wrote: > Remove "from it playing" it makes the sentence confusing and it's implicit (i.e. > if you're stopping it it's because it was previously playing). Done. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:575: BackgroundingLogicOfStoppedAudioAndImmediateTabSwitching) { On 2015/08/06 22:15:58, ncarter wrote: > ProcessPriorityAfterStoppedAudio Done. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:576: ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady()); On 2015/08/06 22:15:58, ncarter wrote: > Seems like you launch the embedded_test_server, but then load your content via > the file URLs, instead of getting URLs via > embedded_test_server()->GetURL("simple_page.html"). You only need one or the > other. I'd prefer the embedded testserver here, since it's fewer lines of code > and the embedded testserver is pretty fast and reliable (unlike the spawned > testserver, which is a different beast). > > From a chrome/ browsertest, you could force the embedded_test_server to also > serve files from content/test/data/ via the following code: > > base::FilePath test_data_dir; > ASSERT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &test_data_dir)); > embedded_test_server()->ServeFilesFromDirectory( > test_data_dir.AppendASCII("content/test/data/")); > > That's allowed, and encouraged even for complex test data -- see this thread: > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/Go0F5PP2Vl8... > > However, I probably wouldn't bother with that just for simple_page.html -- you > can use some other dummy page from chrome/test/data, maybe > chrome/test/data/title1.html > Done. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:578: if (!base::Process::CanBackgroundProcesses()) { On 2015/08/07 19:55:06, gab wrote: > Make this the first line of the test with a comment that this test is invalid on > platforms that can't background. Done. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:579: LOG(ERROR) << "Can't background processes"; On 2015/08/07 19:55:06, gab wrote: > Avoid all types of logging in tests (and in general) if not on true errors (in > which case in tests you should use asserts). > > Having everybody logging their thing is just as good as having no logs but > noisier and there was a massive effort some years ago to get rid of all logs > that happen in regular flow. Done. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:582: base::CommandLine& parsed_command_line = On 2015/08/07 19:55:06, gab wrote: > non-const & is banned by the style-guide. Done. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:584: parsed_command_line.AppendSwitch(switches::kProcessPerTab); On 2015/08/07 19:55:06, gab wrote: > Instead of doing this here, override SetUpCommandLine() in the test fixture. Done. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:587: GURL audio_url = GetAudioPlayingPage(); On 2015/08/07 19:55:06, gab wrote: > Make both of these const Done. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:597: base::Process audio_process = ShowSingletonTab(audio_url); On 2015/08/07 19:55:06, gab wrote: > const Done. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:606: EXPECT_FALSE(IsProcessBackgroundedHelper(&audio_process)); On 2015/08/06 22:15:58, ncarter wrote: > With the while loop above, these EXPECT's are kind of lame -- they can't > possibly fail. Done. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:617: base::RunLoop().RunUntilIdle(); On 2015/08/07 19:55:06, gab (OOO until Aug 13) wrote: > On 2015/08/06 22:15:58, ncarter wrote: > > What'll happen in practice here, is that RunUntilIdle will return immediately > > (since there's no work to do) and this functions as a spinloop, likely pegging > a > > core and possibly stealing scheduler time from the threads we're actually > > waiting for. Also, it means that the test's only failure mode is a timeout, > > which becomes much harder to debug and more expensive to run. > > > > There are only see a handful of cases of this pattern in existing code: > > > https://code.google.com/p/chromium/codesearch#search/&q=while(.*%5Cn)?(.*%5Cn... > > and I'm pretty sure it's generally discouraged. > > > > So, spins should be an absolute last resort. Is there any > > event/callback/observer method we can wait on (maybe from the audio code), to > > catch the point in time where the RPHImpl is told that the transition is done? > > > > Sometimes even just a single content::RunAllPendingInMessageLoop() (without a > > loop) does the trick. > > There is no event we can wait in the browser because this is waiting on the > reaction of the OS/kernel for it to truly background the process (well, > actually, I'm not exactly sure how the kernel handles it but I'd assume the OS's > GetProcessPriority() immediately returns the last priority set by > SetProcessPriority() even if it's not done handling it -- so maybe we don't even > need to loop?! @seb, try that?). > > But I agree otherwise that this would result in an overly busy loop. > > If we have to loop, I'd suggest we add a > > base::PlatformThread::Sleep(TestTimeouts::tiny_timeout()); > > call here between each iteration (while also running the loop just in case it > has something else to do I guess?). Unfortunately, the results from using the OS' GetProcessPriority seems to give flaky results if I dont' do a loop. https://codereview.chromium.org/1214883004/diff/280001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:654: EXPECT_NE(audio_process.Pid(), no_audio_process.Pid()); On 2015/08/07 19:55:06, gab wrote: > A lot of the test setup is the same for all 3 of these tests. > > I think it makes sense to extract the common code to a test fixture. > > That test fixture would be a subclass of ChromeRenderProcessHostTest and would > override SetUpOnMainThread() to do the common work (something like starting up > both tabs and storing their Process pointers in member variables). > > (it would also make sense for the helper methods specific to backgrounding to > move to this backgrounding specific test fixture) Done.
lgtm
@sebsg, one comment below regarding current bot failure, let me know when you've addressed this and fixed the includes as discussed (i.e. add the OS_MACOSX include and "git cl upload --bypass-hooks && git cl try" to make sure things work (beside the DEPS error) Thanks, Gab https://codereview.chromium.org/1214883004/diff/320001/chrome/browser/rendere... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/1214883004/diff/320001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:643: content::ExecuteScript(audio_tab_web_content_, Need to ASSERT_TRUE on content::ExecuteScript. This should fix compile errors: "ignoring return value of function declared with warn_unused_result attribute [-Werror,-Wunused-result] content::ExecuteScript("
https://codereview.chromium.org/1214883004/diff/320001/chrome/browser/rendere... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/1214883004/diff/320001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:643: content::ExecuteScript(audio_tab_web_content_, On 2015/08/18 01:48:05, gab wrote: > Need to ASSERT_TRUE on content::ExecuteScript. > > This should fix compile errors: > "ignoring return value of function declared with warn_unused_result attribute > [-Werror,-Wunused-result] > content::ExecuteScript(" Done.
lgtm w/ a few style comments below. This CL is oh so awesome (especially the test which is a work of art :-)). (oh we of course still need to figure out the DEPS issue..!) Congrats! Gab https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:235: class ChromeRenderProcessHostBackgroundingTest Add a comment describing the properties enabled by this fixture (especially the implicit things the tests depend on, e.g.: it ends off control to the test with two tabs open one called "no audio" in foreground and another called "audio" in background with audio in playing state) https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:262: audio_process_ = ProcessFromHandle( Since you're doing subgroups, feels like getting the |audio_process_| belongs in the previous subgroup. Or I could see it belong here if done this way: // Create a new tab for the no audio page and get the processes corresponding to each page. no_audio_process_ = ShowSingletonTab(no_audio_url_); audio_process_ = ProcessFromHandle( audio_tab_web_content_->GetRenderProcessHost()->GetHandle()); (slight preference on the reading flow... i.e. the other way it felt as completing the previous work for audio and doing bulk work for non-audio together whereas now I find it feels more like the "processes" subgroup of the work... yea picky.. sorry!) https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:289: }; private: DISALLOW_COPY_AND_ASSIGN(ChromeRenderProcessHostBackgroundingTest); (we do this on all classes -- old tests often omitted it, but we try to do it everywhere now) https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:598: Move test fixture here as it's not used by tests above (tests tend to keep things closely related together rather than follow the typical cc file ordering of declarations...). https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:633: // stops playing from a not visible tab. s/not visible/hidden https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:640: // Wait until the two pages are not backgrounded. This assumes things about the test setup, see comment above about making sure this is obvious from the contract of the fixture.
gab@chromium.org changed reviewers: + avi@chromium.org
@avi: question below for you in nick's absence. Thanks, Gab https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:32: #include "content/browser/mach_broker_mac.h" @avi: any opinion on the best way to fix DEPS here (chrome/ can't include from content/ except for content/public)? Shall we: 1) make a file specific rule to allow "content/browser/mach_broker_mac.h" from "chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc" or 2) Move mach_broker_mac.h to content/public? or 3) Expose IsProcessBackgroundedHelper() in content/public? or 4) Your idea? This is now the only blocker for this CL (and without this include the test must be if-def'ed out on Mac which would be unfortunate...). Thanks, Gab
https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:235: class ChromeRenderProcessHostBackgroundingTest On 2015/08/18 21:05:37, gab wrote: > Add a comment describing the properties enabled by this fixture (especially the > implicit things the tests depend on, e.g.: it ends off control to the test with > two tabs open one called "no audio" in foreground and another called "audio" in > background with audio in playing state) Done. https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:262: audio_process_ = ProcessFromHandle( On 2015/08/18 21:05:37, gab wrote: > Since you're doing subgroups, feels like getting the |audio_process_| belongs in > the previous subgroup. > > Or I could see it belong here if done this way: > > // Create a new tab for the no audio page and get the processes corresponding to > each page. > no_audio_process_ = ShowSingletonTab(no_audio_url_); > audio_process_ = ProcessFromHandle( > audio_tab_web_content_->GetRenderProcessHost()->GetHandle()); > > (slight preference on the reading flow... i.e. the other way it felt as > completing the previous work for audio and doing bulk work for non-audio > together whereas now I find it feels more like the "processes" subgroup of the > work... yea picky.. sorry!) Done. https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:289: }; On 2015/08/18 21:05:37, gab wrote: > private: > DISALLOW_COPY_AND_ASSIGN(ChromeRenderProcessHostBackgroundingTest); > > (we do this on all classes -- old tests often omitted it, but we try to do it > everywhere now) Done. https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:598: On 2015/08/18 21:05:37, gab wrote: > Move test fixture here as it's not used by tests above (tests tend to keep > things closely related together rather than follow the typical cc file ordering > of declarations...). Done. https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:633: // stops playing from a not visible tab. On 2015/08/18 21:05:37, gab wrote: > s/not visible/hidden Done. https://codereview.chromium.org/1214883004/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:640: // Wait until the two pages are not backgrounded. On 2015/08/18 21:05:37, gab wrote: > This assumes things about the test setup, see comment above about making sure > this is obvious from the contract of the fixture. Done.
Patch set 17 lgtm++ modulo adopting avi's recommendation for the DEPS issue in the test. Cheers, Gab
avi@chromium.org changed reviewers: + rsesek@chromium.org
On 2015/08/18 21:13:00, gab wrote: > 1) make a file specific rule to allow > "content/browser/mach_broker_mac.h" from > "chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc" Nonono. > 2) Move mach_broker_mac.h to content/public? Maybe. > 3) Expose IsProcessBackgroundedHelper() in content/public? Right now, the fact that you need a mach port to use IsProcessBackgrounded in base/ but the mach ports live in content/ is poor API. Robert is the mach expert here; what do you think? Exposing a working "is this process backgrounded" API from content seems like the least bad choice here. https://codereview.chromium.org/1214883004/diff/380001/chrome/browser/rendere... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/1214883004/diff/380001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:543: // Sets up the browser to end off control to the test with two tabs open: one "to end off control"? I can't parse that. https://codereview.chromium.org/1214883004/diff/380001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:568: // Open a browser, navigate to the audio page and get its WebContent. WebContents. It's a singular plural noun. Or something. https://codereview.chromium.org/1214883004/diff/380001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/380001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2261: // transition in/out of thoses states. Processes are initially launched with "those states" https://codereview.chromium.org/1214883004/diff/380001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2262: // foreground priority, so when the process doesn't exist. "so when the process doesn't exist..." what? Complete that sentence.
https://codereview.chromium.org/1214883004/diff/380001/chrome/browser/rendere... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/1214883004/diff/380001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:543: // Sets up the browser to end off control to the test with two tabs open: one On 2015/08/19 00:30:29, Avi wrote: > "to end off control"? I can't parse that. Done. https://codereview.chromium.org/1214883004/diff/380001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:568: // Open a browser, navigate to the audio page and get its WebContent. On 2015/08/19 00:30:29, Avi wrote: > WebContents. It's a singular plural noun. Or something. Done. https://codereview.chromium.org/1214883004/diff/380001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/380001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2261: // transition in/out of thoses states. Processes are initially launched with On 2015/08/19 00:30:29, Avi wrote: > "those states" Done. https://codereview.chromium.org/1214883004/diff/380001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2262: // foreground priority, so when the process doesn't exist. On 2015/08/19 00:30:29, Avi wrote: > "so when the process doesn't exist..." what? Complete that sentence. Thanks for noticing that, the comment was made when that logic used the process existence state. Forgot to update the comment when I modified the code and left it incomplete.
On 2015/08/19 00:30:29, Avi wrote: > On 2015/08/18 21:13:00, gab wrote: > > 1) make a file specific rule to allow > > "content/browser/mach_broker_mac.h" from > > "chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc" > > Nonono. > > > 2) Move mach_broker_mac.h to content/public? > > Maybe. I'd prefer not. This should be an implementation detail. > > > 3) Expose IsProcessBackgroundedHelper() in content/public? > > Right now, the fact that you need a mach port to use IsProcessBackgrounded in > base/ but the mach ports live in content/ is poor API. Robert is the mach expert > here; what do you think? Exposing a working "is this process backgrounded" API > from content seems like the least bad choice here. Yes, this API is not great, but making base/ know about Mach ports for processes isn't really feasible. That's why we require callers to bring their own port. I think the best option would be to expose GetMachTaskPort() on RenderProcessHost, since there's already a way to get the ProcessHandle from a RPH. The implementation could then consult MachBrokerMac in content/ without exposing that class. I'd maybe even name it GetMachTaskPortForTesting(), for now.
On 2015/08/19 19:30:59, Robert Sesek wrote: > On 2015/08/19 00:30:29, Avi wrote: > > On 2015/08/18 21:13:00, gab wrote: > > > 1) make a file specific rule to allow > > > "content/browser/mach_broker_mac.h" from > > > "chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc" > > > > Nonono. > > > > > 2) Move mach_broker_mac.h to content/public? > > > > Maybe. > > I'd prefer not. This should be an implementation detail. > > > > > > 3) Expose IsProcessBackgroundedHelper() in content/public? > > > > Right now, the fact that you need a mach port to use IsProcessBackgrounded in > > base/ but the mach ports live in content/ is poor API. Robert is the mach > expert > > here; what do you think? Exposing a working "is this process backgrounded" API > > from content seems like the least bad choice here. > > Yes, this API is not great, but making base/ know about Mach ports for processes > isn't really feasible. That's why we require callers to bring their own port. > > I think the best option would be to expose GetMachTaskPort() on > RenderProcessHost, since there's already a way to get the ProcessHandle from a > RPH. The implementation could then consult MachBrokerMac in content/ without > exposing that class. I'd maybe even name it GetMachTaskPortForTesting(), for > now. That option sounds good to me, not sure it warrants a "ForTesting" unless you really don't want people to start using Mach ports in chrome/?
On 2015/08/19 19:38:11, gab wrote: > On 2015/08/19 19:30:59, Robert Sesek wrote: > > On 2015/08/19 00:30:29, Avi wrote: > > > On 2015/08/18 21:13:00, gab wrote: > > > > 1) make a file specific rule to allow > > > > "content/browser/mach_broker_mac.h" from > > > > "chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc" > > > > > > Nonono. > > > > > > > 2) Move mach_broker_mac.h to content/public? > > > > > > Maybe. > > > > I'd prefer not. This should be an implementation detail. > > > > > > > > > 3) Expose IsProcessBackgroundedHelper() in content/public? > > > > > > Right now, the fact that you need a mach port to use IsProcessBackgrounded > in > > > base/ but the mach ports live in content/ is poor API. Robert is the mach > > expert > > > here; what do you think? Exposing a working "is this process backgrounded" > API > > > from content seems like the least bad choice here. > > > > Yes, this API is not great, but making base/ know about Mach ports for > processes > > isn't really feasible. That's why we require callers to bring their own port. > > > > I think the best option would be to expose GetMachTaskPort() on > > RenderProcessHost, since there's already a way to get the ProcessHandle from a > > RPH. The implementation could then consult MachBrokerMac in content/ without > > exposing that class. I'd maybe even name it GetMachTaskPortForTesting(), for > > now. > > That option sounds good to me, not sure it warrants a "ForTesting" unless you > really don't want people to start using Mach ports in chrome/? If we have to open it up later we can, but for now let's limit it.
Patchset #19 (id:410001) has been deleted
Patchset #19 (id:410001) has been deleted
Patchset #19 (id:430001) has been deleted
Patchset #19 (id:450001) has been deleted
Patchset #19 (id:470001) has been deleted
Patchset #19 (id:490001) has been deleted
Patchset #19 (id:510001) has been deleted
Patchset #19 (id:530001) has been deleted
(and one last thing, please fix CL description to only contain title on first line instead of being one giant one-liner description)
Patchset #20 (id:570001) has been deleted
Review of latest 19 per offline request. Looks good, minor tweaks below. https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:570: // corresponding to each page. // Create a new tab for the no audio page and confirm that the processes of each tabs are different and both valid. (and remove empty line between this block and ASSERTs as they really belong together now) https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:571: base::Process no_audio_process_ = ShowSingletonTab(no_audio_url_); |no_audio_process| (no longer a member variable, same for |audio_process|) https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:572: no_audio_tab_web_contents_ = Perhaps move this after the ASSERTs as it's now no longer related to this block so much? https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:583: bool IsProcessBackgroundedHelper(const content::WebContents& web_contents) { I'd say const* is arguably better here so that you don't have to deref everywhere below? Perhaps with DCHECK(web_contents); to enforce the contract that this method doesn't handle null ptrs (in fact pretty sure WebContents tends to be passed by pointer most of the time -- looks like 93 for const* vs 2 for const&: https://code.google.com/p/chromium/codesearch#search/&q=const%5C%20content::W...) https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:585: content::RenderProcessHost* rph = web_contents.GetRenderProcessHost(); const https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:586: base::Process process = ProcessFromHandle(rph->GetHandle()); const https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:589: base::Process process = const https://codereview.chromium.org/1214883004/diff/550001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/1214883004/diff/550001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:167: mach_port_t GetMachTaskPortForTesting() const override; Only need #include <mach/mach.h> for |mach_port_t| I think. e.g., ref: https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/scoped_ma... If so, mach_broker_mac.h can go in .cc :-)
https://codereview.chromium.org/1214883004/diff/550001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/1214883004/diff/550001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:167: mach_port_t GetMachTaskPortForTesting() const override; On 2015/08/24 20:59:22, gab wrote: > Only need #include <mach/mach.h> for |mach_port_t| I think. > > e.g., ref: > https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/scoped_ma... > > If so, mach_broker_mac.h can go in .cc :-) Correct. And there's already an #if for OS_MACOSX on line 27, so use that block. https://codereview.chromium.org/1214883004/diff/550001/content/public/test/mo... File content/public/test/mock_render_process_host.cc (right): https://codereview.chromium.org/1214883004/diff/550001/content/public/test/mo... content/public/test/mock_render_process_host.cc:290: return port; return MACH_PORT_NULL;
LGTM Thanks!
Patchset #22 (id:630001) has been deleted
Patchset #21 (id:610001) has been deleted
https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:570: // corresponding to each page. On 2015/08/24 20:59:22, gab wrote: > // Create a new tab for the no audio page and confirm that the processes of each > tabs are different and both valid. > > (and remove empty line between this block and ASSERTs as they really belong > together now) Done. https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:571: base::Process no_audio_process_ = ShowSingletonTab(no_audio_url_); On 2015/08/24 20:59:22, gab wrote: > |no_audio_process| (no longer a member variable, same for |audio_process|) Done. https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:572: no_audio_tab_web_contents_ = On 2015/08/24 20:59:22, gab wrote: > Perhaps move this after the ASSERTs as it's now no longer related to this block > so much? Done. https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:583: bool IsProcessBackgroundedHelper(const content::WebContents& web_contents) { On 2015/08/24 20:59:22, gab wrote: > I'd say const* is arguably better here so that you don't have to deref > everywhere below? > > Perhaps with > DCHECK(web_contents); > to enforce the contract that this method doesn't handle null ptrs (in fact > pretty sure WebContents tends to be passed by pointer most of the time -- looks > like 93 for const* vs 2 for const&: > https://code.google.com/p/chromium/codesearch#search/&q=const%5C%20content::W...) Done. https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:585: content::RenderProcessHost* rph = web_contents.GetRenderProcessHost(); On 2015/08/24 20:59:22, gab wrote: > const Done. https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:586: base::Process process = ProcessFromHandle(rph->GetHandle()); On 2015/08/24 20:59:22, gab wrote: > const Done. https://codereview.chromium.org/1214883004/diff/550001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:589: base::Process process = On 2015/08/24 20:59:22, gab wrote: > const Done. https://codereview.chromium.org/1214883004/diff/550001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/1214883004/diff/550001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:167: mach_port_t GetMachTaskPortForTesting() const override; On 2015/08/24 20:59:22, gab wrote: > Only need #include <mach/mach.h> for |mach_port_t| I think. > > e.g., ref: > https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/scoped_ma... > > If so, mach_broker_mac.h can go in .cc :-) In fact, I don't think I need to include mach/mach.h :) https://codereview.chromium.org/1214883004/diff/550001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:167: mach_port_t GetMachTaskPortForTesting() const override; On 2015/08/24 21:44:48, Robert Sesek wrote: > On 2015/08/24 20:59:22, gab wrote: > > Only need #include <mach/mach.h> for |mach_port_t| I think. > > > > e.g., ref: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/scoped_ma... > > > > If so, mach_broker_mac.h can go in .cc :-) > > Correct. And there's already an #if for OS_MACOSX on line 27, so use that block. Acknowledged. https://codereview.chromium.org/1214883004/diff/550001/content/public/test/mo... File content/public/test/mock_render_process_host.cc (right): https://codereview.chromium.org/1214883004/diff/550001/content/public/test/mo... content/public/test/mock_render_process_host.cc:290: return port; On 2015/08/24 21:44:48, Robert Sesek wrote: > return MACH_PORT_NULL; Done.
lgtm, thanks!!
lgtm
sebsg@chromium.org changed reviewers: + jam@chromium.org
jam@chromium.org: Please review changes in render_process_host_chrome_browsertest.cc. Thanks!
lgtm
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, miu@chromium.org, nick@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/1214883004/#ps650001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214883004/650001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1214883004/650001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214883004/650001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1214883004/650001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214883004/650001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1214883004/650001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
Patchset #22 (id:670001) has been deleted
Patchset #22 (id:690001) has been deleted
Patchset #22 (id:710001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214883004/730001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1214883004/730001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214883004/750001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1214883004/750001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #23 (id:750001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214883004/770001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1214883004/770001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
Patchset #23 (id:770001) has been deleted
Patchset #22 (id:730001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214883004/790001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1214883004/790001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #22 (id:790001) has been deleted
Patchset #22 (id:810001) has been deleted
Patchset #22 (id:830001) has been deleted
Patchset #22 (id:850001) has been deleted
Patchset #23 (id:890001) has been deleted
Patchset #23 (id:910001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214883004/930001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1214883004/930001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
Patchset #24 (id:950001) has been deleted
Patchset #24 (id:970001) has been deleted
Patchset #23 (id:930001) has been deleted
Patchset #23 (id:990001) has been deleted
Patchset #23 (id:1010001) has been deleted
Patchset #22 (id:870001) has been deleted
Patchset #23 (id:1050001) has been deleted
Patchset #23 (id:1070001) has been deleted
Patchset #22 (id:1030001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214883004/1120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1214883004/1120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
After I rebased, I refactored my code a little. It seems that there is no more process_mac so IsProcessBackgrounded doesn't need a mach port anymore. After that I added a line that sets is_process_backgrounded to the value of IsProcessBackgrounded() because setting a value of true or false was dependent on the platform (mostly android was different). However, it seems that even with that, the tests failed for android. It seems that the backgrouding or foregrounding function must be called even if the value is set to the right thing. I added a if !defined(OS_ANDROID) on the check in UpdateProcessPriority that returns if is_process_backgrounded_ is set to the desired value. That solved the problem. I will write a bug detailing all that so they can fix this behavior. Thanks everyone for you input and comments.
Latest changes lgtm. It's indeed a bug in process_android.cc if they're in a state where they expect SetProcessBackgrounded(false) to be called yet IsProcessBackgrounded() returns false... But wait... actually there is no process_android.cc? How do even redirect the backgrounding call to Java? https://codereview.chromium.org/1214883004/diff/1120001/content/browser/rende... File content/browser/renderer_host/render_process_host_impl.cc (left): https://codereview.chromium.org/1214883004/diff/1120001/content/browser/rende... content/browser/renderer_host/render_process_host_impl.cc:552: if (BootstrapSandboxManager::ShouldEnable()) Hmm why are we removing this? https://codereview.chromium.org/1214883004/diff/1120001/content/browser/rende... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/1120001/content/browser/rende... content/browser/renderer_host/render_process_host_impl.cc:194: #include "content/browser/mach_broker_mac.h" rm this include https://codereview.chromium.org/1214883004/diff/1120001/content/browser/rende... content/browser/renderer_host/render_process_host_impl.cc:2246: #if !defined(OS_ANDROID) Add a TODO with the bug ID here. https://codereview.chromium.org/1214883004/diff/1120001/content/browser/rende... content/browser/renderer_host/render_process_host_impl.cc:2312: is_process_backgrounded_ = Add a comment like: // Not all platforms launch processes in the same backgrounded state, make sure |is_process_backgrounded_| reflects this platform's state.
https://codereview.chromium.org/1214883004/diff/1120001/content/browser/rende... File content/browser/renderer_host/render_process_host_impl.cc (left): https://codereview.chromium.org/1214883004/diff/1120001/content/browser/rende... content/browser/renderer_host/render_process_host_impl.cc:552: if (BootstrapSandboxManager::ShouldEnable()) On 2015/09/30 13:33:31, gab wrote: > Hmm why are we removing this? Good catch, it's a case of ad merging. Thanks. https://codereview.chromium.org/1214883004/diff/1120001/content/browser/rende... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/1120001/content/browser/rende... content/browser/renderer_host/render_process_host_impl.cc:194: #include "content/browser/mach_broker_mac.h" On 2015/09/30 13:33:31, gab wrote: > rm this include Done. https://codereview.chromium.org/1214883004/diff/1120001/content/browser/rende... content/browser/renderer_host/render_process_host_impl.cc:2246: #if !defined(OS_ANDROID) On 2015/09/30 13:33:31, gab wrote: > Add a TODO with the bug ID here. Done. https://codereview.chromium.org/1214883004/diff/1120001/content/browser/rende... content/browser/renderer_host/render_process_host_impl.cc:2312: is_process_backgrounded_ = On 2015/09/30 13:33:31, gab wrote: > Add a comment like: > > // Not all platforms launch processes in the same backgrounded state, make sure > |is_process_backgrounded_| reflects this platform's state. Done.
https://codereview.chromium.org/1214883004/diff/1140001/content/browser/rende... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/1140001/content/browser/rende... content/browser/renderer_host/render_process_host_impl.cc:549: if (BootstrapSandboxManager::ShouldEnable()) Hmmm this is wrong, needs to be inside the '}' on line 546. https://codereview.chromium.org/1214883004/diff/1140001/content/browser/rende... content/browser/renderer_host/render_process_host_impl.cc:2250: // TODO(sebsg): Remove this if when https://crbug.com/537671 is fixed. s/remove this/remove this ifdef/ (to make it clear that the code itself shouldn't be removed) https://codereview.chromium.org/1214883004/diff/1140001/content/browser/rende... content/browser/renderer_host/render_process_host_impl.cc:2318: // sure |is_process_backgrounded_| reflects the platform's initial process s/the/this/
https://chromiumcodereview.appspot.com/1214883004/diff/1140001/content/browse... File content/browser/renderer_host/render_process_host_impl.cc (right): https://chromiumcodereview.appspot.com/1214883004/diff/1140001/content/browse... content/browser/renderer_host/render_process_host_impl.cc:549: if (BootstrapSandboxManager::ShouldEnable()) On 2015/09/30 20:05:23, gab wrote: > Hmmm this is wrong, needs to be inside the '}' on line 546. Done. https://chromiumcodereview.appspot.com/1214883004/diff/1140001/content/browse... content/browser/renderer_host/render_process_host_impl.cc:2250: // TODO(sebsg): Remove this if when https://crbug.com/537671 is fixed. On 2015/09/30 20:05:23, gab wrote: > s/remove this/remove this ifdef/ > > (to make it clear that the code itself shouldn't be removed) Done. https://chromiumcodereview.appspot.com/1214883004/diff/1140001/content/browse... content/browser/renderer_host/render_process_host_impl.cc:2318: // sure |is_process_backgrounded_| reflects the platform's initial process On 2015/09/30 20:05:23, gab wrote: > s/the/this/ Done.
https://chromiumcodereview.appspot.com/1214883004/diff/1160001/content/browse... File content/browser/renderer_host/render_process_host_impl.cc (right): https://chromiumcodereview.appspot.com/1214883004/diff/1160001/content/browse... content/browser/renderer_host/render_process_host_impl.cc:2251: #if !defined(OS_ANDROID) As I understand it, we need an android-specific workaround to force the "update background state" just once, at process startup -- we're correct thereafter. But this #ifdef will cause us to do it even more frequently than that. See below for a possible solution. https://chromiumcodereview.appspot.com/1214883004/diff/1160001/content/browse... content/browser/renderer_host/render_process_host_impl.cc:2321: child_process_launcher_->GetProcess().IsProcessBackgrounded(); How about this: DCHECK(!is_process_backgrounded_); UpdateProcessPriority(); #if defined(OS_ANDROID) if (!is_process_backgrounded_) { is_process_backgrounded_ = !is_process_backgrounded_; UpdateProcessPriority(); } #endif In other words, we just force Android to do a full SetProcessPriority operation at process launch time? I think this would let you get rid of the ifdef in the body of UpdateProcessPriority(), and would result in fewer redundant SetProcessPriority operations overall.
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214883004/1180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1214883004/1180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
https://chromiumcodereview.appspot.com/1214883004/diff/1160001/content/browse... File content/browser/renderer_host/render_process_host_impl.cc (right): https://chromiumcodereview.appspot.com/1214883004/diff/1160001/content/browse... content/browser/renderer_host/render_process_host_impl.cc:2321: child_process_launcher_->GetProcess().IsProcessBackgrounded(); On 2015/09/30 22:05:45, ncarter wrote: > How about this: > > DCHECK(!is_process_backgrounded_); > UpdateProcessPriority(); > #if defined(OS_ANDROID) > if (!is_process_backgrounded_) { > is_process_backgrounded_ = !is_process_backgrounded_; > UpdateProcessPriority(); > } > #endif > > In other words, we just force Android to do a full SetProcessPriority operation > at process launch time? I think this would let you get rid of the ifdef in the > body of UpdateProcessPriority(), and would result in fewer redundant > SetProcessPriority operations overall. IMO calling IsProcessBackgrounded() to get the initial state is the right thing to do here. i.e. the bug on Android IIUC is not that SetBackgrounded(false) needs to be called (that's okay), the bug it this class' assumption that processes start in non-backgrounded mode. The true Android specific bug however is that IsProcessBackgrounded() returns false when they expect a call to SetBackgrounded(false). This looks like it's because the Android hook to background processes is weirdly in child_process_launcher.cc rather than in the base process_android.cc (and there is thus no corresponding IsProcessBackgrounded() in practice). Either way the Android bug shouldn't be fixed in this CL IMO. Also, regarding calling multiple times, the code prior to this CL didn't have such a check (though there were less UpdateProcessPriority() calls). tl;dr; I think IsProcessBackgrounded() is the right thing for initialization and I'm happy with an Android specific hack for now.
https://chromiumcodereview.appspot.com/1214883004/diff/1160001/content/browse... File content/browser/renderer_host/render_process_host_impl.cc (right): https://chromiumcodereview.appspot.com/1214883004/diff/1160001/content/browse... content/browser/renderer_host/render_process_host_impl.cc:2321: child_process_launcher_->GetProcess().IsProcessBackgrounded(); On 2015/10/01 19:35:45, gab wrote: > On 2015/09/30 22:05:45, ncarter wrote: > > How about this: > > > > DCHECK(!is_process_backgrounded_); > > UpdateProcessPriority(); > > #if defined(OS_ANDROID) > > if (!is_process_backgrounded_) { > > is_process_backgrounded_ = !is_process_backgrounded_; > > UpdateProcessPriority(); > > } > > #endif > > > > In other words, we just force Android to do a full SetProcessPriority > operation > > at process launch time? I think this would let you get rid of the ifdef in the > > body of UpdateProcessPriority(), and would result in fewer redundant > > SetProcessPriority operations overall. > > IMO calling IsProcessBackgrounded() to get the initial state is the right thing > to do here. > > i.e. the bug on Android IIUC is not that SetBackgrounded(false) needs to be > called (that's okay), the bug it this class' assumption that processes start in > non-backgrounded mode. > The true Android specific bug however is that IsProcessBackgrounded() returns > false when they expect a call to SetBackgrounded(false). > > This looks like it's because the Android hook to background processes is weirdly > in child_process_launcher.cc rather than in the base process_android.cc (and > there is thus no corresponding IsProcessBackgrounded() in practice). > > Either way the Android bug shouldn't be fixed in this CL IMO. > > Also, regarding calling multiple times, the code prior to this CL didn't have > such a check (though there were less UpdateProcessPriority() calls). > > tl;dr; I think IsProcessBackgrounded() is the right thing for initialization and > I'm happy with an Android specific hack for now. Assuming we fix the Android return value of IsProcessBackgrounded(), won't it be important to still send the ChildProcessMsg_SetProcessBackgrounded IPC? And if so, how will we force it to be sent?
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214883004/1180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1214883004/1180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #27 (id:1180001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214883004/1160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1214883004/1160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://chromiumcodereview.appspot.com/1214883004/diff/1160001/content/browse... File content/browser/renderer_host/render_process_host_impl.cc (right): https://chromiumcodereview.appspot.com/1214883004/diff/1160001/content/browse... content/browser/renderer_host/render_process_host_impl.cc:2321: child_process_launcher_->GetProcess().IsProcessBackgrounded(); On 2015/10/01 21:10:51, ncarter wrote: > On 2015/10/01 19:35:45, gab wrote: > > On 2015/09/30 22:05:45, ncarter wrote: > > > How about this: > > > > > > DCHECK(!is_process_backgrounded_); > > > UpdateProcessPriority(); > > > #if defined(OS_ANDROID) > > > if (!is_process_backgrounded_) { > > > is_process_backgrounded_ = !is_process_backgrounded_; > > > UpdateProcessPriority(); > > > } > > > #endif > > > > > > In other words, we just force Android to do a full SetProcessPriority > > operation > > > at process launch time? I think this would let you get rid of the ifdef in > the > > > body of UpdateProcessPriority(), and would result in fewer redundant > > > SetProcessPriority operations overall. > > > > IMO calling IsProcessBackgrounded() to get the initial state is the right > thing > > to do here. > > > > i.e. the bug on Android IIUC is not that SetBackgrounded(false) needs to be > > called (that's okay), the bug it this class' assumption that processes start > in > > non-backgrounded mode. > > The true Android specific bug however is that IsProcessBackgrounded() returns > > false when they expect a call to SetBackgrounded(false). > > > > This looks like it's because the Android hook to background processes is > weirdly > > in child_process_launcher.cc rather than in the base process_android.cc (and > > there is thus no corresponding IsProcessBackgrounded() in practice). > > > > Either way the Android bug shouldn't be fixed in this CL IMO. > > > > Also, regarding calling multiple times, the code prior to this CL didn't have > > such a check (though there were less UpdateProcessPriority() calls). > > > > tl;dr; I think IsProcessBackgrounded() is the right thing for initialization > and > > I'm happy with an Android specific hack for now. > > Assuming we fix the Android return value of IsProcessBackgrounded(), won't it be > important to still send the ChildProcessMsg_SetProcessBackgrounded IPC? And if > so, how will we force it to be sent? Well IMO if they want it to be called then they should return true from IsProcessBackgrounded(), in which case our first call to UpdateProcessPriority() should trigger the signal.
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214883004/1200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1214883004/1200001
One small issue https://codereview.chromium.org/1214883004/diff/1200001/content/browser/rende... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/1200001/content/browser/rende... content/browser/renderer_host/render_process_host_impl.cc:2303: #else Gab removed this code a couple days ago, but it looks like you've reintroduced it when rebasing ( https://codereview.chromium.org/1377893002 was the deletion )
https://codereview.chromium.org/1214883004/diff/1200001/content/browser/rende... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1214883004/diff/1200001/content/browser/rende... content/browser/renderer_host/render_process_host_impl.cc:2303: #else On 2015/10/05 17:10:39, ncarter wrote: > Gab removed this code a couple days ago, but it looks like you've reintroduced > it when rebasing ( https://codereview.chromium.org/1377893002 was the deletion ) Done.
On 2015/10/05 18:42:40, sebsg wrote: > https://codereview.chromium.org/1214883004/diff/1200001/content/browser/rende... > File content/browser/renderer_host/render_process_host_impl.cc (right): > > https://codereview.chromium.org/1214883004/diff/1200001/content/browser/rende... > content/browser/renderer_host/render_process_host_impl.cc:2303: #else > On 2015/10/05 17:10:39, ncarter wrote: > > Gab removed this code a couple days ago, but it looks like you've reintroduced > > it when rebasing ( https://codereview.chromium.org/1377893002 was the deletion > ) > > Done. lgtm. check it in!
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, jam@chromium.org, dalecurtis@chromium.org, rsesek@chromium.org, miu@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/1214883004/#ps1220001 (title: "Fixed bad rebase merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214883004/1220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1214883004/1220001
Message was sent while issue was closed.
Committed patchset #28 (id:1220001)
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/349188e9ed037b427815c9e5ad55223d043ab6fd Cr-Commit-Position: refs/heads/master@{#352421}
Message was sent while issue was closed.
A revert of this CL (patchset #28 id:1220001) has been created in https://codereview.chromium.org/1383123003/ by henrika@chromium.org. The reason for reverting is: Speculative revert since we see failing tests in the WebRTC waterfall related to audio. See https://build.chromium.org/p/chromium.webrtc/builders/Win7%20Tester/builds/21950 https://build.chromium.org/p/chromium.webrtc/builders/Mac%20Tester/builds/43969. |
