Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7672009:
Lazy creating of background pages --enable-lazy-background-pages) (Closed)
|
Created:
9 years, 4 months ago by Tessa MacDuff Modified:
9 years, 3 months ago CC:
chromium-reviews, Erik does not do reviews, mihaip+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionLazy creation of background pages with --enable-lazy-background-pages
BUG=81752
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98981
Patch Set 1 #
Total comments: 5
Patch Set 2 : Move implementation to ExtensionEventRouter and store pending events in a queue #
Total comments: 20
Patch Set 3 : used linked_ptr and Tasks and no more 'Maybe' functions #Patch Set 4 : fix broken test #
Total comments: 13
Patch Set 5 : address comments #Patch Set 6 : use ExtensionEvent for args to DispatchEventImpl #
Total comments: 5
Patch Set 7 : Switch from Task to ExtensionEvent #
Total comments: 2
Patch Set 8 : create ExtensionEvent on the heap #Patch Set 9 : fix memory leak #
Messages
Total messages: 32 (0 generated)
I just got this working so I wanted to send it to you for quick feedback. The important changes are in browser_action_toolbar_gtk.cc. I'm not sure this is the best place for all the logic, but it felt like the first time we know that the browser action was clicked. I think it probably makes more sense to move it down to ExtensionEventRouter. The latency between click and the handling of the click is noticeable and I would like to reduce it. Let me know if you have ideas.
This will work, but if we follow this pattern, it will lead to a lot of extra code in each API to handle --lazy-background-pages. I was imagining the changes being lower down in the stack, in the place where we dispatch events. Basically, each extension would have a queue of events that need to be delivered to it. When an API wants to dispatch an event it just do so, exactly as today. In the event dispatch code, we first check if the background page has been created (if the extension has one and we are in lazy mode). If it hasn't we create it and queue the event. Once the background page comes up, we dispatch all the queued events to it. This way all extension features can remain mostly ignorant of --lazy-background-pages mode. We can talk more about the specifics in person tomorrow. http://codereview.chromium.org/7672009/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_process_manager.cc (right): http://codereview.chromium.org/7672009/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_process_manager.cc:64: !CommandLine::ForCurrentProcess()->HasSwitch( If you put this check in CreateBackgroundHosts() then you can leave this name the way it was, and CreateBackgroundHost() will be more flexible. http://codereview.chromium.org/7672009/diff/1/chrome/common/chrome_switches.cc File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/7672009/diff/1/chrome/common/chrome_switches.c... chrome/common/chrome_switches.cc:454: const char kEnableLazyBackgroundPages[] = "lazy-background-pages"; The flag name should follow the convention of the others, so --enable-lazy-background-pages.
I agree, that makes a lot more sense. I'll take a whack at it today.
http://codereview.chromium.org/7672009/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_process_manager.cc (right): http://codereview.chromium.org/7672009/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_process_manager.cc:64: !CommandLine::ForCurrentProcess()->HasSwitch( On 2011/08/17 04:24:15, Aaron Boodman wrote: > If you put this check in CreateBackgroundHosts() then you can leave this name > the way it was, and CreateBackgroundHost() will be more flexible. But then I'd only be handling the background pages that get created after a NOTIFICATIONS_EXTENSIONS_READY (chrome startup?). What about NOTIFICATIONS_EXTENSION_LOADED (reloading an extension)? See ExtensionProcessManager::Observe() on line 290 below.
http://codereview.chromium.org/7672009/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_process_manager.cc (right): http://codereview.chromium.org/7672009/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_process_manager.cc:64: !CommandLine::ForCurrentProcess()->HasSwitch( On 2011/08/17 17:51:24, Tessa MacDuff wrote: > On 2011/08/17 04:24:15, Aaron Boodman wrote: > > If you put this check in CreateBackgroundHosts() then you can leave this name > > the way it was, and CreateBackgroundHost() will be more flexible. > > But then I'd only be handling the background pages that get created after a > NOTIFICATIONS_EXTENSIONS_READY (chrome startup?). What about > NOTIFICATIONS_EXTENSION_LOADED (reloading an extension)? See > ExtensionProcessManager::Observe() on line 290 below. Oh, good point. Nevermind.
Matt and I had a perhaps useful idea: instead of having the queue contain 'events to be dispatched', it could be slightly more general: have the queue just contain Task objects (eg base/task.h). That way, you can always say "ensure the background page is loaded, then do <thing>.". Where <thing> is any C++ function, including event dispatch.
I moved all the background page creation stuff to ExtensionEventRouter and implemented a simple pending elements queue. If this looks more-or-less correct I'll adress those TODOs and start thinking about tests. I'll take a look as base/tasks.h but my instinct is that it would be overkill at this stage (but possibly useful when I'm handling more types of events, esp initialization). http://codereview.chromium.org/7672009/diff/1/chrome/common/chrome_switches.cc File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/7672009/diff/1/chrome/common/chrome_switches.c... chrome/common/chrome_switches.cc:454: const char kEnableLazyBackgroundPages[] = "lazy-background-pages"; On 2011/08/17 04:24:15, Aaron Boodman wrote: > The flag name should follow the convention of the others, so > --enable-lazy-background-pages. Done.
http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_event_router.cc (right): http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.cc:190: void ExtensionEventRouter::MaybeDispatchEventImpl( Another 'maybe' function. You could eliminate this one by putting the code directly in DispatchEventImpl. The logic would basically be: <snip> if (CanDispatchEventNow()) { StartBackgroundPage(); Queue(); return; } </snip> You can factor the checking of whether the background page needs to be created into CanDispatchEventNow() without adding too much to DispatchEventImpl(). http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.cc:204: // Create all background pages? I think creating all background pages is the right answer for now. It might need a little refactoring to handle cleanly. You could do that separately if you want. http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.cc:285: pending_events_[extension_id].push_back(event); I'm fine with this for now, but just so you know the task.h stuff is actually very easy to use and would eliminate some of this manual labor. It would allow you to do basically: pending_events_[extension_id].push_back( factory_.NewRunnableMethod( this, &ExtensionEventRouter::DispatchEventImpl, extensin_id, event_name, event_args, ... etc .. ); and then to dispatch them, just loop through and call Run() on each one. No need for a separate struct. New RunnableMethod() returns a Task that essentially replaces your struct. Basically ScopedRunnableMethodFactory.NewRunnableMethod() allows you to say 'run this method with these arguments "later"'. We use them in lots of places in the extension system if you want to have a look. http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_event_router.h (right): http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.h:143: typedef std::vector<PendingEvent> PendingEventsQueue; You can use std::queue. http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.h:144: std::map<std::string, PendingEventsQueue> pending_events_; Nit: this is a bit inefficient because as these maps and vectors resize there will be a lot of copying. You can use base/linked_ptr.h, and make it like: std::vector<linked_ptr<PendingEvent> > std::map<std::string, linked_ptr<PendingEventsQueue> > The linked_ptr will manage the lifetime automatically. There's other examples in Chrome of this usage you can look for examples. http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_process_manager.cc (right): http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_process_manager.cc:60: static void MaybeCreateBackgroundHost( Sorry to nitpick the but "Maybe" functions always bug me. Maybe based on what? The name doesn't describe what the function does very well. How about names like: CreateBackgroundHosts -> CreateBackgroundHostsForProfileStartup CreateBackgroundHost -> CreateBackgroundHostForExtensionLoad I realize they are a bit long, but at least they try to describe the behavior. And then you could just have a separate CreateBackgroundHost that does it unconditionally.
http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_event_router.cc (right): http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.cc:289: pending_events_[extension_id].clear(); Little nit: If these one-liner methods aren't called more than one place in the code, I'd probably just put the code at the call site. It's not saving any complexity to pull out one line. http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.cc:298: // being re-enqueued if the Background Page still isn't all set. Oh, I see. I don't think it's a big deal if they get re-enqueued. It simplifies the code and it's not like we are going to loop. Once the extension loads, if EPM doesn't have it for some reason, we have a serious implementation bug and lots of things are broken.
I agree with all your comments and will address them. Wanted to discuss a few, see below. http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_event_router.cc (right): http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.cc:298: // being re-enqueued if the Background Page still isn't all set. On 2011/08/18 19:29:06, Aaron Boodman wrote: > Oh, I see. I don't think it's a big deal if they get re-enqueued. It simplifies > the code and it's not like we are going to loop. Once the extension loads, if > EPM doesn't have it for some reason, we have a serious implementation bug and > lots of things are broken. I was worried about what would happen if we were adding items to pending_events_ as we were iterating thought it. Seems like it could infinite loop. If you'd prefer, I can pass an extra arg that indicates this event was waiting on the background page so we can failure if it's still not there. Although this case might see unlikely know, it could happen when i start trying to shutdown background pages (i.e. if I shut them down too early). http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_event_router.h (right): http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.h:143: typedef std::vector<PendingEvent> PendingEventsQueue; On 2011/08/18 19:25:55, Aaron Boodman wrote: > You can use std::queue. Yeah, I just thought it was overkill since I never actually pop things off the queue (I just iterate through it all at once and then delete the whole thing). I'm happy to update the rest of the code so that I don't act like it's a queue (i.e. AppendEvent() instead of EnqueueEvent()).
http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_event_router.cc (right): http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.cc:298: // being re-enqueued if the Background Page still isn't all set. On 2011/08/18 20:49:39, Tessa MacDuff wrote: > On 2011/08/18 19:29:06, Aaron Boodman wrote: > > Oh, I see. I don't think it's a big deal if they get re-enqueued. It > simplifies > > the code and it's not like we are going to loop. Once the extension loads, if > > EPM doesn't have it for some reason, we have a serious implementation bug and > > lots of things are broken. > > I was worried about what would happen if we were adding items to pending_events_ > as we were iterating thought it. Seems like it could infinite loop. If you'd > prefer, I can pass an extra arg that indicates this event was waiting on the > background page so we can failure if it's still not there. ok (to extra arg). http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_event_router.h (right): http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.h:143: typedef std::vector<PendingEvent> PendingEventsQueue; On 2011/08/18 20:49:39, Tessa MacDuff wrote: > On 2011/08/18 19:25:55, Aaron Boodman wrote: > > You can use std::queue. > > Yeah, I just thought it was overkill since I never actually pop things off the > queue (I just iterate through it all at once and then delete the whole thing). > I'm happy to update the rest of the code so that I don't act like it's a queue > (i.e. AppendEvent() instead of EnqueueEvent()). ok.
There are still a couple TODOs (including handing broadcast Events) and some try bots failing. But I addressed most of your comments. See below. http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_event_router.cc (right): http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.cc:190: void ExtensionEventRouter::MaybeDispatchEventImpl( On 2011/08/18 19:25:55, Aaron Boodman wrote: > Another 'maybe' function. You could eliminate this one by putting the code > directly in DispatchEventImpl. The logic would basically be: > > <snip> > if (CanDispatchEventNow()) { > StartBackgroundPage(); > Queue(); > return; > } > </snip> > > You can factor the checking of whether the background page needs to be created > into CanDispatchEventNow() without adding too much to DispatchEventImpl(). Done. http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.cc:204: // Create all background pages? On 2011/08/18 19:25:55, Aaron Boodman wrote: > I think creating all background pages is the right answer for now. It might need > a little refactoring to handle cleanly. You could do that separately if you > want. I think I'll do this in a separate CL as since it will take a bit of refactoring and it will be much more interesting when I can shut background pages down. http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.cc:285: pending_events_[extension_id].push_back(event); On 2011/08/18 19:25:55, Aaron Boodman wrote: > I'm fine with this for now, but just so you know the task.h stuff is actually > very easy to use and would eliminate some of this manual labor. > > It would allow you to do basically: > > pending_events_[extension_id].push_back( > factory_.NewRunnableMethod( > this, > &ExtensionEventRouter::DispatchEventImpl, > extensin_id, > event_name, > event_args, > ... etc .. > ); > > and then to dispatch them, just loop through and call Run() on each one. No need > for a separate struct. New RunnableMethod() returns a Task that essentially > replaces your struct. > > Basically ScopedRunnableMethodFactory.NewRunnableMethod() allows you to say 'run > this method with these arguments "later"'. > > We use them in lots of places in the extension system if you want to have a > look. Done. http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.cc:289: pending_events_[extension_id].clear(); On 2011/08/18 19:29:06, Aaron Boodman wrote: > Little nit: If these one-liner methods aren't called more than one place in the > code, I'd probably just put the code at the call site. It's not saving any > complexity to pull out one line. Done. I had to add an additional NewRunnableMethod template to task.h to accomodate 7 args. Let me know if you'd rather I put it in a separate CL or if you'd like me to create a single DispatchEventParams object. http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.cc:298: // being re-enqueued if the Background Page still isn't all set. On 2011/08/18 21:19:23, Aaron Boodman wrote: > On 2011/08/18 20:49:39, Tessa MacDuff wrote: > > On 2011/08/18 19:29:06, Aaron Boodman wrote: > > > Oh, I see. I don't think it's a big deal if they get re-enqueued. It > > simplifies > > > the code and it's not like we are going to loop. Once the extension loads, > if > > > EPM doesn't have it for some reason, we have a serious implementation bug > and > > > lots of things are broken. > > > > I was worried about what would happen if we were adding items to > pending_events_ > > as we were iterating thought it. Seems like it could infinite loop. If > you'd > > prefer, I can pass an extra arg that indicates this event was waiting on the > > background page so we can failure if it's still not there. > > ok (to extra arg). Done. http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_event_router.h (right): http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.h:143: typedef std::vector<PendingEvent> PendingEventsQueue; On 2011/08/18 21:19:23, Aaron Boodman wrote: > On 2011/08/18 20:49:39, Tessa MacDuff wrote: > > On 2011/08/18 19:25:55, Aaron Boodman wrote: > > > You can use std::queue. > > > > Yeah, I just thought it was overkill since I never actually pop things off the > > queue (I just iterate through it all at once and then delete the whole thing). > > > I'm happy to update the rest of the code so that I don't act like it's a queue > > (i.e. AppendEvent() instead of EnqueueEvent()). > > ok. Done. http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_event_router.h:144: std::map<std::string, PendingEventsQueue> pending_events_; On 2011/08/18 19:25:55, Aaron Boodman wrote: > Nit: this is a bit inefficient because as these maps and vectors resize there > will be a lot of copying. > > You can use base/linked_ptr.h, and make it like: > > std::vector<linked_ptr<PendingEvent> > > std::map<std::string, linked_ptr<PendingEventsQueue> > > > The linked_ptr will manage the lifetime automatically. There's other examples in > Chrome of this usage you can look for examples. Done. http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_process_manager.cc (right): http://codereview.chromium.org/7672009/diff/5002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_process_manager.cc:60: static void MaybeCreateBackgroundHost( On 2011/08/18 19:25:55, Aaron Boodman wrote: > Sorry to nitpick the but "Maybe" functions always bug me. Maybe based on what? > The name doesn't describe what the function does very well. > > How about names like: > > CreateBackgroundHosts -> CreateBackgroundHostsForProfileStartup > CreateBackgroundHost -> CreateBackgroundHostForExtensionLoad > > I realize they are a bit long, but at least they try to describe the behavior. > And then you could just have a separate CreateBackgroundHost that does it > unconditionally. Done. I also moved the switch check up out of these functions to Observe(). Let me know what you think.
Cool almost there. http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... File chrome/browser/extensions/extension_event_router.cc (right): http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.cc:82: task_factory_.RevokeAll(); Not necessary -- this happens automatically when task_factory_ is deleted. http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.cc:182: switches::kEnableLazyBackgroundPages)) { Invert test and return early to reduce indenting. http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.cc:194: DCHECK(!was_pending); Can you move this to the call site? if (!CanDispatchEventNow(extension_id)) { CHECK(!was_pending); // shouldn't happen twice! AppendEvent(...); return; } Then you won't have to mess with task.h. Also, I usually prefer CHECK to DCHECK since we will get error reports from the field rather than complaints of silent failure. http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.cc:274: PendingTasksPerExtMap::const_iterator it = pending_tasks_.find(extension_id); You do the find(), then always end up trying to insert anyway, so I don't think you're saving anything. Anyway, better to be clear since this isn't performance critical. The more common pattern is: Thinger* thinger = NULL; ThingerContainer::Iterator it = pending_tasks.find(id); if (it == pending_tasks.end()) { thinger = new Thinger(); pending_tasks_[id] = thinger; } else { thinger = *(it->second); } http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.cc:298: // The second element is the list of tasks (the first is the key/ext id). Nit: You don't have to explain this since it is clear from the type of the container. http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... File chrome/browser/extensions/extension_process_manager.cc (right): http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... chrome/browser/extensions/extension_process_manager.cc:293: if (CommandLine::ForCurrentProcess()->HasSwitch( As discussed in chat, try moving these to CreateBackgroundHostsForExtensionLoad().
http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... File chrome/browser/extensions/extension_event_router.cc (right): http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.cc:82: task_factory_.RevokeAll(); On 2011/08/23 22:41:16, Aaron Boodman wrote: > Not necessary -- this happens automatically when task_factory_ is deleted. Done. http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.cc:182: switches::kEnableLazyBackgroundPages)) { On 2011/08/23 22:41:16, Aaron Boodman wrote: > Invert test and return early to reduce indenting. Done. http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.cc:194: DCHECK(!was_pending); On 2011/08/23 22:41:16, Aaron Boodman wrote: > Can you move this to the call site? > > if (!CanDispatchEventNow(extension_id)) { > CHECK(!was_pending); // shouldn't happen twice! > AppendEvent(...); > return; > } > > Then you won't have to mess with task.h. > > Also, I usually prefer CHECK to DCHECK since we will get error reports from the > field rather than complaints of silent failure. Done. But why does this mean I don't have to mess with task.h? DispatchEventImpl() still has 7 args. http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.cc:274: PendingTasksPerExtMap::const_iterator it = pending_tasks_.find(extension_id); On 2011/08/23 22:41:16, Aaron Boodman wrote: > You do the find(), then always end up trying to insert anyway, so I don't think > you're saving anything. Anyway, better to be clear since this isn't performance > critical. > > The more common pattern is: > > Thinger* thinger = NULL; > ThingerContainer::Iterator it = pending_tasks.find(id); > if (it == pending_tasks.end()) { > thinger = new Thinger(); > pending_tasks_[id] = thinger; > } else { > thinger = *(it->second); > } Oops I just forgot to remove that line. 'it' is never used... Done. http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.cc:298: // The second element is the list of tasks (the first is the key/ext id). On 2011/08/23 22:41:16, Aaron Boodman wrote: > Nit: You don't have to explain this since it is clear from the type of the > container. Done. http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... File chrome/browser/extensions/extension_process_manager.cc (right): http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... chrome/browser/extensions/extension_process_manager.cc:293: if (CommandLine::ForCurrentProcess()->HasSwitch( On 2011/08/23 22:41:16, Aaron Boodman wrote: > As discussed in chat, try moving these to > CreateBackgroundHostsForExtensionLoad(). Done.
LGTM, you'll need to get approval from an owner of base/. http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... File chrome/browser/extensions/extension_event_router.cc (right): http://codereview.chromium.org/7672009/diff/11009/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.cc:194: DCHECK(!was_pending); On 2011/08/24 00:34:23, Tessa MacDuff wrote: > On 2011/08/23 22:41:16, Aaron Boodman wrote: > > Can you move this to the call site? > > > > if (!CanDispatchEventNow(extension_id)) { > > CHECK(!was_pending); // shouldn't happen twice! > > AppendEvent(...); > > return; > > } > > > > Then you won't have to mess with task.h. > > > > Also, I usually prefer CHECK to DCHECK since we will get error reports from > the > > field rather than complaints of silent failure. > > Done. > > But why does this mean I don't have to mess with task.h? DispatchEventImpl() > still has 7 args. Whoops, you're right. I suspect the owner of base/task.h might ask to use a structure here instead of a seven-param method. But I actually prefer the seven-param function I think so here's hoping...
Hi Darin, This change adds to task.h a version of NewRunnableMethod() that takes method and 7 args. Let me know what you think. (I can also add on that takes 6 args while I'm at it.)
+brettw, evan (darin is pretty busy) OWNERS: I realize that large numbers of params to a function is a bit of a code smell, so you might push back on this. In this case, it seems more elegant to me to have the large number of params than to have a special-purpose struct.
I wonder if rather than a special-purpose struct, a struct named ExtensionEvent would be an appropriate argument for a function like AppendEvent(). (The Task you end up using would be a combination of an ExtensionEvent and potentially other stuff that isn't necessarily part of this event, like this bool attribute at the end. But surely less than five args)
base LGTM
Thanks for the speedy reply! Unless you feel strongly Evan, I'll commit this using Brett's approval.
On 2011/08/24 21:36:59, brettw wrote: > base LGTM I only looked at the one file in base, so don't treat my review as thinking it's the best way to do what you want. I just noted that we already had NewRunnableMethods with that many args, so it seems OK to also have the cancelable doohicky also able to create them.
On 2011/08/24 21:31:32, Evan Martin wrote: > I wonder if rather than a special-purpose struct, a struct named ExtensionEvent > would be an appropriate argument for a function like AppendEvent(). That makes sense... something like: ExtensionEvent std::string extension_id std::string event_name std::string event_args GURL event_url Profile* restrict_to_profile std::string cross_incognito_args DispatchEvent(const ExtensionEvent&) DispatchEventImpl(const ExtensionEvent&, bool was_pending) etc...
Since the args differ a bit for all the public DispatchEvent...() functions I'd like to leave them as is (also then callers won't have to know about this new struct). But for all the private/protected functions, sgtm.
On 2011/08/24 22:34:06, Tessa MacDuff wrote: > Since the args differ a bit for all the public DispatchEvent...() functions I'd > like to leave them as is (also then callers won't have to know about this new > struct). But for all the private/protected functions, sgtm. That seems sensible.
Ok, I changed DispatchEventImpl and APpendEvent to use a new ExtensionEvent struct. And I dropped the change to task.h. Let me know what you think.
drive-by great start! http://codereview.chromium.org/7672009/diff/25001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_event_router.cc (right): http://codereview.chromium.org/7672009/diff/25001/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.cc:237: CHECK(!was_pending); Since it can actually happen in a legit case, don't CHECK - just early return. http://codereview.chromium.org/7672009/diff/25001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_event_router.h (right): http://codereview.chromium.org/7672009/diff/25001/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.h:122: ScopedRunnableMethodFactory<ExtensionEventRouter> task_factory_; You don't need a task factory in this case. The purpose of the task factory is to delete all tasks when it goes out of scope (i.e. the ExtensionEventRouter is deleted). But since your tasks are owned and run manually by the event router itself, there is no danger of them running after the event router is deleted. http://codereview.chromium.org/7672009/diff/25001/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.h:122: ScopedRunnableMethodFactory<ExtensionEventRouter> task_factory_; And actually, since you're only using the Tasks for a single purpose, it might be cleaner just to keep a list of pending ExtensionEvents instead of generic Tasks. I know Aaron suggested using Tasks so it can be a generic "thing to do when bg page is ready", but if we want to do that, I think the pending task list belongs on the ExtensionProcessManager instead. Then different consumers can schedule different tasks. The event router will only ever schedule pending events. (Note if you go that route, you will need the task_factory again, since the EPM will own the tasks and could outlive the event router)
I'll ping when the changes are ready, but I'll give Aaron a chance to object. http://codereview.chromium.org/7672009/diff/25001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_event_router.cc (right): http://codereview.chromium.org/7672009/diff/25001/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.cc:237: CHECK(!was_pending); On 2011/08/26 00:13:00, Matt Perry wrote: > Since it can actually happen in a legit case, don't CHECK - just early return. Actually it should never happen. I feel pretty confident that it will never happen unless i make a mistake implementing the shutting down of background pages. I'll clarify the comment. http://codereview.chromium.org/7672009/diff/25001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_event_router.h (right): http://codereview.chromium.org/7672009/diff/25001/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.h:122: ScopedRunnableMethodFactory<ExtensionEventRouter> task_factory_; On 2011/08/26 00:13:00, Matt Perry wrote: > And actually, since you're only using the Tasks for a single purpose, it might > be cleaner just to keep a list of pending ExtensionEvents instead of generic > Tasks. > > I know Aaron suggested using Tasks so it can be a generic "thing to do when bg > page is ready", but if we want to do that, I think the pending task list belongs > on the ExtensionProcessManager instead. Then different consumers can schedule > different tasks. The event router will only ever schedule pending events. > > (Note if you go that route, you will need the task_factory again, since the EPM > will own the tasks and could outlive the event router) Thanks for explaining task factories. I think adding a task list to ExtensionProcessManager would be borderline over-engineering since we don't actually have a use case yet. I'll just change this to use ExtensionEvents and leave a comment about perhaps doing something more general with a task list in ExtensionProcessManager. Unless Aaron objects?
On 2011/08/26 01:12:53, Tessa MacDuff wrote: > I think adding a task list to ExtensionProcessManager would be borderline > over-engineering since we don't actually have a use case yet. I'll just change > this to use ExtensionEvents and leave a comment about perhaps doing something > more general with a task list in ExtensionProcessManager. Unless Aaron objects? :: sigh :: so sorry. Too many cooks, etc. That sounds fine, just check it in already.
I made the change Matt suggested but it's not working. A DCHECK has started failing and I've spent hours trying to figure it out with no success. Let me know if you have any ideas otherwise I'll revert back to the Task version and commit that. http://codereview.chromium.org/7672009/diff/29001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_event_router.cc (right): http://codereview.chromium.org/7672009/diff/29001/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.cc:231: profile_->IsSameProfile(event.restrict_to_profile)); When I dispatch the pending event this check fails because the restrict_to_profiel profile is not the same as it was when I put the event on the pending_events list. Any idea how/why it would change out form under me?
http://codereview.chromium.org/7672009/diff/29001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_event_router.h (right): http://codereview.chromium.org/7672009/diff/29001/chrome/browser/extensions/e... chrome/browser/extensions/extension_event_router.h:128: typedef std::vector<linked_ptr<const ExtensionEvent> > PendingEventsList; IfI make this a std::vector<ExtensionEvent> the error disappears. I assume that's because it makes a copy of the ExtensionEvent but still doesn't explain to me why the restrict_to_profile pointer no longer changes...
Ok, I think I fixed it. (Wow, lunch helps me think!) The ExtensionEvent was being created in the scope of the DispatchEvent...() functions so it was being deleted when that function goes out of scope. I corrected that by creating the ExtensionEvent with new so that it lives on past the first function call.
Change committed as 98981 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
