Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2454073003:
Allow backgrounding processes on Mac (Closed)
DescriptionAllow backgrounding processes on Mac
Based on shrike@'s work in https://codereview.chromium.org/989703002
The original change was reverted as a result of
https://crbug.com/536170. The underlying issue (backgrounding
affecting session restore) was fixed in
https://codereview.chromium.org/1769123002, so this change mostly
adapts the original code to the current state of the codebase and adds
a new feature to gate it.
BUG=460102, 536170
Committed: https://crrev.com/0d2bafc9a7a14e5567201f585110abd0b89e32ae
Cr-Commit-Position: refs/heads/master@{#430289}
Patch Set 1 #Patch Set 2 : Use newly discovered port provider access in chrome/browser to fix task manager and tests #Patch Set 3 : Remove unnecessary forward declare #
Total comments: 17
Patch Set 4 : Review comments #Patch Set 5 : Gate process backgrounding tests on |CanBackgroundProcess| #Patch Set 6 : Syntax error :/ #Patch Set 7 : Stray file #Patch Set 8 : Fix typo #Patch Set 9 : Rename feature and fix comment indentation #
Total comments: 6
Patch Set 10 : Remove pre 10.9 shims and streamline render process host tests #Patch Set 11 : Fix comment again :( #Patch Set 12 : Initialize port provider in test #Patch Set 13 : Forgot ifdef #
Total comments: 4
Patch Set 14 : Address review comments #
Messages
Total messages: 66 (45 generated)
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgrey@chromium.org changed reviewers: + rsesek@chromium.org, shrike@chromium.org, thakis@chromium.org
PTAL Didn't restore https://codereview.chromium.org/1199543003 since it looks like it was reverted because of concerns re: tab snapshotting. The Windows experiment is wrapped up, so went with creating a new feature for this
Looks fine, but I'm confused about the current status of this. The BUG= this links to is marked Fixed, and the CL linked to in the CL description landed and didn't have a revert on it. I guess it was reverted somewhere, for some reason? Can you describe (in the CL description) when, where, and why it was reverted, and what you changed for this (presumed?) reland? https://codereview.chromium.org/2454073003/diff/40001/base/process/process_ma... File base/process/process_mac.cc (right): https://codereview.chromium.org/2454073003/diff/40001/base/process/process_ma... base/process/process_mac.cc:14: // reach 10.9 or higher. erikchen: I know we're pretty close to bumping deployment target to – do you think it'll happen this week, or is it a little further out than that? (Mostly curious, I don't think this CL needs to wait for that)
On 2016/10/31 14:02:54, Nico wrote: > Looks fine, but I'm confused about the current status of this. The BUG= this > links to is marked Fixed, and the CL linked to in the CL description landed and > didn't have a revert on it. I guess it was reverted somewhere, for some reason? > Can you describe (in the CL description) when, where, and why it was reverted, > and what you changed for this (presumed?) reland? > > https://codereview.chromium.org/2454073003/diff/40001/base/process/process_ma... > File base/process/process_mac.cc (right): > > https://codereview.chromium.org/2454073003/diff/40001/base/process/process_ma... > base/process/process_mac.cc:14: // reach 10.9 or higher. > erikchen: I know we're pretty close to bumping deployment target to – do you > think it'll happen this week, or is it a little further out than that? (Mostly > curious, I don't think this CL needs to wait for that) Sorry for the lack of context: The original change was reverted as a result of crbug/536170. The underlying issue (backgrounding affecting session restore) was fixed between the revert and now, so this change mostly adapts the original code to the current state of the codebase and adds a new feature to gate it.
Description was changed from ========== Allow backgrounding processes on Mac Based on shrike@'s work in https://codereview.chromium.org/989703002 BUG=460102 ========== to ========== Allow backgrounding processes on Mac Based on shrike@'s work in https://codereview.chromium.org/989703002 The original change was reverted as a result of crbug/536170. The underlying issue (backgrounding affecting session restore) was fixed between the revert and now, so this change mostly adapts the original code to the current state of the codebase and adds a new feature to gate it. BUG=460102 ==========
On 2016/10/31 14:14:07, lgrey wrote: > On 2016/10/31 14:02:54, Nico wrote: > > Looks fine, but I'm confused about the current status of this. The BUG= this > > links to is marked Fixed, and the CL linked to in the CL description landed > and > > didn't have a revert on it. I guess it was reverted somewhere, for some > reason? > > Can you describe (in the CL description) when, where, and why it was reverted, > > and what you changed for this (presumed?) reland? > > > > > https://codereview.chromium.org/2454073003/diff/40001/base/process/process_ma... > > File base/process/process_mac.cc (right): > > > > > https://codereview.chromium.org/2454073003/diff/40001/base/process/process_ma... > > base/process/process_mac.cc:14: // reach 10.9 or higher. > > erikchen: I know we're pretty close to bumping deployment target to – do you > > think it'll happen this week, or is it a little further out than that? (Mostly > > curious, I don't think this CL needs to wait for that) > > Sorry for the lack of context: > > The original change was reverted as a result of crbug/536170. The underlying > issue (backgrounding affecting session restore) was fixed between the revert and > now, so this change mostly adapts the original code to the current state of the > codebase and adds a new feature to gate it. Ah, thanks. What was the fix that's now landed? (Add to CL description) To make things less confusing for future archaeologists, can you reopen 460102 with a note saying that the change got reverted (ideally with link to the revert) because of bug 536170, and add 536170 to the BUG= line of this CL? (Also, two nits: Don't omit ".com" from "crbug" in the CL description so that rietveld can linkify the link, and wrap CL descriptions at around 80 columns so that I don't need to scroll horizontally in `git log`)
Description was changed from ========== Allow backgrounding processes on Mac Based on shrike@'s work in https://codereview.chromium.org/989703002 The original change was reverted as a result of crbug/536170. The underlying issue (backgrounding affecting session restore) was fixed between the revert and now, so this change mostly adapts the original code to the current state of the codebase and adds a new feature to gate it. BUG=460102 ========== to ========== Allow backgrounding processes on Mac Based on shrike@'s work in https://codereview.chromium.org/989703002 The original change was reverted as a result of crbug.com/536170. The underlying issue (backgrounding affecting session restore) was fixed between the revert and now, so this change mostly adapts the original code to the current state of the codebase and adds a new feature to gate it. BUG=460102 ==========
Description was changed from ========== Allow backgrounding processes on Mac Based on shrike@'s work in https://codereview.chromium.org/989703002 The original change was reverted as a result of crbug.com/536170. The underlying issue (backgrounding affecting session restore) was fixed between the revert and now, so this change mostly adapts the original code to the current state of the codebase and adds a new feature to gate it. BUG=460102 ========== to ========== Allow backgrounding processes on Mac Based on shrike@'s work in https://codereview.chromium.org/989703002 The original change was reverted as a result of https://crbug.com/536170. The underlying issue (backgrounding affecting session restore) was fixed in https://codereview.chromium.org/1769123002, so this change mostly adapts the original code to the current state of the codebase and adds a new feature to gate it. BUG=460102, 536170 ==========
> erikchen: I know we're pretty close to bumping deployment target to – do you > think it'll happen this week, or is it a little further out than that? (Mostly > curious, I don't think this CL needs to wait for that) There's one more CL we're waiting on before we can bump 10.9.
https://codereview.chromium.org/2454073003/diff/40001/base/process/process.h File base/process/process.h (right): https://codereview.chromium.org/2454073003/diff/40001/base/process/process.h#... base/process/process.h:122: // it Move straggler "it" to the next line? https://codereview.chromium.org/2454073003/diff/40001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/2454073003/diff/40001/content/browser/child_p... content/browser/child_process_launcher.cc:391: if (base::FeatureList::IsEnabled(features::kMacBackgroundInactiveTabs)) Is this the only place you need to check for the experiment being enabled? I think UpdateProcessPriority(), for example, can get called from a few different places and would need this check. I feel like I remember the experiment check living in SetProcessBackgrounded() previously, but I guess that won't work with a content feature? If it's possible to live in SetProcessBackgrounded() (or CanBackgroundProcesses() ) that might be a better place, so that you have a single chokepoint for calls to background the process. https://codereview.chromium.org/2454073003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2454073003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2817: // Disable updating process priority on startup for now as it incorrectly It seems like this comment should stay at its current indentation level? https://codereview.chromium.org/2454073003/diff/40001/content/public/common/c... File content/public/common/content_features.h (right): https://codereview.chromium.org/2454073003/diff/40001/content/public/common/c... content/public/common/content_features.h:75: CONTENT_EXPORT extern const base::Feature kMacBackgroundInactiveTabs; Technically the renderers are hidden. I think it's better to call this kMacBackgroundHiddenRenderers so that the feature name is as clear as can be (when you switch to another tab, is the old tab really inactive? Yes and no, because it still might be doing some work).
Overall looks pretty good! https://codereview.chromium.org/2454073003/diff/40001/base/process/process_ma... File base/process/process_mac.cc (right): https://codereview.chromium.org/2454073003/diff/40001/base/process/process_ma... base/process/process_mac.cc:69: DCHECK(IsValid()); Move this before the return check, and on line 97, so that side effect of |port_provider| doesn't influence this. https://codereview.chromium.org/2454073003/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/2454073003/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:663: DLOG(ERROR) << "Port provider " << (port_provider != nullptr); Remove. https://codereview.chromium.org/2454073003/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:791: // Wait until the two pages are not backgrounded. Duplicate comment.
https://codereview.chromium.org/2454073003/diff/40001/base/process/process.h File base/process/process.h (right): https://codereview.chromium.org/2454073003/diff/40001/base/process/process.h#... base/process/process.h:122: // it On 2016/10/31 17:26:04, shrike wrote: > Move straggler "it" to the next line? Done. https://codereview.chromium.org/2454073003/diff/40001/base/process/process_ma... File base/process/process_mac.cc (right): https://codereview.chromium.org/2454073003/diff/40001/base/process/process_ma... base/process/process_mac.cc:69: DCHECK(IsValid()); On 2016/10/31 18:45:39, Robert Sesek wrote: > Move this before the return check, and on line 97, so that side effect of > |port_provider| doesn't influence this. Done. https://codereview.chromium.org/2454073003/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/2454073003/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:663: DLOG(ERROR) << "Port provider " << (port_provider != nullptr); On 2016/10/31 18:45:39, Robert Sesek wrote: > Remove. Whoops! Done https://codereview.chromium.org/2454073003/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:791: // Wait until the two pages are not backgrounded. On 2016/10/31 18:45:39, Robert Sesek wrote: > Duplicate comment. Done. https://codereview.chromium.org/2454073003/diff/40001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/2454073003/diff/40001/content/browser/child_p... content/browser/child_process_launcher.cc:391: if (base::FeatureList::IsEnabled(features::kMacBackgroundInactiveTabs)) On 2016/10/31 17:26:05, shrike wrote: > Is this the only place you need to check for the experiment being enabled? I > think UpdateProcessPriority(), for example, can get called from a few different > places and would need this check. > > I feel like I remember the experiment check living in SetProcessBackgrounded() > previously, but I guess that won't work with a content feature? If it's possible > to live in SetProcessBackgrounded() (or CanBackgroundProcesses() ) that might be > a better place, so that you have a single chokepoint for calls to background the > process. > > Moved to CanBackgroundProcesses and moved the feature definition to process. The only reason I had it in content_features is that's where it seemed like it belonged if the usage was in content. https://codereview.chromium.org/2454073003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2454073003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2817: // Disable updating process priority on startup for now as it incorrectly On 2016/10/31 17:26:05, shrike wrote: > It seems like this comment should stay at its current indentation level? This was from git cl format. Is it OK to override it? https://codereview.chromium.org/2454073003/diff/40001/content/public/common/c... File content/public/common/content_features.h (right): https://codereview.chromium.org/2454073003/diff/40001/content/public/common/c... content/public/common/content_features.h:75: CONTENT_EXPORT extern const base::Feature kMacBackgroundInactiveTabs; On 2016/10/31 17:26:05, shrike wrote: > Technically the renderers are hidden. I think it's better to call this > kMacBackgroundHiddenRenderers so that the feature name is as clear as can be > (when you switch to another tab, is the old tab really inactive? Yes and no, > because it still might be doing some work). Done.
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2454073003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2454073003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2817: // Disable updating process priority on startup for now as it incorrectly On 2016/10/31 19:22:02, lgrey wrote: > On 2016/10/31 17:26:05, shrike wrote: > > It seems like this comment should stay at its current indentation level? > > This was from git cl format. Is it OK to override it? As long as your change follows the spec it should be OK to change what git cl format does. That said, this formatting should be based on the brace nesting level, so it would be surprising for it to be wrong. I applied your patch and the formatting looks correct - I don't know what's going on but it seems like leaving as is should be OK.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2454073003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2454073003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2817: // Disable updating process priority on startup for now as it incorrectly On 2016/10/31 20:32:36, shrike wrote: > On 2016/10/31 19:22:02, lgrey wrote: > > On 2016/10/31 17:26:05, shrike wrote: > > > It seems like this comment should stay at its current indentation level? > > > > This was from git cl format. Is it OK to override it? > > As long as your change follows the spec it should be OK to change what git cl > format does. That said, this formatting should be based on the brace nesting > level, so it would be surprising for it to be wrong. I applied your patch and > the formatting looks correct - I don't know what's going on but it seems like > leaving as is should be OK. `git cl format` aligns comments in front of a preprocessor directive with the alignment of the following preprocessor directive (or the prior one, don't remember right now). If you don't want this, move the comment after the preprocessor directive.
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Fixed up comments, PTAL
lgtm I saw that tests (at least some) are disabled unless process backgrounding is enabled, which reminded me about perf bot testing. I guess that will be covered by the field trial testing config that you will have to submit with the experiment? https://g3doc.corp.google.com/analysis/uma/g3doc/finch/fieldtrial-testing-con...
https://codereview.chromium.org/2454073003/diff/160001/base/process/process_m... File base/process/process_mac.cc (right): https://codereview.chromium.org/2454073003/diff/160001/base/process/process_m... base/process/process_mac.cc:14: // TODO(shrike): Remove the TASK_OVERRIDE_QOS_POLICY ifndef once builders I think this may have happened! https://bugs.chromium.org/p/chromium/issues/detail?id=580152 https://codereview.chromium.org/2454073003/diff/160001/base/process/process_m... base/process/process_mac.cc:122: } else if (!mac::IsAtLeastOS10_9()) { This is always true. https://codereview.chromium.org/2454073003/diff/160001/chrome/browser/rendere... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/2454073003/diff/160001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:753: #if defined(OS_MACOSX) You could refactor this whole block into a protected method on ChromeRenderProcessHostTest to reduce the copypasta.
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2454073003/diff/160001/base/process/process_m... File base/process/process_mac.cc (right): https://codereview.chromium.org/2454073003/diff/160001/base/process/process_m... base/process/process_mac.cc:14: // TODO(shrike): Remove the TASK_OVERRIDE_QOS_POLICY ifndef once builders On 2016/11/03 20:18:42, Robert Sesek wrote: > I think this may have happened! > https://bugs.chromium.org/p/chromium/issues/detail?id=580152 Done. https://codereview.chromium.org/2454073003/diff/160001/base/process/process_m... base/process/process_mac.cc:122: } else if (!mac::IsAtLeastOS10_9()) { On 2016/11/03 20:18:42, Robert Sesek wrote: > This is always true. Done. https://codereview.chromium.org/2454073003/diff/160001/chrome/browser/rendere... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/2454073003/diff/160001/chrome/browser/rendere... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:753: #if defined(OS_MACOSX) On 2016/11/03 20:18:42, Robert Sesek wrote: > You could refactor this whole block into a protected method on > ChromeRenderProcessHostTest to reduce the copypasta. Done.
LGTM w/ two nits https://codereview.chromium.org/2454073003/diff/240001/base/process/process_m... File base/process/process_mac.cc (right): https://codereview.chromium.org/2454073003/diff/240001/base/process/process_m... base/process/process_mac.cc:10: #include <mach/mach.h> nit: goes between lines 5 and 7 https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.chromium.org/2454073003/diff/240001/base/process/process_m... base/process/process_mac.cc:58: if (!CanBackgroundProcesses()) { Move this check up to line 51, to match IsProcesBackgrounded().
lgrey@chromium.org changed reviewers: + avi@chromium.org
Avi, Nico PTAL for content and base OWNERS, respectively, when convenient :) https://codereview.chromium.org/2454073003/diff/240001/base/process/process_m... File base/process/process_mac.cc (right): https://codereview.chromium.org/2454073003/diff/240001/base/process/process_m... base/process/process_mac.cc:10: #include <mach/mach.h> On 2016/11/04 16:51:16, Robert Sesek wrote: > nit: goes between lines 5 and 7 > > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.chromium.org/2454073003/diff/240001/base/process/process_m... base/process/process_mac.cc:58: if (!CanBackgroundProcesses()) { On 2016/11/04 16:51:16, Robert Sesek wrote: > Move this check up to line 51, to match IsProcesBackgrounded(). Done.
lgtm
content lgtm
The CQ bit was checked by lgrey@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shrike@chromium.org, rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2454073003/#ps260001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Allow backgrounding processes on Mac Based on shrike@'s work in https://codereview.chromium.org/989703002 The original change was reverted as a result of https://crbug.com/536170. The underlying issue (backgrounding affecting session restore) was fixed in https://codereview.chromium.org/1769123002, so this change mostly adapts the original code to the current state of the codebase and adds a new feature to gate it. BUG=460102, 536170 ========== to ========== Allow backgrounding processes on Mac Based on shrike@'s work in https://codereview.chromium.org/989703002 The original change was reverted as a result of https://crbug.com/536170. The underlying issue (backgrounding affecting session restore) was fixed in https://codereview.chromium.org/1769123002, so this change mostly adapts the original code to the current state of the codebase and adds a new feature to gate it. BUG=460102, 536170 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Allow backgrounding processes on Mac Based on shrike@'s work in https://codereview.chromium.org/989703002 The original change was reverted as a result of https://crbug.com/536170. The underlying issue (backgrounding affecting session restore) was fixed in https://codereview.chromium.org/1769123002, so this change mostly adapts the original code to the current state of the codebase and adds a new feature to gate it. BUG=460102, 536170 ========== to ========== Allow backgrounding processes on Mac Based on shrike@'s work in https://codereview.chromium.org/989703002 The original change was reverted as a result of https://crbug.com/536170. The underlying issue (backgrounding affecting session restore) was fixed in https://codereview.chromium.org/1769123002, so this change mostly adapts the original code to the current state of the codebase and adds a new feature to gate it. BUG=460102, 536170 Committed: https://crrev.com/0d2bafc9a7a14e5567201f585110abd0b89e32ae Cr-Commit-Position: refs/heads/master@{#430289} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/0d2bafc9a7a14e5567201f585110abd0b89e32ae Cr-Commit-Position: refs/heads/master@{#430289} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
