Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1096313003:
Create interface "EventPageTracker" for checking an extension's (Closed)
|
Created:
5 years, 8 months ago by Kevin M Modified:
5 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate interface "EventPageTracker" for checking an extension's
suspend state and waking it up.
Make ProcessManager inherit from EventPageTracker.
Add new WakeExtension method to ProcessManager.
R=kalman@chromium.org,mfoltz@chromium.org
BUG=
Committed: https://crrev.com/24b29b2f35f869b17579d6e95290482cec8ea4f1
Cr-Commit-Position: refs/heads/master@{#327412}
Patch Set 1 #Patch Set 2 : More documentation. #
Total comments: 7
Patch Set 3 : Code review feedback. #
Total comments: 16
Patch Set 4 : Code review feedback. #
Total comments: 2
Patch Set 5 : 80col rewrap #
Total comments: 2
Patch Set 6 : Merge with master. #Patch Set 7 : Fix IWYU linter error while I'm at it. #
Messages
Total messages: 22 (6 generated)
https://codereview.chromium.org/1096313003/diff/20001/extensions/browser/even... File extensions/browser/event_page_tracker.h (right): https://codereview.chromium.org/1096313003/diff/20001/extensions/browser/even... extensions/browser/event_page_tracker.h:27: const std::string& extension_id) = 0; GetEventPageForExtension? Just "background host" implies that it's all background pages. https://codereview.chromium.org/1096313003/diff/20001/extensions/browser/proc... File extensions/browser/process_manager.cc (right): https://codereview.chromium.org/1096313003/diff/20001/extensions/browser/proc... extensions/browser/process_manager.cc:419: callback.Run(true); Re-entrant callbacks lead to insidious bugs. Either post to the current message loop, or signal to caller via a bool return value if already active (and don't call the callback in that case). https://codereview.chromium.org/1096313003/diff/20001/extensions/browser/proc... extensions/browser/process_manager.cc:424: base::Bind(&OnExtensionLoad, callback)); Is there a better / more descriptive name than just "OnExtensionLoad"?
marshallk@google.com changed reviewers: + marshallk@google.com
https://codereview.chromium.org/1096313003/diff/20001/extensions/browser/even... File extensions/browser/event_page_tracker.h (right): https://codereview.chromium.org/1096313003/diff/20001/extensions/browser/even... extensions/browser/event_page_tracker.h:27: const std::string& extension_id) = 0; On 2015/04/24 20:05:08, kalman wrote: > GetEventPageForExtension? Just "background host" implies that it's all > background pages. This function is a pre-existing method in ProcessManager that has been promoted to this interface. Do you want me to rename that method, or create a new one? If I'm creating a new method for getting an extension's suspend state, I would prefer to create a method with a simpler bool return value. It'd clean up the interface quite a bit - no need for callers to reason about ExtensionHost objects etc. https://codereview.chromium.org/1096313003/diff/20001/extensions/browser/proc... File extensions/browser/process_manager.cc (right): https://codereview.chromium.org/1096313003/diff/20001/extensions/browser/proc... extensions/browser/process_manager.cc:419: callback.Run(true); On 2015/04/24 20:05:08, kalman wrote: > Re-entrant callbacks lead to insidious bugs. Either post to the current message > loop, or signal to caller via a bool return value if already active (and don't > call the callback in that case). Good point, using return value. Updated docs. https://codereview.chromium.org/1096313003/diff/20001/extensions/browser/proc... extensions/browser/process_manager.cc:424: base::Bind(&OnExtensionLoad, callback)); On 2015/04/24 20:05:08, kalman wrote: > Is there a better / more descriptive name than just "OnExtensionLoad"? How's this? "PropagateExtensionWakeResult"
https://codereview.chromium.org/1096313003/diff/20001/extensions/browser/even... File extensions/browser/event_page_tracker.h (right): https://codereview.chromium.org/1096313003/diff/20001/extensions/browser/even... extensions/browser/event_page_tracker.h:27: const std::string& extension_id) = 0; On 2015/04/24 21:04:53, Kevin Marshall wrote: > On 2015/04/24 20:05:08, kalman wrote: > > GetEventPageForExtension? Just "background host" implies that it's all > > background pages. > > This function is a pre-existing method in ProcessManager that has been promoted > to this interface. Do you want me to rename that method, or create a new one? If > I'm creating a new method for getting an extension's suspend state, I would > prefer to create a method with a simpler bool return value. It'd clean up the > interface quite a bit - no need for callers to reason about ExtensionHost > objects etc. Sure, if you prefer a bool then makes sense to create a new interface which just calls into GetBackgroundHostForExtension.
Mostly copy editing suggestions for the documentation. And unit tests? :) How expensive is this? It looks like it will iterate through background_hosts_ each time. :-/ https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... File extensions/browser/event_page_tracker.h (right): https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:17: // Defines a set of methods that can be used to track an extension's event page Or just "Tracks an extension's..." as all classes define a set of methods :) https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:17: // Defines a set of methods that can be used to track an extension's event page Maybe: Tracks an extension's event page suspend state, and wakes the event page on demand. https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:26: virtual ExtensionHost* GetBackgroundHostForExtension( Why not have an IsSuspended(extension_id) that is a more readable form of (GBHFE != null)? https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:29: // Wakes an extension from a suspended state and calls |callback| after it is As a nit, I would omit everything after |callback| since it seems like this method doesn't guarantee wakeup. https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:30: // reactivated. The callback is passed true if the extension was s/The callback/|callback|/ https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:30: // reactivated. The callback is passed true if the extension was s/reactivated/awoken/ https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:31: // woken up successfully, or false if an error occurred. If an error occurs, is the suspend/wake status of the event page known? Maybe the callback should take two arguments: - an enum { AWAKE, SUSPENDED, UNKNOWN } with the status - an error boolean. https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:33: // Function returns true if a wake operation is scheduled, or false if the s/Function// https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:34: // extension is already active and does not require waking. s/active/awake/ and omit the rest
> "And unit tests"? I spent a couple hours trying to get this to work, but the new functionality uses on ExtensionHost objects, which can't be constructed in unit tests. From process_manager_unittest.cc: // NOTE: This test and those that follow do not try to create ExtensionsHosts // because ExtensionHost is tightly coupled to WebContents and can't be // constructed in unit tests. WRT performance, kalman@: is an O(n) iteration through the items of ExtensionHostSet acceptable for performance, or could I move ExtensionHostSet to a hash_map for quick O(1) access? https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... File extensions/browser/event_page_tracker.h (right): https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:17: // Defines a set of methods that can be used to track an extension's event page On 2015/04/24 22:54:48, mark a. foltz wrote: > Or just "Tracks an extension's..." as all classes define a set of methods :) Done. https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:17: // Defines a set of methods that can be used to track an extension's event page On 2015/04/24 22:54:48, mark a. foltz wrote: > Maybe: > > Tracks an extension's event page suspend state, and wakes the event page on > demand. Acknowledged. https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:26: virtual ExtensionHost* GetBackgroundHostForExtension( On 2015/04/24 22:54:48, mark a. foltz wrote: > Why not have an IsSuspended(extension_id) that is a more readable form of > (GBHFE != null)? Done. I originally created this class to simply expose a subset of existing ProcessManager methods, but it makes sense to have the methods be standalone. https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:29: // Wakes an extension from a suspended state and calls |callback| after it is On 2015/04/24 22:54:48, mark a. foltz wrote: > As a nit, I would omit everything after |callback| since it seems like this > method doesn't guarantee wakeup. How's this? https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:30: // reactivated. The callback is passed true if the extension was On 2015/04/24 22:54:48, mark a. foltz wrote: > s/The callback/|callback|/ Done. https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:33: // Function returns true if a wake operation is scheduled, or false if the On 2015/04/24 22:54:48, mark a. foltz wrote: > s/Function// Done. https://codereview.chromium.org/1096313003/diff/40001/extensions/browser/even... extensions/browser/event_page_tracker.h:34: // extension is already active and does not require waking. On 2015/04/24 22:54:48, mark a. foltz wrote: > s/active/awake/ and omit the rest Done.
On 2015/04/27 19:45:38, Kevin M wrote: > > "And unit tests"? > > I spent a couple hours trying to get this to work, but the new functionality > uses on ExtensionHost objects, which can't be constructed in unit tests. > > From process_manager_unittest.cc: > // NOTE: This test and those that follow do not try to create ExtensionsHosts > // because ExtensionHost is tightly coupled to WebContents and can't be > // constructed in unit tests. WebContents actually can be constructed in unit tests, you just need to use a WebContentsTester I think. That might be tough if there's no examples to copy, though. Shame that the ProcessManager tests don't try. rdevlin.cronin@ might know more. > > > WRT performance, kalman@: is an O(n) iteration through the items of > ExtensionHostSet acceptable for performance, or could I move ExtensionHostSet to > a hash_map for quick O(1) access? Iteration is fine. There aren't going to be nearly enough ExtensionHosts for that to matter. Vector would probably end up being more efficient.
On 2015/04/27 20:24:46, kalman wrote: > On 2015/04/27 19:45:38, Kevin M wrote: > > > "And unit tests"? > > > > I spent a couple hours trying to get this to work, but the new functionality > > uses on ExtensionHost objects, which can't be constructed in unit tests. > > > > From process_manager_unittest.cc: > > // NOTE: This test and those that follow do not try to create > ExtensionsHosts > > // because ExtensionHost is tightly coupled to WebContents and can't be > > // constructed in unit tests. > > WebContents actually can be constructed in unit tests, you just need to use a > WebContentsTester I think. That might be tough if there's no examples to copy, > though. Shame that the ProcessManager tests don't try. rdevlin.cronin@ might > know more. Re WebContents in unit tests - check out TestWebContentsFactory (https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...). It's a little basic, since I wrote it for pretty specific uses, but it might suffice for you here.
lgtm % one comment. Regarding unit tests, if there is an existing solution that lets you mock the extension host properly, then I would like to see a unit test in this patch. If there's some refactoring required elsewhere that seems to be better done in a different patch. https://codereview.chromium.org/1096313003/diff/60001/extensions/browser/even... File extensions/browser/event_page_tracker.h (right): https://codereview.chromium.org/1096313003/diff/60001/extensions/browser/even... extensions/browser/event_page_tracker.h:28: // The callback will be passed true if the extension was Rewrap this to 80 cols.
Hey devlin/kalman, Thanks for the WebContents link. I think that thoroughly testing this change might require a significant time investment that exceeds the scope of this single CL. ExtensionHost is tightly coupled to its own instance of WebContents. It also depends on SiteInstance, which is a complex object which features many low-level details and lacks mock or test subclasses. To cleanly test it, I'd need to make changes fairly deeply in ProcessManager and its constituent components, which given the essential nature of these components, should be done carefully to avoid regressions. I'd need to do it in conjunction with a subject matter expert as this is a bit out of my immediate purview. ;) Alternatively I test it by inserting templated test objects into ProcessManager, but that's not ideal, either. WTYT? Are you comfortable with the scope this change to land it w/o tests for the time being? https://codereview.chromium.org/1096313003/diff/60001/extensions/browser/even... File extensions/browser/event_page_tracker.h (right): https://codereview.chromium.org/1096313003/diff/60001/extensions/browser/even... extensions/browser/event_page_tracker.h:28: // The callback will be passed true if the extension was On 2015/04/27 23:26:40, mark a. foltz wrote: > Rewrap this to 80 cols. Done.
https://codereview.chromium.org/1096313003/diff/80001/extensions/browser/even... File extensions/browser/event_page_tracker.h (right): https://codereview.chromium.org/1096313003/diff/80001/extensions/browser/even... extensions/browser/event_page_tracker.h:31: // or false if the event page was already awake. Add a comment to the effect of "|callback| will be run asynchronously if true, and never run if false". In light of that you may want to tweak the comment earlier "calls |callback| after it is reactivated." https://codereview.chromium.org/1096313003/diff/80001/extensions/browser/proc... File extensions/browser/process_manager.h (right): https://codereview.chromium.org/1096313003/diff/80001/extensions/browser/proc... extensions/browser/process_manager.h:90: virtual ExtensionHost* GetBackgroundHostForExtension( Doesn't need to be virtual anymore.
meant to say "lgtm and I'm fine with not adding tests"
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1096313003/#ps100001 (title: "Merge with master.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096313003/100001
The CQ bit was unchecked by marshallk@google.com
The CQ bit was checked by marshallk@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1096313003/#ps120001 (title: "Fix IWYU linter error while I'm at it.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096313003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/24b29b2f35f869b17579d6e95290482cec8ea4f1 Cr-Commit-Position: refs/heads/master@{#327412} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
