Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
415343002:
Upstream accessibility font size preferences. (Closed)
|
Created:
6 years, 5 months ago by sunangel Modified:
6 years, 3 months ago CC:
chromium-reviews, aboxhall+watch_chromium.org, yuzo+watch_chromium.org, plundblad+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, smaslo Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionUpstream accessibility font size preferences.
Moves ForceEnableZoom, UserSetForceEnableZoom, and
FontScaleFactor preferences upstream and adds support for
observing all those preferences in Java.
UserSetForceEnableZoom is currently stored using Android
SharedPreferences, which are application global, so this
class is currently not keyed off the profile. Preferably
this should be migrated to be per profile, at which point
the FontSizePrefs could be keyed off the profile instead of
being an application global singleton.
BUG=383630
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289185
Patch Set 1 #
Total comments: 12
Patch Set 2 : Made callback methods private, test expect statements changed, styling #
Total comments: 29
Patch Set 3 : FontPrefs only for Android platform, untied FontScale and ForceEnableZoom #Patch Set 4 : Patch set 3 but including sync - use as new base! #Patch Set 5 : Added destroy support, renamed SetUserSetForceEnableZoom #
Total comments: 6
Patch Set 6 : Changed return of Add/Remove Observer, added tests #Patch Set 7 : add const to PrefService #Patch Set 8 : brackets of return statement #
Total comments: 9
Patch Set 9 : Formatting #
Total comments: 2
Patch Set 10 : early return for removeObserver #
Total comments: 24
Patch Set 11 : Sync and rebase baseline for comparison #Patch Set 12 : Made FontSizePrefs singleton, added Observer support for UserSetForceEnableZoom #
Total comments: 10
Patch Set 13 : Nits #Patch Set 14 : Moved files into android/accessibility #Patch Set 15 : Testing moved to UiThread #
Total comments: 1
Patch Set 16 : Running getter methods on UiThread #
Total comments: 2
Messages
Total messages: 42 (0 generated)
A very quick first pass. https://codereview.chromium.org/415343002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:29: public static final float FORCE_ENABLE_ZOOM_THRESHOLD_MULTIPLIER = 1.3f; How was this number determined? Discuss that in a comment above the value. https://codereview.chromium.org/415343002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:32: private SharedPreferences mSharedPreferences; Make the constructor initialized variables final. https://codereview.chromium.org/415343002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:108: if (wrappedObserver != null) { When are you expecting this to be null? This sounds like a good assert in addObserver. https://codereview.chromium.org/415343002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:121: nativeSetFontScaleFactor(mFontSizePrefsAndroidPtr, font); Consider having a corresponding java side setFontScaleFactor to improve readability. https://codereview.chromium.org/415343002/diff/20001/chrome/browser/accessibi... File chrome/browser/accessibility/font_size_prefs.h (right): https://codereview.chromium.org/415343002/diff/20001/chrome/browser/accessibi... chrome/browser/accessibility/font_size_prefs.h:30: void OnFontScaleFactorPrefsChanged(); These feel like they should be in a separate observer class. https://codereview.chromium.org/415343002/diff/20001/chrome/chrome_tests_unit... File chrome/chrome_tests_unit.gypi (left): https://codereview.chromium.org/415343002/diff/20001/chrome/chrome_tests_unit... chrome/chrome_tests_unit.gypi:2585: Any reason why this was removed?
Please see new patch. https://codereview.chromium.org/415343002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:29: public static final float FORCE_ENABLE_ZOOM_THRESHOLD_MULTIPLIER = 1.3f; Done...comment was used from old AccessibilityPreferences comment. On 2014/07/25 16:49:19, robliao wrote: > How was this number determined? Discuss that in a comment above the value. https://codereview.chromium.org/415343002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:32: private SharedPreferences mSharedPreferences; On 2014/07/25 16:49:19, robliao wrote: > Make the constructor initialized variables final. Done. https://codereview.chromium.org/415343002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:108: if (wrappedObserver != null) { If someone called for example, removeObserver(new Observer()) On 2014/07/25 16:49:19, robliao wrote: > When are you expecting this to be null? This sounds like a good assert in > addObserver. https://codereview.chromium.org/415343002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:121: nativeSetFontScaleFactor(mFontSizePrefsAndroidPtr, font); On 2014/07/25 16:49:19, robliao wrote: > Consider having a corresponding java side setFontScaleFactor to improve > readability. Done. https://codereview.chromium.org/415343002/diff/20001/chrome/browser/accessibi... File chrome/browser/accessibility/font_size_prefs.h (right): https://codereview.chromium.org/415343002/diff/20001/chrome/browser/accessibi... chrome/browser/accessibility/font_size_prefs.h:30: void OnFontScaleFactorPrefsChanged(); Made them private instead. Is the spacing okay between them? On 2014/07/25 16:49:19, robliao wrote: > These feel like they should be in a separate observer class. https://codereview.chromium.org/415343002/diff/20001/chrome/chrome_tests_unit... File chrome/chrome_tests_unit.gypi (left): https://codereview.chromium.org/415343002/diff/20001/chrome/chrome_tests_unit... chrome/chrome_tests_unit.gypi:2585: Oops accident, should be fixed now. On 2014/07/25 16:49:19, robliao wrote: > Any reason why this was removed?
More comments... https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:114: if (wrappedObserver != null) { When is this expected to be null? https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:125: public void setAndProcessFontScaleFactor(float font) { fontScaleFactor Also, add a javadoc for this method and any other non-trivial one. https://source.android.com/source/code-style.html#use-javadoc-standard-comments https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:127: setFontScaleFactor(font); Okay, now that I've had more time to look at this, can you do all the sets at the end? At face value, it seems newTextScale would always == font. https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:130: if (oldTextScale < threshold I think you can take both of these... oldTextScale < threshold <= newTextScale newTextScale < threshold <= oldTextScale And roll them into... float changeInTextScale = newTextScale - oldTextScale; boolean changeExceedsThreshold = Math.abs(changeInTextScale) > FORCE_ENABLE_ZOOM_THRESHOLD_MULTIPLIER; Then the next comparison would be if (changeInTextScale > 0 && !getForceEnableZoom) { ... https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:134: // force enable zoom even if the user has previously set it. We can gather this much from the code. Is there any reason why we do this? If so, put that as the comment. Same as below. https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:150: public void onUserSetForceEnableZoomChanged(boolean enabled) { Does this need to be public? https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:164: public float getFontScaleFactor() { Order these so that related gets and sets are grouped together. https://codereview.chromium.org/415343002/diff/100001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java (right): https://codereview.chromium.org/415343002/diff/100001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:23: FontSizePrefs.FORCE_ENABLE_ZOOM_THRESHOLD_MULTIPLIER - 0.4f; 0.4f arbitrary? https://codereview.chromium.org/415343002/diff/100001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:102: //Checks that font scale for both observers is correctly changed and when the font scale is Space after // https://codereview.chromium.org/415343002/diff/100001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:121: private float mFontSize; Tabs = 4 Spaces and below. https://codereview.chromium.org/415343002/diff/100001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs.cc (right): https://codereview.chromium.org/415343002/diff/100001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs.cc:44: void FontSizePrefs::OnFontScaleFactorPrefsChanged() { Follow same order as header file. https://codereview.chromium.org/415343002/diff/100001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs.h (right): https://codereview.chromium.org/415343002/diff/100001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs.h:38: scoped_ptr<PrefChangeRegistrar> pref_change_registrar_; Declaration order - These go last, but before the DISALLOW by convention. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order https://codereview.chromium.org/415343002/diff/100001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs_android.h (right): https://codereview.chromium.org/415343002/diff/100001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.h:44: // FontSizePrefs::Observer implementation. The above comment covers this so you can remove it.
Please see new patch. Explanation about logic for ForceEnableZoom and FontScale -- I moved this to AccessibilityPrefs.java and will update that CL shortly because the logic tying those 2 variables existed previously only in AccessPrefs and I realized it did not make sense to have them always tied together for any change in FontScaleFactor. https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:114: if (wrappedObserver != null) { If someone calls removeObserver(new Observer)...Ideally no one would call this but it's just for safety checking. On 2014/07/25 23:49:34, robliao wrote: > When is this expected to be null? https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:125: public void setAndProcessFontScaleFactor(float font) { if you meant change the param name to fontScaleFactor, then done. On 2014/07/25 23:49:34, robliao wrote: > fontScaleFactor > > Also, add a javadoc for this method and any other non-trivial one. > https://source.android.com/source/code-style.html#use-javadoc-standard-comments https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:127: setFontScaleFactor(font); Yes, can do that. On 2014/07/25 23:49:34, robliao wrote: > Okay, now that I've had more time to look at this, can you do all the sets at > the end? > > At face value, it seems newTextScale would always == font. https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:130: if (oldTextScale < threshold I don't think this works... For example, old=0.5, new = 1.5, threshold=1.3, change=1 would make the bool false in this case but should be true On 2014/07/25 23:49:34, robliao wrote: > I think you can take both of these... > oldTextScale < threshold <= newTextScale > newTextScale < threshold <= oldTextScale > > And roll them into... > float changeInTextScale = newTextScale - oldTextScale; > boolean changeExceedsThreshold = Math.abs(changeInTextScale) > > FORCE_ENABLE_ZOOM_THRESHOLD_MULTIPLIER; > > Then the next comparison would be > if (changeInTextScale > 0 && !getForceEnableZoom) { ... https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:134: // force enable zoom even if the user has previously set it. I am not quite sure but these were the comments in the old AcessibilityPreferences as to why AccessibilityPreferences wanted to change the font so I just put them back in here. I actually think that this logic would fit better into AccessibilityPreferences now that I think about it since it was the only property with font that would previously also change force enable zoom, so I will move the code there. On 2014/07/25 23:49:34, robliao wrote: > We can gather this much from the code. Is there any reason why we do this? If > so, put that as the comment. > > Same as below. https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:150: public void onUserSetForceEnableZoomChanged(boolean enabled) { Yes, because inner class cannot distinguish when the set force enable zoom is a trigger from font change versus user set. when On 2014/07/25 23:49:34, robliao wrote: > Does this need to be public? https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:164: public float getFontScaleFactor() { On 2014/07/25 23:49:34, robliao wrote: > Order these so that related gets and sets are grouped together. Done. https://codereview.chromium.org/415343002/diff/100001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java (right): https://codereview.chromium.org/415343002/diff/100001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:23: FontSizePrefs.FORCE_ENABLE_ZOOM_THRESHOLD_MULTIPLIER - 0.4f; Yes, but now that the threshold has been moved to AccessibilityPrefs, the AccessibilityPrefs test can test that, so the font sets and gets are now completely arbitrary and independent of threshold in this test. I will make a comment in the AccessPrefs test about the 0.4 being arbitrary though. On 2014/07/25 23:49:34, robliao wrote: > 0.4f arbitrary? https://codereview.chromium.org/415343002/diff/100001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:102: //Checks that font scale for both observers is correctly changed and when the font scale is On 2014/07/25 23:49:34, robliao wrote: > Space after // Done. https://codereview.chromium.org/415343002/diff/100001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:121: private float mFontSize; On 2014/07/25 23:49:34, robliao wrote: > Tabs = 4 Spaces and below. Done. https://codereview.chromium.org/415343002/diff/100001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs.cc (right): https://codereview.chromium.org/415343002/diff/100001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs.cc:44: void FontSizePrefs::OnFontScaleFactorPrefsChanged() { On 2014/07/25 23:49:34, robliao wrote: > Follow same order as header file. Done. https://codereview.chromium.org/415343002/diff/100001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs.h (right): https://codereview.chromium.org/415343002/diff/100001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs.h:38: scoped_ptr<PrefChangeRegistrar> pref_change_registrar_; On 2014/07/25 23:49:34, robliao wrote: > Declaration order - These go last, but before the DISALLOW by convention. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order Done. https://codereview.chromium.org/415343002/diff/100001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs_android.h (right): https://codereview.chromium.org/415343002/diff/100001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.h:44: // FontSizePrefs::Observer implementation. On 2014/07/25 23:49:34, robliao wrote: > The above comment covers this so you can remove it. Done.
Are you intending this to concurrently commit with 170468 on the Clank side? https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:130: if (oldTextScale < threshold Ah, gotcha. I think I now get what this code does. On 2014/07/28 21:17:19, sunangel wrote: > I don't think this works... > For example, old=0.5, new = 1.5, threshold=1.3, change=1 would make the bool > false in this case but should be true > > On 2014/07/25 23:49:34, robliao wrote: > > I think you can take both of these... > > oldTextScale < threshold <= newTextScale > > newTextScale < threshold <= oldTextScale > > > > And roll them into... > > float changeInTextScale = newTextScale - oldTextScale; > > boolean changeExceedsThreshold = Math.abs(changeInTextScale) > > > FORCE_ENABLE_ZOOM_THRESHOLD_MULTIPLIER; > > > > Then the next comparison would be > > if (changeInTextScale > 0 && !getForceEnableZoom) { ... >
Updated in response to comments from upstream CL https://chrome-internal-review.googlesource.com/#/c/170468/ Please see new patch as compared to patch 5 (base including sync).
https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:114: if (wrappedObserver != null) { On 2014/07/28 21:17:20, sunangel wrote: > If someone calls removeObserver(new Observer)...Ideally no one would call this > but it's just for safety checking. > On 2014/07/25 23:49:34, robliao wrote: > > When is this expected to be null? > It should be okay to crash here. Callers should know that they're unregistering an invalid observer. https://codereview.chromium.org/415343002/diff/230001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/230001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:105: if (mObserverMap.get(obs) == null) { Maybe useful to assert a double registration has occurred. https://codereview.chromium.org/415343002/diff/230001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs_android.h (right): https://codereview.chromium.org/415343002/diff/230001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.h:45: PrefService* pref_service_; Make this const.
https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:114: if (wrappedObserver != null) { Tommy asked me to add this in because it was originally not null checked. On 2014/07/30 22:09:07, robliao wrote: > On 2014/07/28 21:17:20, sunangel wrote: > > If someone calls removeObserver(new Observer)...Ideally no one would call this > > but it's just for safety checking. > > On 2014/07/25 23:49:34, robliao wrote: > > > When is this expected to be null? > > > > It should be okay to crash here. Callers should know that they're unregistering > an invalid observer. https://codereview.chromium.org/415343002/diff/230001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/230001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:105: if (mObserverMap.get(obs) == null) { On 2014/07/30 22:09:07, robliao wrote: > Maybe useful to assert a double registration has occurred. Changed return instead. https://codereview.chromium.org/415343002/diff/230001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs_android.h (right): https://codereview.chromium.org/415343002/diff/230001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.h:45: PrefService* pref_service_; Cannot because later, call set and non-const member functions. On 2014/07/30 22:09:07, robliao wrote: > Make this const.
https://codereview.chromium.org/415343002/diff/230001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs_android.h (right): https://codereview.chromium.org/415343002/diff/230001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.h:45: PrefService* pref_service_; On 2014/07/31 00:29:26, sunangel wrote: > Cannot because later, call set and non-const member functions. > On 2014/07/30 22:09:07, robliao wrote: > > Make this const. > Should be PrefService* const pref_service_; That should be okay.
https://codereview.chromium.org/415343002/diff/230001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs_android.h (right): https://codereview.chromium.org/415343002/diff/230001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.h:45: PrefService* pref_service_; On 2014/07/31 00:53:40, robliao wrote: > On 2014/07/31 00:29:26, sunangel wrote: > > Cannot because later, call set and non-const member functions. > > On 2014/07/30 22:09:07, robliao wrote: > > > Make this const. > > > > Should be PrefService* const pref_service_; > > That should be okay. Done.
On 2014/07/31 01:09:55, sunangel wrote: > https://codereview.chromium.org/415343002/diff/230001/chrome/browser/accessib... > File chrome/browser/accessibility/font_size_prefs_android.h (right): > > https://codereview.chromium.org/415343002/diff/230001/chrome/browser/accessib... > chrome/browser/accessibility/font_size_prefs_android.h:45: PrefService* > pref_service_; > On 2014/07/31 00:53:40, robliao wrote: > > On 2014/07/31 00:29:26, sunangel wrote: > > > Cannot because later, call set and non-const member functions. > > > On 2014/07/30 22:09:07, robliao wrote: > > > > Make this const. > > > > > > > Should be PrefService* const pref_service_; > > > > That should be okay. > > Done. Some nits. Go ahead and add OWNERs.
Hi Tommy, Please take a look when you have a chance. Thanks! Angie
Huh, these didn't get published. https://codereview.chromium.org/415343002/diff/330001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/330001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:31: private static HashMap<Profile, FontSizePrefs> sFontSizeMap = Make this final. https://codereview.chromium.org/415343002/diff/330001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:107: FontSizePrefsObserverWrapper wrappedObserver = new New should go on next line. https://codereview.chromium.org/415343002/diff/330001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:107: FontSizePrefsObserverWrapper wrappedObserver = new new should go on next line. https://codereview.chromium.org/415343002/diff/330001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs_android.cc (right): https://codereview.chromium.org/415343002/diff/330001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.cc:87: FOR_EACH_OBSERVER(Observer, Consistency: use the 4 space linebreak that OnForceEnableZoomPrefsChanged below. https://codereview.chromium.org/415343002/diff/330001/chrome/browser/android/... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/415343002/diff/330001/chrome/browser/android/... chrome/browser/android/chrome_jni_registrar.cc:142: { "FontSizePrefsAndroid", FontSizePrefsAndroid::Register}, Add space after register
https://codereview.chromium.org/415343002/diff/330001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/330001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:31: private static HashMap<Profile, FontSizePrefs> sFontSizeMap = On 2014/08/01 17:36:33, robliao wrote: > Make this final. Done. https://codereview.chromium.org/415343002/diff/330001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:107: FontSizePrefsObserverWrapper wrappedObserver = new On 2014/08/01 17:36:33, robliao wrote: > new should go on next line. Done. https://codereview.chromium.org/415343002/diff/330001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs_android.cc (right): https://codereview.chromium.org/415343002/diff/330001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.cc:87: FOR_EACH_OBSERVER(Observer, On 2014/08/01 17:36:33, robliao wrote: > Consistency: use the 4 space linebreak that OnForceEnableZoomPrefsChanged below. Done...but this disagrees with git cl format. Not sure why it formats one but not the other. https://codereview.chromium.org/415343002/diff/330001/chrome/browser/android/... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/415343002/diff/330001/chrome/browser/android/... chrome/browser/android/chrome_jni_registrar.cc:142: { "FontSizePrefsAndroid", FontSizePrefsAndroid::Register}, On 2014/08/01 17:36:33, robliao wrote: > Add space after register Done.
lgtm https://codereview.chromium.org/415343002/diff/370001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/370001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:120: if (wrappedObserver != null) { Use early return pattern if (wrapperObserver == null) return false;
https://codereview.chromium.org/415343002/diff/370001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/370001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:120: if (wrappedObserver != null) { On 2014/08/04 17:09:57, robliao wrote: > Use early return pattern if (wrapperObserver == null) return false; Done.
https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:20: * Interface for Font Scale Factor, Force Enable Zoom, and User Set Force Enable Zoom preferences. This is not an interface. Could you try to describe what these preferences are used for? How do they work together? What happens if you change them? https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:31: private final static HashMap<Profile, FontSizePrefs> sFontSizeMap = static before members. https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:34: /* /** throughout this file for these style comments. https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:36: * this observer does not observe changes in UserSetForceEnableZoom. Why? This sounds unreasonable. https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:46: public static class FontSizePrefsObserverWrapper { can this class be private? https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:80: public FontSizePrefs(Profile profile, Context context) { private? https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:82: mSharedPreferences = PreferenceManager.getDefaultSharedPreferences(context); Shouldn't there be an observer for the shared preferences in this class? Maybe add it in the constructor? See http://developer.android.com/reference/android/content/SharedPreferences.OnSh... https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:132: sharedPreferencesEditor.putBoolean(PREF_USER_SET_FORCE_ENABLE_ZOOM, enabled); Are these per profile? I don't think they are, and in that case, I believe we should probably just have a singleton instance of this class instead of a map of them. :-( Which means getInstance(Context context) method. Another thing you could do, is do what I described here first, and then add a migration step for old users downstream separately, and make this then be keyed off profile. https://codereview.chromium.org/415343002/diff/390001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs_android.cc (right): https://codereview.chromium.org/415343002/diff/390001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.cc:59: bool FontSizePrefsAndroid::Register(JNIEnv* env) { could you have this after add/remove observer? https://codereview.chromium.org/415343002/diff/390001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.cc:79: jlong observerPtr) { nit: hackercase in C++, not camelCase throughout this file. https://codereview.chromium.org/415343002/diff/390001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs_android.h (right): https://codereview.chromium.org/415343002/diff/390001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.h:16: class FontSizePrefsAndroid { What does this class do? Does it have a corresponding java class? https://codereview.chromium.org/415343002/diff/390001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.h:52: class FontSizePrefsObserverAndroid : public FontSizePrefsAndroid::Observer { same as above? Refer to Java class?
https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:20: * Interface for Font Scale Factor, Force Enable Zoom, and User Set Force Enable Zoom preferences. Okay tried to describe it better. They are not so On 2014/08/06 23:39:25, nyquist wrote: > This is not an interface. Could you try to describe what these preferences are > used for? How do they work together? What happens if you change them? https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:31: private final static HashMap<Profile, FontSizePrefs> sFontSizeMap = On 2014/08/06 23:39:25, nyquist wrote: > static before members. Done. https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:34: /* On 2014/08/06 23:39:25, nyquist wrote: > /** throughout this file for these style comments. Done. https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:36: * this observer does not observe changes in UserSetForceEnableZoom. Haha okay changed this. On 2014/08/06 23:39:25, nyquist wrote: > Why? This sounds unreasonable. https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:46: public static class FontSizePrefsObserverWrapper { On 2014/08/06 23:39:25, nyquist wrote: > can this class be private? Done. https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:80: public FontSizePrefs(Profile profile, Context context) { On 2014/08/06 23:39:25, nyquist wrote: > private? Done. https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:82: mSharedPreferences = PreferenceManager.getDefaultSharedPreferences(context); On 2014/08/06 23:39:26, nyquist wrote: > Shouldn't there be an observer for the shared preferences in this class? Maybe > add it in the constructor? See > http://developer.android.com/reference/android/content/SharedPreferences.OnSh... Done. https://codereview.chromium.org/415343002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:132: sharedPreferencesEditor.putBoolean(PREF_USER_SET_FORCE_ENABLE_ZOOM, enabled); On 2014/08/06 23:39:25, nyquist wrote: > Are these per profile? I don't think they are, and in that case, I believe we > should probably just have a singleton instance of this class instead of a map of > them. :-( Which means getInstance(Context context) method. > > Another thing you could do, is do what I described here first, and then add a > migration step for old users downstream separately, and make this then be keyed > off profile. Done. https://codereview.chromium.org/415343002/diff/390001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs_android.cc (right): https://codereview.chromium.org/415343002/diff/390001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.cc:59: bool FontSizePrefsAndroid::Register(JNIEnv* env) { On 2014/08/06 23:39:26, nyquist wrote: > could you have this after add/remove observer? Done. https://codereview.chromium.org/415343002/diff/390001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.cc:79: jlong observerPtr) { On 2014/08/06 23:39:26, nyquist wrote: > nit: hackercase in C++, not camelCase > throughout this file. Done. https://codereview.chromium.org/415343002/diff/390001/chrome/browser/accessib... File chrome/browser/accessibility/font_size_prefs_android.h (right): https://codereview.chromium.org/415343002/diff/390001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.h:16: class FontSizePrefsAndroid { Tried to describe it better...again not sure if it was good enough :( On 2014/08/06 23:39:26, nyquist wrote: > What does this class do? Does it have a corresponding java class? https://codereview.chromium.org/415343002/diff/390001/chrome/browser/accessib... chrome/browser/accessibility/font_size_prefs_android.h:52: class FontSizePrefsObserverAndroid : public FontSizePrefsAndroid::Observer { On 2014/08/06 23:39:26, nyquist wrote: > same as above? Refer to Java class? Done.
https://codereview.chromium.org/415343002/diff/450001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/450001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:34: private final SharedPreferences.Editor mSharedPreferencesEditor; I think I'd get this on-the-fly instead. https://codereview.chromium.org/415343002/diff/450001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:156: if (key.equals(PREF_USER_SET_FORCE_ENABLE_ZOOM)) { Flip this (PREF_US...equals(key)). https://codereview.chromium.org/415343002/diff/450001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java (right): https://codereview.chromium.org/415343002/diff/450001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:24: private SharedPreferences.Editor mSharedPreferencesEditor; This is unused, so remove this. https://codereview.chromium.org/415343002/diff/450001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:29: Context context = getInstrumentation().getTargetContext(); How about Instrumentation context = new AdvancedMockContext(getInstrumentation().getTargetContext())? It has automatic support for InMemorySharedPreferences. https://codereview.chromium.org/415343002/diff/450001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:37: mSharedPreferencesEditor.remove(FontSizePrefs.PREF_USER_SET_FORCE_ENABLE_ZOOM).commit(); This teardown should not be necessary for the in-memory version.
https://codereview.chromium.org/415343002/diff/450001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java (right): https://codereview.chromium.org/415343002/diff/450001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:34: private final SharedPreferences.Editor mSharedPreferencesEditor; On 2014/08/07 23:15:42, nyquist wrote: > I think I'd get this on-the-fly instead. Done. https://codereview.chromium.org/415343002/diff/450001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:156: if (key.equals(PREF_USER_SET_FORCE_ENABLE_ZOOM)) { On 2014/08/07 23:15:42, nyquist wrote: > Flip this (PREF_US...equals(key)). Done. https://codereview.chromium.org/415343002/diff/450001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java (right): https://codereview.chromium.org/415343002/diff/450001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:24: private SharedPreferences.Editor mSharedPreferencesEditor; Need this for tear down. On 2014/08/07 23:15:42, nyquist wrote: > This is unused, so remove this. https://codereview.chromium.org/415343002/diff/450001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:29: Context context = getInstrumentation().getTargetContext(); Cannot change. On 2014/08/07 23:15:42, nyquist wrote: > How about Instrumentation context = new > AdvancedMockContext(getInstrumentation().getTargetContext())? > > It has automatic support for InMemorySharedPreferences. https://codereview.chromium.org/415343002/diff/450001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:37: mSharedPreferencesEditor.remove(FontSizePrefs.PREF_USER_SET_FORCE_ENABLE_ZOOM).commit(); On 2014/08/07 23:15:42, nyquist wrote: > This teardown should not be necessary for the in-memory version. Cannot.
add a newline after the first line of the CL description and also update it to the current behavior. How about: ### Upstream accessibility font size preferences. Moves ForceEnableZoom, UserSetForceEnableZoom and FontScaleFactor preferences upstream and adds support for observing all those preferences in Java. UserSetForceEnableZoom is currently stored using Android SharedPreferences, which are application global, so this class is currently not keyed off the profile. Preferably this should be migrated to be per profile, at which point the FontSizePrefs could be keyed off the profile instead of being an application global singleton. BUG=383630 ### otherwise 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/415343002/490001
The CQ bit was unchecked by sunangel@chromium.org
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/415343002/510001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
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/415343002/530001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
https://codereview.chromium.org/415343002/diff/530001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java (right): https://codereview.chromium.org/415343002/diff/530001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:48: assertEquals(newTextScale, mFontSizePrefs.getFontScaleFactor()); From the test crash it seems like you would have to wrap the getters as well. [FATAL:pref_service.cc(120)] Check failed: CalledOnValidThread(). signal 6 (SIGABRT) at 0x00006424 (code=-6), thread 25650 (ationTestRunner) pid: 25636, tid: 25650, name: ationTestRunner >>> org.chromium.chrome.shell <<< signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr -------- r0 00000000 r1 00006432 r2 00000006 r3 00000000 r4 00000006 r5 00000000 r6 00006432 r7 0000010c r8 75415828 r9 4013d384 sl 00000000 fp 7541599c ip 754153ac sp 75415228 lr 40101fe5 pc 40110f90 Stack Trace: RELADDR FUNCTION FILE:LINE 00021f90 tgkill+12 /system/lib/libc.so 00012fe1 pthread_kill+48 /system/lib/libc.so 000131f5 raise+10 /system/lib/libc.so 00011f2b <unknown> /system/lib/libc.so 00021844 abort+4 /system/lib/libc.so 00231099 base::debug::BreakDebugger()+8 libgcc2.c:0 00247eb3 logging::LogMessage::~LogMessage()+358 libgcc2.c:0 006ee7ed PrefService::GetDouble(char const*) const+56 libgcc2.c:0 002b25e7 FontSizePrefsAndroid::GetFontScaleFactor(_JNIEnv*, _jobject*)+10 libgcc2.c:0 002b2839 GetFontScaleFactor(_JNIEnv*, _jobject*, long long)+76 libgcc2.c:0 0001dbcc dvmPlatformInvoke+112 /system/lib/libdvm.so 0004e123 dvmCallJNIMethod(unsigned int const*, JValue*, Method const*, Thread*)+398 /system/lib/libdvm.so 00026fe0 <unknown> /system/lib/libdvm.so 0002dfa0 dvmMterpStd(Thread*)+76 /system/lib/libdvm.so 0002b638 dvmInterpret(Thread*, Method const*, JValue*)+184 /system/lib/libdvm.so 00060865 dvmInvokeMethod(Object*, Method const*, ArrayObject*, ArrayObject*, ClassObject*, bool)+392 /system/lib/libdvm.so 000687c7 <unknown> /system/lib/libdvm.so 00026fe0 <unknown> /system/lib/libdvm.so 0002dfa0 dvmMterpStd(Thread*)+76 /system/lib/libdvm.so 0002b638 dvmInterpret(Thread*, Method const*, JValue*)+184 /system/lib/libdvm.so 00060581 dvmCallMethodV(Thread*, Method const*, Object*, bool, JValue*, std::__va_list)+336 /system/lib/libdvm.so 000605a5 dvmCallMethod(Thread*, Method const*, Object*, JValue*, ...)+20 /system/lib/libdvm.so 0005528b <unknown> /system/lib/libdvm.so 0000d170 __thread_entry+72 /system/lib/libc.so 0000d308 pthread_create+240 /system/lib/libc.so
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/415343002/550001
Message was sent while issue was closed.
Change committed as 289185
Message was sent while issue was closed.
https://codereview.chromium.org/415343002/diff/550001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/415343002/diff/550001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:16: 'browser/android/accessibility/font_size_prefs_android.h', 'browser/android/accessibility_util.cc', 1 file name per line please.
Message was sent while issue was closed.
https://codereview.chromium.org/415343002/diff/550001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/415343002/diff/550001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:16: 'browser/android/accessibility/font_size_prefs_android.h', 'browser/android/accessibility_util.cc', On 2014/09/03 01:47:32, Lei Zhang wrote: > 1 file name per line please. see https://codereview.chromium.org/533173003/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
