1111729 - Default to show URL in toolbar instead of page title

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

For clarity about where the user is at, we should change the default here.

I think for now we should still keep the option available in settings, and we can monitor UI telemetry to see if people are changing it back.

Comment 1

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1111729/margaret

Comment 2

[:Margaret Leibovic]

/r/1483 - Bug 1111729 - Default to show URL in toolbar instead of page title

Pull down this commit:

hg pull review -r d877b5f53e447916d20ba4edbd8f73195ea0a495

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

https://reviewboard.mozilla.org/r/1481/#review883

Ship It!

Comment 4

[:Margaret Leibovic]

I suspect this may break some robocop tests. Exploratory try push:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a00d6a8a50e3

Comment 5

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8536770 [details]
MozReview Request: bz://1111729/margaret

Drive-by: Don't forget to remove the preference [1] and the Toolbar code where we handle it ([2] should be a start).

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/xml/preferences_display.xml#18
[2]: https://mxr.mozilla.org/mozilla-central/search?string=titlebarMode&find=\.java%24&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 6

[:Margaret Leibovic]

I mentioned in comment 0 that I think we should keep the preference, but just change the default. Maybe I'm wrong about this?

mfinkle/antlam, what do you think? Do you think we should leave the preference, or totally yank the feature?

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

I'm not convinced we need to keep the Setting. It's useless on Tablets now as well.

Telemetry for browser.chrome.titlebarMode (percentage of all Settings)
Nightly: 0.75%
Aurora:  1.05%
Beta:    0.77%
Release: 1.17%

We might as well go all the way.

Comment 8

[:Margaret Leibovic]

Okay, I'll update my patch and post for re-review. Good catch, Mike!

I also need to fix Robocop tests.

Comment 9

[:Margaret Leibovic]

This requires a hell of a lot of test updates. I worry that we may run into timing issues if we can't wait for the title to change to decide a page is done being loaded. However, this seems like a shitty way to determine that a page was loaded, so if there are intermittent issues here, maybe we should add a "load" listener. We currently wait for "DOMContentLoaded", but if a test want a page to be totally finished rendering, that's probably not a good thing to wait on.

In any case, these were passing locally (as many as I had patience to run), and I made a try push here to see if this works on automation:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b9944ac4dc29

Comment 10

[:Margaret Leibovic]

Cc'ing gbrown, since this is going to affect a lot of robocop tests.

Comment 11

[Geoff Brown [:gbrown]]

Thanks. 

Be aware of bug 1105522 - UITests may fail without triggering an orange or reporting any failure - and its dependents - there are a bunch of broken UITests - I am trying to finish up that work.

Comment 12

[:Margaret Leibovic]

That first try push still missed a bunch, so I made a new one:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3288874c9b94

It looks like the only problem is testSessionOOMRestore, I'll need to dig into what's going wrong there.

Comment 13

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #12)
> That first try push still missed a bunch, so I made a new one:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3288874c9b94
> 
> It looks like the only problem is testSessionOOMRestore, I'll need to dig
> into what's going wrong there.

I saw this fail once locally, but now it keeps passing, so this makes me think that there's some raciness causing a problem here.

It's a bit confusing that it seems to be checking tab2 from testSessionOOMRestore first, but it's hard to follow the NavigationWalker index logic, so I'm not going to worry about this.

In any case, it seems to find that we're at page3 when we should be at page4 in the first check. bnicholson, do you have any ideas what may be going on here?

The only think I changed here is removing the waitForText in onItem in SessionTest, but I would think that the waitForCondition in verifyUrlBarTitle would handle that. But I don't know why we would already be at page3 before checking that we were at page4.

Here's the from the relevant log snippet from my try run:

