Closed Bug 1058243 Opened 10 years ago Closed 10 years ago

Redundant, confusing link in Add-on Manager to AMO

Categories

(Firefox for Android Graveyard :: Add-on Manager, defect)

34 Branch
Other
Android
defect
Not set
normal

Tracking

(firefox35 verified, firefox36 verified)

VERIFIED FIXED
Firefox 35
Tracking Status
firefox35 --- verified
firefox36 --- verified

People

(Reporter: tecgirl, Assigned: tim_tim2000, Mentored)

References

Details

(Whiteboard: [lang=js][lang=html])

Attachments

(3 files, 1 obsolete file)

The Add-on manager has two navigation points to AMO. The first is an Extension icon with a caret at the top to the right of "Your Add-ons". Tapping this will take you to AMO. It's confusing since 1) It's next to the "Your Add-ons" title, 2) There's a cell with the same Extension icon with the title "Browse all Firefox Add-ons".

Proposal: Remove icon, caret and navigation point at the top of the Add-on manager.
Blocks: 1053397
Originally we didn't have that bottom row (added in bug 722902), but we probably should have gotten rid of that icon when we did that :)

To fix this bug, we'll need to edit these files:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutAddons.xhtml
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutAddons.js
Mentor: margaret.leibovic
Whiteboard: [lang=js][lang=html]
Attached patch Proposed patch for bug 1058243 (obsolete) — Splinter Review
Assignee: nobody → tim_tim2000
Status: NEW → ASSIGNED
Attachment #8488995 - Flags: review?(margaret.leibovic)
Comment on attachment 8488995 [details] [diff] [review]
Proposed patch for bug 1058243

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

Thanks for the patch! r+ with one comment addressed.

Also, could you upload a screenshot for Robin to look at? If she thinks it looks okay, I can land an updated version of this patch for you.

::: mobile/android/chrome/content/aboutAddons.xhtml
@@ -31,5 @@
>    </menu>
>  
>    <div id="addons-header" class="header">
>      <div>&aboutAddons.header2;</div>
> -    <div id="header-button" role="button" aria-label="&aboutAddons.browseAll;" pref="extensions.getAddons.browseAddons" />

You can also remove this string:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/aboutAddons.dtd#8
Attachment #8488995 - Flags: review?(margaret.leibovic) → review+
I'm only wondering how do I compile Android version and install it on my phone in order to make a screenshot...
Attachment #8488995 - Attachment is obsolete: true
(In reply to Timur Timirkhanov from comment #4)
> Created attachment 8492696 [details] [diff] [review]
> Patch with string deleted from dtd
> 
> I'm only wondering how do I compile Android version and install it on my
> phone in order to make a screenshot...

I didn't realize you didn't make a build to test! Getting a build environment set up is the first thing you should do before jumping into writing patches.

You should follow the instructions here:
https://wiki.mozilla.org/Mobile/Fennec/Android

You should join #mobile on irc.mozilla.org to ask questions if you run into any problems getting a build set up. Good luck!
Comment on attachment 8492696 [details] [diff] [review]
Patch with string deleted from dtd

I tested this myself, and it looks good. I want to land this sooner rather than later, so I'll just land it. But still let me know if you want help getting a build set up for other bugs!
Attachment #8492696 - Flags: review+
Had to back this out for test failures :(

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

We need to update this test:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testAddonManager.java#58

We can probably change it to just click on the "Browse all Firefox Add-ons" text instead of the icon in the top right corner.
I don't see much value to trying to make sure the "Browse all Firefox add-ons" link works, since we're not even checking to see if the URL we wanted loaded. It seems like the main value of this part of the test is to make sure we switch to an open add-on manager instance, rather than opening a new tab, so that's what I decided to do here.

Passed locally. Try push here:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=66434116e444
Attachment #8499813 - Flags: review?(michael.l.comella)
Comment on attachment 8499813 [details] [diff] [review]
(Part 2) Update testAddonManager to open a blank new tab instead of trying to click link in about:addons

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

Just one nit about updating the class comment.

::: mobile/android/base/tests/testAddonManager.java
@@ -42,5 @@
>          // Load the about:addons page and verify it was loaded
>          loadAndPaint(url);
>          verifyPageTitle(StringHelper.ADDONS_LABEL);
>  
> -        // Change the AMO URL so we do not try to navigate to a live webpage

This was so fragile that I'm happy to see it removed.  But you'll need to update the comment at the top of the test explaining what the test does; it no longer tries to tap the AMO link.
Attachment #8499813 - Flags: review?(michael.l.comella) → review+
https://hg.mozilla.org/mozilla-central/rev/c1cef1625a48
https://hg.mozilla.org/mozilla-central/rev/92bcf77b60ed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Verified as fixed in
Builds:
Firefox for Android 36.0a1 (2014-11-03)
Firefox for Android 35.0a2 (2014-11-03)
Device: Motorola Razr (Android 4.1.2)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.