1066250 - Consider always showing URL in toolbar in new tablet UI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

As we we'll show title in the tab strip anyway. No need to show title twice in the UI.

Comment 1

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Sounds good to me!

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

And it's like more desktop too!

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Always show the url in the toolbar on new tablet

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Remove title in toolbar preference on new tablet

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8488282 [details] [diff] [review]
Part 1: Always show the url in the toolbar on new tablet

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

Nice.

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
@@ +322,5 @@
>              return;
>          }
>  
>          // If the pref to show the URL isn't set, just use the tab's display title.
> +        if (!mPrefs.shouldShowUrl(getContext()) || url == null) {

I think you can just use mActivity instead of getContext() here.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8488285 [details] [diff] [review]
Part 2: Remove title in toolbar preference on new tablet

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

You probably need to update testSettingsMenuItems accordingly.

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +726,5 @@
>                              return true;
>                          }
>                      });
> +                } else if (PREFS_DISPLAY_TITLEBAR_MODE.equals(key) &&
> +                           NewTabletUI.isEnabled(getApplicationContext())) {

It's safe to just use 'this' instead of getApplicationContext() here.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Always show the url in the toolbar on new tablet

Address nit.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Remove title in toolbar preference on new tablet

Address nit.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Tests in part 3.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 3: Update testSettingsMenuItems

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

try for part 3 (comment 10): https://tbpl.mozilla.org/?tree=Try&rev=90ec2f68e989

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 8488947 [details] [diff] [review]
Part 3: Update testSettingsMenuItems

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

Looks good.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/84d550c7cee2
https://hg.mozilla.org/integration/fx-team/rev/74e510d20ad3
https://hg.mozilla.org/integration/fx-team/rev/f00d45e4c159

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/84d550c7cee2
https://hg.mozilla.org/mozilla-central/rev/74e510d20ad3
https://hg.mozilla.org/mozilla-central/rev/f00d45e4c159