Closed Bug 1111729 Opened 5 years ago Closed 5 years ago

Default to show URL in toolbar instead of page title

Categories

(Firefox for Android :: General, defect)

35 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 37
Tracking Status
relnote-firefox --- 37+

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8536770 - Flags: review?(mark.finkle)
/r/1483 - Bug 1111729 - Default to show URL in toolbar instead of page title

Pull down this commit:

hg pull review -r d877b5f53e447916d20ba4edbd8f73195ea0a495
Attachment #8536770 - Flags: review?(mark.finkle) → review+
I suspect this may break some robocop tests. Exploratory try push:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a00d6a8a50e3
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?
Flags: needinfo?(mark.finkle)
Flags: needinfo?(alam)
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.
Flags: needinfo?(mark.finkle)
Okay, I'll update my patch and post for re-review. Good catch, Mike!

I also need to fix Robocop tests.
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
Cc'ing gbrown, since this is going to affect a lot of robocop tests.
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.
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.
Blocks: 1025866
(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
Flags: needinfo?(alam) → needinfo?(bnicholson)
Attachment #8536770 - Flags: review?(michael.l.comella)
Attachment #8536770 - Flags: review-
Attachment #8536770 - Flags: review+
/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
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
Flags: needinfo?(bnicholson)
(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! :)
Flags: needinfo?(margaret.leibovic)
Blocks: 1113788
(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 :)
Flags: needinfo?(margaret.leibovic)
Attachment #8536770 - Flags: review?(michael.l.comella) → review+
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?
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.
https://hg.mozilla.org/mozilla-central/rev/51ca6ef95772
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
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?
Flags: needinfo?(margaret.leibovic)
Depends on: 1117841
(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.
Flags: needinfo?(margaret.leibovic)
Depends on: 1118039
Depends on: 1118835
relnoted as "URL bar now displays URL instead of page title"
Depends on: 1123904
QA Contact: teodora.vermesan
Attachment #8536770 - Attachment is obsolete: true
Attachment #8618927 - Flags: review+
You need to log in before you can comment on or make changes to this bug.