Add user script support to extensions.

Add user script support to extensions.

This is implemented mostly by relying on the existing
user script code. But since extension user scripts are
declared, not discovered in a directory, I had to add
support for adding 'lone' user scripts to
UserScriptMaster. This led to a bit of refactoring.

Note that this CL relies on:
http://codereview.chromium.org/18352

[Aaron Boodman, 2009-01-20 03:14:28]

http://codereview.chromium.org/18198/diff/1/2
File base/directory_watcher_win.cc (right):

http://codereview.chromium.org/18198/diff/1/2#newcode48
Line 48: TRUE,  // Watch subtree.
Note: no other code uses this class yet, so it is OK to change this.

[Erik does not do reviews, 2009-01-20 23:57:47]

http://codereview.chromium.org/18198/diff/22/201
File base/directory_watcher_win.cc (right):

http://codereview.chromium.org/18198/diff/22/201#newcode48
Line 48: TRUE,  // Watch subtree.
I bet this is more expensive than before.  It seems like it should be an option.

http://codereview.chromium.org/18198/diff/22/203
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/18198/diff/22/203#newcode69
Line 69: // Tell UserScriptMaster to also watch the extensions directory for
changes
Is this only for developer mode?  If so, perhaps have a flag about a loaded
extension that marks it this way.  Monitoring directories adds OS overhead that
we should avoid if it's never going to do anything.

http://codereview.chromium.org/18198/diff/22/204
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/18198/diff/22/204#newcode77
Line 77: // ExtensionService have the same lifetime.
I found this comment a bit misleading at first.  It felt like it was saying that
ExtensionService owned UserScriptMaster or vice versa.  Maybe say something like
browser init and shutdown ensures that this is always valid during
ExtensionService's lifetime.

http://codereview.chromium.org/18198/diff/22/208
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/18198/diff/22/208#newcode340
Line 340: user_script_master_ = new UserScriptMaster(
why create these variables if the services aren't enabled?

[Aaron Boodman, 2009-01-21 08:05:04]

http://codereview.chromium.org/18198/diff/22/201
File base/directory_watcher_win.cc (right):

http://codereview.chromium.org/18198/diff/22/201#newcode48
Line 48: TRUE,  // Watch subtree.
On 2009/01/20 23:57:47, Erik Kay wrote:
> I bet this is more expensive than before.  It seems like it should be an
option.

I was going to, but then I realized that this was already watching subdirs on
vista because of an apparent bug in this API. (see:
code.google.com/p/chromium/issues/detail?id=5072).

I didn't see the bug because I am developing on vista so the test passed :: sigh
::.

Since these kind of fixme bugs tend to not actually get fixed, I think the best
thing to do is the highest-common-denominator of making the behavior consistent
across platforms, which is also what we happen to need in this case.

In the future, if we find that we need to make this optimization, we can revisit
the vista bug. I added a note with the background to the code.

http://codereview.chromium.org/18198/diff/22/203
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/18198/diff/22/203#newcode69
Line 69: // Tell UserScriptMaster to also watch the extensions directory for
changes
On 2009/01/20 23:57:47, Erik Kay wrote:
> Is this only for developer mode?  If so, perhaps have a flag about a loaded
> extension that marks it this way.  Monitoring directories adds OS overhead
that
> we should avoid if it's never going to do anything.

Yes, it is so that when you're developing an extension, you don't have to keep
restarting the browser to see the changes, which I think is pretty crucial.

I agree that watching the extension install directory is non-ideal, though. I
think the best way to fix this is to implement your idea for an --extension
flag, which would become the suggested way to develop extensions. the install
directory would then become something that is mainly used by the system, not
users or developers. So it would be OK if you have to restart to see changes to
that directory.

I've added an item about implementing --extension to the todo list.

http://codereview.chromium.org/18198/diff/22/204
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/18198/diff/22/204#newcode77
Line 77: // ExtensionService have the same lifetime.
On 2009/01/20 23:57:47, Erik Kay wrote:
> I found this comment a bit misleading at first.  It felt like it was saying
that
> ExtensionService owned UserScriptMaster or vice versa.  Maybe say something
like
> browser init and shutdown ensures that this is always valid during
> ExtensionService's lifetime.

Hm, looking at this again, I think that ExtensionService can actually outlive
UserScriptMaster (in the case where the backend posts a message back to the
frontend thread, but profile has died), so I made this into a ref-counted
pointer, which eliminates the need for the comment about lifetime.

http://codereview.chromium.org/18198/diff/22/208
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/18198/diff/22/208#newcode340
Line 340: user_script_master_ = new UserScriptMaster(
On 2009/01/20 23:57:47, Erik Kay wrote:
> why create these variables if the services aren't enabled?

It prevents branches (and thus chance for bugs) between the services being
enabled and not-enabled. The rest of the code works as normal, it just ends up
that there happens to not be any extensions or user scripts installed because we
prevented them from being loaded at this one chokepoint.

[M-A Ruel, 2009-01-21 17:54:44]

http://codereview.chromium.org/18198/diff/22/201
File base/directory_watcher_win.cc (right):

http://codereview.chromium.org/18198/diff/22/201#newcode48
Line 48: TRUE,  // Watch subtree.
May I add that i have no specific opinion on this, but I wouldn't say that it is
more expensive when there is no activity in the subdirectories so I'm fine with
the change.

[Erik does not do reviews, 2009-01-21 20:24:36]

LGTM

http://codereview.chromium.org/18198/diff/22/203
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/18198/diff/22/203#newcode69
Line 69: // Tell UserScriptMaster to also watch the extensions directory for
changes
On 2009/01/21 08:05:04, Aaron Boodman wrote:
> On 2009/01/20 23:57:47, Erik Kay wrote:
> > Is this only for developer mode?  If so, perhaps have a flag about a loaded
> > extension that marks it this way.  Monitoring directories adds OS overhead
> that
> > we should avoid if it's never going to do anything.
> 
> Yes, it is so that when you're developing an extension, you don't have to keep
> restarting the browser to see the changes, which I think is pretty crucial.
> 
> I agree that watching the extension install directory is non-ideal, though. I
> think the best way to fix this is to implement your idea for an --extension
> flag, which would become the suggested way to develop extensions. the install
> directory would then become something that is mainly used by the system, not
> users or developers. So it would be OK if you have to restart to see changes
to
> that directory.
> 
> I've added an item about implementing --extension to the todo list.

Could you add a TODO here as well then?

[Evan Martin, 2009-01-25 15:57:17]

BTW, the reason I originally passed FALSE in for that subdirectory watcher is
that it's more work to implement the recursive watch on Linux.  It's not a
horrifying amount of work (basically you need to watch each subdirectory
manually, and create the watches in an order such that if someone adds/removes a
subdirectory while you're in the middle of your recursive scan you do the right
thing), but it wasn't needed at the time.

We should figure out what the correct thing for users to do is and then make
that work on XP/Vista.  I am fine with that figuring out to not happen here.  :)