Keep lazy background page alive while there are pending network requests or

Keep lazy background page alive while there are pending network requests or
visible extension views.

BUG=81752
TEST=no

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=125061

[Yoyo Zhou, 2012-03-02 03:10:45]

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_event_router.cc (right):

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_event_router.cc:365: // Balanced in
DispatchPendingEvents, after the page has loaded.
Is it possible for there to be a race between loading the lazy background page
for some other reason and receiving an event?

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_process_manager.cc:311:
render_view_host->site_instance()->GetSite().host());
Maybe a TODO: it seems like this could be a helper function to get an Extension
from a RenderViewHost. I think I've seen other places it could be used.

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_process_manager.cc:469: case
content::NOTIFICATION_WEB_CONTENTS_CONNECTED: {
Naively, it seems like this should be balanced with WEB_CONTENTS_DISCONNECTED.

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_process_manager.h (right):

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_process_manager.h:149: typedef
std::map<RenderViewHost*, content::ViewType> ExtensionRenderViews;
I think this typename should have "Map".

http://codereview.chromium.org/9562017/diff/1/chrome/browser/net/chrome_netwo...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/9562017/diff/1/chrome/browser/net/chrome_netwo...
chrome/browser/net/chrome_network_delegate.cc:61: Profile* profile =
reinterpret_cast<Profile*>(profile_id);
Might be a good idea to DCHECK we're on the UI thread.

http://codereview.chromium.org/9562017/diff/1/chrome/test/data/extensions/api...
File
chrome/test/data/extensions/api_test/lazy_background_page/wait_for_request/background.js
(right):

http://codereview.chromium.org/9562017/diff/1/chrome/test/data/extensions/api...
chrome/test/data/extensions/api_test/lazy_background_page/wait_for_request/background.js:22:
// Start a request that will "never" finish (at least, not for 1000
I'm mildly surprised that the test server doesn't have a never-completing
option, but I didn't see one either.

[Matt Perry, 2012-03-02 20:25:20]

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_event_router.cc (right):

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_event_router.cc:365: // Balanced in
DispatchPendingEvents, after the page has loaded.
On 2012/03/02 03:10:46, Yoyo Zhou wrote:
> Is it possible for there to be a race between loading the lazy background page
> for some other reason and receiving an event?

The only thing that causes a lazy bg page to load is receiving an event.

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_process_manager.cc:311:
render_view_host->site_instance()->GetSite().host());
On 2012/03/02 03:10:46, Yoyo Zhou wrote:
> Maybe a TODO: it seems like this could be a helper function to get an
Extension
> from a RenderViewHost. I think I've seen other places it could be used.

Added a local helper.

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_process_manager.cc:469: case
content::NOTIFICATION_WEB_CONTENTS_CONNECTED: {
On 2012/03/02 03:10:46, Yoyo Zhou wrote:
> Naively, it seems like this should be balanced with WEB_CONTENTS_DISCONNECTED.

No need - UnregisterRenderViewHost cleans up our RenderViewHost list. All this
notification does is give us a convenient time after RegisterRenderViewHost
where the view_type is available (view_type never changes).

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_process_manager.h (right):

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_process_manager.h:149: typedef
std::map<RenderViewHost*, content::ViewType> ExtensionRenderViews;
On 2012/03/02 03:10:46, Yoyo Zhou wrote:
> I think this typename should have "Map".

I disagree. I don't like putting container types in the type names, because if
you decide to change the container type, you have to rename the typedef (like I
had to do with this CL).

http://codereview.chromium.org/9562017/diff/1/chrome/browser/net/chrome_netwo...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/9562017/diff/1/chrome/browser/net/chrome_netwo...
chrome/browser/net/chrome_network_delegate.cc:61: Profile* profile =
reinterpret_cast<Profile*>(profile_id);
On 2012/03/02 03:10:46, Yoyo Zhou wrote:
> Might be a good idea to DCHECK we're on the UI thread.

Done.

[Yoyo Zhou, 2012-03-02 20:31:58]

LGTM

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_process_manager.h (right):

http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_process_manager.h:149: typedef
std::map<RenderViewHost*, content::ViewType> ExtensionRenderViews;
On 2012/03/02 20:25:21, Matt Perry wrote:
> On 2012/03/02 03:10:46, Yoyo Zhou wrote:
> > I think this typename should have "Map".
> 
> I disagree. I don't like putting container types in the type names, because if
> you decide to change the container type, you have to rename the typedef (like
I
> had to do with this CL).

I don't think that's a significant cost. As someone reading the code, I
shouldn't have to find the typedef to understand what kind of type it really is.

Although no type name is better than a wrong one (I think ExtensionSet had
something that claimed to be a set but was actually a map, e.g.).

[Matt Perry, 2012-03-02 23:37:59]

On 2012/03/02 20:31:58, Yoyo Zhou wrote:
> LGTM
> 
>
http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
> File chrome/browser/extensions/extension_process_manager.h (right):
> 
>
http://codereview.chromium.org/9562017/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_process_manager.h:149: typedef
> std::map<RenderViewHost*, content::ViewType> ExtensionRenderViews;
> On 2012/03/02 20:25:21, Matt Perry wrote:
> > On 2012/03/02 03:10:46, Yoyo Zhou wrote:
> > > I think this typename should have "Map".
> > 
> > I disagree. I don't like putting container types in the type names, because
if
> > you decide to change the container type, you have to rename the typedef
(like
> I
> > had to do with this CL).
> 
> I don't think that's a significant cost. As someone reading the code, I
> shouldn't have to find the typedef to understand what kind of type it really
is.

In general, I don't think need to know what kind of type it is. Besides, when
reading the code, you're going to be seeing the variable name, not the type
name.

[Yoyo Zhou, 2012-03-02 23:40:11]

> Besides,
> when
> reading the code, you're going to be seeing the variable name, not the type
> name.

Ok, that's a good point.

[Matt Perry, 2012-03-06 00:34:08]

Will, could I get an OWNERS review for chrome/browser/net/*

[willchan no longer on Chromium, 2012-03-06 00:54:33]

lgtm

http://codereview.chromium.org/9562017/diff/6001/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/9562017/diff/6001/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:68: if (render_view_host) {
Took me a second to realize that not all URLRequests on the URLRequestContext
are associated with a renderer. A comment to explain this would be nice.

[Matt Perry, 2012-03-06 01:12:02]

http://codereview.chromium.org/9562017/diff/6001/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/9562017/diff/6001/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:68: if (render_view_host) {
On 2012/03/06 00:54:34, willchan wrote:
> Took me a second to realize that not all URLRequests on the URLRequestContext
> are associated with a renderer. A comment to explain this would be nice.

Done.