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)
Tracking
(firefox23 fixed, firefox24 fixed)
RESOLVED
FIXED
Firefox 24
People
(Reporter: tetsuharu, Assigned: tetsuharu)
References
Details
(Whiteboard: [block uplift on bug 872504])
Attachments
(1 file, 1 obsolete file)
14.71 KB,
patch
|
wesj
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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?
Comment 1•11 years ago
|
||
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)
Comment 2•11 years ago
|
||
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)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #747141 -
Flags: review?(wjohnston)
Comment 4•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #747141 -
Flags: review?(wjohnston) → review+
Assignee | ||
Comment 5•11 years ago
|
||
(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
Assignee | ||
Comment 6•11 years ago
|
||
Update the patch.
Attachment #747141 -
Attachment is obsolete: true
Attachment #747611 -
Flags: review?(wjohnston)
Assignee | ||
Comment 7•11 years ago
|
||
try: https://tbpl.mozilla.org/?tree=Try&rev=9ca359eaa754
Updated•11 years ago
|
Attachment #747611 -
Flags: review?(wjohnston) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 8•11 years ago
|
||
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?
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5438b25832b0
Assignee: nobody → saneyuki.s.snyk
Keywords: checkin-needed
Comment 10•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5438b25832b0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Comment 11•11 years ago
|
||
(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.
Updated•11 years ago
|
Whiteboard: [block uplift on bug bug 872504]
Updated•11 years ago
|
Whiteboard: [block uplift on bug bug 872504] → [block uplift on bug 872504]
Comment 12•11 years ago
|
||
triage drive-by: waiting for aurora nomination on bug 872504
Updated•11 years ago
|
status-firefox23:
--- → affected
status-firefox24:
--- → fixed
Updated•11 years ago
|
Attachment #747611 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•