Theme Preferences for Distilled Pages

Theme Preferences for Distilled Pages

Adds support for allowing users to change the appearance of 
distilled pages to different themes. The current themes are 
light, dark and sepia.

The theme of the page is controlled by the CSS class of the 
body element. When the page is loaded, this CSS class is set
 by inserting the CSS class into the HTML.  When a setting is 
changed while a distilled page is open, the CSS class is set 
by calling the JavaScript function useTheme which then updates 
the body element's CSS class.

BUG=383630
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283303

[robliao, 2014-06-17 18:01:50]

https://codereview.chromium.org/341563002/diff/20001/chrome/browser/dom_disti...
File chrome/browser/dom_distiller/lazy_dom_distiller_service.cc (right):

https://codereview.chromium.org/341563002/diff/20001/chrome/browser/dom_disti...
chrome/browser/dom_distiller/lazy_dom_distiller_service.cc:24: reader_mode_prefs
= scoped_ptr<ReaderModePrefs>(
Should be able to do
reader_mode_prefs.reset(new ...)

https://codereview.chromium.org/341563002/diff/20001/chrome/browser/dom_disti...
File chrome/browser/dom_distiller/lazy_dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/20001/chrome/browser/dom_disti...
chrome/browser/dom_distiller/lazy_dom_distiller_service.h:57: virtual
ReaderModePrefs* GetReaderModePrefs() OVERRIDE;
Add linebreak after this line.

Also make this method const (like GetSyncableService).

https://codereview.chromium.org/341563002/diff/20001/chrome/browser/dom_disti...
chrome/browser/dom_distiller/lazy_dom_distiller_service.h:75:
scoped_ptr<ReaderModePrefs> reader_mode_prefs;
Add underscore at the end for member variables.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
File components/dom_distiller/DEPS (right):

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/DEPS:2:
"+components/pref_registry/pref_registry_syncable.h",
Omit the header file and include the pref_registry directory.

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_service.cc (right):

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_service.cc:48:
reader_mode_prefs_(reader_mode_prefs.Pass()) {
What is the lifetime model for reader mode prefs? Who creates it, how is it
passed, and then who is responsible for destroying it?

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:28:
ReaderModePrefs::ReaderModePrefs(PrefService* current_pref_service) {
Consistency: pref_service as a name should work fine.

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:30:
ReaderModePrefs::pref_service = current_pref_service;
Put this in a constructor initializer list.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:32: 
Remove extra line.

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:34: std::string
ReaderModePrefs::ReplaceCSSTemplateValues() {
This should really go into the viewer code near GetCss. This class can be used
solely for managing the preference service.

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:77: } // namespace
dom_distiller
One more space required between } and //

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.h:29:
ReaderModePrefs(PrefService* pref_service);
Use this declaration order for your public and private sections.:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

+ one line break here.

[smaslo, 2014-06-17 22:44:08]

https://codereview.chromium.org/341563002/diff/20001/chrome/browser/dom_disti...
File chrome/browser/dom_distiller/lazy_dom_distiller_service.cc (right):

https://codereview.chromium.org/341563002/diff/20001/chrome/browser/dom_disti...
chrome/browser/dom_distiller/lazy_dom_distiller_service.cc:24: reader_mode_prefs
= scoped_ptr<ReaderModePrefs>(
On 2014/06/17 18:01:49, robliao wrote:
> Should be able to do
> reader_mode_prefs.reset(new ...)

Done.

https://codereview.chromium.org/341563002/diff/20001/chrome/browser/dom_disti...
File chrome/browser/dom_distiller/lazy_dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/20001/chrome/browser/dom_disti...
chrome/browser/dom_distiller/lazy_dom_distiller_service.h:57: virtual
ReaderModePrefs* GetReaderModePrefs() OVERRIDE;
On 2014/06/17 18:01:49, robliao wrote:
> Add linebreak after this line.
> 
> Also make this method const (like GetSyncableService). 

Done.

https://codereview.chromium.org/341563002/diff/20001/chrome/browser/dom_disti...
chrome/browser/dom_distiller/lazy_dom_distiller_service.h:75:
scoped_ptr<ReaderModePrefs> reader_mode_prefs;
On 2014/06/17 18:01:49, robliao wrote:
> Add underscore at the end for member variables.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

Done.

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
File components/dom_distiller/DEPS (right):

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/DEPS:2:
"+components/pref_registry/pref_registry_syncable.h",
On 2014/06/17 18:01:49, robliao wrote:
> Omit the header file and include the pref_registry directory.

Done.

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_service.cc (right):

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_service.cc:48:
reader_mode_prefs_(reader_mode_prefs.Pass()) {
It is created in dom_distiller_service_factory.  Each separate
dom_distiller_service will have its own instance of ReaderModePrefs (Passed in
at construction).  The instance in dom_distiller_service (reader_mode_prefs_)
will be destroyed automatically when the dom_distiller_service is destroyed
because it is a scoped_ptr.

On 2014/06/17 18:01:49, robliao wrote:
> What is the lifetime model for reader mode prefs? Who creates it, how is it
> passed, and then who is responsible for destroying it?

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:28:
ReaderModePrefs::ReaderModePrefs(PrefService* current_pref_service) {
On 2014/06/17 18:01:49, robliao wrote:
> Consistency: pref_service as a name should work fine.

Done.

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:30:
ReaderModePrefs::pref_service = current_pref_service;
On 2014/06/17 18:01:49, robliao wrote:
> Put this in a constructor initializer list.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

Done.

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:32: 
On 2014/06/17 18:01:49, robliao wrote:
> Remove extra line.

Done.

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:34: std::string
ReaderModePrefs::ReplaceCSSTemplateValues() {
On 2014/06/17 18:01:50, robliao wrote:
> This should really go into the viewer code near GetCss. This class can be used
> solely for managing the preference service.

Done.

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:77: } // namespace
dom_distiller
On 2014/06/17 18:01:49, robliao wrote:
> One more space required between } and //

Done.

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/20001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.h:29:
ReaderModePrefs(PrefService* pref_service);
On 2014/06/17 18:01:50, robliao wrote:
> Use this declaration order for your public and private sections.:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> 
> + one line break here.

Done.

[robliao, 2014-06-17 23:08:16]

Some nits.

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:19: const char
kHigh_Contrast_Pref[] = "reader_mode.high_contrast";
Remove one level of indentation.

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:25: :profile_(profile)
{
Add Space between : and profile_(...

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:37: }
Add linebreak here.

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.h:18: extern const char
kHigh_Contrast_Pref[];
Remove extra indentation.

Constant Name: kHighConstratPref
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta...

This namespace could also probably go into the dom_distiller namespace as a new
file. Check out how
components/*/pref_names.[cc|h] are defined

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.h:29: private:
Add one linebreak above private:

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
components/dom_distiller/core/viewer.cc:30: namespace {
Add an extra linebreak here.

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
components/dom_distiller/core/viewer.cc:31: const char WHITE[] = "#FFF";
Sorry, I got mixed up in Chrome Windows conventions and Google C++ Conventions.

This should be kWhite and kBlack.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta...

[smaslo, 2014-06-18 16:57:15]

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:19: const char
kHigh_Contrast_Pref[] = "reader_mode.high_contrast";
On 2014/06/17 23:08:15, robliao wrote:
> Remove one level of indentation.

Done.

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:25: :profile_(profile)
{
On 2014/06/17 23:08:15, robliao wrote:
> Add Space between : and profile_(...

Done.

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.cc:37: }
On 2014/06/17 23:08:15, robliao wrote:
> Add linebreak here.

Done.

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.h:18: extern const char
kHigh_Contrast_Pref[];
On 2014/06/17 23:08:15, robliao wrote:
> Remove extra indentation.
> 
> Constant Name: kHighConstratPref
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta...
> 
> This namespace could also probably go into the dom_distiller namespace as a
new
> file. Check out how
> components/*/pref_names.[cc|h] are defined

Done.

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
components/dom_distiller/core/reader_mode_preferences.h:29: private:
On 2014/06/17 23:08:15, robliao wrote:
> Add one linebreak above private:

Done.

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
components/dom_distiller/core/viewer.cc:30: namespace {
On 2014/06/17 23:08:16, robliao wrote:
> Add an extra linebreak here.

Done.

https://codereview.chromium.org/341563002/diff/40001/components/dom_distiller...
components/dom_distiller/core/viewer.cc:31: const char WHITE[] = "#FFF";
On 2014/06/17 23:08:16, robliao wrote:
> Sorry, I got mixed up in Chrome Windows conventions and Google C++
Conventions.
> 
> This should be kWhite and kBlack.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta...

Done.

[robliao, 2014-06-18 18:09:27]

One last comment.

https://codereview.chromium.org/341563002/diff/100001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/100001/components/dom_distille...
components/dom_distiller/core/viewer.cc:32: const char kWHITE[] = "#FFF";
The constant should be kWhite (lower case names after a k).

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names

[smaslo, 2014-06-18 18:20:40]

https://codereview.chromium.org/341563002/diff/100001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/100001/components/dom_distille...
components/dom_distiller/core/viewer.cc:32: const char kWHITE[] = "#FFF";
On 2014/06/18 18:09:27, robliao wrote:
> The constant should be kWhite (lower case names after a k).
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names

Done.

[robliao, 2014-06-18 21:37:34]

On 2014/06/18 18:20:40, smaslo wrote:
>
https://codereview.chromium.org/341563002/diff/100001/components/dom_distille...
> File components/dom_distiller/core/viewer.cc (right):
> 
>
https://codereview.chromium.org/341563002/diff/100001/components/dom_distille...
> components/dom_distiller/core/viewer.cc:32: const char kWHITE[] = "#FFF";
> On 2014/06/18 18:09:27, robliao wrote:
> > The constant should be kWhite (lower case names after a k).
> > 
> >
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names
> 
> Done.

lgtm. Also add battre for owner approval for
chrome/browser/prefs/browser_prefs.cc

[smaslo, 2014-06-18 22:12:45]

Owners:  Please provide owner approval for these files:

battre:
chrome/browser/prefs/browser_prefs.cc

nyquist:
All others

[smaslo, 2014-06-20 21:30:43]

Hello,

This is a just a reminder that I need owner approval for the following files:

battre:
chrome/browser/prefs/browser_prefs.cc

nyquist:
All others

[battre, 2014-06-23 08:37:24]

Hi.

Sorry, I was OOO.

I have a couple of nits and one important architectural change (don't depend on
chrome/browser/profile from a component).

Best regards,
Dominic

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/lazy_dom_distiller_service.cc (right):

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/lazy_dom_distiller_service.cc:24:
reader_mode_prefs_.reset(new ReaderModePrefs(profile_));
Move this after service_factory_(service_factory) ?

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/DEPS (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/DEPS:2: "+chrome/browser/profiles",
This is against the spirit of components. They should not depend on the browser.
The component should be reusable by iOS and that does not have a
chrome/browser/profiles.

You can use base/prefs/pref_service.h and inject that into your component.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_pref_names.cc (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_pref_names.cc:1: // Copyright 2014
The Chromium Authros. All rights reserved.
nit: Authros

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_pref_names.cc:11: const char
kHighContrastPref[] = "reader_mode.high_contrast";
Please add a comment.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:32: void
ReaderModePrefs::OnChangeHighContrast(bool new_value) {
WDYT of renaming this to SetHighContrastPref(bool new_value)?

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:8: #include <string>
Not used.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:10: class PrefService;
Not used

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:21:
ReaderModePrefs(Profile* profile);
explicit

Please pass the PrefService instead of the profile.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:24: bool
GetHighContrastPref();
Nit: Mark as const?

Please add a comment what true and false represent.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/viewer.cc:51: }
Opt:

Wouldn't this be more readable if you phrase it as
if (reader_mode_prefs->GetHighContrastPref())
  return "background-color: #000;\ncolor: #FFF;";  // White on black.
else
  return "background-color: #FFF;\ncolor: #000;";  // Black on white.

Note that you don't add a \n after the color.

[nyquist, 2014-06-23 16:03:06]

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/lazy_dom_distiller_service.cc (right):

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/lazy_dom_distiller_service.cc:24:
reader_mode_prefs_.reset(new ReaderModePrefs(profile_));
On 2014/06/23 08:37:23, battre wrote:
> Move this after service_factory_(service_factory) ?

Also, like battre has mentioned, this is probably not needed. Doing this here,
creates another ReaderModePrefs, which might explain what you were seeing last
week.

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/lazy_dom_distiller_service.cc:99: return
reader_mode_prefs_.get();
This could probably just delegate to the DomDistillerService, by calling
instance()->...

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/lazy_dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/lazy_dom_distiller_service.h:76:
scoped_ptr<ReaderModePrefs> reader_mode_prefs_;
Could this be moved to DomDistillerService instead? (In //components).

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:417:
dom_distiller::ReaderModePrefs::RegisterProfilePrefs(registry);
Why are these prefs only for Android?
If that is intentional, we could change them to be non-synced prefs, because
prefs are currently not synced on Android.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_pref_names.h (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_pref_names.h:12: extern const char
kHighContrastPref[];
Is this in use outside of reader_mode_preferences.cc? Could you just have this
constant directly defined in an anonymous namespace in that file and remove this
header and the corresponding cc file?

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.cc (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.cc:246: }
Nit: newline after this.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:109:
scoped_ptr<ReaderModePrefs> reader_mode_prefs);
Does this require you to update the tests?

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:14: #include
"components/dom_distiller/core/url_constants.h"
Are all these includes in use?

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:32: void
ReaderModePrefs::OnChangeHighContrast(bool new_value) {
On 2014/06/23 08:37:23, battre wrote:
> WDYT of renaming this to SetHighContrastPref(bool new_value)?

Like battre suggests, having consistent naming across setting and getting the
preference is a good thing.

It is a little bit hard to see what this method or pref does, so maybe use
naming such as:

SetUseHighContrast(bool)
bool GetUseHighContrast()

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:33:
profile_->GetPrefs()->SetBoolean(prefs::kHighContrastPref, new_value);
This code and below will become simpler when you just pass in the pref service.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:24: bool
GetHighContrastPref();
On 2014/06/23 08:37:23, battre wrote:
> Nit: Mark as const?
> 
> Please add a comment what true and false represent.

As a suggestion, one easy-to-read way of documenting a bool value is by using
'whether'. For example you could write something like:
// Returns whether high contrast mode should be used.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/viewer.cc:32: const char kWhite[] = "#FFF";
Nit: I think these two can go away if you follow battre's comment.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/viewer.cc:35: std::string
GetBodyInsertionText(ReaderModePrefs* reader_mode_prefs) {
Is there a way of writing a test to ensure we use the correct background-color
and color for the CSS?

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/viewer.cc:163: return
ReplaceCSSTemplateValues(reader_mode_prefs);
This is only used once, so maybe you could inline this?

[smaslo, 2014-06-23 22:14:34]

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/lazy_dom_distiller_service.cc (right):

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/lazy_dom_distiller_service.cc:24:
reader_mode_prefs_.reset(new ReaderModePrefs(profile_));
On 2014/06/23 08:37:23, battre wrote:
> Move this after service_factory_(service_factory) ?

Removed completely.

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/lazy_dom_distiller_service.cc:24:
reader_mode_prefs_.reset(new ReaderModePrefs(profile_));
On 2014/06/23 16:03:05, nyquist wrote:
> On 2014/06/23 08:37:23, battre wrote:
> > Move this after service_factory_(service_factory) ?
> 
> Also, like battre has mentioned, this is probably not needed. Doing this here,
> creates another ReaderModePrefs, which might explain what you were seeing last
> week.

Done.

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/lazy_dom_distiller_service.cc:99: return
reader_mode_prefs_.get();
On 2014/06/23 16:03:05, nyquist wrote:
> This could probably just delegate to the DomDistillerService, by calling
> instance()->...

Done.

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/lazy_dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/lazy_dom_distiller_service.h:76:
scoped_ptr<ReaderModePrefs> reader_mode_prefs_;
On 2014/06/23 16:03:05, nyquist wrote:
> Could this be moved to DomDistillerService instead? (In //components).

Done.

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/341563002/diff/120001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:417:
dom_distiller::ReaderModePrefs::RegisterProfilePrefs(registry);
On 2014/06/23 16:03:05, nyquist wrote:
> Why are these prefs only for Android?
> If that is intentional, we could change them to be non-synced prefs, because
> prefs are currently not synced on Android.

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/DEPS (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/DEPS:2: "+chrome/browser/profiles",
On 2014/06/23 08:37:23, battre wrote:
> This is against the spirit of components. They should not depend on the
browser.
> The component should be reusable by iOS and that does not have a
> chrome/browser/profiles.
> 
> You can use base/prefs/pref_service.h and inject that into your component.

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_pref_names.cc (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_pref_names.cc:1: // Copyright 2014
The Chromium Authros. All rights reserved.
On 2014/06/23 08:37:23, battre wrote:
> nit: Authros

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_pref_names.cc:11: const char
kHighContrastPref[] = "reader_mode.high_contrast";
On 2014/06/23 08:37:23, battre wrote:
> Please add a comment.

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_pref_names.h (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_pref_names.h:12: extern const char
kHighContrastPref[];
On 2014/06/23 16:03:06, nyquist wrote:
> Is this in use outside of reader_mode_preferences.cc? Could you just have this
> constant directly defined in an anonymous namespace in that file and remove
this
> header and the corresponding cc file?

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.cc (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.cc:246: }
On 2014/06/23 16:03:06, nyquist wrote:
> Nit: newline after this.

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:109:
scoped_ptr<ReaderModePrefs> reader_mode_prefs);
On 2014/06/23 16:03:06, nyquist wrote:
> Does this require you to update the tests?

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:14: #include
"components/dom_distiller/core/url_constants.h"
On 2014/06/23 16:03:06, nyquist wrote:
> Are all these includes in use?

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:32: void
ReaderModePrefs::OnChangeHighContrast(bool new_value) {
On 2014/06/23 08:37:23, battre wrote:
> WDYT of renaming this to SetHighContrastPref(bool new_value)?

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:32: void
ReaderModePrefs::OnChangeHighContrast(bool new_value) {
On 2014/06/23 16:03:06, nyquist wrote:
> On 2014/06/23 08:37:23, battre wrote:
> > WDYT of renaming this to SetHighContrastPref(bool new_value)?
> 
> Like battre suggests, having consistent naming across setting and getting the
> preference is a good thing.
> 
> It is a little bit hard to see what this method or pref does, so maybe use
> naming such as:
> 
> SetUseHighContrast(bool)
> bool GetUseHighContrast()

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:33:
profile_->GetPrefs()->SetBoolean(prefs::kHighContrastPref, new_value);
On 2014/06/23 16:03:06, nyquist wrote:
> This code and below will become simpler when you just pass in the pref
service.

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:8: #include <string>
On 2014/06/23 08:37:23, battre wrote:
> Not used.

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:10: class PrefService;
On 2014/06/23 08:37:23, battre wrote:
> Not used

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:21:
ReaderModePrefs(Profile* profile);
On 2014/06/23 08:37:23, battre wrote:
> explicit
> 
> Please pass the PrefService instead of the profile.

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:24: bool
GetHighContrastPref();
On 2014/06/23 08:37:23, battre wrote:
> Nit: Mark as const?
> 
> Please add a comment what true and false represent.

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:24: bool
GetHighContrastPref();
On 2014/06/23 16:03:06, nyquist wrote:
> On 2014/06/23 08:37:23, battre wrote:
> > Nit: Mark as const?
> > 
> > Please add a comment what true and false represent.
> 
> As a suggestion, one easy-to-read way of documenting a bool value is by using
> 'whether'. For example you could write something like:
> // Returns whether high contrast mode should be used.

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/viewer.cc:32: const char kWhite[] = "#FFF";
On 2014/06/23 16:03:06, nyquist wrote:
> Nit: I think these two can go away if you follow battre's comment.

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/viewer.cc:35: std::string
GetBodyInsertionText(ReaderModePrefs* reader_mode_prefs) {
The only way I've heard so far is doing a string test to see if the CSS file
contains the correct string.  For example, we could test if the CSS file
contains "background-color: #000;". However, this method seems rather fragile.

On 2014/06/23 16:03:06, nyquist wrote:
> Is there a way of writing a test to ensure we use the correct background-color
> and color for the CSS?

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/viewer.cc:51: }
On 2014/06/23 08:37:23, battre wrote:
> Opt:
> 
> Wouldn't this be more readable if you phrase it as
> if (reader_mode_prefs->GetHighContrastPref())
>   return "background-color: #000;\ncolor: #FFF;";  // White on black.
> else
>   return "background-color: #FFF;\ncolor: #000;";  // Black on white.
> 
> Note that you don't add a \n after the color.

Done.

https://codereview.chromium.org/341563002/diff/120001/components/dom_distille...
components/dom_distiller/core/viewer.cc:163: return
ReplaceCSSTemplateValues(reader_mode_prefs);
On 2014/06/23 16:03:06, nyquist wrote:
> This is only used once, so maybe you could inline this?

Done.

[battre, 2014-06-24 07:46:40]

Just a couple of nits remaining.

Once fixed, this is sufficiently LGTM that I am happy to delegate the final
sign-off to nyquist@, who is in your timezone.

Thanks,
Dominic

https://codereview.chromium.org/341563002/diff/160001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/lazy_dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/160001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/lazy_dom_distiller_service.h:57: virtual
ReaderModePrefs* GetReaderModePrefs() const OVERRIDE;
Actually, I would not mark this const as it gives access to a mutable object.

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:94: virtual
ReaderModePrefs* GetReaderModePrefs() const;
ditto - IMO this should not be const.

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:23: void
ReaderModePrefs::RegisterProfilePrefs(
// static
void ReaderModePrefs::RegisterProfilePrefs(

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:18:
ReaderModePrefs(PrefService* pref_service);
explicit ReaderModePrefs(PrefService* pref_service);

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:28: PrefService*
pref_service_;
DISALLOW_COPY_AND_ASSIGN(ReaderModePrefs);

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
components/dom_distiller/core/viewer.cc:36: else {
Nit: } else {

[smaslo, 2014-06-24 18:11:18]

https://codereview.chromium.org/341563002/diff/160001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/lazy_dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/160001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/lazy_dom_distiller_service.h:57: virtual
ReaderModePrefs* GetReaderModePrefs() const OVERRIDE;
On 2014/06/24 07:46:39, battre wrote:
> Actually, I would not mark this const as it gives access to a mutable object.

Done.

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:94: virtual
ReaderModePrefs* GetReaderModePrefs() const;
On 2014/06/24 07:46:40, battre wrote:
> ditto - IMO this should not be const.

Done.

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:23: void
ReaderModePrefs::RegisterProfilePrefs(
On 2014/06/24 07:46:40, battre wrote:
> // static
> void ReaderModePrefs::RegisterProfilePrefs(

Done.

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:18:
ReaderModePrefs(PrefService* pref_service);
On 2014/06/24 07:46:40, battre wrote:
> explicit ReaderModePrefs(PrefService* pref_service);

Done.

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:28: PrefService*
pref_service_;
On 2014/06/24 07:46:40, battre wrote:
> DISALLOW_COPY_AND_ASSIGN(ReaderModePrefs);

Done.

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/160001/components/dom_distille...
components/dom_distiller/core/viewer.cc:36: else {
On 2014/06/24 07:46:40, battre wrote:
> Nit: } else {

Done.

[nyquist, 2014-06-24 23:08:50]

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
File components/dom_distiller/DEPS (right):

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/DEPS:2: "+components/pref_registry",
Nit: sort

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
File components/dom_distiller/core/css/distilledpage.css (right):

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:12: $1
This probably has to be rebased.

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:94: virtual
ReaderModePrefs* GetReaderModePrefs();
make this pure virtual (add = 0). Also, could you add a comment for this?

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service_unittest.cc (right):

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service_unittest.cc:94:
scoped_ptr<ReaderModePrefs>(new ReaderModePrefs(NULL))));
Could you just pass an empty scoped_ptr here?

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:12: // True if user
prefers high-contrast (dark) appearance when in reader mode.
s/True if user prefers/Whether user prefers/

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:13: const char
kHighContrastPref[] = "reader_mode.high_contrast";
I wonder if we should prefix this preference with the component name? So maybe:
dom_distiller.high_contrast

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:5: #ifndef
COMPONENTS_DOM_DISTILLER_CORE_READER_MODE_SETTINGS_H_
COMPONENTS_DOM_DISTILLER_CORE_READER_MODE_PREFERENCES_H_

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:18: class
ReaderModePrefs {
Add a comment which describes what this class is supposed to do.

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:31:
DISALLOW_COPY_AND_ASSIGN(ReaderModePrefs);
Nit: newline before DISALLOW...

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/viewer.cc:34: return "background-color:
#000;\ncolor: #FFF;";  // White on black.
Could you have this as a CSS class on the document body instead, and then pass
in the current class to ReplaceHtmlTemplateValues?

I think that will set you up for success for automatically changing it without
reloading the page later.

[smaslo, 2014-06-26 20:05:38]

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
File components/dom_distiller/DEPS (right):

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/DEPS:2: "+components/pref_registry",
On 2014/06/24 23:08:49, nyquist wrote:
> Nit: sort

Done.

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
File components/dom_distiller/core/css/distilledpage.css (right):

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:12: $1
On 2014/06/24 23:08:49, nyquist wrote:
> This probably has to be rebased.

Done.

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:94: virtual
ReaderModePrefs* GetReaderModePrefs();
On 2014/06/24 23:08:49, nyquist wrote:
> make this pure virtual (add = 0). Also, could you add a comment for this?

Done.

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service_unittest.cc (right):

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service_unittest.cc:94:
scoped_ptr<ReaderModePrefs>(new ReaderModePrefs(NULL))));
On 2014/06/24 23:08:49, nyquist wrote:
> Could you just pass an empty scoped_ptr here?

Done.

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:12: // True if user
prefers high-contrast (dark) appearance when in reader mode.
On 2014/06/24 23:08:49, nyquist wrote:
> s/True if user prefers/Whether user prefers/

Changed entirely

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:13: const char
kHighContrastPref[] = "reader_mode.high_contrast";
On 2014/06/24 23:08:49, nyquist wrote:
> I wonder if we should prefix this preference with the component name? So
maybe:
> dom_distiller.high_contrast

Done.

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:5: #ifndef
COMPONENTS_DOM_DISTILLER_CORE_READER_MODE_SETTINGS_H_
On 2014/06/24 23:08:49, nyquist wrote:
> COMPONENTS_DOM_DISTILLER_CORE_READER_MODE_PREFERENCES_H_

Done.

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:18: class
ReaderModePrefs {
On 2014/06/24 23:08:49, nyquist wrote:
> Add a comment which describes what this class is supposed to do.

Done.

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:31:
DISALLOW_COPY_AND_ASSIGN(ReaderModePrefs);
On 2014/06/24 23:08:49, nyquist wrote:
> Nit: newline before DISALLOW...

Done.

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/180001/components/dom_distille...
components/dom_distiller/core/viewer.cc:34: return "background-color:
#000;\ncolor: #FFF;";  // White on black.
On 2014/06/24 23:08:49, nyquist wrote:
> Could you have this as a CSS class on the document body instead, and then pass
> in the current class to ReplaceHtmlTemplateValues?
> 
> I think that will set you up for success for automatically changing it without
> reloading the page later.

Done.

[nyquist, 2014-06-26 20:39:59]

I must've started the review before you sent your comments. Sorry about that.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:47: //
ViewRequestDelegate implementation.
// ViewRequestDelegate implementation:

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:56: //
WebContentsObserver:
// content::WebContentsObserver implementation:

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:68: //
ReaderModePrefs::Observer
// ReaderModePrefs::Observer implementation:

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:69: virtual void
OnChangeTheme(ReaderModePrefs* reader_mode_prefs) OVERRIDE;
Also, is it possible to make all of these interface implementations private?

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:71: void
SetBodyClassName();
How about SendBodyClassName()? And could it take the theme as an argument? Also,
could this method be private?

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:111:
ReaderModePrefs* reader_mode_prefs_;
Add a comment. For example, what can we get from this member that we use?

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:128:
reader_mode_prefs->AddObserver(this);
Nit: Use reader_mode_prefs_

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:224:
SendJavaScript("alert(0);");
You probably don't want this.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:346: void
DomDistillerViewerSource::RequestViewerHandle::OnChangeTheme(
Could you move these methods up to the rest of the methods for
DomDistillerViewerSource::RequestViewerHandle?

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:352: std::string
s = "document.body.className='";
Could you maybe use std::ostringstream?

std::ostringstream js;
js << "document.body.className='" << reader_mode_prefs_->GetBodyCssClass() <<
"';";
SendJavaScript(js.str());

If the middle line is too long, maybe just store the body CSS class as a
variable.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.h (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
It seems like this file contains only stray changes. Please revert back to
original.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:12: #include
"components/dom_distiller/core/reader_mode_preferences.h"
Remove.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:24: : public
content::URLDataSource {
Could this be on one line?

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:46: 
Is this needed?

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/core/css/distilledpage.css (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:74: /* Base typography. */
I don't think this is rebased correctly. It seems like your CL is deleting all
the new CSS code.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:83: .normal {
How about .light or .day?
And then correspondingly .dark or .night?

My concern is that very-dark-grey-on-white is also kind of high contrast in a
sense.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:94: // Returns the
ReaderModePrefs owned by the instance of DomDistillerService
Add period at the end of the comment.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:16: // Css class names
- must agree with css/distilledpage.css.
s/Css/CSS/

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:17: const char
kHighContrastClassName[] = "highcontrast";
Could these move to the viewer.cc instead?

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:58: if(GetThemePref()
== (int) Theme::kHighContrast) {
Are these casts necessary?

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:73: }  // namespace
dom_distiller
Nit: Is this file missing a newline at the end now?

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:19: // Interface for
preferences used in reader mode view
Nit: All comments like this should end with a period.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:23: // Possible themes
for reader mode
Nit: end with period

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:25: kNormal,
See comment in CSS about naming. Make sure these two enum values are also
updated.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:36: class Observer {
I think the observer class usually goes at the top.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:38: virtual void
OnChangeTheme(ReaderModePrefs* reader_mode_prefs) = 0;
Do you need the pointer to the ReaderModePrefs? Would it be better to help the
observer by passing in the new theme?

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:44: const char*
GetBodyCssClass();
Could this method make take in a theme as an argument, and then move this method
to the viewer.[cc|h]?
Can this be const?

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:45: Theme
GetThemePref();
Can this return const and also be const itself?

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/viewer.cc:16: #include
"components/dom_distiller/core/reader_mode_preferences.h"
Remove this.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/viewer.cc:132: return
ResourceBundle::GetSharedInstance().GetRawDataResource(
Even though this is not wrong, I'd just revert this whole file, since you have
no other changes in it.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/core/viewer.h (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/viewer.h:20: class ReaderModePrefs;
I don't think this is needed anymore.

[smaslo, 2014-06-27 18:33:18]

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:47: //
ViewRequestDelegate implementation.
On 2014/06/26 20:39:57, nyquist wrote:
> // ViewRequestDelegate implementation:

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:56: //
WebContentsObserver:
On 2014/06/26 20:39:57, nyquist wrote:
> // content::WebContentsObserver implementation:

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:68: //
ReaderModePrefs::Observer
On 2014/06/26 20:39:58, nyquist wrote:
> // ReaderModePrefs::Observer implementation:

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:69: virtual void
OnChangeTheme(ReaderModePrefs* reader_mode_prefs) OVERRIDE;
On 2014/06/26 20:39:58, nyquist wrote:
> Also, is it possible to make all of these interface implementations private?

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:71: void
SetBodyClassName();
On 2014/06/26 20:39:57, nyquist wrote:
> How about SendBodyClassName()? And could it take the theme as an argument?
Also,
> could this method be private?

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:111:
ReaderModePrefs* reader_mode_prefs_;
On 2014/06/26 20:39:57, nyquist wrote:
> Add a comment. For example, what can we get from this member that we use?

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:128:
reader_mode_prefs->AddObserver(this);
On 2014/06/26 20:39:57, nyquist wrote:
> Nit: Use reader_mode_prefs_

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:224:
SendJavaScript("alert(0);");
On 2014/06/26 20:39:58, nyquist wrote:
> You probably don't want this.

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:346: void
DomDistillerViewerSource::RequestViewerHandle::OnChangeTheme(
On 2014/06/26 20:39:57, nyquist wrote:
> Could you move these methods up to the rest of the methods for
> DomDistillerViewerSource::RequestViewerHandle?

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:352: std::string
s = "document.body.className='";
On 2014/06/26 20:39:57, nyquist wrote:
> Could you maybe use std::ostringstream?
> 
> std::ostringstream js;
> js << "document.body.className='" << reader_mode_prefs_->GetBodyCssClass() <<
> "';";
> SendJavaScript(js.str());
> 
> If the middle line is too long, maybe just store the body CSS class as a
> variable.

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.h (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2014/06/26 20:39:58, nyquist wrote:
> It seems like this file contains only stray changes. Please revert back to
> original.

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:12: #include
"components/dom_distiller/core/reader_mode_preferences.h"
On 2014/06/26 20:39:58, nyquist wrote:
> Remove.

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:24: : public
content::URLDataSource {
On 2014/06/26 20:39:58, nyquist wrote:
> Could this be on one line?

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:46: 
On 2014/06/26 20:39:58, nyquist wrote:
> Is this needed?

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/core/css/distilledpage.css (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:74: /* Base typography. */
I copied the file from master and added then readded my changes so the CSS.

On 2014/06/26 20:39:58, nyquist wrote:
> I don't think this is rebased correctly. It seems like your CL is deleting all
> the new CSS code.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:83: .normal {
On 2014/06/26 20:39:58, nyquist wrote:
> How about .light or .day?
> And then correspondingly .dark or .night?
> 
> My concern is that very-dark-grey-on-white is also kind of high contrast in a
> sense.

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:94: // Returns the
ReaderModePrefs owned by the instance of DomDistillerService
On 2014/06/26 20:39:58, nyquist wrote:
> Add period at the end of the comment.

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:16: // Css class names
- must agree with css/distilledpage.css.
On 2014/06/26 20:39:58, nyquist wrote:
> s/Css/CSS/

I wasn't sure exactly what you meant by this comment.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:17: const char
kHighContrastClassName[] = "highcontrast";
On 2014/06/26 20:39:58, nyquist wrote:
> Could these move to the viewer.cc instead?

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:58: if(GetThemePref()
== (int) Theme::kHighContrast) {
On 2014/06/26 20:39:58, nyquist wrote:
> Are these casts necessary?

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:73: }  // namespace
dom_distiller
On 2014/06/26 20:39:58, nyquist wrote:
> Nit: Is this file missing a newline at the end now?

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:19: // Interface for
preferences used in reader mode view
On 2014/06/26 20:39:59, nyquist wrote:
> Nit: All comments like this should end with a period.

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:23: // Possible themes
for reader mode
On 2014/06/26 20:39:59, nyquist wrote:
> Nit: end with period

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:25: kNormal,
On 2014/06/26 20:39:58, nyquist wrote:
> See comment in CSS about naming. Make sure these two enum values are also
> updated.

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:36: class Observer {
On 2014/06/26 20:39:58, nyquist wrote:
> I think the observer class usually goes at the top.

I put it below the enum because it needs to know about the enum.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:38: virtual void
OnChangeTheme(ReaderModePrefs* reader_mode_prefs) = 0;
On 2014/06/26 20:39:58, nyquist wrote:
> Do you need the pointer to the ReaderModePrefs? Would it be better to help the
> observer by passing in the new theme?

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:44: const char*
GetBodyCssClass();
On 2014/06/26 20:39:59, nyquist wrote:
> Could this method make take in a theme as an argument, and then move this
method
> to the viewer.[cc|h]?
> Can this be const?

I made it a static method rather than a const method.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:45: Theme
GetThemePref();
On 2014/06/26 20:39:58, nyquist wrote:
> Can this return const and also be const itself?

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/viewer.cc:16: #include
"components/dom_distiller/core/reader_mode_preferences.h"
On 2014/06/26 20:39:59, nyquist wrote:
> Remove this.

Now needed for enum declaration.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/viewer.cc:132: return
ResourceBundle::GetSharedInstance().GetRawDataResource(
On 2014/06/26 20:39:59, nyquist wrote:
> Even though this is not wrong, I'd just revert this whole file, since you have
> no other changes in it.

Done.

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
File components/dom_distiller/core/viewer.h (right):

https://codereview.chromium.org/341563002/diff/200001/components/dom_distille...
components/dom_distiller/core/viewer.h:20: class ReaderModePrefs;
On 2014/06/26 20:39:59, nyquist wrote:
> I don't think this is needed anymore.

I added the header file because I need the enum declaration.

[nyquist, 2014-06-27 20:48:27]

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:69: const
ReaderModePrefs* GetReaderModePrefs();
Remove.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:84: // Sets CSS
class name for the webpage's HTML body element.
Remove

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:258:
ReaderModePrefs::Theme theme) {
Nit: new_theme

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:259:
SetBodyClassName();
This is the only place that method is used, so just inline it here.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:262: void
DomDistillerViewerSource::RequestViewerHandle::SetBodyClassName() {
Require theme as argument:
ReaderModePrefs::Theme theme
Then use that in the string stream

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:269: const
ReaderModePrefs* DomDistillerViewerSource::RequestViewerHandle::
Remove.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:334: // The
service did not return a |ViewerHandle|, which means the
Just get it from the service like you do above. That way you don't have to add
the accessor method.
Then move this code down to where it was before.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.h (right):

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:44: private:
Maybe you could remove this diff?

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:21: public:
But: unnecessary newline I think?

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:25: kLight,
Awesome!

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:36: void
RemoveObserver(Observer* obs);
Could you put these below the static register method? It's nice to group them
like this though.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:42: void
SetThemePref(Theme new_theme);
Just SetTheme

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:44: const Theme
GetThemePref() const;
Just GetTheme

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:48:
ObserverList<Observer> observer_list_;
Could this just be 'observers_'?

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:50: void
NotifyOnChangeTheme(Theme theme);
What does this method do? (Add comment)

Also, put above private members

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/viewer.cc:188: const char*
GetBodyCssClass(ReaderModePrefs::Theme theme) {
I guess this could be added a unittest for?

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/viewer.cc:194: return kLightClassName;
Remove the else block and just return kLightClassName unindented.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
File components/dom_distiller/core/viewer.h (right):

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/viewer.h:14: #include
"components/dom_distiller/core/reader_mode_preferences.h"
Can you instead forward declare this in the header and move the include to the
.cc file?

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/viewer.h:72: const char*
GetBodyCssClass(ReaderModePrefs::Theme theme);
Could this return std::string? Also can this be const? I don't think it changes
anything (add const before the ; )

[smaslo, 2014-06-30 17:57:15]

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:69: const
ReaderModePrefs* GetReaderModePrefs();
On 2014/06/27 20:48:26, nyquist wrote:
> Remove.

It removed it, but I had to make the reader_mode_prefs_ variable public so that
the outer class DomDistillerViewerSource has access to it.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:84: // Sets CSS
class name for the webpage's HTML body element.
On 2014/06/27 20:48:26, nyquist wrote:
> Remove

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:258:
ReaderModePrefs::Theme theme) {
On 2014/06/27 20:48:26, nyquist wrote:
> Nit: new_theme

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:259:
SetBodyClassName();
On 2014/06/27 20:48:26, nyquist wrote:
> This is the only place that method is used, so just inline it here.

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:262: void
DomDistillerViewerSource::RequestViewerHandle::SetBodyClassName() {
On 2014/06/27 20:48:26, nyquist wrote:
> Require theme as argument:
> ReaderModePrefs::Theme theme
> Then use that in the string stream

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:269: const
ReaderModePrefs* DomDistillerViewerSource::RequestViewerHandle::
On 2014/06/27 20:48:26, nyquist wrote:
> Remove.

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:334: // The
service did not return a |ViewerHandle|, which means the
On 2014/06/27 20:48:26, nyquist wrote:
> Just get it from the service like you do above. That way you don't have to add
> the accessor method.
> Then move this code down to where it was before. 

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.h (right):

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:44: private:
On 2014/06/27 20:48:26, nyquist wrote:
> Maybe you could remove this diff?

I checked it from master again, so it should have none of my changes.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:21: public:
On 2014/06/27 20:48:26, nyquist wrote:
> But: unnecessary newline I think?

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:25: kLight,
On 2014/06/27 20:48:26, nyquist wrote:
> Awesome!

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:36: void
RemoveObserver(Observer* obs);
On 2014/06/27 20:48:26, nyquist wrote:
> Could you put these below the static register method? It's nice to group them
> like this though. 

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:42: void
SetThemePref(Theme new_theme);
On 2014/06/27 20:48:26, nyquist wrote:
> Just SetTheme

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:44: const Theme
GetThemePref() const;
On 2014/06/27 20:48:26, nyquist wrote:
> Just GetTheme

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:48:
ObserverList<Observer> observer_list_;
On 2014/06/27 20:48:26, nyquist wrote:
> Could this just be 'observers_'?

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:50: void
NotifyOnChangeTheme(Theme theme);
On 2014/06/27 20:48:26, nyquist wrote:
> What does this method do? (Add comment)
> 
> Also, put above private members

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/viewer.cc:188: const char*
GetBodyCssClass(ReaderModePrefs::Theme theme) {
On 2014/06/27 20:48:27, nyquist wrote:
> I guess this could be added a unittest for?

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/viewer.cc:194: return kLightClassName;
On 2014/06/27 20:48:27, nyquist wrote:
> Remove the else block and just return kLightClassName unindented.

Done.

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
File components/dom_distiller/core/viewer.h (right):

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/viewer.h:14: #include
"components/dom_distiller/core/reader_mode_preferences.h"
The compiler doesn't allow forward declarations of enums and I need the Theme
enum.

On 2014/06/27 20:48:27, nyquist wrote:
> Can you instead forward declare this in the header and move the include to the
> .cc file?

https://codereview.chromium.org/341563002/diff/280001/components/dom_distille...
components/dom_distiller/core/viewer.h:72: const char*
GetBodyCssClass(ReaderModePrefs::Theme theme);
On 2014/06/27 20:48:27, nyquist wrote:
> Could this return std::string? Also can this be const? I don't think it
changes
> anything (add const before the ; )

Made std::string, but couldn't make it const.  I tried to make it const but got
a compiler error since it is not a member function.

[nyquist, 2014-06-30 21:35:41]

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:253:
std::ostringstream js;
Create a method in the viewer for Get...Js().
The python method would take in a string representation of the theme, and do the
translation to the CSS class name (they would happen to have the same value, but
I'd prefer if you'd specify the CSS class name as a normal string).

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
File components/dom_distiller/core/css/distilledpage.css (right):

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:85: /* Classes for light,
dark and sepia. */
Maybe add the concept of "theme" to this comment?

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:43: void
ReaderModePrefs::SetTheme(Theme new_theme) {
Could you add a test for this, ensuring that values are set correctly, and that
the observers are also called?

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:45:
NotifyOnChangeTheme(new_theme);
This should be called in a new task.
MessageLoop::current()::PostTask(...)

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:20: class
ReaderModePrefs {
Rename this across the board to DistilledPagePrefs, including in the change
description.

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/core/viewer.cc:32: // CSS class names - must agree with
css/distilledpage.css.
Refer to JavaScript function instead.

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/core/viewer.cc:33: const char kDarkClassName[] =
"dark";
s/ClassName/JsTheme/ across these. Example. kDarkJsTheme.

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/core/viewer.cc:188: const std::string
GetBodyCssClass(ReaderModePrefs::Theme theme) {
Move to anonymous namespace and rename to 'GetJSTheme'

[smaslo, 2014-07-07 17:14:15]

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:253:
std::ostringstream js;
I'm not sure what you are talking about in your second paragraph.

On 2014/06/30 21:35:40, nyquist wrote:
> Create a method in the viewer for Get...Js().
> The python method would take in a string representation of the theme, and do
the
> translation to the CSS class name (they would happen to have the same value,
but
> I'd prefer if you'd specify the CSS class name as a normal string).

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
File components/dom_distiller/core/css/distilledpage.css (right):

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:85: /* Classes for light,
dark and sepia. */
On 2014/06/30 21:35:41, nyquist wrote:
> Maybe add the concept of "theme" to this comment?

Done.

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.cc (right):

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:43: void
ReaderModePrefs::SetTheme(Theme new_theme) {
On 2014/06/30 21:35:41, nyquist wrote:
> Could you add a test for this, ensuring that values are set correctly, and
that
> the observers are also called?

Done.

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.cc:45:
NotifyOnChangeTheme(new_theme);
On 2014/06/30 21:35:41, nyquist wrote:
> This should be called in a new task.
> MessageLoop::current()::PostTask(...)

Done.

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
File components/dom_distiller/core/reader_mode_preferences.h (right):

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/core/reader_mode_preferences.h:20: class
ReaderModePrefs {
I removed it from the code and the issue description.  Is the issue description
the same thing as the change description?  I could not get rid of it from the
patches, though - you still see reader_mode_preferences.cc and
reader_mode_preferences.h.

On 2014/06/30 21:35:41, nyquist wrote:
> Rename this across the board to DistilledPagePrefs, including in the change
> description.

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/core/viewer.cc:32: // CSS class names - must agree with
css/distilledpage.css.
On 2014/06/30 21:35:41, nyquist wrote:
> Refer to JavaScript function instead.

Done.

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/core/viewer.cc:33: const char kDarkClassName[] =
"dark";
On 2014/06/30 21:35:41, nyquist wrote:
> s/ClassName/JsTheme/ across these. Example. kDarkJsTheme.

Done.

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/core/viewer.cc:188: const std::string
GetBodyCssClass(ReaderModePrefs::Theme theme) {
On 2014/06/30 21:35:41, nyquist wrote:
> Move to anonymous namespace and rename to 'GetJSTheme'

Done.

[smaslo, 2014-07-07 17:15:43]

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/341563002/diff/300001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:253:
std::ostringstream js;
On 2014/07/07 17:14:14, smaslo wrote:
> I'm not sure what you are talking about in your second paragraph.
> 
> On 2014/06/30 21:35:40, nyquist wrote:
> > Create a method in the viewer for Get...Js().
> > The python method would take in a string representation of the theme, and do
> the
> > translation to the CSS class name (they would happen to have the same value,
> but
> > I'd prefer if you'd specify the CSS class name as a normal string).
> 

Done.

[nyquist, 2014-07-07 19:12:49]

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
File components/dom_distiller/core/css/distilledpage.css (right):

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:86: * Must agree with
classes returned by GetJsTheme(theme) in viewer.cc */
I think you want to refer to the JavaScript here.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:93: color: #FFF;
This is very hard to read for <code><pre> blocks.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:156: color: #222;
This looks almost indistinguishable from the color of the text itself, which
might make normal underlines in the document be hard to separate from links.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:160: color: #3838FF;
This color was hard to read for me.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:164: color: #551A8B;
This is hard to read on the dark background.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs.cc (right):

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:16: // True if user
prefers high-contrast (dark) appearance for distilled page.
This comment needs to be updated.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:59: return (Theme)
pref_service_->GetInteger(kThemePref);
This is an unsafe cast, since the values on disk could have been changed.

I think you must add an extra enum value "THEME_COUNT" or something like that
and then do bounds checking here.
If the bounds check fails, you probably want to reset the pref to the default
value to ensure the bounds checking will work next time.. (just extract out the
Theme::kLight from the registration to an anonymous const and use it here and
during registration).

See
https://code.google.com/p/chromium/codesearch#chromium/src/sync/internal_api/...
for example of a _COUNT style last enum value.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs.h (right):

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.h:40: explicit
DistilledPagePrefs(PrefService* pref_service);
How about this ordering for the public methods?

Constructor
Destructor
static RegisterProfilePrefs
SetTheme
GetTheme
AddObserver
RemoveObserver

For the style guide in ordering, see:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Note: We often try to order methods in the .cc file the same way as in the
header.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.h:43: void SetTheme(Theme
new_theme);
Add comment (What is this used for? Why is it needed?)

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.h:45: const Theme GetTheme()
const;
Add comment.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs_unittests.cc (right):

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:14: namespace {
I think you can end this namespace after the observer definition (i.e. just
before you declare the DistilledPagePrefsTest class).

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:16: void
RunLoop() {
Could you inline this to base::RunLoop().RunUntilIdle() ?

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:22: : public
DistilledPagePrefs::Observer {
Nit: Does this really need to be on its own line?

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:24: virtual void
OnChangeTheme(DistilledPagePrefs::Theme) OVERRIDE;
I think this method and GetTheme can probably just be defined inline here.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:25:
DistilledPagePrefs::Theme GetTheme();
How about adding using DistilledPagePrefs::Observer given this is a unit test?

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:28:
DistilledPagePrefs::Theme theme;
theme_

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:31: void
TestingObserver::OnChangeTheme(
same line after adding 'using'?

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:59: RunLoop();
Add an EXPECT_EQ("", ...) before the call to RunLoop() to ensure that the task
was posted correctly. This requires the implementation to always post a task
instead of just notifying the observers in the SetTheme method.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:66:
TEST_F(DistilledPagePrefsTest, TestingMultipleObservers) {
This is not currently testing multiple observers, but I agree that it should.
See comment below.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:69:
distilled_page_prefs_->SetTheme(DistilledPagePrefs::Theme::kSepia);
This has already been tested in the method above. How about adding two observers
first, setting theme, removing one observer, and calling SetTheme again? You
naturally need to add the appropriate EXPECT_EQ calls to ensure the state is as
expected.

Since the test will then be a bit more convoluted, you might want to add a
comment or two. For example that in the last EXPECT_EQ series on the two
observers, only one observer should have been updated to the new theme or
something along those lines.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/viewer.cc:201: js << "document.body.className='"
<<
Instead of doing this, could you instead call a JavaScript method here? See
GetToggleLoadingIndicatorJs above.

I think you could call something like:

useTheme('INSERT_THEME_HERE');

And then you'd map the kDarkJsTheme etc. to their corresponding CSS classes in
the .js file. The CSS file can stay as is (except the comment about what it has
to keep in sync with).

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
File components/dom_distiller/core/viewer.h (right):

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/viewer.h:72: // Returns JavaScript corresponding
to a specific theme.
"corresponding to setting a spe..."?

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
File components/dom_distiller/core/viewer_unittest.cc (right):

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/viewer_unittest.cc:139: std::string kDarkJs =
"document.body.className='dark';";
Nit: These needs to be updated once you call the JS method instead.

[smaslo, 2014-07-08 19:58:12]

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
File components/dom_distiller/core/css/distilledpage.css (right):

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:86: * Must agree with
classes returned by GetJsTheme(theme) in viewer.cc */
On 2014/07/07 19:12:48, nyquist wrote:
> I think you want to refer to the JavaScript here.

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:93: color: #FFF;
On 2014/07/07 19:12:48, nyquist wrote:
> This is very hard to read for <code><pre> blocks.

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:156: color: #222;
On 2014/07/07 19:12:48, nyquist wrote:
> This looks almost indistinguishable from the color of the text itself, which
> might make normal underlines in the document be hard to separate from links.

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:160: color: #3838FF;
On 2014/07/07 19:12:48, nyquist wrote:
> This color was hard to read for me.

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:164: color: #551A8B;
On 2014/07/07 19:12:48, nyquist wrote:
> This is hard to read on the dark background.

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs.cc (right):

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:16: // True if user
prefers high-contrast (dark) appearance for distilled page.
On 2014/07/07 19:12:48, nyquist wrote:
> This comment needs to be updated.

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:59: return (Theme)
pref_service_->GetInteger(kThemePref);
Done.  I also added a check to SetTheme() to prevent a wrong value from being
stored and passed through NotifyOnChangeTheme().  I had to remove the const
specifier since the function will now change values.

On 2014/07/07 19:12:48, nyquist wrote:
> This is an unsafe cast, since the values on disk could have been changed.
> 
> I think you must add an extra enum value "THEME_COUNT" or something like that
> and then do bounds checking here.
> If the bounds check fails, you probably want to reset the pref to the default
> value to ensure the bounds checking will work next time.. (just extract out
the
> Theme::kLight from the registration to an anonymous const and use it here and
> during registration).
> 
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/sync/internal_api/...
> for example of a _COUNT style last enum value.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs.h (right):

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.h:40: explicit
DistilledPagePrefs(PrefService* pref_service);
On 2014/07/07 19:12:48, nyquist wrote:
> How about this ordering for the public methods?
> 
> Constructor
> Destructor
> static RegisterProfilePrefs
> SetTheme
> GetTheme
> AddObserver
> RemoveObserver
> 
> For the style guide in ordering, see:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> 
> Note: We often try to order methods in the .cc file the same way as in the
> header.

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.h:43: void SetTheme(Theme
new_theme);
On 2014/07/07 19:12:48, nyquist wrote:
> Add comment (What is this used for? Why is it needed?)

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.h:45: const Theme GetTheme()
const;
On 2014/07/07 19:12:48, nyquist wrote:
> Add comment.

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs_unittests.cc (right):

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:14: namespace {
On 2014/07/07 19:12:49, nyquist wrote:
> I think you can end this namespace after the observer definition (i.e. just
> before you declare the DistilledPagePrefsTest class).

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:16: void
RunLoop() {
On 2014/07/07 19:12:49, nyquist wrote:
> Could you inline this to base::RunLoop().RunUntilIdle() ?

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:22: : public
DistilledPagePrefs::Observer {
On 2014/07/07 19:12:49, nyquist wrote:
> Nit: Does this really need to be on its own line?

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:24: virtual void
OnChangeTheme(DistilledPagePrefs::Theme) OVERRIDE;
On 2014/07/07 19:12:48, nyquist wrote:
> I think this method and GetTheme can probably just be defined inline here.

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:25:
DistilledPagePrefs::Theme GetTheme();
Could not add "using DistilledPagePrefs::Theme;" because of compiler error
message: "using declaration cannot refer to class member".
On 2014/07/07 19:12:49, nyquist wrote:
> How about adding using DistilledPagePrefs::Observer given this is a unit test?

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:28:
DistilledPagePrefs::Theme theme;
On 2014/07/07 19:12:48, nyquist wrote:
> theme_

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:31: void
TestingObserver::OnChangeTheme(
On 2014/07/07 19:12:48, nyquist wrote:
> same line after adding 'using'?

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:59: RunLoop();
On 2014/07/07 19:12:49, nyquist wrote:
> Add an EXPECT_EQ("", ...) before the call to RunLoop() to ensure that the task
> was posted correctly. This requires the implementation to always post a task
> instead of just notifying the observers in the SetTheme method.

Done. Constructor for TestingObserver now initializes theme_ to kLight.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:66:
TEST_F(DistilledPagePrefsTest, TestingMultipleObservers) {
On 2014/07/07 19:12:48, nyquist wrote:
> This is not currently testing multiple observers, but I agree that it should.
> See comment below.

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:69:
distilled_page_prefs_->SetTheme(DistilledPagePrefs::Theme::kSepia);
On 2014/07/07 19:12:49, nyquist wrote:
> This has already been tested in the method above. How about adding two
observers
> first, setting theme, removing one observer, and calling SetTheme again? You
> naturally need to add the appropriate EXPECT_EQ calls to ensure the state is
as
> expected.
> 
> Since the test will then be a bit more convoluted, you might want to add a
> comment or two. For example that in the last EXPECT_EQ series on the two
> observers, only one observer should have been updated to the new theme or
> something along those lines.

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/viewer.cc:201: js << "document.body.className='"
<<
On 2014/07/07 19:12:49, nyquist wrote:
> Instead of doing this, could you instead call a JavaScript method here? See
> GetToggleLoadingIndicatorJs above.
> 
> I think you could call something like:
> 
> useTheme('INSERT_THEME_HERE');
> 
> And then you'd map the kDarkJsTheme etc. to their corresponding CSS classes in
> the .js file. The CSS file can stay as is (except the comment about what it
has
> to keep in sync with).

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
File components/dom_distiller/core/viewer.h (right):

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/viewer.h:72: // Returns JavaScript corresponding
to a specific theme.
On 2014/07/07 19:12:49, nyquist wrote:
> "corresponding to setting a spe..."?

Done.

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
File components/dom_distiller/core/viewer_unittest.cc (right):

https://codereview.chromium.org/341563002/diff/320001/components/dom_distille...
components/dom_distiller/core/viewer_unittest.cc:139: std::string kDarkJs =
"document.body.className='dark';";
On 2014/07/07 19:12:49, nyquist wrote:
> Nit: These needs to be updated once you call the JS method instead.

Done.

[nyquist, 2014-07-08 22:26:48]

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs.cc (right):

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:40: if (new_theme >=
Theme::kThemeCount) {
When a parameter is already an enum you don't have to bounds check it, so this
last edit should be unnecessary.

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:50: if
(pref_service_->GetInteger(kThemePref) >= Theme::kThemeCount) {
You also need to check for negative values. How about read the value into an
int, and then do bounds checking on it?
foobar >= 0 && foobar < THEME_COUNT

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:51:
SetTheme(Theme::kLight);
I think you could:
1) read to local variable
2) bounds check
  - if OOB, do like you do now, and then just return Theme::kLight inside the
if.
3) return a cast of the local variable.

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:53: return (Theme)
pref_service_->GetInteger(kThemePref);
I don't think you want to read from disk again here (in case someone is wrong
with reading files)

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs.h (right):

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.h:25: kLight,
enum values should be all capital and without the k. So LIGHT, DARK, SEPIA,
THEME_COUNT. Since they are persisted to disk I'd add = 0 on the first (LIGHT)
one too.

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
components/dom_distiller/core/viewer.cc:32: // JS Themes. Must agree with
useTheme() in dom_distiller_viewer.js
Nit: "JS themes" and add a period at the end of the comment.

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
components/dom_distiller/core/viewer.cc:214: std::string str = "useTheme('" +
GetJsTheme(theme) + "');";
inline the return

[smaslo, 2014-07-09 19:42:26]

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs.cc (right):

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:40: if (new_theme >=
Theme::kThemeCount) {
On 2014/07/08 22:26:47, nyquist wrote:
> When a parameter is already an enum you don't have to bounds check it, so this
> last edit should be unnecessary.

Done.

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:50: if
(pref_service_->GetInteger(kThemePref) >= Theme::kThemeCount) {
On 2014/07/08 22:26:48, nyquist wrote:
> You also need to check for negative values. How about read the value into an
> int, and then do bounds checking on it?
> foobar >= 0 && foobar < THEME_COUNT

Done.

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:51:
SetTheme(Theme::kLight);
On 2014/07/08 22:26:48, nyquist wrote:
> I think you could:
> 1) read to local variable
> 2) bounds check
>   - if OOB, do like you do now, and then just return Theme::kLight inside the
> if.
> 3) return a cast of the local variable.

Done.

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:53: return (Theme)
pref_service_->GetInteger(kThemePref);
On 2014/07/08 22:26:48, nyquist wrote:
> I don't think you want to read from disk again here (in case someone is wrong
> with reading files)

Done.

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs.h (right):

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.h:25: kLight,
On 2014/07/08 22:26:48, nyquist wrote:
> enum values should be all capital and without the k. So LIGHT, DARK, SEPIA,
> THEME_COUNT. Since they are persisted to disk I'd add = 0 on the first (LIGHT)
> one too.

Done.

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
components/dom_distiller/core/viewer.cc:32: // JS Themes. Must agree with
useTheme() in dom_distiller_viewer.js
On 2014/07/08 22:26:48, nyquist wrote:
> Nit: "JS themes" and add a period at the end of the comment.

Done.

https://codereview.chromium.org/341563002/diff/360001/components/dom_distille...
components/dom_distiller/core/viewer.cc:214: std::string str = "useTheme('" +
GetJsTheme(theme) + "');";
On 2014/07/08 22:26:48, nyquist wrote:
> inline the return

Done.

[nyquist, 2014-07-09 20:38:32]

There are also other compile errors, please look at the tryjob logs.

https://codereview.chromium.org/341563002/diff/420001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/dom_distiller_service_factory.h (right):

https://codereview.chromium.org/341563002/diff/420001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/dom_distiller_service_factory.h:28:
scoped_ptr<DistilledPagePrefs> distilled_page_prefs);
This requires a change in
//chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:251: void
DomDistillerViewerSource::RequestViewerHandle::OnChangeTheme(
It should be possible to test this in
//chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc

I think you can test both the initial state and the changes that happens while
the distilled page is showing.

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs.cc (right):

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:50:
SetTheme(Theme::LIGHT);
Nit: Add a comment saying something like "persisted data was incorrectly read,
so try to clean up by storing the default."

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs.h (right):

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.h:44: const Theme GetTheme();
this const has no effect

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:111:
scoped_ptr<DistilledPagePrefs> distilled_page_prefs);
This requires a change to
//components/dom_distiller/standalone/content_extractor.cc

[smaslo, 2014-07-10 17:01:06]

Still working on try-bot errors.

https://codereview.chromium.org/341563002/diff/420001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/dom_distiller_service_factory.h (right):

https://codereview.chromium.org/341563002/diff/420001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/dom_distiller_service_factory.h:28:
scoped_ptr<DistilledPagePrefs> distilled_page_prefs);
On 2014/07/09 20:38:32, nyquist wrote:
> This requires a change in
> //chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc

Done.

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:251: void
DomDistillerViewerSource::RequestViewerHandle::OnChangeTheme(
On 2014/07/09 20:38:32, nyquist wrote:
> It should be possible to test this in
> //chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc
> 
> I think you can test both the initial state and the changes that happens while
> the distilled page is showing.

Done.

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs.cc (right):

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:50:
SetTheme(Theme::LIGHT);
On 2014/07/09 20:38:32, nyquist wrote:
> Nit: Add a comment saying something like "persisted data was incorrectly read,
> so try to clean up by storing the default."

Done.

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs.h (right):

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.h:44: const Theme GetTheme();
On 2014/07/09 20:38:32, nyquist wrote:
> this const has no effect

Done.

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/341563002/diff/420001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:111:
scoped_ptr<DistilledPagePrefs> distilled_page_prefs);
Does this instance need to have a real version or can I pass in a NULL pointer? 
"content_extractor.cc" implies that it will only extract the content and nothing
else.  Also, this class currently has no access to the PrefService - we would
need to inject it in.

On 2014/07/09 20:38:32, nyquist wrote:
> This requires a change to
> //components/dom_distiller/standalone/content_extractor.cc

[nyquist, 2014-07-11 01:41:05]

https://codereview.chromium.org/341563002/diff/580001/components/dom_distille...
File components/dom_distiller/standalone/content_extractor.cc (right):

https://codereview.chromium.org/341563002/diff/580001/components/dom_distille...
components/dom_distiller/standalone/content_extractor.cc:105:
scoped_ptr<DistilledPagePrefs>()));
I think this needs a preference. The content extractor distills web pages, so
with the prefs being NULL, it would fail to get them when it needs to set the
initial CSS class of the HTML.

[smaslo, 2014-07-11 22:37:45]

asargent:  
Please provide owner approval for 
chrome/browser/extensions/api/reading_list_private/reading_list_private_apitest.cc

blundell:
Please provide owner approval for 
components/pref_registry.gypi

TestingPrefServiceSyncable is dependent on TestingPrefStore 
but dependency was not included previously.

https://codereview.chromium.org/341563002/diff/580001/components/dom_distille...
File components/dom_distiller/standalone/content_extractor.cc (right):

https://codereview.chromium.org/341563002/diff/580001/components/dom_distille...
components/dom_distiller/standalone/content_extractor.cc:105:
scoped_ptr<DistilledPagePrefs>()));
On 2014/07/11 01:41:05, nyquist wrote:
> I think this needs a preference. The content extractor distills web pages, so
> with the prefs being NULL, it would fail to get them when it needs to set the
> initial CSS class of the HTML.

Done.

[nyquist, 2014-07-11 23:20:06]

https://codereview.chromium.org/341563002/diff/640001/components/pref_registr...
File components/pref_registry.gypi (right):

https://codereview.chromium.org/341563002/diff/640001/components/pref_registr...
components/pref_registry.gypi:32: '../base/base.gyp:base_prefs_test_support',
Local dependencies go first.

[smaslo, 2014-07-11 23:26:40]

https://codereview.chromium.org/341563002/diff/640001/components/pref_registr...
File components/pref_registry.gypi (right):

https://codereview.chromium.org/341563002/diff/640001/components/pref_registr...
components/pref_registry.gypi:32: '../base/base.gyp:base_prefs_test_support',
On 2014/07/11 23:20:06, nyquist wrote:
> Local dependencies go first.

Done.

[nyquist, 2014-07-14 17:56:00]

lgtm

https://codereview.chromium.org/341563002/diff/660001/components/components_t...
File components/components_tests.gyp (right):

https://codereview.chromium.org/341563002/diff/660001/components/components_t...
components/components_tests.gyp:736:
'components.gyp:pref_registry_test_support',
Nit: Does this have trailing whitespace?

[asargent_no_longer_on_chrome, 2014-07-14 18:16:27]

chrome/browser/extensions/* lgtm

[blundell, 2014-07-15 06:42:54]

//components/pref_registry.gypi LGTM

[smaslo, 2014-07-15 14:52:39]

The CQ bit was checked by smaslo@chromium.org

[commit-bot: I haz the power, 2014-07-15 14:54:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/smaslo@chromium.org/341563002/660001

[commit-bot: I haz the power, 2014-07-15 21:06:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[commit-bot: I haz the power, 2014-07-16 00:30:18]

Change committed as 283303

[nyquist, 2014-07-18 17:36:59]

https://codereview.chromium.org/341563002/diff/660001/components/dom_distille...
File components/dom_distiller/core/css/distilledpage.css (right):

https://codereview.chromium.org/341563002/diff/660001/components/dom_distille...
components/dom_distiller/core/css/distilledpage.css:246: code {
I don't think we want this block.