Font Family Preferences for Distilled Pages

Font Family Preferences for Distilled Pages

This CL adds backend support for Font Family preferences
within DistilledPagePrefs. The font families supported are
sans-serif (default), serif, and monospace. Serif and
monospace fonts are set by the browser, while sans-serif is
defined as Open Sans, and then the browser default sans-serif
font. 

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

[robliao, 2014-08-10 02:43:46]

https://codereview.chromium.org/430473007/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java
(right):

https://codereview.chromium.org/430473007/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:112:
* TODO(smaslo): add GUI and front end implementation for FontFamily.
This todo should go on the inside of the method..

https://codereview.chromium.org/430473007/diff/120001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java
(right):

https://codereview.chromium.org/430473007/diff/120001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:18:
import org.chromium.content.browser.test.util.UiUtils;
Insert linebreak below.

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller.gypi:181: 'dom_distiller_core_font_family_java',
Alphabetize.

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller.gypi:215: 'target_name':
'dom_distiller_core_font_family_java',
Alphabetize target_name order.

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:193:
viewer::GetUnsafeArticleHtml(article_proto,
Consistency: Wrap these arguments to the next line.

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
File components/dom_distiller/content/resources/dom_distiller_viewer.js (right):

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/content/resources/dom_distiller_viewer.js:18: function
useFontFamily(fontFamily){
Space between ) and {

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/content/resources/dom_distiller_viewer.js:45: // Relies
on the classname order of the body being Theme class, then Font
Any way to do these where we don't rely on order?

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:16: // Path to the
integers corresponding to user's preference font family and
Go ahead and just add a new comment, leaving the old comment the same.

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:57: // Persisted data was
incorrect, trying to clean it up by storing the
When does this happen?

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:101: }
Leave this linebreak in.

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_android.cc:29:
(DistilledPagePrefs::FontFamily)font_family);
Does static_cast work here?

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:26:
DistilledPagePrefs::FontFamily GetFontFamily() { return font_; }
Linebreak above here.

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/viewer.cc:153: title, unsafe_article_html,
"visible", original_url, theme, font_family);
Adding just font_family at the end should be okay. Same as below.

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/viewer_unittest.cc:158:
EXPECT_EQ(kSerifJsFontFamily.compare(viewer::GetDistilledPageFontFamilyJs(
Go ahead and linebreak after the first (.

[sunangel, 2014-08-11 21:51:33]

https://codereview.chromium.org/430473007/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java
(right):

https://codereview.chromium.org/430473007/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:112:
* TODO(smaslo): add GUI and front end implementation for FontFamily.
On 2014/08/10 02:43:44, robliao wrote:
> This todo should go on the inside of the method..

Done.

https://codereview.chromium.org/430473007/diff/120001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java
(right):

https://codereview.chromium.org/430473007/diff/120001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:18:
import org.chromium.content.browser.test.util.UiUtils;
On 2014/08/10 02:43:44, robliao wrote:
> Insert linebreak below.

Done.

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller.gypi:181: 'dom_distiller_core_font_family_java',
On 2014/08/10 02:43:44, robliao wrote:
> Alphabetize.

Done.

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller.gypi:215: 'target_name':
'dom_distiller_core_font_family_java',
On 2014/08/10 02:43:44, robliao wrote:
> Alphabetize target_name order.

Done.

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:193:
viewer::GetUnsafeArticleHtml(article_proto,
Can you explain what you mean? Git cl format turned it into this.
On 2014/08/10 02:43:45, robliao wrote:
> Consistency: Wrap these arguments to the next line.

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
File components/dom_distiller/content/resources/dom_distiller_viewer.js (right):

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/content/resources/dom_distiller_viewer.js:18: function
useFontFamily(fontFamily){
On 2014/08/10 02:43:45, robliao wrote:
> Space between ) and {

Done.

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/content/resources/dom_distiller_viewer.js:18: function
useFontFamily(fontFamily){
On 2014/08/10 02:43:45, robliao wrote:
> Space between ) and {

Done.

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/content/resources/dom_distiller_viewer.js:45: // Relies
on the classname order of the body being Theme class, then Font
explored other options with nyquist@
conclusion: leave as is
On 2014/08/10 02:43:45, robliao wrote:
> Any way to do these where we don't rely on order?

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:16: // Path to the
integers corresponding to user's preference font family and
On 2014/08/10 02:43:45, robliao wrote:
> Go ahead and just add a new comment, leaving the old comment the same.

Done.

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:57: // Persisted data was
incorrect, trying to clean it up by storing the
Tommy said to use this as a safeguard (see Susan's CL for GetTheme, does the
same thing).
On 2014/08/10 02:43:45, robliao wrote:
> When does this happen?

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:101: }
On 2014/08/10 02:43:45, robliao wrote:
> Leave this linebreak in.

Done.

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_android.cc:29:
(DistilledPagePrefs::FontFamily)font_family);
On 2014/08/10 02:43:45, robliao wrote:
> Does static_cast work here?

Done.

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_unittests.cc:26:
DistilledPagePrefs::FontFamily GetFontFamily() { return font_; }
On 2014/08/10 02:43:45, robliao wrote:
> Linebreak above here.

Done.

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/viewer.cc:153: title, unsafe_article_html,
"visible", original_url, theme, font_family);
Done but contradicts git cl format. Also done for below for consistency. 
On 2014/08/10 02:43:45, robliao wrote:
> Adding just font_family at the end should be okay. Same as below.

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/viewer_unittest.cc:158:
EXPECT_EQ(kSerifJsFontFamily.compare(viewer::GetDistilledPageFontFamilyJs(
Done and did it for below for consistency.
On 2014/08/10 02:43:46, robliao wrote:
> Go ahead and linebreak after the first (.

[robliao, 2014-08-11 22:00:45]

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:193:
viewer::GetUnsafeArticleHtml(article_proto,
On 2014/08/11 21:51:33, sunangel wrote:
> Can you explain what you mean? Git cl format turned it into this.
> On 2014/08/10 02:43:45, robliao wrote:
> > Consistency: Wrap these arguments to the next line.
> 

Look at GetUnsafePartialArticleHtml below. Use that style.

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_android.cc:29:
(DistilledPagePrefs::FontFamily)font_family);
On 2014/08/11 21:51:33, sunangel wrote:
> On 2014/08/10 02:43:45, robliao wrote:
> > Does static_cast work here?
> 
> Done.

Cool. Update the casts below as well.

https://codereview.chromium.org/430473007/diff/160001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java
(right):

https://codereview.chromium.org/430473007/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:114:
* TODO(smaslo): add GUI and front end implementation for FontFamily.
A // style comment will suffice.

https://codereview.chromium.org/430473007/diff/160001/components/dom_distille...
File components/dom_distiller.gypi (right):

https://codereview.chromium.org/430473007/diff/160001/components/dom_distille...
components/dom_distiller.gypi:203: 'target_name':
'dom_distiller_core_font_family_java',
This should sort before dom_distiller_core_jni_headers

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

https://codereview.chromium.org/430473007/diff/160001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:62: return
(FontFamily)font_family;
static_cast

[sunangel, 2014-08-12 02:09:56]

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:193:
viewer::GetUnsafeArticleHtml(article_proto,
On 2014/08/11 22:00:45, robliao wrote:
> On 2014/08/11 21:51:33, sunangel wrote:
> > Can you explain what you mean? Git cl format turned it into this.
> > On 2014/08/10 02:43:45, robliao wrote:
> > > Consistency: Wrap these arguments to the next line.
> > 
> 
> Look at GetUnsafePartialArticleHtml below. Use that style.

Done.

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

https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_android.cc:29:
(DistilledPagePrefs::FontFamily)font_family);
On 2014/08/11 22:00:45, robliao wrote:
> On 2014/08/11 21:51:33, sunangel wrote:
> > On 2014/08/10 02:43:45, robliao wrote:
> > > Does static_cast work here?
> > 
> > Done.
> 
> Cool. Update the casts below as well.

Done.

https://codereview.chromium.org/430473007/diff/160001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java
(right):

https://codereview.chromium.org/430473007/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:114:
* TODO(smaslo): add GUI and front end implementation for FontFamily.
On 2014/08/11 22:00:45, robliao wrote:
> A // style comment will suffice.

Done.

https://codereview.chromium.org/430473007/diff/160001/components/dom_distille...
File components/dom_distiller.gypi (right):

https://codereview.chromium.org/430473007/diff/160001/components/dom_distille...
components/dom_distiller.gypi:203: 'target_name':
'dom_distiller_core_font_family_java',
On 2014/08/11 22:00:45, robliao wrote:
> This should sort before dom_distiller_core_jni_headers

Done.

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

https://codereview.chromium.org/430473007/diff/160001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:62: return
(FontFamily)font_family;
On 2014/08/11 22:00:45, robliao wrote:
> static_cast

Done.

[robliao, 2014-08-12 17:20:26]

On 2014/08/12 02:09:56, sunangel wrote:
>
https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
> File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):
> 
>
https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
> components/dom_distiller/content/dom_distiller_viewer_source.cc:193:
> viewer::GetUnsafeArticleHtml(article_proto,
> On 2014/08/11 22:00:45, robliao wrote:
> > On 2014/08/11 21:51:33, sunangel wrote:
> > > Can you explain what you mean? Git cl format turned it into this.
> > > On 2014/08/10 02:43:45, robliao wrote:
> > > > Consistency: Wrap these arguments to the next line.
> > > 
> > 
> > Look at GetUnsafePartialArticleHtml below. Use that style.
> 
> Done.
> 
>
https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
> File components/dom_distiller/core/distilled_page_prefs_android.cc (right):
> 
>
https://codereview.chromium.org/430473007/diff/120001/components/dom_distille...
> components/dom_distiller/core/distilled_page_prefs_android.cc:29:
> (DistilledPagePrefs::FontFamily)font_family);
> On 2014/08/11 22:00:45, robliao wrote:
> > On 2014/08/11 21:51:33, sunangel wrote:
> > > On 2014/08/10 02:43:45, robliao wrote:
> > > > Does static_cast work here?
> > > 
> > > Done.
> > 
> > Cool. Update the casts below as well.
> 
> Done.
> 
>
https://codereview.chromium.org/430473007/diff/160001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java
> (right):
> 
>
https://codereview.chromium.org/430473007/diff/160001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:114:
> * TODO(smaslo): add GUI and front end implementation for FontFamily.
> On 2014/08/11 22:00:45, robliao wrote:
> > A // style comment will suffice.
> 
> Done.
> 
>
https://codereview.chromium.org/430473007/diff/160001/components/dom_distille...
> File components/dom_distiller.gypi (right):
> 
>
https://codereview.chromium.org/430473007/diff/160001/components/dom_distille...
> components/dom_distiller.gypi:203: 'target_name':
> 'dom_distiller_core_font_family_java',
> On 2014/08/11 22:00:45, robliao wrote:
> > This should sort before dom_distiller_core_jni_headers
> 
> Done.
> 
>
https://codereview.chromium.org/430473007/diff/160001/components/dom_distille...
> File components/dom_distiller/core/distilled_page_prefs.cc (right):
> 
>
https://codereview.chromium.org/430473007/diff/160001/components/dom_distille...
> components/dom_distiller/core/distilled_page_prefs.cc:62: return
> (FontFamily)font_family;
> On 2014/08/11 22:00:45, robliao wrote:
> > static_cast
> 
> Done.

Looks good so far. Go ahead and loop in nyquist@

[Yaron, 2014-08-13 01:58:47]

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
File
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/FontFamily.template
(right):

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/FontFamily.template:7:
// An auto-generated enum for Distilled Page Theme preferences as used by
FontFamily

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
File components/dom_distiller/content/resources/dom_distiller_viewer.js (right):

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/content/resources/dom_distiller_viewer.js:27: // Relies
on the classname order of the body being Theme class, then Font
Hrmm. This seems a little fragile (this is a shared resource for everything
operating in the page) but I guess it's currently a very controlled environment
so is ok for now.

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/content/resources/dom_distiller_viewer.js:30: cssClass
= themeClass + " " + cssClass;
No need for intermediate assignment. Just do it below.

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

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:17: const char kFontPref[]
= "dom_distiller.font";
dom_distiller.font_family

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

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_android.cc:29:
static_cast<DistilledPagePrefs::FontFamily>(font_family));
indent 2 more

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_android.cc:33: return
(int)distilled_page_prefs_->GetFontFamily();
space after "(int)"

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

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/core/font_family_list.h:13: // value. THEME_COUNT used
only by native enum.
FONT_FAMILY_COUNT

[nyquist, 2014-08-13 05:44:31]

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
File components/dom_distiller/content/resources/dom_distiller_viewer.js (right):

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/content/resources/dom_distiller_viewer.js:27: // Relies
on the classname order of the body being Theme class, then Font
On 2014/08/13 01:58:47, Yaron wrote:
> Hrmm. This seems a little fragile (this is a shared resource for everything
> operating in the page) but I guess it's currently a very controlled
environment
> so is ok for now.

I was discussing this offline with sunangel@ before this CL, and this was a
suggestion she had for doing this quickly which sounded reasonable.

Other possible ways:
- pass in old and new classname and do search/replace (regexp split on
whitespace + replace?)
- having an object (or just standalone methods and global state) in JavaScript
which has methods for each class type. After setting the theme / font face, etc
on it, it would generate a space-separated list of classes of its current state
set it on the body.

I think the latter sounds most promising, and might not be too hard to do, but I
think I would be fine with not doing it now. The one thing about this though is
that it would have to get it's initial state for both of the current class types
somehow, either from parsing the body-tag, or from calling the methods before
anything else. Some ways could just be to call the methods immediately (or
queueing them up to ensure the page is ready) by sending a message from the
browser process, or we could inline the calls in the HTML, or it could be parsed
from the HTML body class at startup. Anyway, I'm sure we can find a good way to
do this eventually.

[Yaron, 2014-08-13 16:49:29]

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
File components/dom_distiller/content/resources/dom_distiller_viewer.js (right):

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/content/resources/dom_distiller_viewer.js:27: // Relies
on the classname order of the body being Theme class, then Font
On 2014/08/13 05:44:31, nyquist wrote:
> On 2014/08/13 01:58:47, Yaron wrote:
> > Hrmm. This seems a little fragile (this is a shared resource for everything
> > operating in the page) but I guess it's currently a very controlled
> environment
> > so is ok for now.
> 
> I was discussing this offline with sunangel@ before this CL, and this was a
> suggestion she had for doing this quickly which sounded reasonable.
> 
> Other possible ways:
> - pass in old and new classname and do search/replace (regexp split on
> whitespace + replace?)
> - having an object (or just standalone methods and global state) in JavaScript
> which has methods for each class type. After setting the theme / font face,
etc
> on it, it would generate a space-separated list of classes of its current
state
> set it on the body.
> 
> I think the latter sounds most promising, and might not be too hard to do, but
I
> think I would be fine with not doing it now. The one thing about this though
is
> that it would have to get it's initial state for both of the current class
types
> somehow, either from parsing the body-tag, or from calling the methods before
> anything else. Some ways could just be to call the methods immediately (or
> queueing them up to ensure the page is ready) by sending a message from the
> browser process, or we could inline the calls in the HTML, or it could be
parsed
> from the HTML body class at startup. Anyway, I'm sure we can find a good way
to
> do this eventually.

Agreed that the latter approach is better but what we have here is fine for now.

[sunangel, 2014-08-13 18:09:04]

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
File
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/FontFamily.template
(right):

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/FontFamily.template:7:
// An auto-generated enum for Distilled Page Theme preferences as used by
On 2014/08/13 01:58:47, Yaron wrote:
> FontFamily

Done.

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
File components/dom_distiller/content/resources/dom_distiller_viewer.js (right):

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/content/resources/dom_distiller_viewer.js:30: cssClass
= themeClass + " " + cssClass;
On 2014/08/13 01:58:47, Yaron wrote:
> No need for intermediate assignment. Just do it below.

Done.

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

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs.cc:17: const char kFontPref[]
= "dom_distiller.font";
On 2014/08/13 01:58:47, Yaron wrote:
> dom_distiller.font_family

Done.

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

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_android.cc:29:
static_cast<DistilledPagePrefs::FontFamily>(font_family));
On 2014/08/13 01:58:47, Yaron wrote:
> indent 2 more

Done.

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_android.cc:33: return
(int)distilled_page_prefs_->GetFontFamily();
On 2014/08/13 01:58:47, Yaron wrote:
> space after "(int)"

Done.

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

https://codereview.chromium.org/430473007/diff/180001/components/dom_distille...
components/dom_distiller/core/font_family_list.h:13: // value. THEME_COUNT used
only by native enum.
On 2014/08/13 01:58:47, Yaron wrote:
> FONT_FAMILY_COUNT

Done.

[Yaron, 2014-08-13 18:37:00]

lgtm

[nyquist, 2014-08-13 19:56:59]

rubberstamp lgtm since yaron doesn't have OWNERS yet.

[sunangel, 2014-08-13 20:11:30]

The CQ bit was checked by sunangel@chromium.org

[commit-bot: I haz the power, 2014-08-13 20:12:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sunangel@chromium.org/430473007/220001

[commit-bot: I haz the power, 2014-08-14 05:27:48]

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

[commit-bot: I haz the power, 2014-08-14 05:41:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-14 05:41:56]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nyquist, 2014-08-14 16:54:17]

also, I copied the subject to be the first line of the CL description as well.
Rietveld is a bit odd here, where you have to copy-paste the subject.

Anyway, feel free to send to CQ after you fix the presubmit issue.

https://codereview.chromium.org/430473007/diff/220001/components/dom_distille...
File components/dom_distiller/core/distilled_page_prefs_android.cc (right):

https://codereview.chromium.org/430473007/diff/220001/components/dom_distille...
components/dom_distiller/core/distilled_page_prefs_android.cc:37:
distilled_page_prefs_->SetTheme(static_cast<DistilledPagePrefs::Theme>(theme));
This line is 81 characters, so it needs to be wrapped.

[sunangel, 2014-08-14 16:59:56]

The CQ bit was checked by sunangel@chromium.org

[commit-bot: I haz the power, 2014-08-14 17:04:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sunangel@chromium.org/430473007/240001

[sunangel, 2014-08-14 17:57:28]

The CQ bit was unchecked by sunangel@chromium.org

[sunangel, 2014-08-14 20:05:02]

The CQ bit was checked by sunangel@chromium.org

[commit-bot: I haz the power, 2014-08-14 20:08:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sunangel@chromium.org/430473007/280001

[commit-bot: I haz the power, 2014-08-15 05:25:32]

Committed patchset #7 (280001) as 289801