Support autocomplete pref

VERIFIED FIXED in Firefox 34

Status

()

Firefox for Android
Awesomescreen
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

Trunk
Firefox 34
All
Android
Points:
---
Bug Flags:
in-moztrap ?

Firefox Tracking Flags

(firefox34 verified, relnote-firefox 34+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
We should respect the browser.urlbar.autocomplete.enabled pref, which is included in Fennec but didn't do anything.
(Assignee)

Comment 1

4 years ago
Created attachment 8471734 [details] [diff] [review]
Make BrowserToolbar own toolbar prefs (v1)

I think we should reuse ToolbarTitlePrefs and make it cover toolbar prefs in general. This patch renames ToolbarTitlePrefs to ToolbarPrefs and makes BrowserToolbar own an instance of ToolbarPrefs. ToolbarDisplayLayout then receives prefs through the ToolbarDisplayLayout.setPrefs call.
Attachment #8471734 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 2

4 years ago
Created attachment 8471736 [details] [diff] [review]
Respect toolbar autocomplete enabled pref (v1)

Add pref support for toolbar autocomplete and don't autocomplete if the pref is set to disabled.
Attachment #8471736 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8471734 [details] [diff] [review]
Make BrowserToolbar own toolbar prefs (v1)

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

Looks good overall. I'd like to avoid breaking ToolbarPrefs encapsulation though.

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
@@ +96,5 @@
>      private boolean mIsAttached;
>  
>      private ThemedTextView mTitle;
>      private int mTitlePadding;
> +    private ToolbarPrefs prefs;

Use mPrefs for in-file consistency. Feel free to submit a separate patch changing the coding style for the whole file.

@@ +254,5 @@
>          mSiteSecurity.setNextFocusDownId(nextId);
>          mPageActionLayout.setNextFocusDownId(nextId);
>      }
>  
> +    void setPrefs(final ToolbarPrefs prefs) {

setToolbarPrefs() for consistency.

::: mobile/android/base/toolbar/ToolbarPrefs.java
@@ -48,5 @@
>               mPrefObserverId = null;
>          }
>      }
>  
> -    private class TitlePrefsHandler extends PrefsHelper.PrefHandlerBase {

This change breaks ToolbarPrefs's encapsulation. You can simply create a TitlePrefsHandler instance in the constructor and reuse it whenever open() triggers a PrefsHelper.getPrefs() call.
Attachment #8471734 - Flags: review?(lucasr.at.mozilla) → feedback+
Comment on attachment 8471736 [details] [diff] [review]
Respect toolbar autocomplete enabled pref (v1)

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

Looks good, just wonder if this could be optimized a bit so that we don't do unnecessary work in BrowserSearch.

::: mobile/android/base/toolbar/ToolbarEditText.java
@@ +50,5 @@
>      private OnCommitListener mCommitListener;
>      private OnDismissListener mDismissListener;
>      private OnFilterListener mFilterListener;
>  
> +    private ToolbarPrefs prefs;

mPrefs for in-file consistency.

@@ +117,5 @@
>          // Any autocomplete text would have been overwritten, so reset our autocomplete states.
>          resetAutocompleteState();
>      }
>  
> +    void setPrefs(final ToolbarPrefs prefs) {

setToolbarPrefs for consistency.

@@ +247,5 @@
>      @Override
>      public final void onAutocomplete(final String result) {
>          // If mDiscardAutoCompleteResult is true, we temporarily disabled
>          // autocomplete (due to backspacing, etc.) and we should bail early.
> +        if (mDiscardAutoCompleteResult || !prefs.shouldAutocomplete()) {

At this point, we've already done all the work to autocomplete i.e. iterate the cursor, find matching entries, etc. Wouldn't is better do it here?

http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ToolbarEditText.java#465

In other words, initialize it like:

doAutocomplete = prefs.shouldAutocomplete();

This way browser search we simply not do anything around autocompletion.
Attachment #8471736 - Flags: review?(lucasr.at.mozilla) → feedback+
(Assignee)

Comment 5

4 years ago
Created attachment 8472679 [details] [diff] [review]
Make BrowserToolbar own toolbar prefs (v2)

(In reply to Lucas Rocha (:lucasr) from comment #3)
> Comment on attachment 8471734 [details] [diff] [review]
> Make BrowserToolbar own toolbar prefs (v1)
> 
> Review of attachment 8471734 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good overall. I'd like to avoid breaking ToolbarPrefs encapsulation
> though.
> 
> ::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
> @@ +96,5 @@
> >      private boolean mIsAttached;
> >  
> >      private ThemedTextView mTitle;
> >      private int mTitlePadding;
> > +    private ToolbarPrefs prefs;
> 
> Use mPrefs for in-file consistency. Feel free to submit a separate patch
> changing the coding style for the whole file.

Done.

> @@ +254,5 @@
> >          mSiteSecurity.setNextFocusDownId(nextId);
> >          mPageActionLayout.setNextFocusDownId(nextId);
> >      }
> >  
> > +    void setPrefs(final ToolbarPrefs prefs) {

Done.

> ::: mobile/android/base/toolbar/ToolbarPrefs.java
> @@ -48,5 @@
> >               mPrefObserverId = null;
> >          }
> >      }
> >  
> > -    private class TitlePrefsHandler extends PrefsHelper.PrefHandlerBase {
> 
> This change breaks ToolbarPrefs's encapsulation. You can simply create a
> TitlePrefsHandler instance in the constructor and reuse it whenever open()
> triggers a PrefsHelper.getPrefs() call.

Done.
Attachment #8471734 - Attachment is obsolete: true
Attachment #8472679 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 6

4 years ago
Created attachment 8472680 [details] [diff] [review]
Bug 1052595 - Respect toolbar autocomplete enabled pref;

(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 8471736 [details] [diff] [review]
> Respect toolbar autocomplete enabled pref (v1)
> 
> Review of attachment 8471736 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just wonder if this could be optimized a bit so that we don't do
> unnecessary work in BrowserSearch.
> 
> ::: mobile/android/base/toolbar/ToolbarEditText.java
> @@ +50,5 @@
> >      private OnCommitListener mCommitListener;
> >      private OnDismissListener mDismissListener;
> >      private OnFilterListener mFilterListener;
> >  
> > +    private ToolbarPrefs prefs;
> 
> mPrefs for in-file consistency.

Done.

> @@ +117,5 @@
> >          // Any autocomplete text would have been overwritten, so reset our autocomplete states.
> >          resetAutocompleteState();
> >      }
> >  
> > +    void setPrefs(final ToolbarPrefs prefs) {

Done.

> @@ +247,5 @@
> >      @Override
> >      public final void onAutocomplete(final String result) {
> >          // If mDiscardAutoCompleteResult is true, we temporarily disabled
> >          // autocomplete (due to backspacing, etc.) and we should bail early.
> > +        if (mDiscardAutoCompleteResult || !prefs.shouldAutocomplete()) {
> 
> At this point, we've already done all the work to autocomplete i.e. iterate
> the cursor, find matching entries, etc. Wouldn't is better do it here?
> 
> http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/
> ToolbarEditText.java#465
> 
> In other words, initialize it like:
> 
> doAutocomplete = prefs.shouldAutocomplete();
> 
> This way browser search we simply not do anything around autocompletion.

Good idea! Fixed.
Attachment #8471736 - Attachment is obsolete: true
Attachment #8472680 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8472679 [details] [diff] [review]
Make BrowserToolbar own toolbar prefs (v2)

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

Awesome.
Attachment #8472679 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8472680 [details] [diff] [review]
Bug 1052595 - Respect toolbar autocomplete enabled pref;

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

Great.
Attachment #8472680 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/62ca6c36df49
https://hg.mozilla.org/mozilla-central/rev/d482acc75ce2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Release Note Request (optional, but appreciated)
[Why is this notable]: Desktop parity
[Suggested wording]: Autocomplete pref now respected on Android
[Links (documentation, blog post, etc)]: This bug
relnote-firefox: --- → ?
Flags: in-moztrap?(fennec)
Added with "Autocomplete preference respected" as wording in the release notes. No need to specify "on android" because it will only show on the mobile page.
relnote-firefox: ? → 34+
(Assignee)

Comment 13

3 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #12)
> Added with "Autocomplete preference respected" as wording in the release
> notes. No need to specify "on android" because it will only show on the
> mobile page.

Can it be changed to "URL bar autocomplete preference respected"? Just "autocomplete" is a bit unclear.
Flags: needinfo?(sledru)
I changed the note to "Added URL bar autocomplete preference". This may not be 100% accurate but I think using the work "respected" did not make for a generally well understandable release note.
Flags: needinfo?(sledru)
Verified as fixed in
Builds: 
Firefox for Android 34.0a2 (2014-09-21)

Device:
Asus Transformer Pad TF300T (Android 4.2.1)
Status: RESOLVED → VERIFIED
status-firefox34: --- → verified
You need to log in before you can comment on or make changes to this bug.