Connect UserStyleSheetWatcher to FileWatcher to have changes to

Connect UserStyleSheetWatcher to FileWatcher to have changes to
Default/User StyleSheets/Custom.css instantly change layout in
all your tabs.

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

[tony, 2010-03-10 06:50:12]

It's pretty cool to watch all your tabs change when you save your changes to
Custom.css.

[Paweł Hajdan Jr., 2010-03-10 09:33:21]

You may want to talk with John Abd-El-Malek about the "cached MessageLoop
pointer" if you want to know more, or possibly get some alternative solutions.

http://codereview.chromium.org/799005/diff/1/2
File chrome/browser/user_style_sheet_watcher.cc (right):

http://codereview.chromium.org/799005/diff/1/2#newcode18
chrome/browser/user_style_sheet_watcher.cc:18: }
nit:  // namespace

http://codereview.chromium.org/799005/diff/1/2#newcode101
chrome/browser/user_style_sheet_watcher.cc:101:
g_browser_process->file_thread()->message_loop());
I'm worried about the "cached MessageLoop pointer". It seems we strongly prefer
ChromeThread ID instead, because it's safer, especially during shutdown. That
may mean moving file_watcher to chrome/browser, so that it can use
chrome_thread.

http://codereview.chromium.org/799005/diff/1/3
File chrome/browser/user_style_sheet_watcher.h (right):

http://codereview.chromium.org/799005/diff/1/3#newcode56
chrome/browser/user_style_sheet_watcher.h:56: scoped_ptr<FileWatcher>
file_watcher_;
Is it possible to just stack-allocate file_watcher_? It seems you just need the
information whether it's already watching some path.

[Evan Martin, 2010-03-10 16:03:01]

http://codereview.chromium.org/799005/diff/1/2
File chrome/browser/user_style_sheet_watcher.cc (right):

http://codereview.chromium.org/799005/diff/1/2#newcode15
chrome/browser/user_style_sheet_watcher.cc:15: const char kStyleSheetDir[] =
"User StyleSheets";
Comments, e.g. "name of dir used within user data dir".

Do we support any user style sheets other than this custom.css?  Maybe it'd make
more sense to just put a better-named file in the containing directory?

In the CSS spec they always have a space -- "Style Sheets", so maybe you should
have one here too.

http://codereview.chromium.org/799005/diff/1/2#newcode16
chrome/browser/user_style_sheet_watcher.cc:16: const char kUserStyleSheetFile[]
= "Custom.css";
Capital file name with extension is weird to me.  Not sure if it's weird to you
too, just pointing it out.

http://codereview.chromium.org/799005/diff/1/2#newcode66
chrome/browser/user_style_sheet_watcher.cc:66: FilePath css_file =
style_sheet_dir.AppendASCII(kUserStyleSheetFile);
It's a bit weird we create the file if it's missing -- isn't that more work? 
Most users won't use this...

[tony, 2010-03-11 07:16:04]

http://codereview.chromium.org/799005/diff/1/2
File chrome/browser/user_style_sheet_watcher.cc (right):

http://codereview.chromium.org/799005/diff/1/2#newcode15
chrome/browser/user_style_sheet_watcher.cc:15: const char kStyleSheetDir[] =
"User StyleSheets";
On 2010/03/10 16:03:01, Evan Martin wrote:
> Comments, e.g. "name of dir used within user data dir".

Done.

> Do we support any user style sheets other than this custom.css?  Maybe it'd
make
> more sense to just put a better-named file in the containing directory?

Not currently, but Darin suggested we might some day (the example being that
Firefox has 2, one for chrome:// files and one for web content).  It's in a
subdirectory because of details of the FileWatcher implementation (it still
watches the parent of the file to get create/delete notifications).

> In the CSS spec they always have a space -- "Style Sheets", so maybe you
should
> have one here too.

Meh, too much bikeshedding over file name from Darin in the last codereview. 
I'm not renaming it again. :)

http://codereview.chromium.org/799005/diff/1/2#newcode16
chrome/browser/user_style_sheet_watcher.cc:16: const char kUserStyleSheetFile[]
= "Custom.css";
On 2010/03/10 16:03:01, Evan Martin wrote:
> Capital file name with extension is weird to me.  Not sure if it's weird to
you
> too, just pointing it out.

I originally didn't have a file extension, but Darin requested it (so editors
will highlight the file).

http://codereview.chromium.org/799005/diff/1/2#newcode66
chrome/browser/user_style_sheet_watcher.cc:66: FilePath css_file =
style_sheet_dir.AppendASCII(kUserStyleSheetFile);
On 2010/03/10 16:03:01, Evan Martin wrote:
> It's a bit weird we create the file if it's missing -- isn't that more work? 
> Most users won't use this...

The feature is currently behind a command line flag, so it won't be created for
most users.  For users who want it, I think it helps to pre-create the file
since it would be easy to get wrong (file case, spaces, singular vs plural) and
having it created for you makes it easier to find and edit.

http://codereview.chromium.org/799005/diff/1/2#newcode101
chrome/browser/user_style_sheet_watcher.cc:101:
g_browser_process->file_thread()->message_loop());
On 2010/03/10 09:33:21, Paweł Hajdan Jr. wrote:
> I'm worried about the "cached MessageLoop pointer". It seems we strongly
prefer
> ChromeThread ID instead, because it's safer, especially during shutdown. That
> may mean moving file_watcher to chrome/browser, so that it can use
> chrome_thread.

I'm not sure this is a problem since the cached message loop (file thread) will
always be shutdown before the UI thread.  It's possible that the code here would
be run after the file thread is gone, so I added a null check for that.

http://codereview.chromium.org/799005/diff/1/3
File chrome/browser/user_style_sheet_watcher.h (right):

http://codereview.chromium.org/799005/diff/1/3#newcode56
chrome/browser/user_style_sheet_watcher.h:56: scoped_ptr<FileWatcher>
file_watcher_;
On 2010/03/10 09:33:21, Paweł Hajdan Jr. wrote:
> Is it possible to just stack-allocate file_watcher_? It seems you just need
the
> information whether it's already watching some path.

If I stack allocate it, I need a bool to know if I've called FileWatcher::Watch
yet.  I don't want to add the filewatcher until after I've done the first load. 
I think the code is a bit more readable without an extra bool.

[Paweł Hajdan Jr., 2010-03-11 07:36:21]

I see, scoped_ptr for file_watcher_ is fine then. LGTM after
http://codereview.chromium.org/864001 lands.