First cut at custom user style sheets.

First cut at custom user style sheets.

Enabled with the --enable-user-stylesheet flag which
causes chrome to read
<user-data-dir>/<profile>/User StyleSheet/Custom.css
at startup and set it as the user style sheet.

This version never reloads the user style sheet, I'll
have to bring back FileWatcher for that.  I also put the user
stylesheet in a subdir because the implementation of
FileWatcher will watch the parent dir (this is what the OS
apis give me) and watching the profile dir will cause
lots of activity.

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

[tony, 2010-03-05 05:32:12]

ping
I sat on this a while because I wanted to work out the implementation of
FileWatcher: http://codereview.chromium.org/661359

[darin (slow to review), 2010-03-05 08:18:18]

http://codereview.chromium.org/660349/diff/2001/3008
File chrome/browser/user_style_sheet_watcher.cc (right):

http://codereview.chromium.org/660349/diff/2001/3008#newcode46
chrome/browser/user_style_sheet_watcher.cc:46: FilePath style_sheet_dir =
profile_path.AppendASCII("User StyleSheet");
does this requirement change now that you have created the FileWatcher?

http://codereview.chromium.org/660349/diff/2001/3008#newcode52
chrome/browser/user_style_sheet_watcher.cc:52: FilePath css_file =
style_sheet_dir.AppendASCII("Custom");
since this is a CSS file, shouldn't it have a .css suffix?

http://codereview.chromium.org/660349/diff/2001/3009
File chrome/browser/user_style_sheet_watcher.h (right):

http://codereview.chromium.org/660349/diff/2001/3009#newcode49
chrome/browser/user_style_sheet_watcher.h:49: NotificationRegistrar registrar_;
you probably care what thread UserStyleSheetWatcher dies on.  for that reason, i
think you should supply the second template parameter to
base::RefCountedThreadSafe.  see ChromeThread for sample parameters. 
DeleteOnUIThread is probably what you want.

http://codereview.chromium.org/660349/diff/2001/3013
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/660349/diff/2001/3013#newcode305
chrome/common/chrome_switches.cc:305: const char kEnableUserStyleSheet[]     =
"enable-user-stylesheet";
nit: indent the "=" to line up vertically with the others

[tony, 2010-03-05 08:29:34]

http://codereview.chromium.org/660349/diff/2001/3008
File chrome/browser/user_style_sheet_watcher.cc (right):

http://codereview.chromium.org/660349/diff/2001/3008#newcode46
chrome/browser/user_style_sheet_watcher.cc:46: FilePath style_sheet_dir =
profile_path.AppendASCII("User StyleSheet");
On 2010/03/05 08:18:18, darin wrote:
> does this requirement change now that you have created the FileWatcher?

Unfortunately, no.  FileWatcher still hooks at the directory level (it's the
only way to get notifications for delete and create).  I could put the file in
the profile, but it will lead to lots of notifications that we ignore.

http://codereview.chromium.org/660349/diff/2001/3008#newcode52
chrome/browser/user_style_sheet_watcher.cc:52: FilePath css_file =
style_sheet_dir.AppendASCII("Custom");
On 2010/03/05 08:18:18, darin wrote:
> since this is a CSS file, shouldn't it have a .css suffix?

We don't put .sqlite or .json on our other files, so I didn't put an extension
here.  I don't actually care one way or another.

http://codereview.chromium.org/660349/diff/2001/3009
File chrome/browser/user_style_sheet_watcher.h (right):

http://codereview.chromium.org/660349/diff/2001/3009#newcode49
chrome/browser/user_style_sheet_watcher.h:49: NotificationRegistrar registrar_;
On 2010/03/05 08:18:18, darin wrote:
> you probably care what thread UserStyleSheetWatcher dies on.  for that reason,
i
> think you should supply the second template parameter to
> base::RefCountedThreadSafe.  see ChromeThread for sample parameters. 
> DeleteOnUIThread is probably what you want.

