Delay request loading until all user scripts that request depends on are ready.

Delay request loading until all user scripts that request depends on are ready.

BUG=11547
TEST=See bug.

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

[Matt Perry, 2009-08-22 00:11:54]

I still need to write some tests.

[Aaron Boodman, 2009-08-24 18:01:58]

Nice

http://codereview.chromium.org/173239/diff/1/3
File chrome/browser/extensions/user_script_listener.cc (right):

http://codereview.chromium.org/173239/diff/1/3#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009

http://codereview.chromium.org/173239/diff/1/3#newcode20
Line 20: DCHECK_EQ(ui_loop_, MessageLoop::current());
If you are going to mandate this, why have it passed in at all?

http://codereview.chromium.org/173239/diff/1/3#newcode31
Line 31: UserScriptListener::~UserScriptListener() {
Remove empty destructor?

http://codereview.chromium.org/173239/diff/1/3#newcode49
Line 49: AutoLock lock(lock_);
It's unfortunate to acquire a lock on every frame request. Since
user_script_ready_ is a bool, can we move it above the lock check?

http://codereview.chromium.org/173239/diff/1/3#newcode57
Line 57: it != url_patterns_.end(); ++it) {
Also, it seems like since you are keeping a copy of the url patterns, you could
get rid of the lock completely by updating the url pattern list by using
PostTask.

http://codereview.chromium.org/173239/diff/1/4
File chrome/browser/extensions/user_script_listener.h (right):

http://codereview.chromium.org/173239/diff/1/4#newcode24
Line 24: class UserScriptListener
Nomenclature nit: This class is really a ContentScriptListener. "user scripts"
are the standalone files ending in ".user.js" that are enabled by
--enable-user-scripts.

http://codereview.chromium.org/173239/diff/1/4#newcode60
Line 60: Lock lock_;
Can you clarify in the header of this class what threads are involved, as an
overview?

[Matt Perry, 2009-08-25 00:39:06]

I added a unit test.

http://codereview.chromium.org/173239/diff/1/3
File chrome/browser/extensions/user_script_listener.cc (right):

http://codereview.chromium.org/173239/diff/1/3#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/08/24 18:01:58, Aaron Boodman wrote:
> 2009

Done.

http://codereview.chromium.org/173239/diff/1/3#newcode20
Line 20: DCHECK_EQ(ui_loop_, MessageLoop::current());
On 2009/08/24 18:01:58, Aaron Boodman wrote:
> If you are going to mandate this, why have it passed in at all?

Yeah it's weird, but I'm trying to be consistent with the save_file_manager_ and
others in ResourceDispatcherHost.

http://codereview.chromium.org/173239/diff/1/3#newcode31
Line 31: UserScriptListener::~UserScriptListener() {
On 2009/08/24 18:01:58, Aaron Boodman wrote:
> Remove empty destructor?

Done.

http://codereview.chromium.org/173239/diff/1/3#newcode57
Line 57: it != url_patterns_.end(); ++it) {
On 2009/08/24 18:01:58, Aaron Boodman wrote:
> Also, it seems like since you are keeping a copy of the url patterns, you
could
> get rid of the lock completely by updating the url pattern list by using
> PostTask.

Thanks for the suggestion. It's much less scary without locking.

http://codereview.chromium.org/173239/diff/1/4
File chrome/browser/extensions/user_script_listener.h (right):

http://codereview.chromium.org/173239/diff/1/4#newcode60
Line 60: Lock lock_;
On 2009/08/24 18:01:58, Aaron Boodman wrote:
> Can you clarify in the header of this class what threads are involved, as an
> overview?

Done.

[Aaron Boodman, 2009-08-25 00:49:43]

lgtm

http://codereview.chromium.org/173239/diff/15/1007
File chrome/browser/extensions/user_script_listener_unittest.cc (right):

http://codereview.chromium.org/173239/diff/15/1007#newcode50
Line 50: request.first_party_for_cookies = url;  // bypass third-party cookie
blocking
80 cols