Closed Bug 868845 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Linux
defect
Not set
normal

Tracking

(firefox23 fixed, firefox24 fixed)

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?
Depends on: 778216
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: 11 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+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: