Closed Bug 868845 Opened 7 years ago Closed 7 years ago

Convert the way to store preference of show url to pref.js.

Categories

(Firefox for Android :: General, defect)

ARM
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 24
Tracking Status
firefox23 --- fixed
firefox24 --- fixed

People

(Reporter: tetsuharu, Assigned: tetsuharu)

References

Details

(Whiteboard: [block uplift on bug 872504])

Attachments

(1 file, 1 obsolete file)

This is proposal which is related to bug 778216: https://hg.mozilla.org/mozilla-central/rev/19d8817e53bb

I feel it's better to using pref.js about storing 'original' settings for this purpose. Are there any problem to using pref.js for performance?
By pref.js, do you mean the gecko preferences service (Services.prefs)?

I bet wesj just made this a java pref because we don't need it anywhere on the gecko side right now. There's a lot of discussion in bug 778216, which I didn't read closely. Maybe wesj can explain that decision.
Flags: needinfo?(wjohnston)
Yeah. I just didn't see any reason to make it a gecko side pref really. I'm not sure we want the extra code complexity, but I'd look at a patch.
Flags: needinfo?(wjohnston)
Attached patch patch (obsolete) — Splinter Review
Attachment #747141 - Flags: review?(wjohnston)
Comment on attachment 747141 [details] [diff] [review]
patch

Review of attachment 747141 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! I wish we could just use a bool pref, but the simplicity of this with the preference screen implementation makes sense.

::: mobile/android/base/BrowserToolbar.java
@@ +136,5 @@
>          mAnimatingEntry = false;
>          mShowUrl = false;
>  
> +        // listen to the title bar pref.
> +        mPrefObserverId = PrefsHelper.getPref(PREF_TITLEBAR_MODE, new PrefsHelper.PrefHandlerBase() {

I don't really like these inline classes. How do you feel about just making BrowserToolbar implement PrefHandlerBase?

@@ +141,4 @@
>              @Override
> +            public void prefValue(String pref, String str) {
> +                int value = Integer.parseInt(str);
> +                boolean isShowUrl = false;

Lets try something like shouldShowUrl. Since there aren't likely to ever be other modes (and we can update if there are), I'd just do shouldShowUrl = (value == 1);

@@ +144,5 @@
> +                boolean isShowUrl = false;
> +                if (value == 0) {
> +                    isShowUrl = false;
> +                }
> +                else if (value == 1) {

Put the else and the } brace on the same line.

::: mobile/android/base/resources/values/arrays.xml
@@ +114,5 @@
> +        <item>@string/pref_titlebar_mode_url</item>
> +    </string-array>
> +    <string-array name="pref_titlebar_mode_values">
> +        <item>0</item>
> +        <item>1</item>

Nice!

::: mobile/android/chrome/content/browser.js
@@ +1032,5 @@
>        // indicating enabled/disabled. Since the Java UI uses the type to
>        // determine which ui elements to show, we need to normalize these
>        // preferences to be actual booleans.
>        switch (prefName) {
> +        case "browser.chrome.titlebar_mode":

Use camel case here. browser.chrome.titlebarMode
Attachment #747141 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #4)
> Nice! I wish we could just use a bool pref, but the simplicity of this with
> the preference screen implementation makes sense.

Thank you!
I hesitated about that whether we should use a bool pref or not. However, I decided with these reasons:
* the simplicity with the curren implementation of preference screen (Thank you for your pointing.)
* I thought that it'll be good if we use int pref as "mode". Because we can keep it for the future expandability. 


> ::: mobile/android/base/BrowserToolbar.java
> @@ +136,5 @@
> >          mAnimatingEntry = false;
> >          mShowUrl = false;
> >  
> > +        // listen to the title bar pref.
> > +        mPrefObserverId = PrefsHelper.getPref(PREF_TITLEBAR_MODE, new PrefsHelper.PrefHandlerBase() {
> 
> I don't really like these inline classes. How do you feel about just making
> BrowserToolbar implement PrefHandlerBase?

I thought we should use a inline class now from other parts in our current code base: http://mxr.mozilla.org/mozilla-central/search?string=PrefHandler&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Attached patch patch v2Splinter Review
Update the patch.
Attachment #747141 - Attachment is obsolete: true
Attachment #747611 - Flags: review?(wjohnston)
Attachment #747611 - Flags: review?(wjohnston) → review+
Comment on attachment 747611 [details] [diff] [review]
patch v2

Review of attachment 747611 [details] [diff] [review]:
-----------------------------------------------------------------

This change will resets the pref of showing an url in address bar. I think it's better for user that we land this patch on same release version which is introduced the feature of showing an url in address bar.
Attachment #747611 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/5438b25832b0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Depends on: 872504
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #8)
> Comment on attachment 747611 [details] [diff] [review]
> patch v2
> 
> Review of attachment 747611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This change will resets the pref of showing an url in address bar. I think
> it's better for user that we land this patch on same release version which
> is introduced the feature of showing an url in address bar.

This shouldn't be uplifted until bug 872504 is fixed.
Whiteboard: [block uplift on bug bug 872504]
Whiteboard: [block uplift on bug bug 872504] → [block uplift on bug 872504]
triage drive-by: waiting for aurora nomination on bug 872504
Attachment #747611 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.