New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 864257 link

Starred by 1 user
Status: Fixed
Owner:
█ romax@chromium.org
Last visit > 30 days ago
Closed: Jul 27
Cc:
twelling...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Cleaning up ChromePreferenceManager

Project Member Reported by romax@chromium.org, Jul 16 
We are having too many methods in the class and once a new feature is added, there will be two more methods.

quoting comment from twellington@ (https://chromium-review.googlesource.com/c/chromium/src/+/1134587):
"""
Instead of adding new methods here, can we add generic #readBoolean and #writeBoolean similar to what we did for #readInt and #writeInt & contextual search (see public static's at the top of this file)?

We've seen an explosion in methods here and it would be nice if we didn't need to add two new methods for each new preference.
"""

However I'm concerned about how to deal with the default values. Seems the previous approach only works for preference values with a default value of 0.

There might be 2 ways:
1. Keep default value predefined and if the pref is not set but asked, return the default value.
2. Create two methods: readBooleanWithDefaultFalse() and readBooleanWithDefaultTrue().

Comment 1 by twelling...@chromium.org, Jul 16 

Another option is to create a #readBoolean(String key, boolean defaultValue) so that callers are responsible for determining the default.

Comment 2 by romax@chromium.org, Jul 16 

Yes.

I thought the default value here should be the same with the default value of the corresponding flag (?) But seems like it's not straightforward to build a link between preferences and native flags (like  chrome/browser/android/chrome_feature_list.cc)

Initially I was thinking that letting every caller be responsible for the default value of their call might need them having too much information. But now I don't really have a strong preference on which way to go.

Do you have any strong opinions here?

Comment 3 by twelling...@chromium.org, Jul 17 

> I thought the default value here should be the same with the default value of the corresponding flag (?) But seems like it's not straightforward to build a link between preferences and native flags (like  chrome/browser/android/chrome_feature_list.cc)

We often use SharedPreferences to cache values that we need to read before native (and therefore chrome_feature_list.cc) is loaded, so we shouldn't do this.

> Initially I was thinking that letting every caller be responsible for the default value of their call might need them having too much information. But now I don't really have a strong preference on which way to go.

I don't think that's too much information. The caller already needs to know what it's semantically retrieving. We were previously baking that into the individual methods in ChromePreferenceManager.. now we'd be baking into the call sites, which seems fine to me.

Comment 4 by romax@chromium.org, Jul 27

Status: Started (was: Untriaged)
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 27 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ab438e424d63c4940e901f60077d636d28374512

commit ab438e424d63c4940e901f60077d636d28374512
Author: Yafei Duan <romax@chromium.org>
Date: Fri Jul 27 21:34:00 2018

Cleaning up boolean flags in ChromePreferenceManager

For boolean flags in ChromePreferenceManager, removed their setters and
getters, and let caller call writeBoolean/readBoolean directly with the
predefined keys.

Bug:  864257 
Change-Id: I80c6511ff2b8ca3139a4a50a26989aa5e4495703
Reviewed-on: https://chromium-review.googlesource.com/1141358
Commit-Queue: Yafei Duan <romax@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578806}
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/java/src/org/chromium/chrome/browser/component_updater/VrAssetsComponentInstaller.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/java/src/org/chromium/chrome/browser/instantapps/InstantAppsHandler.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/java/src/org/chromium/chrome/browser/preferences/SignInPreference.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsMetrics.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/java/src/org/chromium/chrome/browser/vr/VrShellDelegate.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/javatests/src/org/chromium/chrome/browser/partnercustomizations/PartnerHomepageUnitTest.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/HomeSheetCardsUiCaptureTest.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/HomeSheetNoTilesUiCaptureTest.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/HomeSheetTilesUiCaptureTest.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetUiCaptureTest.java
[modify] https://crrev.com/ab438e424d63c4940e901f60077d636d28374512/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java

Comment 6 by romax@chromium.org, Jul 27

Status: Fixed (was: Started)
leaving the flag for ChromeModernDesign behind, since it has >40 occurrences (https://cs.chromium.org/search/?q=ChromeModernDesignEnabled+lang:java&type=cs).
►

Sign in to add a comment