Closed Bug 860326 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 23
Tracking Status
firefox22 --- verified
firefox23 --- verified
fennec 22+ ---

People

(Reporter: aaronmt, Assigned: Margaret)

References

Details

(Keywords: regression, reproducible, Whiteboard: [NavBar])

Attachments

(3 files, 2 obsolete files)

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)
OS: Mac OS X → Android
Hardware: x86 → ARM
After I rotate my device, everything is fine and dandy!
Unlikely graphics
Summary: Regression: Invalidation issues on about:home prevent newly added top-sites from being shown; URL bar retains address entered → Regression: Issues on about:home prevent newly added top-sites from being shown; URL bar retains address entered
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).
Assignee: nobody → margaret.leibovic
Blocks: 839602
Summary: Regression: Issues on about:home prevent newly added top-sites from being shown; URL bar retains address entered → Regression: URL bar retains address entered after editing a top site on about:home
^
Flags: needinfo?(wjohnston)
It looks like we aren't using this Target.PICK_SITE data anywhere else. It's nice to have a use for it now!
Attachment #736035 - Flags: review?(wjohnston)
Oops, we actually do still need to call fromAwesomeBarSearch for the animation to work. We should just avoid passing it a url.
Attachment #736035 - Attachment is obsolete: true
Attachment #736035 - Flags: review?(wjohnston)
Attachment #736040 - Flags: review?(wjohnston)
tracking-fennec: ? → 22+
Attachment #736040 - Flags: review?(wjohnston) → review+
Flags: needinfo?(wjohnston)
Whiteboard: [NavBar]
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
Attachment #736040 - Flags: approval-mozilla-aurora?
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)
Attached patch patch v2Splinter Review
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
Attachment #736040 - Attachment is obsolete: true
Attachment #736040 - Flags: approval-mozilla-aurora?
Attachment #738153 - Flags: review?(wjohnston)
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?
Attachment #738153 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/098faa24c569
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
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
Attachment #738153 - Flags: approval-mozilla-aurora?
Comment on attachment 738153 [details] [diff] [review]
patch v2

low risk fix for a 22 regression, uplift away!
Attachment #738153 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on:
-build: Firefox for Android 23.0a1 (2013-04-18)
-device: LG Nexus 4
-OS: Android 4.2.2
Status: RESOLVED → VERIFIED
Verified fixed on:
-build: Firefox for Android 22 Beta 1 (2013-05-15)
-device: LG Nexus 4
-OS: Android 4.2.2
You need to log in before you can comment on or make changes to this bug.