Good point. Fixed.

http://codereview.chromium.org/660349/diff/2001/3013
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/660349/diff/2001/3013#newcode305
chrome/common/chrome_switches.cc:305: const char kEnableUserStyleSheet[]     =
"enable-user-stylesheet";
On 2010/03/05 08:18:18, darin wrote:
> nit: indent the "=" to line up vertically with the others

Done.

[darin (slow to review), 2010-03-05 08:42:58]

On Fri, Mar 5, 2010 at 12:29 AM, <tony@chromium.org> wrote:

>
> http://codereview.chromium.org/660349/diff/2001/3008
> File chrome/browser/user_style_sheet_watcher.cc (right):
>
> http://codereview.chromium.org/660349/diff/2001/3008#newcode46
> chrome/browser/user_style_sheet_watcher.cc:46: FilePath style_sheet_dir
> = profile_path.AppendASCII("User StyleSheet");
> On 2010/03/05 08:18:18, darin wrote:
>
>> does this requirement change now that you have created the
>>
> FileWatcher?
>
> Unfortunately, no.  FileWatcher still hooks at the directory level (it's
> the only way to get notifications for delete and create).  I could put
> the file in the profile, but it will lead to lots of notifications that
> we ignore.
>
>
Ah, OK.  Would it ever be the case that this directory could contain
multiple stylesheets?  Should it be named "User StyleSheets" instead?



>
> http://codereview.chromium.org/660349/diff/2001/3008#newcode52
> chrome/browser/user_style_sheet_watcher.cc:52: FilePath css_file =
> style_sheet_dir.AppendASCII("Custom");
> On 2010/03/05 08:18:18, darin wrote:
>
>> since this is a CSS file, shouldn't it have a .css suffix?
>>
>
> We don't put .sqlite or .json on our other files, so I didn't put an
> extension here.  I don't actually care one way or another.
>
>
Right, but unlike those files, isn't this one intended to be modified
by hand?  It seems like the .css extension would help the text
editor know how to syntax highlight the contents :-)



> http://codereview.chromium.org/660349
>

[tony, 2010-03-05 08:55:03]

On 2010/03/05 08:42:58, darin wrote:
> Ah, OK.  Would it ever be the case that this directory could contain
> multiple stylesheets?  Should it be named "User StyleSheets" instead?

I can't think of a use case for multiple stylesheets.  If we have multiple
stylesheets, we'll need to change the command line flag too.  I was trying to
keep this directory name and the flag the same.

> Right, but unlike those files, isn't this one intended to be modified
> by hand?  It seems like the .css extension would help the text
> editor know how to syntax highlight the contents :-)

Good point. Change the file to Custom.css and update the change description.

[darin (slow to review), 2010-03-05 08:57:25]

On Fri, Mar 5, 2010 at 12:55 AM, <tony@chromium.org> wrote:

> On 2010/03/05 08:42:58, darin wrote:
>
>> Ah, OK.  Would it ever be the case that this directory could contain
>> multiple stylesheets?  Should it be named "User StyleSheets" instead?
>>
>
> I can't think of a use case for multiple stylesheets.  If we have multiple
> stylesheets, we'll need to change the command line flag too.  I was trying
> to
> keep this directory name and the flag the same.
>
>
I know why I thought of multiple stylesheets... Mozilla has one for content
documents and one for chrome documents.  Perhaps we might want to have
support for a user stylesheet that gets loaded into DOM UI or extension UI?

-Darin



>
>  Right, but unlike those files, isn't this one intended to be modified
>> by hand?  It seems like the .css extension would help the text
>> editor know how to syntax highlight the contents :-)
>>
>
> Good point. Change the file to Custom.css and update the change
> description.
>
>
> http://codereview.chromium.org/660349
>

[darin (slow to review), 2010-03-05 08:57:52]

LGTM btw

[tony, 2010-03-05 09:02:35]

User StyleSheets it is. :)

Thanks for the review, I'll land this on Monday.