15:10:47     INFO -  TEST-START | testSessionOOMRestore
15:10:47     INFO -  TEST-PASS | testSessionOOMRestore | Robocop tests need the test device screen to be powered on. -
15:10:47     INFO -  TEST-PASS | testSessionOOMRestore | Given message occurred for registered event: {"type":"Gecko:Ready"} - Gecko:Ready should equal Gecko:Ready
15:10:47     INFO -  EventExpecter: no longer listening for Gecko:Ready
15:10:47     INFO -  TEST-PASS | testSessionOOMRestore | The correct number of tabs are opened - 3 should equal 3
15:10:47     INFO -  TEST-PASS | testSessionOOMRestore | The url argument is not null - http://mochi.test:8888/tests/robocop/robocop_dynamic.sjs?id=page3 should not equal null
15:10:47     INFO -  TEST-PASS | testSessionOOMRestore | Page title is correct - mochi.test:8888/tests/robocop/robocop_dynamic.sjs?id=page3 should equal mochi.test:8888/tests/robocop/robocop_dynamic.sjs?id=page3
15:10:47     INFO -  TEST-PASS | testSessionOOMRestore | Given message occurred for registered event: {"type":"Content:PageShow","userRequested":"","tabID":1} - Content:PageShow should equal Content:PageShow
15:10:47     INFO -  EventExpecter: no longer listening for Content:PageShow
15:10:47     INFO -  TEST-PASS | testSessionOOMRestore | The url argument is not null - http://mochi.test:8888/tests/robocop/robocop_dynamic.sjs?id=page4 should not equal null
15:10:47     INFO -  waitForCondition timeout after 15000 ms.
15:10:47  WARNING -  TEST-UNEXPECTED-FAIL | testSessionOOMRestore | Page title is correct - got mochi.test:8888/tests/robocop/robocop_dynamic.sjs?id=page3, expected mochi.test:8888/tests/robocop/robocop_dynamic.sjs?id=page4
15:10:47     INFO -  0 ERROR Exception caught during test! - junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testSessionOOMRestore | Page title is correct - got mochi.test:8888/tests/robocop/robocop_dynamic.sjs?id=page3, expected mochi.test:8888/tests/robocop/robocop_dynamic.sjs?id=page4
15:10:47     INFO -  	at junit.framework.Assert.fail(Assert.java:47)
15:10:47     INFO -  	at org.mozilla.gecko.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:128)
15:10:47     INFO -  	at org.mozilla.gecko.FennecMochitestAssert.ok(FennecMochitestAssert.java:150)
15:10:47     INFO -  	at org.mozilla.gecko.FennecMochitestAssert.is(FennecMochitestAssert.java:156)
15:10:47     INFO -  	at org.mozilla.gecko.tests.BaseTest.verifyUrlBarTitle(BaseTest.java:553)
15:10:47     INFO -  	at org.mozilla.gecko.tests.SessionTest$1$1.onItem(SessionTest.java:195)
15:10:47     INFO -  	at org.mozilla.gecko.tests.SessionTest$1$1.onItem(SessionTest.java:192)
15:10:47     INFO -  	at org.mozilla.gecko.tests.SessionTest$NavigationWalker.walk(SessionTest.java:97)
15:10:47     INFO -  	at org.mozilla.gecko.tests.SessionTest$1.onItem(SessionTest.java:192)
15:10:47     INFO -  	at org.mozilla.gecko.tests.SessionTest$1.onItem(SessionTest.java:180)
15:10:47     INFO -  	at org.mozilla.gecko.tests.SessionTest$NavigationWalker.walk(SessionTest.java:94)
15:10:47     INFO -  	at org.mozilla.gecko.tests.SessionTest.verifySessionTabs(SessionTest.java:180)
15:10:47     INFO -  	at org.mozilla.gecko.tests.testSessionOOMRestore.testSessionOOMRestore(testSessionOOMRestore.java:52)
15:10:47     INFO -  	at java.lang.reflect.Method.invokeNative(Native Method)
15:10:47     INFO -  	at java.lang.reflect.Method.invoke(Method.java:511)
15:10:47     INFO -  	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
15:10:47     INFO -  	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
15:10:47     INFO -  	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
15:10:47     INFO -  	at org.mozilla.gecko.tests.BaseTest.runTest(BaseTest.java:159)
15:10:47     INFO -  	at junit.framework.TestCase.runBare(TestCase.java:127)
15:10:47     INFO -  	at junit.framework.TestResult$1.protect(TestResult.java:106)
15:10:47     INFO -  	at junit.framework.TestResult.runProtected(TestResult.java:124)
15:10:47     INFO -  	at junit.framework.TestResult.run(TestResult.java:109)
15:10:47     INFO -  	at junit.framework.TestCase.run(TestCase.java:118)
15:10:47     INFO -  	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
15:10:47     INFO -  	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
15:10:47     INFO -  	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:545)
15:10:47     INFO -  	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551)
15:10:47  WARNING -  TEST-UNEXPECTED-FAIL | testSessionOOMRestore | Exception caught - junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testSessionOOMRestore | Page title is correct - got mochi.test:8888/tests/robocop/robocop_dynamic.sjs?id=page3, expected mochi.test:8888/tests/robocop/robocop_dynamic.sjs?id=page4
15:10:47     INFO -  TEST-OK | testSessionOOMRestore | took 20945ms
15:10:47     INFO -  TEST-START | Shutdown
15:10:47     INFO -  Passed: 7
15:10:47     INFO -  Failed: 2
15:10:47     INFO -  Todo: 0

