Add a status message "Waiting for extension Foo..." when there's a request

Add a status message "Waiting for extension Foo..." when there's a request
blocked on an extension with a webrequest API event handler.

BUG=82618
TEST=no


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96226

[Matt Perry, 2011-07-29 00:36:54]

Dominic: please review extensions/*
Will: please review everything else (unless you're not the right person to
review)
Darin: please take a look at my rietveld comment in resource_dispatcher_host.cc.
I changed the logic for when we update load states for the status message. It
looked wrong to me, but it was obviously intentional, so I wanted to double
check if I was missing something.

http://codereview.chromium.org/7523042/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (left):

http://codereview.chromium.org/7523042/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:1859: if
(info->last_load_state() != load_state || update_upload_progress) {
I believe this logic was wrong. Consider this scenario: a page has 2 pending
requests associated with it (say, two images to load).
1. Both requests start off in the "resolving host" state.
2. RequestA resolves, loads, and completes. Maybe it was served from a fast
host.
3. RequestB is still pending, in the "resolving host" state.

What I'd expect: the status should go back to showing "Resolving host", since
we're now waiting on RequestB.
What happens: Since RequestB has not changed its load_state, we don't send a
LoadStateChanged notification. The last notification we sent was for RequestA
completing, so the tab thinks it's state is "done loading".

4. Now RequestB advances to the "waiting for response" state, and sits there for
a while.

What happens: Now last_load_state != load_state for RequestB, so we update the
load state. The tab now shows the status "waiting for response".

Long story short, it doesn't seem useful to keep track of a request's last load
state. Instead, each update should just report the "most interesting" load state
of all currently pending requests.

[battre, 2011-07-29 03:35:34]

-battre
+jochen
(vacation)

[jochen (gone - plz use gerrit), 2011-07-29 07:17:37]

extension/* LGTM

why no test? make an extension that blocks indefinitely, check that the tab
contents load state eventually calls out that extension and then just kill the
extension renderer?

[willchan no longer on Chromium, 2011-07-29 07:34:37]

http://codereview.chromium.org/7523042/diff/4001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/7523042/diff/4001/net/url_request/url_request....
net/url_request/url_request.h:432: // Returns the current load state for the
request. |param| is an optional
I think you're working forward from the assumption that NetworkDelegate implies
extensions, and therefore when we're blocked on the delegate, we must always
want a parameter to attach to the LoadState. I'd argue this is not necessarily
true. There are possible future consumers who won't want this.

Therefore, I think that the network stack client should be storing this. Whoever
is calling GetLoadState() (RDH, right?) should, if the value indicates we're
blocked on the delegate, query the appropriate object (the delegate probably) to
figure out the reason for the blockage, using the URLRequest* as a lookup key.

I guess then we'd need to add a new method to the NetworkDelegate to allow
querying why a URLRequest* is blocked. I'm not sure I really like adding it to
NetworkDelegate since not all consumers need it and it burdens them with an
implementation, but I guess the alternative is unwieldy (having content/
subclass NetworkDelegate to introduce a new interface with a single added
method. I think this is the only alternative since content/ can't directly
depend on ChromeNetworkDelegate/ExtensionWebRequestAPI since those are chrome/
classes). So I guess I'm fine with adding an interface to NetworkDelegate for
this, with a comment indicating we should consider moving it out in the future.

http://codereview.chromium.org/7523042/diff/4001/net/url_request/url_request....
net/url_request/url_request.h:611: // Called when the delegate blocks or
unblocks this request when intercepting
These should be private. I think you made them public for URLRequestHttpJob to
call. I'm trying to tighten the interfaces. Any special interface needed to
access details in URLRequest from a job should be private in URLRequest, and be
protected in URLRequestJob. So URLRequestHttpJob should access these member
functions via its parent's protected member function.

[Matt Perry, 2011-07-29 21:30:48]

http://codereview.chromium.org/7523042/diff/4001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/7523042/diff/4001/net/url_request/url_request....
net/url_request/url_request.h:432: // Returns the current load state for the
request. |param| is an optional
On 2011/07/29 07:34:38, willchan wrote:
> I think you're working forward from the assumption that NetworkDelegate
implies
> extensions, and therefore when we're blocked on the delegate, we must always
> want a parameter to attach to the LoadState.

Agreed, I took care to avoid this assumption. That's why I made the param
optional. Extensions will use it, but other delegate consumers might not.

> I guess then we'd need to add a new method to the NetworkDelegate to allow
> querying why a URLRequest* is blocked.

Doesn't this suffer from the same problem you point out above? :) Namely, the
assumption that "blocked on NetworkDelegate" implies "there's some string
explaining why".

[Matt Perry, 2011-08-01 22:45:06]

-willchan
+eroman

Eric, can you take over this review for Will? He's out of town this week.

[Matt Perry, 2011-08-03 23:31:52]

ping

[eroman, 2011-08-03 23:43:55]

(hadn't noticed reviewer changed to me).

I am leaving work soon, so I may not publish comments until tomorrow.

[eroman, 2011-08-05 01:54:22]

lgtm on net/*

http://codereview.chromium.org/7523042/diff/4001/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/7523042/diff/4001/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:1070: load_state_.state =
net::LOAD_STATE_IDLE;
nit: how about load_state_ = LoadStateWithParams(IDLE, "");

http://codereview.chromium.org/7523042/diff/4001/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/7523042/diff/4001/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.h:185: const string16&
load_state_host() const { return load_state_host_; }
I wander if this can be worked into LoadStateWithParams now.

http://codereview.chromium.org/7523042/diff/4001/net/base/load_states.h
File net/base/load_states.h (right):

http://codereview.chromium.org/7523042/diff/4001/net/base/load_states.h#newco...
net/base/load_states.h:86: std::string param;
Can you make this a string16? Seems there are already conversion from UTF16 -->
UTF8 going on, since this is intended for display seems simpler to just keep it
wide.

http://codereview.chromium.org/7523042/diff/4001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/7523042/diff/4001/net/url_request/url_request....
net/url_request/url_request.h:611: // Called when the delegate blocks or
unblocks this request when intercepting
On 2011/07/29 07:34:38, willchan wrote:
> These should be private. I think you made them public for URLRequestHttpJob to
> call. I'm trying to tighten the interfaces. Any special interface needed to
> access details in URLRequest from a job should be private in URLRequest, and
be
> protected in URLRequestJob. So URLRequestHttpJob should access these member
> functions via its parent's protected member function.

Matt, can you address Will's comment? (Note that URLRequest job is friended).

[Matt Perry, 2011-08-08 22:14:33]

http://codereview.chromium.org/7523042/diff/4001/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/7523042/diff/4001/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:1070: load_state_.state =
net::LOAD_STATE_IDLE;
On 2011/08/05 01:54:22, eroman wrote:
> nit: how about load_state_ = LoadStateWithParams(IDLE, "");

Done.

http://codereview.chromium.org/7523042/diff/4001/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/7523042/diff/4001/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.h:185: const string16&
load_state_host() const { return load_state_host_; }
On 2011/08/05 01:54:22, eroman wrote:
> I wander if this can be worked into LoadStateWithParams now.

I think it could, as long as we're OK with a special case check in
ResourceDispatcherHost for the WAITING_FOR_RESPONSE load state. I wasn't sure if
that was better API-wise, so I left it as-is.

http://codereview.chromium.org/7523042/diff/4001/net/base/load_states.h
File net/base/load_states.h (right):

http://codereview.chromium.org/7523042/diff/4001/net/base/load_states.h#newco...
net/base/load_states.h:86: std::string param;
On 2011/08/05 01:54:22, eroman wrote:
> Can you make this a string16? Seems there are already conversion from UTF16
-->
> UTF8 going on, since this is intended for display seems simpler to just keep
it
> wide.

Done.

http://codereview.chromium.org/7523042/diff/4001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/7523042/diff/4001/net/url_request/url_request....
net/url_request/url_request.h:611: // Called when the delegate blocks or
unblocks this request when intercepting
On 2011/08/05 01:54:22, eroman wrote:
> On 2011/07/29 07:34:38, willchan wrote:
> > These should be private. I think you made them public for URLRequestHttpJob
to
> > call. I'm trying to tighten the interfaces. Any special interface needed to
> > access details in URLRequest from a job should be private in URLRequest, and
> be
> > protected in URLRequestJob. So URLRequestHttpJob should access these member
> > functions via its parent's protected member function.
> 
> Matt, can you address Will's comment? (Note that URLRequest job is friended).

Done.

[commit-bot: I haz the power, 2011-08-09 23:31:10]

Presubmit check for 7523042-15001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/render_view_host.h,content/browser/renderer_host/resource_dispatcher_host.cc,content/browser/renderer_host/resource_dispatcher_host.h,content/browser/tab_contents/tab_contents.cc,content/browser/renderer_host/render_view_host_delegate.h,content/browser/renderer_host/resource_dispatcher_host_request_info.cc,content/browser/renderer_host/resource_dispatcher_host_request_info.h,content/browser/renderer_host/render_view_host.cc,content/browser/tab_contents/tab_contents.h

Presubmit checks took 1.2s to calculate.

[Matt Perry, 2011-08-09 23:32:00]

+jam for content/ approval

[jam, 2011-08-10 02:43:56]

content lgtm

[commit-bot: I haz the power, 2011-08-10 22:28:24]

Change committed as 96226