Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
340403004:
Java wrapper for DistilledPagePrefs. (Closed)
|
Created:
6 years, 6 months ago by sunangel Modified:
6 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, smaslo, rgustafson Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionJava wrapper for DistilledPagePrefs.
Adds Java version of DomDistillerService and
DistilledPagePrefs with support for setting and retrieving
the current Theme.
The DomDistillerService can be retrieved by using the
DomDistillerServiceFactory, which maps a profile to an
instance of the service.
BUG=383630
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283742
Patch Set 1 #Patch Set 2 : Reupload not including Susan's changes #Patch Set 3 : Spacing issues #
Total comments: 6
Patch Set 4 : Reader Mode Prefs High Contrast Mode moved upstream #Patch Set 5 : DomDistillerService Java wrapper created #Patch Set 6 : Baseline for comparison for next patch: includes sync and Susan's patches #Patch Set 7 : DomDistillerService Java wrapper #
Total comments: 35
Patch Set 8 : Styling issues #Patch Set 9 : Line breaks, commenting #
Total comments: 14
Patch Set 10 : Commenting revised, formatting #Patch Set 11 : DomDistillerServiceFactory comment spacing fixed #
Total comments: 10
Patch Set 12 : Spacing and indentation #
Total comments: 60
Patch Set 13 : namespaces to classes, native init, enum #
Total comments: 110
Patch Set 14 : base after sync #Patch Set 15 : formatting, getter in distilled page prefs for theme #Patch Set 16 : Added test #Patch Set 17 : Formatting changes, added test #
Total comments: 61
Patch Set 18 : stylistic changes, rename ThemeTestPref, remove unnecessary files #
Total comments: 6
Patch Set 19 : spacing, fixed theme_list #
Total comments: 14
Patch Set 20 : commenting, updated theme_list with Susan's patch #
Total comments: 2
Patch Set 21 : synced, use for comparison for next patch #Patch Set 22 : test upload #Patch Set 23 : modified DEPS for test dependencies #Messages
Total messages: 33 (0 generated)
https://codereview.chromium.org/340403004/diff/40001/components/dom_distiller... File components/dom_distiller/core/profile_utils_android.cc (right): https://codereview.chromium.org/340403004/diff/40001/components/dom_distiller... components/dom_distiller/core/profile_utils_android.cc:17: void SetHighContrastMode(JNIEnv* env, jobject obj, jlong ptr, Apply 80 Character limit. https://codereview.chromium.org/340403004/diff/40001/components/dom_distiller... components/dom_distiller/core/profile_utils_android.cc:20: reinterpret_cast<ReaderModePrefs*> (ptr); Remove space between > and ( https://codereview.chromium.org/340403004/diff/40001/components/dom_distiller... components/dom_distiller/core/profile_utils_android.cc:21: DCHECK(myReaderModePrefs); A DCHECK isn't necessary here. We're going to crash on the next line, achieving the same effect.
See new patch. FYI, "profile_utils_android.cc" name changed into "reader_mode_preferences_android.cc" https://codereview.chromium.org/340403004/diff/40001/components/dom_distiller... File components/dom_distiller/core/profile_utils_android.cc (right): https://codereview.chromium.org/340403004/diff/40001/components/dom_distiller... components/dom_distiller/core/profile_utils_android.cc:17: void SetHighContrastMode(JNIEnv* env, jobject obj, jlong ptr, On 2014/06/20 21:13:45, robliao wrote: > Apply 80 Character limit. Done. https://codereview.chromium.org/340403004/diff/40001/components/dom_distiller... components/dom_distiller/core/profile_utils_android.cc:20: reinterpret_cast<ReaderModePrefs*> (ptr); On 2014/06/20 21:13:45, robliao wrote: > Remove space between > and ( Done. https://codereview.chromium.org/340403004/diff/40001/components/dom_distiller... components/dom_distiller/core/profile_utils_android.cc:21: DCHECK(myReaderModePrefs); On 2014/06/20 21:13:45, robliao wrote: > A DCHECK isn't necessary here. We're going to crash on the next line, achieving > the same effect. Done.
PTAL
https://codereview.chromium.org/340403004/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java (right): https://codereview.chromium.org/340403004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:11: /** Add one linebreak above here. https://codereview.chromium.org/340403004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:19: public static DomDistillerService getDomDistillerServiceForProfile(Profile Break Profile type declaration on the next line. https://codereview.chromium.org/340403004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:23: mDDS = new DomDistillerService( This should be 4 spaces over along with all other instances. https://codereview.chromium.org/340403004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:24: nativeGetDomDistillerServiceForProfile(profile)); Statement continuation, needs 8 spaces. https://codereview.chromium.org/340403004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:33: private static native long nativeGetDomDistillerServiceForProfile(Profile Break Profile type declaration on the next line. https://codereview.chromium.org/340403004/diff/140001/chrome/browser/android/... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/340403004/diff/140001/chrome/browser/android/... chrome/browser/android/chrome_jni_registrar.cc:131: { "DomDistillerServiceFactory", RegisterDomDistillerServiceFactory}, Alphabetize. https://codereview.chromium.org/340403004/diff/140001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/340403004/diff/140001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:557: 'browser/dom_distiller/dom_distiller_service_factory_android.cc', Alphabetize. https://codereview.chromium.org/340403004/diff/140001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:3706: 'android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java', Alphabetize in gypi and all other instances. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java (right): https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:10: Remove linebreak here. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:18: nativeInit(nativeDomDistillerServicePtr); Statement continuations are 8 spaces. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:27: private native long nativeGetReaderModePrefsPointer Keep the paren with the function name. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/ReaderModePreferencesAndroid.java (right): https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/ReaderModePreferencesAndroid.java:12: Remove linebreak https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.cc (right): https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:28: return reinterpret_cast<intptr_t>(service_->GetReaderModePrefs()); Worth commenting why this works and where this lives. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.h (right): https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:16: DomDistillerServiceAndroid(JNIEnv* env, Section ordering - public, then protected, then private. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:22: #endif // COMPONENTS_DOM_DISTILLER_CORE_DOM_DISTILLER_SERVICE_ANDROID_H One more linebreak above here. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... File components/dom_distiller/core/reader_mode_preferences_android.h (right): https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.h:11: Multiple successive namespace declarations can be stacked with each other. Apply to cc file as well. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Form... https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.h:17: } One linebreak above this line.
PLAT, review is good to go. https://codereview.chromium.org/340403004/diff/140001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java (right): https://codereview.chromium.org/340403004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:11: /** On 2014/06/26 18:42:15, robliao wrote: > Add one linebreak above here. Done. https://codereview.chromium.org/340403004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:19: public static DomDistillerService getDomDistillerServiceForProfile(Profile On 2014/06/26 18:42:15, robliao wrote: > Break Profile type declaration on the next line. Done. https://codereview.chromium.org/340403004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:23: mDDS = new DomDistillerService( On 2014/06/26 18:42:15, robliao wrote: > This should be 4 spaces over along with all other instances. Done. https://codereview.chromium.org/340403004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:24: nativeGetDomDistillerServiceForProfile(profile)); On 2014/06/26 18:42:15, robliao wrote: > Statement continuation, needs 8 spaces. Done. https://codereview.chromium.org/340403004/diff/140001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:33: private static native long nativeGetDomDistillerServiceForProfile(Profile On 2014/06/26 18:42:15, robliao wrote: > Break Profile type declaration on the next line. Done. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java (right): https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:10: On 2014/06/26 18:42:15, robliao wrote: > Remove linebreak here. Done. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:18: nativeInit(nativeDomDistillerServicePtr); On 2014/06/26 18:42:15, robliao wrote: > Statement continuations are 8 spaces. Done. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:27: private native long nativeGetReaderModePrefsPointer On 2014/06/26 18:42:15, robliao wrote: > Keep the paren with the function name. Done. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.h (right): https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:22: #endif // COMPONENTS_DOM_DISTILLER_CORE_DOM_DISTILLER_SERVICE_ANDROID_H On 2014/06/26 18:42:15, robliao wrote: > One more linebreak above here. Done.
https://codereview.chromium.org/340403004/diff/140001/chrome/browser/android/... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/340403004/diff/140001/chrome/browser/android/... chrome/browser/android/chrome_jni_registrar.cc:131: { "DomDistillerServiceFactory", RegisterDomDistillerServiceFactory}, On 2014/06/26 18:42:15, robliao wrote: > Alphabetize. Done. https://codereview.chromium.org/340403004/diff/140001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/340403004/diff/140001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:557: 'browser/dom_distiller/dom_distiller_service_factory_android.cc', On 2014/06/26 18:42:15, robliao wrote: > Alphabetize. Done. https://codereview.chromium.org/340403004/diff/140001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:3706: 'android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java', On 2014/06/26 18:42:15, robliao wrote: > Alphabetize in gypi and all other instances. Done. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/ReaderModePreferencesAndroid.java (right): https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/ReaderModePreferencesAndroid.java:12: On 2014/06/26 18:42:15, robliao wrote: > Remove linebreak Done. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.cc (right): https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:28: return reinterpret_cast<intptr_t>(service_->GetReaderModePrefs()); On 2014/06/26 18:42:15, robliao wrote: > Worth commenting why this works and where this lives. Done. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.h (right): https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:16: DomDistillerServiceAndroid(JNIEnv* env, On 2014/06/26 18:42:15, robliao wrote: > Section ordering - public, then protected, then private. Done. https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... File components/dom_distiller/core/reader_mode_preferences_android.h (right): https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.h:11: Do you just mean to not put the line breaks between them? On 2014/06/26 18:42:15, robliao wrote: > Multiple successive namespace declarations can be stacked with each other. Apply > to cc file as well. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Form... https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.h:17: } On 2014/06/26 18:42:15, robliao wrote: > One linebreak above this line. Done.
https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... File components/dom_distiller/core/reader_mode_preferences_android.h (right): https://codereview.chromium.org/340403004/diff/140001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.h:11: Yes. On 2014/06/27 16:27:26, sunangel wrote: > Do you just mean to not put the line breaks between them? > On 2014/06/26 18:42:15, robliao wrote: > > Multiple successive namespace declarations can be stacked with each other. > Apply > > to cc file as well. > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Form... > https://codereview.chromium.org/340403004/diff/180001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java (right): https://codereview.chromium.org/340403004/diff/180001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:13: * Returns Java DomDistillerService. This comment should go with getDomDistillerServiceForProfile and a description of the class goes here. https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... File components/dom_distiller/android/component_jni_registrar.cc (right): https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... components/dom_distiller/android/component_jni_registrar.cc:9: #include "components/dom_distiller/android/component_jni_registrar.h" This include was in the correct place before. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java (right): https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:19: nativeGetReaderModePrefsPointer(nativeDomDistillerServiceAndroid)); Statement continuations are 8 spaces. https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.h (right): https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:12: public: The indent spacing was correct the first round, just not the ordering. One space for public/protected/private and then two spaces for the rest of the declarations. https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:23: #endif // COMPONENTS_DOM_DISTILLER_CORE_DOM_DISTILLER_SERVICE_ANDROID_H Add one linebreak above here. https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... File components/dom_distiller/core/reader_mode_preferences_android.cc (right): https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.cc:22: else Else on previous line and use braces. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... Also two space indentation for C++. https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... File components/dom_distiller/core/reader_mode_preferences_android.h (right): https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.h:16: } } // namespace android
PTAL. https://codereview.chromium.org/340403004/diff/180001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java (right): https://codereview.chromium.org/340403004/diff/180001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:13: * Returns Java DomDistillerService. On 2014/06/27 17:37:12, robliao wrote: > This comment should go with getDomDistillerServiceForProfile and a description > of the class goes here. Done. https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... File components/dom_distiller/android/component_jni_registrar.cc (right): https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... components/dom_distiller/android/component_jni_registrar.cc:9: #include "components/dom_distiller/android/component_jni_registrar.h" On 2014/06/27 17:37:12, robliao wrote: > This include was in the correct place before. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... Done. https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java (right): https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:19: nativeGetReaderModePrefsPointer(nativeDomDistillerServiceAndroid)); On 2014/06/27 17:37:12, robliao wrote: > Statement continuations are 8 spaces. Done. https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.h (right): https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:12: public: On 2014/06/27 17:37:12, robliao wrote: > The indent spacing was correct the first round, just not the ordering. > One space for public/protected/private and then two spaces for the rest of the > declarations. Done. https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:23: #endif // COMPONENTS_DOM_DISTILLER_CORE_DOM_DISTILLER_SERVICE_ANDROID_H On 2014/06/27 17:37:12, robliao wrote: > Add one linebreak above here. Done. https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... File components/dom_distiller/core/reader_mode_preferences_android.cc (right): https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.cc:22: else On 2014/06/27 17:37:12, robliao wrote: > Else on previous line and use braces. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... > > Also two space indentation for C++. Done. https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... File components/dom_distiller/core/reader_mode_preferences_android.h (right): https://codereview.chromium.org/340403004/diff/180001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.h:16: } On 2014/06/27 17:37:12, robliao wrote: > } // namespace android Done.
https://codereview.chromium.org/340403004/diff/220001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java (right): https://codereview.chromium.org/340403004/diff/220001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:18: new HashMap<Profile, DomDistillerService>(); Does this need to be a thread-safe hash-map? https://codereview.chromium.org/340403004/diff/220001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:21: * Returns Java DomDistillerService for given Profile. Align the asterisks. https://codereview.chromium.org/340403004/diff/220001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:31: else { } else { https://codereview.chromium.org/340403004/diff/220001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java (right): https://codereview.chromium.org/340403004/diff/220001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:20: nativeDomDistillerServiceAndroid)); Statement continuations are 8 spaces :-) https://codereview.chromium.org/340403004/diff/220001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.h (right): https://codereview.chromium.org/340403004/diff/220001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:21: Remove linebreak here.
https://codereview.chromium.org/340403004/diff/220001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java (right): https://codereview.chromium.org/340403004/diff/220001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:18: new HashMap<Profile, DomDistillerService>(); I don't think so, already asked nyquist@ once before, will double check when he reviews On 2014/06/27 18:08:49, robliao wrote: > Does this need to be a thread-safe hash-map? https://codereview.chromium.org/340403004/diff/220001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:21: * Returns Java DomDistillerService for given Profile. On 2014/06/27 18:08:49, robliao wrote: > Align the asterisks. Done. https://codereview.chromium.org/340403004/diff/220001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:31: else { On 2014/06/27 18:08:49, robliao wrote: > } else { Done. https://codereview.chromium.org/340403004/diff/220001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java (right): https://codereview.chromium.org/340403004/diff/220001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:20: nativeDomDistillerServiceAndroid)); haha okay sorry I thought it would be 8 spaces from the beginning. On 2014/06/27 18:08:49, robliao wrote: > Statement continuations are 8 spaces :-)
PTAL. https://codereview.chromium.org/340403004/diff/220001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.h (right): https://codereview.chromium.org/340403004/diff/220001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:21: On 2014/06/27 18:08:49, robliao wrote: > Remove linebreak here. Done.
On 2014/06/27 18:17:17, sunangel wrote: > PTAL. > > https://codereview.chromium.org/340403004/diff/220001/components/dom_distille... > File components/dom_distiller/core/dom_distiller_service_android.h (right): > > https://codereview.chromium.org/340403004/diff/220001/components/dom_distille... > components/dom_distiller/core/dom_distiller_service_android.h:21: > On 2014/06/27 18:08:49, robliao wrote: > > Remove linebreak here. > > Done. Style lgtm. Add @nyquist for OWNER CR and Approval.
https://codereview.chromium.org/340403004/diff/240001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java (right): https://codereview.chromium.org/340403004/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:6: Nit: Ordering. See https://source.android.com/source/code-style.html#order-import-statements https://codereview.chromium.org/340403004/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:17: static HashMap<Profile, DomDistillerService> profiles_to_DDS = private static final. Also, use camel case in java for members. Static should start with s. See https://source.android.com/source/code-style.html#follow-field-naming-convent... How about: sServiceMap? https://codereview.chromium.org/340403004/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:23: public static DomDistillerService getDomDistillerServiceForProfile( This class is already called DomDistillerServiceFactory. I think you can just use getForProfile https://codereview.chromium.org/340403004/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:25: DomDistillerService mDDS; The m-prefix is for members. I think you can just use |service| here. Also, I think you can probably just get the service here directly. It can still be null, but you don't have to get it twice. https://codereview.chromium.org/340403004/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:30: } else { Could you add an assert that this is only called on the UI thread, to ensure there are no concurrency issues here? ThreadUtils.assertOnUiThread() or something along those lines in the top of this method. https://codereview.chromium.org/340403004/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:36: private static native long nativeGetDomDistillerServiceForProfile( private static native DomDistillerService nativeGetForProfile(Profile profile); https://codereview.chromium.org/340403004/diff/240001/chrome/browser/dom_dist... File chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc (right): https://codereview.chromium.org/340403004/diff/240001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:15: static jlong GetDomDistillerServiceForProfile(JNIEnv* env, Could this instead return the jobject for DomDistillerService? https://codereview.chromium.org/340403004/diff/240001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:18: Profile* nativeProfilePointer = ProfileAndroid::FromProfileAndroid(j_profile); Just |profile| https://codereview.chromium.org/340403004/diff/240001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:22: return reinterpret_cast<intptr_t>(service); I think here you can new the DomDistillerServiceAndroid, passing in the pointer to the service. Then, you can call an accessor-method to get the Java-object. To do that though, this would have to be a real class that could be friended. https://codereview.chromium.org/340403004/diff/240001/chrome/browser/dom_dist... File chrome/browser/dom_distiller/dom_distiller_service_factory_android.h (right): https://codereview.chromium.org/340403004/diff/240001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.h:10: bool RegisterDomDistillerServiceFactory(JNIEnv* env); Namespace dom_distiller and android? https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... File components/dom_distiller.gypi (right): https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller.gypi:93: 'dom_distiller/core/reader_mode_preferences_android.h', Nit: ordering https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java (right): https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:12: private long nativeDomDistillerServiceAndroid; mDomDis... https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:13: private ReaderModePreferencesAndroid mReaderModePreferencesAndroid; Remove Android suffix. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/ReaderModePreferencesAndroid.java (right): https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/ReaderModePreferencesAndroid.java:12: @JNINamespace("dom_distiller::reader_mode_preferences::android") Also here, reader_mode_preferences namespace. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/ReaderModePreferencesAndroid.java:13: public final class ReaderModePreferencesAndroid { Remove Android suffix and rename to DistilledPagePrefs https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/ReaderModePreferencesAndroid.java:15: long nativeReaderModePrefsPtr; private final https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/ReaderModePreferencesAndroid.java:17: public ReaderModePreferencesAndroid(long nativePtr) { make this private. do the same dance for creating it from native as for DomDistillerService. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/ReaderModePreferencesAndroid.java:21: public void setHighContrastMode(boolean enabled) { Rebase to using themes. You probably have to generate that enum list. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.cc (right): https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:11: DomDistillerServiceAndroid::DomDistillerServiceAndroid( This constructor can call a private static create method on the Java object, passing in |this|. This would call a private constructor passing in the pointer to |this|. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:14: dom_distiller::DomDistillerService* ptr) { Nit: service https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:15: service_ = ptr; How about you call a static create method here on the Java object? https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:18: jlong Init(JNIEnv* env, jobject obj, jlong servicePtr) { I think we can turn this around and you could instead call a static create-method from the constructor. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:19: dom_distiller::DomDistillerService* s = |service| https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:30: jlong DomDistillerServiceAndroid::GetReaderModePrefsPointer(JNIEnv* env, This can instead return the jobject DistilledPagePrefs https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.h (right): https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:11: class DomDistillerServiceAndroid { Put in namespaces dom_distiller and android https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:17: jlong GetReaderModePrefsPointer(JNIEnv* env, jobject obj); Private? https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:19: private: Add a new method to access the Java object. This could be private. Also, add the DomDistillerServiceFactoryAndroid class as a forward declared friend. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... File components/dom_distiller/core/reader_mode_preferences_android.cc (right): https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.cc:11: namespace reader_mode_preferences { remove namespace https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.cc:14: void SetHighContrastMode(JNIEnv* env, This should probably be SetTheme https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.cc:17: jboolean enabled) { Shouldn't this take in a theme? You might have to auto-generate the Java classes for this enum at that point. See for example: ./components/cronet/android/java/src/org/chromium/net/UrlRequestError.template https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.cc:26: bool RegisterReaderModePrefsAndroid(JNIEnv* env) { DistilledPagePrefs https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... File components/dom_distiller/core/reader_mode_preferences_android.h (right): https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.h:11: namespace reader_mode_preferences { Use a class instead of namespace: DistilledPagePrefsAndroid
New patch set. https://codereview.chromium.org/340403004/diff/240001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java (right): https://codereview.chromium.org/340403004/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:6: On 2014/06/30 20:33:22, nyquist wrote: > Nit: Ordering. See > https://source.android.com/source/code-style.html#order-import-statements Done. https://codereview.chromium.org/340403004/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:17: static HashMap<Profile, DomDistillerService> profiles_to_DDS = On 2014/06/30 20:33:22, nyquist wrote: > private static final. Also, use camel case in java for members. Static should > start with s. See > https://source.android.com/source/code-style.html#follow-field-naming-convent... > How about: > sServiceMap? Done. https://codereview.chromium.org/340403004/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:23: public static DomDistillerService getDomDistillerServiceForProfile( On 2014/06/30 20:33:22, nyquist wrote: > This class is already called DomDistillerServiceFactory. I think you can just > use getForProfile Done. https://codereview.chromium.org/340403004/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:25: DomDistillerService mDDS; On 2014/06/30 20:33:22, nyquist wrote: > The m-prefix is for members. I think you can just use |service| here. Also, I > think you can probably just get the service here directly. It can still be null, > but you don't have to get it twice. Done. https://codereview.chromium.org/340403004/diff/240001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:30: } else { I think I did this ....not sure if I did it right. (see next patch) On 2014/06/30 20:33:22, nyquist wrote: > Could you add an assert that this is only called on the UI thread, to ensure > there are no concurrency issues here? ThreadUtils.assertOnUiThread() or > something along those lines in the top of this method. https://codereview.chromium.org/340403004/diff/240001/chrome/browser/dom_dist... File chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc (right): https://codereview.chromium.org/340403004/diff/240001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:15: static jlong GetDomDistillerServiceForProfile(JNIEnv* env, On 2014/06/30 20:33:22, nyquist wrote: > Could this instead return the jobject for DomDistillerService? Done. https://codereview.chromium.org/340403004/diff/240001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:18: Profile* nativeProfilePointer = ProfileAndroid::FromProfileAndroid(j_profile); On 2014/06/30 20:33:22, nyquist wrote: > Just |profile| Done. https://codereview.chromium.org/340403004/diff/240001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:22: return reinterpret_cast<intptr_t>(service); On 2014/06/30 20:33:23, nyquist wrote: > I think here you can new the DomDistillerServiceAndroid, passing in the pointer > to the service. > Then, you can call an accessor-method to get the Java-object. To do that though, > this would have to be a real class that could be friended. Done. https://codereview.chromium.org/340403004/diff/240001/chrome/browser/dom_dist... File chrome/browser/dom_distiller/dom_distiller_service_factory_android.h (right): https://codereview.chromium.org/340403004/diff/240001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.h:10: bool RegisterDomDistillerServiceFactory(JNIEnv* env); On 2014/06/30 20:33:23, nyquist wrote: > Namespace dom_distiller and android? Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... File components/dom_distiller.gypi (right): https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller.gypi:93: 'dom_distiller/core/reader_mode_preferences_android.h', On 2014/06/30 20:33:23, nyquist wrote: > Nit: ordering Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java (right): https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:12: private long nativeDomDistillerServiceAndroid; On 2014/06/30 20:33:23, nyquist wrote: > mDomDis... Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:13: private ReaderModePreferencesAndroid mReaderModePreferencesAndroid; On 2014/06/30 20:33:23, nyquist wrote: > Remove Android suffix. Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/ReaderModePreferencesAndroid.java (right): https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/ReaderModePreferencesAndroid.java:13: public final class ReaderModePreferencesAndroid { On 2014/06/30 20:33:23, nyquist wrote: > Remove Android suffix and rename to DistilledPagePrefs Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/ReaderModePreferencesAndroid.java:15: long nativeReaderModePrefsPtr; On 2014/06/30 20:33:23, nyquist wrote: > private final Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.cc (right): https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:11: DomDistillerServiceAndroid::DomDistillerServiceAndroid( On 2014/06/30 20:33:23, nyquist wrote: > This constructor can call a private static create method on the Java object, > passing in |this|. This would call a private constructor passing in the pointer > to |this|. Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:14: dom_distiller::DomDistillerService* ptr) { On 2014/06/30 20:33:23, nyquist wrote: > Nit: service Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:15: service_ = ptr; On 2014/06/30 20:33:23, nyquist wrote: > How about you call a static create method here on the Java object? Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:18: jlong Init(JNIEnv* env, jobject obj, jlong servicePtr) { On 2014/06/30 20:33:23, nyquist wrote: > I think we can turn this around and you could instead call a static > create-method from the constructor. Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:19: dom_distiller::DomDistillerService* s = On 2014/06/30 20:33:23, nyquist wrote: > |service| Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:30: jlong DomDistillerServiceAndroid::GetReaderModePrefsPointer(JNIEnv* env, not doing this as per conversation On 2014/06/30 20:33:23, nyquist wrote: > This can instead return the jobject DistilledPagePrefs https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.h (right): https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:11: class DomDistillerServiceAndroid { On 2014/06/30 20:33:23, nyquist wrote: > Put in namespaces dom_distiller and android Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:17: jlong GetReaderModePrefsPointer(JNIEnv* env, jobject obj); If I make it private, DomDistillerService seems to not be able to call it either...unless I make DomDistillerService a friend too...do you have a preference? On 2014/06/30 20:33:23, nyquist wrote: > Private? https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:19: private: second part done..first part not done as per conversation On 2014/06/30 20:33:23, nyquist wrote: > Add a new method to access the Java object. This could be private. > Also, add the DomDistillerServiceFactoryAndroid class as a forward declared > friend. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... File components/dom_distiller/core/reader_mode_preferences_android.cc (right): https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.cc:11: namespace reader_mode_preferences { On 2014/06/30 20:33:24, nyquist wrote: > remove namespace Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.cc:14: void SetHighContrastMode(JNIEnv* env, On 2014/06/30 20:33:24, nyquist wrote: > This should probably be SetTheme Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.cc:17: jboolean enabled) { On 2014/06/30 20:33:24, nyquist wrote: > Shouldn't this take in a theme? > You might have to auto-generate the Java classes for this enum at that point. > See for example: > ./components/cronet/android/java/src/org/chromium/net/UrlRequestError.template Done. https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.cc:26: bool RegisterReaderModePrefsAndroid(JNIEnv* env) { just turned into register because made it in a class On 2014/06/30 20:33:24, nyquist wrote: > DistilledPagePrefs https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... File components/dom_distiller/core/reader_mode_preferences_android.h (right): https://codereview.chromium.org/340403004/diff/240001/components/dom_distille... components/dom_distiller/core/reader_mode_preferences_android.h:11: namespace reader_mode_preferences { On 2014/06/30 20:33:24, nyquist wrote: > Use a class instead of namespace: DistilledPagePrefsAndroid Done.
https://codereview.chromium.org/340403004/diff/350001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java (right): https://codereview.chromium.org/340403004/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:29: DomDistillerService mDDS = sServiceMap.get(profile); s/mDDS/service/ https://codereview.chromium.org/340403004/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:36: private static native DomDistillerService nativeGetForProfile(Profile profile); missing newline before this. https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... File chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc (right): https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:7: remove empty line https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:9: #include "chrome/browser/dom_distiller/dom_distiller_service_factory_android.h" this include should go at the top, and then an empty line, and then the rest. https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:12: remove empty line https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:14: remove empty line https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:27: DomDistillerServiceAndroid* d = new DomDistillerServiceAndroid(service); service_android https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... File chrome/browser/dom_distiller/dom_distiller_service_factory_android.h (right): https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.h:14: remove empty line https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.h:18: remove empty line https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller.gypi (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller.gypi:86: 'dom_distiller/core/theme.h', Where is this file? I think you want to include dom_distiller/core/theme_list.h https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller.gypi:95: ], undo this whitespace change https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller.gypi:196: 'target_name': 'theme_java', dom_distiller_core_theme_java https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/android/component_jni_registrar.cc (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/component_jni_registrar.cc:10: remove empty line. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/component_jni_registrar.cc:27: return base::android::RegisterNativeMethods(env, Nit: I think |env| goes on the next line, but 'git cl format' would tell. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:8: * Wrapper for the dom_distiller::reader_mode_preferences. "Wrapper for the native dom_distiller::DistilledPagePrefs." https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:14: DistilledPagePrefs(long nativeReaderModePrefs) { distilledPagePrefsPtr https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:18: public void setTheme(Theme theme) { Would you ever want to get the current theme in the UI? Also, that might be an easy way to test this (by using both set and get in the test). https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:21: remove one empty line https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:23: private native long nativeInit(long distilledPagePref); distilledPagePrefsPtr https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:25: int color_mode); int theme https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:27: remove empty line (just go to the last } and press Enter, then save.) https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:12: * Wrapper for the dom_distiller::reader_mode_preferences. "Wrapper for the native dom_distiller::DomDistillerService." https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:20: DomDistillerService(long nativeDomDistillerAndroidServicePtr) { private https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:39: private static native long nativeGetDistilledPagePrefsPointer( Nit: s/Pointer/Ptr/ https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template:7: // An auto-generated interface for Distilled Page Theme preferences as used by this is an enum, not an interface. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template:8: // com.google.android.apps.chrome.preferences.ReaderModePreferences and remove this line about com.google.android.apps.chrome.preferences https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template:9: // org.chromium.components.dom_distiller.core.DistilledPagePrefs and "the Java class org.chromium.components.dom_distiller.core.DistilledPagePrefs and the corresponding native class dom_distiller::android::DistilledPagePrefsAndroid." https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template:14: #define DEFINE_THEME(x,y,z) y(z), rename y to name and z to value. and rename x to unused https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/content/dom_distiller_viewer_source.cc (left): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/content/dom_distiller_viewer_source.cc:15: <<<<<<< ours I don't think this is in the checked-in version of the file. Could it be that your local branch is based off something that's not correct? https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs.h (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs.h:25: #define DEFINE_THEME(x, y, z) x = z, rename x to name and z to value. rename y to unused. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.cc (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:9: remove empty line https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:11: add namespace dom_distiller and android. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:13: dom_distiller::DistilledPagePrefs* ptr):distilled_page_prefs_(ptr) {} call the parameter for distilled_page_prefs. See style guide for how this should be written: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In... https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:19: jint color_mode) { s/color_mode/theme/ https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:21: (dom_distiller::DistilledPagePrefs::Theme)color_mode); whitespace after cast. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:24: jlong Init(JNIEnv* env, jobject obj, jlong servicePtr) { This is not the service pointer anymore. Rename to distilledPagePrefsPtr https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:25: dom_distiller::DistilledPagePrefs* ptr = s/ptr/distilledPagePrefs/ https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:27: DistilledPagePrefsAndroid* dom_distiller_service_android = distilled_page_prefs_android https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.h (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:12: remove empty line. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:14: class DistilledPagePrefsAndroid{ add namespace dom_distiller and android and remove "dom_distiller::" in the places below. Also, missing whitespace before { https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:15: remove empty line https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:18: dom_distiller::DistilledPagePrefs* d); Rename |d| to something clearer. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:20: void SetTheme(JNIEnv* env, jobject obj, jint color_mode); rename color_mode to theme. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:21: virtual ~DistilledPagePrefsAndroid(); Put the destructor after the destructor. See declaration ordering at: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:25: add DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.cc (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:8: remove empty line https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:12: remove empty line https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:19: dom_distiller::DomDistillerService* service):service_(service) { See style guide for how this should be written: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In... https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:33: jobject obj) { indent seems to be off? https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.h (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:11: Remove empty line. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:19: class DomDistillerServiceAndroid { What is this class? (add comment to explain). https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:22: friend class DomDistillerServiceFactoryAndroid; Add a comment which explains why this is needed. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:23: DomDistillerServiceAndroid(dom_distiller::DomDistillerService* ptr); s/ptr/service/. Also, remove namespace, since you're already in it I believe. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:25: virtual ~DomDistillerServiceAndroid(); move up. see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/core/theme_list.h (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/theme_list.h:11: Could you add a comment here that explains what the three different arguments are?
When reviewing, please compare patch 14 (base after syncing) with patch 15 (new patch) to see changes https://codereview.chromium.org/340403004/diff/350001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java (right): https://codereview.chromium.org/340403004/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:29: DomDistillerService mDDS = sServiceMap.get(profile); On 2014/07/08 01:02:32, nyquist wrote: > s/mDDS/service/ Done. https://codereview.chromium.org/340403004/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:36: private static native DomDistillerService nativeGetForProfile(Profile profile); On 2014/07/08 01:02:32, nyquist wrote: > missing newline before this. Done. https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... File chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc (right): https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:7: On 2014/07/08 01:02:32, nyquist wrote: > remove empty line Done. https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:9: #include "chrome/browser/dom_distiller/dom_distiller_service_factory_android.h" On 2014/07/08 01:02:32, nyquist wrote: > this include should go at the top, and then an empty line, and then the rest. Done. https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:12: On 2014/07/08 01:02:32, nyquist wrote: > remove empty line Done. https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:14: On 2014/07/08 01:02:32, nyquist wrote: > remove empty line Done. https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:27: DomDistillerServiceAndroid* d = new DomDistillerServiceAndroid(service); On 2014/07/08 01:02:32, nyquist wrote: > service_android Done. https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... File chrome/browser/dom_distiller/dom_distiller_service_factory_android.h (right): https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.h:14: On 2014/07/08 01:02:32, nyquist wrote: > remove empty line Done. https://codereview.chromium.org/340403004/diff/350001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.h:18: On 2014/07/08 01:02:32, nyquist wrote: > remove empty line Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller.gypi (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller.gypi:86: 'dom_distiller/core/theme.h', On 2014/07/08 01:02:33, nyquist wrote: > Where is this file? I think you want to include dom_distiller/core/theme_list.h Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller.gypi:95: ], On 2014/07/08 01:02:32, nyquist wrote: > undo this whitespace change Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller.gypi:196: 'target_name': 'theme_java', On 2014/07/08 01:02:32, nyquist wrote: > dom_distiller_core_theme_java Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/android/component_jni_registrar.cc (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/component_jni_registrar.cc:10: On 2014/07/08 01:02:33, nyquist wrote: > remove empty line. Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/component_jni_registrar.cc:27: return base::android::RegisterNativeMethods(env, On 2014/07/08 01:02:33, nyquist wrote: > Nit: I think |env| goes on the next line, but 'git cl format' would tell. Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:8: * Wrapper for the dom_distiller::reader_mode_preferences. On 2014/07/08 01:02:33, nyquist wrote: > "Wrapper for the native dom_distiller::DistilledPagePrefs." Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:14: DistilledPagePrefs(long nativeReaderModePrefs) { On 2014/07/08 01:02:33, nyquist wrote: > distilledPagePrefsPtr Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:18: public void setTheme(Theme theme) { I added a getter. On 2014/07/08 01:02:33, nyquist wrote: > Would you ever want to get the current theme in the UI? Also, that might be an > easy way to test this (by using both set and get in the test). https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:21: On 2014/07/08 01:02:33, nyquist wrote: > remove one empty line Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:23: private native long nativeInit(long distilledPagePref); On 2014/07/08 01:02:33, nyquist wrote: > distilledPagePrefsPtr Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:25: int color_mode); On 2014/07/08 01:02:33, nyquist wrote: > int theme Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:27: On 2014/07/08 01:02:33, nyquist wrote: > remove empty line (just go to the last } and press Enter, then save.) Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:12: * Wrapper for the dom_distiller::reader_mode_preferences. On 2014/07/08 01:02:33, nyquist wrote: > "Wrapper for the native dom_distiller::DomDistillerService." Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:20: DomDistillerService(long nativeDomDistillerAndroidServicePtr) { On 2014/07/08 01:02:33, nyquist wrote: > private Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:39: private static native long nativeGetDistilledPagePrefsPointer( On 2014/07/08 01:02:33, nyquist wrote: > Nit: s/Pointer/Ptr/ Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template:7: // An auto-generated interface for Distilled Page Theme preferences as used by On 2014/07/08 01:02:33, nyquist wrote: > this is an enum, not an interface. Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template:8: // com.google.android.apps.chrome.preferences.ReaderModePreferences and On 2014/07/08 01:02:33, nyquist wrote: > remove this line about com.google.android.apps.chrome.preferences Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template:9: // org.chromium.components.dom_distiller.core.DistilledPagePrefs and On 2014/07/08 01:02:33, nyquist wrote: > "the Java class org.chromium.components.dom_distiller.core.DistilledPagePrefs > and the corresponding native class > dom_distiller::android::DistilledPagePrefsAndroid." Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template:14: #define DEFINE_THEME(x,y,z) y(z), On 2014/07/08 01:02:33, nyquist wrote: > rename y to name and z to value. and rename x to unused Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/content/dom_distiller_viewer_source.cc (left): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/content/dom_distiller_viewer_source.cc:15: <<<<<<< ours Should not be in CL, removed On 2014/07/08 01:02:33, nyquist wrote: > I don't think this is in the checked-in version of the file. Could it be that > your local branch is based off something that's not correct? https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs.h (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs.h:25: #define DEFINE_THEME(x, y, z) x = z, On 2014/07/08 01:02:33, nyquist wrote: > rename x to name and z to value. rename y to unused. Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.cc (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:9: On 2014/07/08 01:02:34, nyquist wrote: > remove empty line Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:11: On 2014/07/08 01:02:34, nyquist wrote: > add namespace dom_distiller and android. Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:13: dom_distiller::DistilledPagePrefs* ptr):distilled_page_prefs_(ptr) {} On 2014/07/08 01:02:34, nyquist wrote: > call the parameter for distilled_page_prefs. > > See style guide for how this should be written: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In... Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:19: jint color_mode) { On 2014/07/08 01:02:34, nyquist wrote: > s/color_mode/theme/ Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:21: (dom_distiller::DistilledPagePrefs::Theme)color_mode); On 2014/07/08 01:02:34, nyquist wrote: > whitespace after cast. Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:24: jlong Init(JNIEnv* env, jobject obj, jlong servicePtr) { On 2014/07/08 01:02:34, nyquist wrote: > This is not the service pointer anymore. Rename to distilledPagePrefsPtr Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:25: dom_distiller::DistilledPagePrefs* ptr = On 2014/07/08 01:02:34, nyquist wrote: > s/ptr/distilledPagePrefs/ Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:27: DistilledPagePrefsAndroid* dom_distiller_service_android = On 2014/07/08 01:02:33, nyquist wrote: > distilled_page_prefs_android Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.h (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:12: On 2014/07/08 01:02:34, nyquist wrote: > remove empty line. Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:14: class DistilledPagePrefsAndroid{ On 2014/07/08 01:02:34, nyquist wrote: > add namespace dom_distiller and android and remove "dom_distiller::" in the > places below. Also, missing whitespace before { Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:15: On 2014/07/08 01:02:34, nyquist wrote: > remove empty line Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:18: dom_distiller::DistilledPagePrefs* d); On 2014/07/08 01:02:34, nyquist wrote: > Rename |d| to something clearer. Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:20: void SetTheme(JNIEnv* env, jobject obj, jint color_mode); On 2014/07/08 01:02:34, nyquist wrote: > rename color_mode to theme. Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:21: virtual ~DistilledPagePrefsAndroid(); On 2014/07/08 01:02:34, nyquist wrote: > Put the destructor after the destructor. See declaration ordering at: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:25: On 2014/07/08 01:02:34, nyquist wrote: > add DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.cc (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:8: On 2014/07/08 01:02:34, nyquist wrote: > remove empty line Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:12: On 2014/07/08 01:02:34, nyquist wrote: > remove empty line Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:19: dom_distiller::DomDistillerService* service):service_(service) { On 2014/07/08 01:02:34, nyquist wrote: > See style guide for how this should be written: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In... Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:33: jobject obj) { On 2014/07/08 01:02:34, nyquist wrote: > indent seems to be off? Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.h (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:11: On 2014/07/08 01:02:35, nyquist wrote: > Remove empty line. Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:19: class DomDistillerServiceAndroid { On 2014/07/08 01:02:35, nyquist wrote: > What is this class? (add comment to explain). Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:22: friend class DomDistillerServiceFactoryAndroid; On 2014/07/08 01:02:35, nyquist wrote: > Add a comment which explains why this is needed. Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:23: DomDistillerServiceAndroid(dom_distiller::DomDistillerService* ptr); On 2014/07/08 01:02:34, nyquist wrote: > s/ptr/service/. Also, remove namespace, since you're already in it I believe. Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:25: virtual ~DomDistillerServiceAndroid(); On 2014/07/08 01:02:35, nyquist wrote: > move up. see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... Done. https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... File components/dom_distiller/core/theme_list.h (right): https://codereview.chromium.org/340403004/diff/350001/components/dom_distille... components/dom_distiller/core/theme_list.h:11: On 2014/07/08 01:02:35, nyquist wrote: > Could you add a comment here that explains what the three different arguments > are? Done.
Added test, see new patch.
https://codereview.chromium.org/340403004/diff/570001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java (right): https://codereview.chromium.org/340403004/diff/570001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:15: * DomDistillerServiceFactory maps Profiles to DomDistillerServices. Nit: Maybe "to {@link DomDistillerService} instances."? Maybe also add something like: "Each {@link Profile} will at most have one instance of the service. If the service does not already exist, it will be created on the first access." https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java (right): https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java:18: * Ensures that Distilled Page Theme Preferences are working "Test class for {@link DistilledPagePrefs}." https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java:20: public class ThemePrefTest extends ChromeShellTestBase { Rename this to DistilledPagePrefsTest https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java:22: @Feature({"ReaderMode"}) s/ReaderMode/DomDistiller/ https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java:25: ThreadUtils.runOnUiThreadBlocking(new Runnable() { Annotate this test with @UiThreadTest instead. https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java:30: DistilledPagePrefs mDistilledPagePrefs = m-prefix is only for member fields. Either remove the prefix, or move this to a setup method and store as a member. https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java:32: assertNotNull(service); This should come before you use it, so move up to after you get the service. https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java:34: //Check default theme Add space after // and add a period at the end of the comment throughout this file. https://codereview.chromium.org/340403004/diff/570001/chrome/browser/android/... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/340403004/diff/570001/chrome/browser/android/... chrome/browser/android/chrome_jni_registrar.cc:129: {"DomDistillerServiceFactory", I don't think you want to change the style of this file in this CL. Could you revert the formatting of this file, and use the previous format? https://codereview.chromium.org/340403004/diff/570001/chrome/browser/dom_dist... File chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc (right): https://codereview.chromium.org/340403004/diff/570001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:8: #include "base/android/jni_string.h" Nit: is this needed? https://codereview.chromium.org/340403004/diff/570001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:10: #include "chrome/browser/profiles/profile.h" Nit: is this needed? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller.gypi (left): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller.gypi:82: 'dom_distiller/core/reader_mode_preferences.cc', I don't understand where these two lines come from. Is your branch correctly tracking https://codereview.chromium.org/341563002/ ? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:22: nativeSetTheme(mDistilledPagePrefsAndroid, theme.asNativeEnum()); Nit: How would you feel about adding a TODO for adding observer support to Java so that UI can be updated across tabs. https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:21: mDomDistillerServiceAndroid = Nit: Does this fit on one line (100 chars in Java)? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:25: mDomDistillerServiceAndroid)); move this line to the previous line? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:33: private static DomDistillerService create( does this fit on one line? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template:24: public int asNativeEnum() { package protected? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template:28: public static Theme getThemeForValue(int value) { package protected? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/content/dom_distiller_viewer_source.cc (left): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/content/dom_distiller_viewer_source.cc:28: // Copyright 2014 The Chromium Authors. All rights reserved. I don't understand this diff. https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.cc (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:25: distilled_page_prefs_->SetTheme((DistilledPagePrefs::Theme)theme); I think a whitespace is missing after the cast. Could you check with git cl format? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:29: return (int)distilled_page_prefs_->GetTheme(); same as above about whitespace https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.h (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:24: jint GetThemeValue(JNIEnv* env, jobject obj); s/Value// https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_unittests.cc (left): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_unittests.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. I don't think you want to delete this unittest. https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service.h (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/dom_distiller_service.h:16: #include "components/dom_distiller/core/distilled_page_prefs.h" Even though this is correct, this seems like unnecessary diff in your CL. Same below. Since there is only style-changes in your CL, I would recommend you just remove this file from your CL. https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.h (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:24: friend class DomDistillerServiceFactoryAndroid; "Friend declarations should always be in the private section" More info in the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:31: // Points to a Java instance of DomDistillerService Nit: Always period at end of comments. https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/core/theme_list.h (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/theme_list.h:13: DEFINE_THEME(kLight, LIGHT, 0) Rebase this, and then you'll see that the names will be the same again in fact! So then you can remove the double-name trick you did.
New patch. https://codereview.chromium.org/340403004/diff/570001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java (right): https://codereview.chromium.org/340403004/diff/570001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:15: * DomDistillerServiceFactory maps Profiles to DomDistillerServices. On 2014/07/09 23:49:03, nyquist wrote: > Nit: Maybe "to {@link DomDistillerService} instances."? > > Maybe also add something like: "Each {@link Profile} will at most have one > instance of the service. If the service does not already exist, it will be > created on the first access." Done. https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java (right): https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java:18: * Ensures that Distilled Page Theme Preferences are working On 2014/07/09 23:49:04, nyquist wrote: > "Test class for {@link DistilledPagePrefs}." Done. https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java:20: public class ThemePrefTest extends ChromeShellTestBase { On 2014/07/09 23:49:04, nyquist wrote: > Rename this to DistilledPagePrefsTest Done. https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java:22: @Feature({"ReaderMode"}) On 2014/07/09 23:49:03, nyquist wrote: > s/ReaderMode/DomDistiller/ Done. https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java:25: ThreadUtils.runOnUiThreadBlocking(new Runnable() { I think I did this...not sure if I did it right. On 2014/07/09 23:49:04, nyquist wrote: > Annotate this test with @UiThreadTest instead. https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java:30: DistilledPagePrefs mDistilledPagePrefs = On 2014/07/09 23:49:04, nyquist wrote: > m-prefix is only for member fields. Either remove the prefix, or move this to a > setup method and store as a member. Done. https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java:32: assertNotNull(service); On 2014/07/09 23:49:04, nyquist wrote: > This should come before you use it, so move up to after you get the service. Done. https://codereview.chromium.org/340403004/diff/570001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/ThemePrefTest.java:34: //Check default theme On 2014/07/09 23:49:03, nyquist wrote: > Add space after // and add a period at the end of the comment throughout this > file. Done. https://codereview.chromium.org/340403004/diff/570001/chrome/browser/android/... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/340403004/diff/570001/chrome/browser/android/... chrome/browser/android/chrome_jni_registrar.cc:129: {"DomDistillerServiceFactory", On 2014/07/09 23:49:04, nyquist wrote: > I don't think you want to change the style of this file in this CL. Could you > revert the formatting of this file, and use the previous format? Done. https://codereview.chromium.org/340403004/diff/570001/chrome/browser/dom_dist... File chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc (right): https://codereview.chromium.org/340403004/diff/570001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:8: #include "base/android/jni_string.h" On 2014/07/09 23:49:04, nyquist wrote: > Nit: is this needed? Done. https://codereview.chromium.org/340403004/diff/570001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.cc:10: #include "chrome/browser/profiles/profile.h" Yes. On 2014/07/09 23:49:04, nyquist wrote: > Nit: is this needed? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller.gypi (left): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller.gypi:82: 'dom_distiller/core/reader_mode_preferences.cc', fixed. On 2014/07/09 23:49:04, nyquist wrote: > I don't understand where these two lines come from. Is your branch correctly > tracking https://codereview.chromium.org/341563002/ ? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:22: nativeSetTheme(mDistilledPagePrefsAndroid, theme.asNativeEnum()); It already updates across tabs ...I think it's because of auto-refresh with the JS and the way the notifiers work. On 2014/07/09 23:49:04, nyquist wrote: > Nit: How would you feel about adding a TODO for adding observer support to Java > so that UI can be updated across tabs. https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:21: mDomDistillerServiceAndroid = On 2014/07/09 23:49:04, nyquist wrote: > Nit: Does this fit on one line (100 chars in Java)? Done. https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:25: mDomDistillerServiceAndroid)); On 2014/07/09 23:49:04, nyquist wrote: > move this line to the previous line? Done. https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:33: private static DomDistillerService create( No. On 2014/07/09 23:49:04, nyquist wrote: > does this fit on one line? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template:24: public int asNativeEnum() { Done. I remember you said that sometimes people add a comment or something to show that they didn't forget the scope? Not sure if I did that right.. On 2014/07/09 23:49:04, nyquist wrote: > package protected? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template:28: public static Theme getThemeForValue(int value) { On 2014/07/09 23:49:04, nyquist wrote: > package protected? Done. https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/content/dom_distiller_viewer_source.cc (left): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/content/dom_distiller_viewer_source.cc:28: // Copyright 2014 The Chromium Authors. All rights reserved. Neither do I. Base is incorrect, fixed. On 2014/07/09 23:49:04, nyquist wrote: > I don't understand this diff. https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.cc (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:25: distilled_page_prefs_->SetTheme((DistilledPagePrefs::Theme)theme); not missing a space. added a space, ran git cl format, it removed the space. On 2014/07/09 23:49:04, nyquist wrote: > I think a whitespace is missing after the cast. Could you check with git cl > format? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:29: return (int)distilled_page_prefs_->GetTheme(); same as above. On 2014/07/09 23:49:04, nyquist wrote: > same as above about whitespace https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.h (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:24: jint GetThemeValue(JNIEnv* env, jobject obj); If you meant change GetThemeValue to GetTheme I did that. On 2014/07/09 23:49:05, nyquist wrote: > s/Value// https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_unittests.cc (left): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_unittests.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/07/09 23:49:03, nyquist wrote: > I don't think you want to delete this unittest. Done. https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service.h (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/dom_distiller_service.h:16: #include "components/dom_distiller/core/distilled_page_prefs.h" On 2014/07/09 23:49:05, nyquist wrote: > Even though this is correct, this seems like unnecessary diff in your CL. Same > below. Since there is only style-changes in your CL, I would recommend you just > remove this file from your CL. Done. https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.h (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:24: friend class DomDistillerServiceFactoryAndroid; On 2014/07/09 23:49:05, nyquist wrote: > "Friend declarations should always be in the private section" > > More info in the style guide: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... Done. https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.h:31: // Points to a Java instance of DomDistillerService On 2014/07/09 23:49:05, nyquist wrote: > Nit: Always period at end of comments. Done. https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/core/theme_list.h (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/theme_list.h:13: DEFINE_THEME(kLight, LIGHT, 0) On 2014/07/09 23:49:05, nyquist wrote: > Rebase this, and then you'll see that the names will be the same again in fact! > So then you can remove the double-name trick you did. Done.
https://codereview.chromium.org/340403004/diff/570001/chrome/browser/android/... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/340403004/diff/570001/chrome/browser/android/... chrome/browser/android/chrome_jni_registrar.cc:129: {"DomDistillerServiceFactory", On 2014/07/10 14:31:52, sunangel wrote: > On 2014/07/09 23:49:04, nyquist wrote: > > I don't think you want to change the style of this file in this CL. Could you > > revert the formatting of this file, and use the previous format? > > Done. But this line must still be there, no? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:22: nativeSetTheme(mDistilledPagePrefsAndroid, theme.asNativeEnum()); I was thinking about the UI, not the distilled page. Say you have two tabs open (for example on a multi window device). Clicking one selection should update both the UI selection and the page. As it looks today I think only the latter applies. On 2014/07/10 14:31:52, sunangel wrote: > It already updates across tabs ...I think it's because of auto-refresh with the > JS and the way the notifiers work. > > On 2014/07/09 23:49:04, nyquist wrote: > > Nit: How would you feel about adding a TODO for adding observer support to > Java > > so that UI can be updated across tabs. > https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:33: private static DomDistillerService create( Are you sure? It looks to be around 80 characters, and the java columns are 100. On 2014/07/10 14:31:53, sunangel wrote: > No. On 2014/07/09 23:49:04, nyquist wrote: > > does this fit on one line? > https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.h (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:24: jint GetThemeValue(JNIEnv* env, jobject obj); Yes. Thanks! On 2014/07/10 14:31:53, sunangel wrote: > If you meant change GetThemeValue to GetTheme I did that. > On 2014/07/09 23:49:05, nyquist wrote: > > s/Value// > https://codereview.chromium.org/340403004/diff/670001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java (right): https://codereview.chromium.org/340403004/diff/670001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:33: assertEquals(distilledPagePrefs.getTheme(), Theme.LIGHT); (Expected, Actual) across these asserts https://codereview.chromium.org/340403004/diff/670001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template (right): https://codereview.chromium.org/340403004/diff/670001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template:24: // Package protected. Nit: I don't think we usually have these types of comments in chromium. https://codereview.chromium.org/340403004/diff/670001/components/dom_distille... File components/dom_distiller/core/theme_list.h (right): https://codereview.chromium.org/340403004/diff/670001/components/dom_distille... components/dom_distiller/core/theme_list.h:13: DEFINE_THEME(kLight, LIGHT, 0) These seems to still be wrong.
See new patch. https://codereview.chromium.org/340403004/diff/570001/chrome/browser/android/... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/340403004/diff/570001/chrome/browser/android/... chrome/browser/android/chrome_jni_registrar.cc:129: {"DomDistillerServiceFactory", Yes sorry, base was incorrect. On 2014/07/10 15:59:58, nyquist wrote: > On 2014/07/10 14:31:52, sunangel wrote: > > On 2014/07/09 23:49:04, nyquist wrote: > > > I don't think you want to change the style of this file in this CL. Could > you > > > revert the formatting of this file, and use the previous format? > > > > Done. > > But this line must still be there, no? https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:22: nativeSetTheme(mDistilledPagePrefsAndroid, theme.asNativeEnum()); okay, to do added. On 2014/07/10 15:59:58, nyquist wrote: > I was thinking about the UI, not the distilled page. > > Say you have two tabs open (for example on a multi window device). Clicking one > selection should update both the UI selection and the page. As it looks today I > think only the latter applies. > > On 2014/07/10 14:31:52, sunangel wrote: > > It already updates across tabs ...I think it's because of auto-refresh with > the > > JS and the way the notifiers work. > > > > On 2014/07/09 23:49:04, nyquist wrote: > > > Nit: How would you feel about adding a TODO for adding observer support to > > Java > > > so that UI can be updated across tabs. > > > https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java (right): https://codereview.chromium.org/340403004/diff/570001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:33: private static DomDistillerService create( Oh okay sorry did not know that. Fixed now. On 2014/07/10 15:59:58, nyquist wrote: > Are you sure? It looks to be around 80 characters, and the java columns are > 100. > > On 2014/07/10 14:31:53, sunangel wrote: > > No. On 2014/07/09 23:49:04, nyquist wrote: > > > does this fit on one line? > > > https://codereview.chromium.org/340403004/diff/670001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java (right): https://codereview.chromium.org/340403004/diff/670001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:33: assertEquals(distilledPagePrefs.getTheme(), Theme.LIGHT); On 2014/07/10 15:59:58, nyquist wrote: > (Expected, Actual) across these asserts Done. https://codereview.chromium.org/340403004/diff/670001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template (right): https://codereview.chromium.org/340403004/diff/670001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/Theme.template:24: // Package protected. On 2014/07/10 15:59:58, nyquist wrote: > Nit: I don't think we usually have these types of comments in chromium. Done. https://codereview.chromium.org/340403004/diff/670001/components/dom_distille... File components/dom_distiller/core/theme_list.h (right): https://codereview.chromium.org/340403004/diff/670001/components/dom_distille... components/dom_distiller/core/theme_list.h:13: DEFINE_THEME(kLight, LIGHT, 0) On 2014/07/10 15:59:58, nyquist wrote: > These seems to still be wrong. Done.
Also, please add a more thorough change description. https://codereview.chromium.org/340403004/diff/690001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java (right): https://codereview.chromium.org/340403004/diff/690001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:30: Profile profile) { Nit: move up https://codereview.chromium.org/340403004/diff/690001/chrome/browser/dom_dist... File chrome/browser/dom_distiller/dom_distiller_service_factory_android.h (right): https://codereview.chromium.org/340403004/diff/690001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.h:13: class DomDistillerServiceFactoryAndroid { Could you add a comment here that says that this is not supposed to be used, except from the Java class DomDistillerServiceFactory? https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:21: // TODO(sunangel): add observer support from this Java class to native Nit: Add https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.cc (right): https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:29: * Returns native pointer to native distilled page preferences registered with Returns pointer to native DistilledPagePrefs registered with DomDistillerService. https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... File components/dom_distiller/core/theme_list.h (right): https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... components/dom_distiller/core/theme_list.h:11: // First argument represents native enum, second argument represents Java enum, This comment should be updated. https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... components/dom_distiller/core/theme_list.h:13: DEFINE_THEME(LIGHT, 0) Please update components/dom_distiller/core/distilled_page_prefs.h https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... components/dom_distiller/core/theme_list.h:15: DEFINE_THEME(SEPIA, 2) I think this is missing THEME_COUNT
Not sure if the new change description is good enough... https://codereview.chromium.org/340403004/diff/690001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java (right): https://codereview.chromium.org/340403004/diff/690001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DomDistillerServiceFactory.java:30: Profile profile) { On 2014/07/11 00:19:02, nyquist wrote: > Nit: move up Done. https://codereview.chromium.org/340403004/diff/690001/chrome/browser/dom_dist... File chrome/browser/dom_distiller/dom_distiller_service_factory_android.h (right): https://codereview.chromium.org/340403004/diff/690001/chrome/browser/dom_dist... chrome/browser/dom_distiller/dom_distiller_service_factory_android.h:13: class DomDistillerServiceFactoryAndroid { On 2014/07/11 00:19:02, nyquist wrote: > Could you add a comment here that says that this is not supposed to be used, > except from the Java class DomDistillerServiceFactory? Done. https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:21: // TODO(sunangel): add observer support from this Java class to native On 2014/07/11 00:19:02, nyquist wrote: > Nit: Add Done. https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.cc (right): https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:29: * Returns native pointer to native distilled page preferences registered with On 2014/07/11 00:19:02, nyquist wrote: > Returns pointer to native DistilledPagePrefs registered with > DomDistillerService. Done. https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... File components/dom_distiller/core/theme_list.h (right): https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... components/dom_distiller/core/theme_list.h:11: // First argument represents native enum, second argument represents Java enum, On 2014/07/11 00:19:02, nyquist wrote: > This comment should be updated. Done. https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... components/dom_distiller/core/theme_list.h:13: DEFINE_THEME(LIGHT, 0) On 2014/07/11 00:19:02, nyquist wrote: > Please update components/dom_distiller/core/distilled_page_prefs.h Done. https://codereview.chromium.org/340403004/diff/690001/components/dom_distille... components/dom_distiller/core/theme_list.h:15: DEFINE_THEME(SEPIA, 2) On 2014/07/11 00:19:02, nyquist wrote: > I think this is missing THEME_COUNT Done.
https://codereview.chromium.org/340403004/diff/710001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.cc (right): https://codereview.chromium.org/340403004/diff/710001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:29: * Returns native pointer to native DistilledPagePrefs registered with Move this comment to the header file.
Could you please rebase this CL as well?
How about something like this for the description? " Java wrapper for DistilledPagePrefs. Adds Java version of DomDistillerService and DistilledPagePrefs with support for setting and retrieving the current Theme. The DomDistillerService can be retrieved by using the DomDistillerServiceFactory, which maps a profile to an instance of the service." Remember to use a max-length of 70 for descriptions, and have a newline between the title and the rest of the message.
done, see new patch. https://codereview.chromium.org/340403004/diff/710001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service_android.cc (right): https://codereview.chromium.org/340403004/diff/710001/components/dom_distille... components/dom_distiller/core/dom_distiller_service_android.cc:29: * Returns native pointer to native DistilledPagePrefs registered with On 2014/07/14 18:06:25, nyquist wrote: > Move this comment to the header file. Done.
lgtm
The CQ bit was checked by sunangel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sunangel@chromium.org/340403004/900001
Message was sent while issue was closed.
Change committed as 283742 |
