1052595 - Support autocomplete pref

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

We should respect the browser.urlbar.autocomplete.enabled pref, which is included in Fennec but didn't do anything.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Respect toolbar autocomplete enabled pref (v1)

Add pref support for toolbar autocomplete and don't autocomplete if the pref is set to disabled.

Comment 3

[Lucas Rocha (:lucasr)]

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.

Comment 4

[Lucas Rocha (:lucasr)]

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.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8472679 [details] [diff] [review]
Make BrowserToolbar own toolbar prefs (v2)

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

Awesome.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8472680 [details] [diff] [review]
Bug 1052595 - Respect toolbar autocomplete enabled pref;

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

Great.

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/fx-team/rev/62ca6c36df49
https://hg.mozilla.org/integration/fx-team/rev/d482acc75ce2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/62ca6c36df49
https://hg.mozilla.org/mozilla-central/rev/d482acc75ce2

Comment 11

[Kevin Brosnan [Ex-Mozilla]]

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

Comment 12

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

(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.

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 15

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in
Builds: 
Firefox for Android 34.0a2 (2014-09-21)

Device:
Asus Transformer Pad TF300T (Android 4.2.1)