Comment 14

[:Margaret Leibovic]

/r/1483 - Bug 1111729 - Default to show URL in toolbar instead of page title, and remove preference

Pull down this commit:

hg pull review -r 41b50de9bc0b7e84171a07d0247ac4a0a588eef1

Comment 15

[:Margaret Leibovic]

I just added back the waitForText to fix testSessionOOMRestore. I'm not clear why the waitForCondition in verifyUrlBarTitle doesn't handle this for us, but I also feel like digging into that is out of scope for this bug, since I'm just maintaining the status quo here.

This latest patch is consistently green (I only did retriggers on the job that was failing before, the last try push has a lot of retriggers on all the other jobs):

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4be39667f318

Comment 16

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to :Margaret Leibovic from comment #15)
> I just added back the waitForText to fix testSessionOOMRestore. I'm not
> clear why the waitForCondition in verifyUrlBarTitle doesn't handle this for
> us, but I also feel like digging into that is out of scope for this bug,
> since I'm just maintaining the status quo here.

File please! :)

Comment 17

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #16)
> (In reply to :Margaret Leibovic from comment #15)
> > I just added back the waitForText to fix testSessionOOMRestore. I'm not
> > clear why the waitForCondition in verifyUrlBarTitle doesn't handle this for
> > us, but I also feel like digging into that is out of scope for this bug,
> > since I'm just maintaining the status quo here.
> 
> File please! :)

Bug 1113788 :)

Comment 18

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Looking good!

Comment 19

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

https://reviewboard.mozilla.org/r/1481/#review1051

I love having less code to maintain...

::: mobile/android/base/tests/SessionTest.java
(Diff revision 2)
> +                        verifyUrlBarTitle(page.url);

Why is this page.url and not text?

Comment 20

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/1481/#review1053

> Why is this page.url and not text?

verifyUrlBarText expects a raw URL, not the display text. It contains its own URL->display text logic, which makes this a bit sub-optimal, but I don't feel like re-working verifyUrlBarText just for this case. We can deal with this is bug 1113788.

Comment 21

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/51ca6ef95772

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/51ca6ef95772

Comment 23

[Teodora Vermesan (:TeoVermesan)]

The URL is now shown in toolbar instead of the page title, and the option is not present anymore in settings, so:
Verified as fixed on:
Device: Alcatel One Touch
OS: Android 4.1.2
Build: Firefox for Android 37.0a1 (2015-01-04)

But going to Settings -> Display, the summary is "Text, title bar, full-screen browsing", there is no option related anymore with title bar. Shouldn't the summary be updated?

Comment 24

[:Margaret Leibovic]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #23)
> The URL is now shown in toolbar instead of the page title, and the option is
> not present anymore in settings, so:
> Verified as fixed on:
> Device: Alcatel One Touch
> OS: Android 4.1.2
> Build: Firefox for Android 37.0a1 (2015-01-04)
> 
> But going to Settings -> Display, the summary is "Text, title bar,
> full-screen browsing", there is no option related anymore with title bar.
> Shouldn't the summary be updated?

Good catch! I filed bug 1117841.

Comment 25

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

relnoted as "URL bar now displays URL instead of page title"

Comment 27

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1111729 - Default to show URL in toolbar instead of page title, and remove preference