860326 - Regression: URL bar retains address entered after editing a top site on about:home

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (04/10) - Screenshot

Currently when one adds a top-site on about:home, it does not appear in it's newly added spot until about:home re-inflates, such as rotating the phone. The address bar retains the URL entered giving off the impression that the browser should have visited the URL but did not.

In the attached screenshot I added 'androidandme.com'; it does not appear into a top-site position. The address bar retains the URL entered but it should be displaying the empty placeholder text, 'Enter Search or Address'.

Also see video: http://www.youtube.com/watch?v=h9B6d6MC2XQ 

From IRC:

11:51 margaret:	 AaronMT: hm, i wonder if that's a regression from bug 839602


--
Nightly (04/10)
LG Nexus 4 (Android 4.2.2)

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Nightly (04/10) - Screenshot - After rotation

After I rotate my device, everything is fine and dandy!

Comment 2

[Aaron Train [:aaronmt]]

Unlikely graphics

Comment 3

[:Margaret Leibovic]

I'm morphing this bug to just be about the regression from bug 839602.

We should file a separate bug about updating the thumbnails, although I think that may already be filed (maybe wesj knows).

Comment 4

[Aaron Train [:aaronmt]]

^

Comment 5

[:Margaret Leibovic]

Attachment: Don't update BrowserToolbar after editing a top site on about:home

It looks like we aren't using this Target.PICK_SITE data anywhere else. It's nice to have a use for it now!

Comment 6

[:Margaret Leibovic]

Attachment: Don't update BrowserToolbar url after editing a top site on about:home

Oops, we actually do still need to call fromAwesomeBarSearch for the animation to work. We should just avoid passing it a url.

Comment 7

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d41c64ab235

Comment 8

[:Margaret Leibovic]

Comment on attachment 736040 [details] [diff] [review]
Don't update BrowserToolbar url after editing a top site on about:home

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 839602
User impact if declined: pinned site url will end up in the urlbar after the user edits a pinned site
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): low-risk, avoids some logic when the awesomescreen closes in the edit pinned site case
String or IDL/UUID changes made by this patch: n/a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for robocop crashes.


https://tbpl.mozilla.org/php/getParsedLog.php?id=21873038&tree=Mozilla-Inbound

0 INFO SimpleTest START
1 INFO TEST-START | testAllPagesTab
2 INFO TEST-PASS | testAllPagesTab | Awesomebar URL typed properly - http://mochi.test:8888/tests/robocop/robocop_big_link.html should equal http://mochi.test:8888/tests/robocop/robocop_big_link.html
3 INFO TEST-PASS | testAllPagesTab | Three tabs shown - 3 should equal 3
4 INFO TEST-PASS | testAllPagesTab | Strip is hidden - false should equal false
5 INFO TEST-PASS | testAllPagesTab | checking that all pages list exists and has 5 children (the default bookmarks) - android.widget.ListView@4430e2b0 should not equal null
6 INFO TEST-PASS | testAllPagesTab | TextView is filled in - Firefox: Customize with add-ons
7 INFO TEST-PASS | testAllPagesTab | TextView is filled in - https://addons.mozilla.org/en-US/android/
8 INFO TEST-PASS | testAllPagesTab | Correct number of ImageViews visible - 2 should equal 2
9 INFO TEST-PASS | testAllPagesTab | TextView is filled in - Firefox: Support
10 INFO TEST-PASS | testAllPagesTab | TextView is filled in - http://support.mozilla.org/en-US/products/mobile
11 INFO TEST-PASS | testAllPagesTab | Correct number of ImageViews visible - 2 should equal 2
12 INFO TEST-PASS | testAllPagesTab | TextView is filled in - Firefox Start
13 INFO TEST-PASS | testAllPagesTab | TextView is filled in - about:home
14 INFO TEST-PASS | testAllPagesTab | Correct number of ImageViews visible - 2 should equal 2
15 INFO TEST-PASS | testAllPagesTab | TextView is filled in - Firefox: About your browser
16 INFO TEST-PASS | testAllPagesTab | TextView is filled in - about:firefox
17 INFO TEST-PASS | testAllPagesTab | Correct number of ImageViews visible - 2 should equal 2
18 INFO TEST-PASS | testAllPagesTab | TextView is filled in - Big Link
19 INFO TEST-PASS | testAllPagesTab | TextView is filled in - http://mochi.test:8888/tests/robocop/robocop_big_link.html
20 INFO TEST-PASS | testAllPagesTab | Correct number of ImageViews visible - 1 should equal 1
INFO | automation.py | Application ran for: 0:01:09.782211
INFO | zombiecheck | Reading PID log: /tmp/tmp5nMrdhpidlog
PROCESS-CRASH | java-exception | java.lang.NullPointerException	at org.mozilla.gecko.BrowserApp.onActivityResult(BrowserApp.java:806)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Whoops, forgot the cset link.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a26ebb943ea

Comment 11

[:Margaret Leibovic]

Attachment: patch v2

The problem was a NPE when data is null, which can happen if the activity was cancelled. I decided to rework the patch a little bit to make the logic more explicit. Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=74a263fd5abd

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 738153 [details] [diff] [review]
patch v2

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

::: mobile/android/base/BrowserApp.java
@@ +803,5 @@
>          super.onActivityResult(requestCode, resultCode, data);
> +
> +        // Don't update the url in the toolbar if the activity was cancelled, or if it was launched to pick a site.
> +        if (resultCode != Activity.RESULT_OK ||
> +            data.getStringExtra(AwesomeBar.TARGET_KEY).equals(AwesomeBar.Target.PICK_SITE.toString())) {

Hmm... so the order here is important, as data may be null. I'm not sure, but do you think a comment would be helpful?

Comment 13

[:Margaret Leibovic]

Green on try:
https://tbpl.mozilla.org/?tree=Try&rev=7fbaaca3dbd1

https://hg.mozilla.org/integration/mozilla-inbound/rev/098faa24c569

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/098faa24c569

Comment 15

[:Margaret Leibovic]

Comment on attachment 738153 [details] [diff] [review]
patch v2

Same approval request as before, but now this patch doesn't cause crashes :)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 839602
User impact if declined: pinned site url will end up in the urlbar after the user edits a pinned site
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): low-risk, avoids some logic when the awesomescreen closes in the edit pinned site case
String or IDL/UUID changes made by this patch: n/a

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 738153 [details] [diff] [review]
patch v2

low risk fix for a 22 regression, uplift away!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/89963091a650

Comment 18

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
-build: Firefox for Android 23.0a1 (2013-04-18)
-device: LG Nexus 4
-OS: Android 4.2.2

Comment 19

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
-build: Firefox for Android 22 Beta 1 (2013-05-15)
-device: LG Nexus 4
-OS: Android 4.2.2