Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2689963003:
Don't report a keepalive count for persistent pages. (Closed)
DescriptionDon't report a keepalive count for persistent pages.
Rather than reporting a keepalive count of zero for extensions with
persistent background pages, return -1, to indicate that the keepalive
count is meaningless for that extension.
BUG=688766
Review-Url: https://codereview.chromium.org/2689963003
Cr-Commit-Position: refs/heads/master@{#450495}
Committed: https://chromium.googlesource.com/chromium/src/+/714dde19e175737a2915afce6e303ff587818368
Patch Set 1 #Patch Set 2 : Update ProcessManagerBrowserTests for GetLazyKeepaliveCount #
Total comments: 6
Patch Set 3 : Address review comments #
Total comments: 2
Patch Set 4 : Fix typo #
Messages
Total messages: 33 (21 generated)
The CQ bit was checked by wez@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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
wez@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Devlin, At present the ProcessManager reports a keepalive count of zero for persistent background pages, causing them to be reported in the new Task Manager column. I can fix that in the TaskManager sampler, by explicitly checking whether the Extension has a lazy background page, and not calling GetLazyKeepaliveCount in that case, but it seems strange to have GLKC returning zero for persistent pages. WDYT?
On 2017/02/13 05:09:12, Wez wrote: > Devlin, > > At present the ProcessManager reports a keepalive count of zero for persistent > background pages, causing them to be reported in the new Task Manager column. > > I can fix that in the TaskManager sampler, by explicitly checking whether the > Extension has a lazy background page, and not calling GetLazyKeepaliveCount in > that case, but it seems strange to have GLKC returning zero for persistent > pages. > > WDYT? Having a -1 return result to indicate that it's not an event page (and thus can never have a keepalive count) sgtm. From a quick codesearch, it also looks like this is almost exclusively called from tests, so it should be reasonable to update. Looks like a few bots are failing; mind re-pinging when they're all green? But conceptually, this sgtm.
On 2017/02/13 16:12:54, Devlin wrote: > Having a -1 return result to indicate that it's not an event page (and thus can > never have a keepalive count) sgtm. From a quick codesearch, it also looks like > this is almost exclusively called from tests, so it should be reasonable to > update. Done. I added sanity-checks that the extension has a lazy background page, as well. One thing that wasn't clear is whether an extension with no explicit background page should still return true for HasBackgroundPage and/or HasLazyBackgroundPage?
The CQ bit was checked by wez@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.
> One thing that wasn't clear is whether an extension with no explicit background > page should still return true for HasBackgroundPage and/or > HasLazyBackgroundPage? Depends on what you mean by "explicit background page". An extension with no background presence (i.e., nothing specified for a background* key in the manifest) should return false, and -1 for keepalive. An extension that specifies background scripts, but not an explicit page, should still have a (possibly lazy) background page because we generate one for it. I think that BackgroundInfo does the right thing for both of these scenarios already. https://codereview.chromium.org/2689963003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (left): https://codereview.chromium.org/2689963003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:334: EXPECT_EQ(0, pm->GetLazyKeepaliveCount(popup.get())); Any reason to not update these, rather than remove them? https://codereview.chromium.org/2689963003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2689963003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:612: EXPECT_FALSE(BackgroundInfo::HasPersistentBackgroundPage(extension.get())); nit: prefer EXPECT_TRUE(BackgroundInfo::HasLazyBackgroundPage(extension.get()); https://codereview.chromium.org/2689963003/diff/20001/extensions/browser/proc... File extensions/browser/process_manager.h (right): https://codereview.chromium.org/2689963003/diff/20001/extensions/browser/proc... extensions/browser/process_manager.h:120: // Returns -1 if |extension| has a persistent background page. nit: s/has a persistent background page/doesn't have a lazy background page.
The CQ bit was checked by wez@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...
On 2017/02/14 16:18:24, Devlin wrote: > > One thing that wasn't clear is whether an extension with no explicit > background > > page should still return true for HasBackgroundPage and/or > > HasLazyBackgroundPage? > > Depends on what you mean by "explicit background page". An extension with no > background presence (i.e., nothing specified for a background* key in the > manifest) should return false, and -1 for keepalive. An extension that > specifies background scripts, but not an explicit page, should still have a > (possibly lazy) background page because we generate one for it. > > I think that BackgroundInfo does the right thing for both of these scenarios > already. So it does - perfect :)
https://codereview.chromium.org/2689963003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (left): https://codereview.chromium.org/2689963003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:334: EXPECT_EQ(0, pm->GetLazyKeepaliveCount(popup.get())); On 2017/02/14 16:18:24, Devlin wrote: > Any reason to not update these, rather than remove them? D'oh; done. Good point, although this test is DISABLED anyway. :( (Sounds like |frame_observer.Wait()| isn't reliable?) https://codereview.chromium.org/2689963003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2689963003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:612: EXPECT_FALSE(BackgroundInfo::HasPersistentBackgroundPage(extension.get())); On 2017/02/14 16:18:24, Devlin wrote: > nit: prefer EXPECT_TRUE(BackgroundInfo::HasLazyBackgroundPage(extension.get()); Done. Also added a sanity-check for the baseline keepalive count. https://codereview.chromium.org/2689963003/diff/20001/extensions/browser/proc... File extensions/browser/process_manager.h (right): https://codereview.chromium.org/2689963003/diff/20001/extensions/browser/proc... extensions/browser/process_manager.h:120: // Returns -1 if |extension| has a persistent background page. On 2017/02/14 16:18:24, Devlin wrote: > nit: s/has a persistent background page/doesn't have a lazy background page. Done.
lgtm https://codereview.chromium.org/2689963003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (left): https://codereview.chromium.org/2689963003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:353: EXPECT_EQ(0, pm->GetLazyKeepaliveCount(popup.get())); nitty nit: might be worth keeping this (as a check for -1) to ensure that a popup doesn't increment keepalive for an extension without a background page.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 wez@chromium.org to run a CQ dry run
https://codereview.chromium.org/2689963003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (left): https://codereview.chromium.org/2689963003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:353: EXPECT_EQ(0, pm->GetLazyKeepaliveCount(popup.get())); On 2017/02/14 17:48:37, Devlin wrote: > nitty nit: might be worth keeping this (as a check for -1) to ensure that a > popup doesn't increment keepalive for an extension without a background page. Done.
The CQ bit was unchecked by wez@chromium.org
The CQ bit was checked by wez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2689963003/#ps60001 (title: "Fix typo")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by wez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487107730363600,
"parent_rev": "2859a2ac06ab5d9df6706cc45525dc4a2085051c", "commit_rev":
"714dde19e175737a2915afce6e303ff587818368"}
Message was sent while issue was closed.
Description was changed from ========== Don't report a keepalive count for persistent pages. Rather than reporting a keepalive count of zero for extensions with persistent background pages, return -1, to indicate that the keepalive count is meaningless for that extension. BUG=688766 ========== to ========== Don't report a keepalive count for persistent pages. Rather than reporting a keepalive count of zero for extensions with persistent background pages, return -1, to indicate that the keepalive count is meaningless for that extension. BUG=688766 Review-Url: https://codereview.chromium.org/2689963003 Cr-Commit-Position: refs/heads/master@{#450495} Committed: https://chromium.googlesource.com/chromium/src/+/714dde19e175737a2915afce6e30... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/714dde19e175737a2915afce6e30... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
