635375 - Preferences > add-ons > get add-ons 'go to page' disabled

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Kevin Brosnan [Ex-Mozilla]]

Attachment: disabled 'go to page' screenshot

Open the preferences
Select the Extensions window
Scroll down to the Get add-ons section
Select one of the add-ons

results:
'Go to Page' button is disabled
expected:
clickable button or no button

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Guess most of the extensions don't provide a homepage. Expected 'go to page' to take me to the amo page for the extension.

Comment 2

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

Yes, "Go to Page" should take you to the AMO page of the extension. This regressed.

Comment 3

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

Might be a bug, or normal behavior, in the AddonRepository. The "addon" object returned by the repository has a "homepageURL" property. That property is used by Fennec as the target for the Go To Page button.

In the AddonRepository, the code first tries the add the author supplied hompageURL in the property. The homepageURL is optional. As a fallback, the code will use the AMO home page for the add-on.

The code never seems to add the fallback. Or maybe it adds the AMO page first, then overwrites the empty homepageURL.

Sets the homepageURL:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonRepository.jsm#901

Sets the AMO home page:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonRepository.jsm#976

I noticed that the AMO page ("learnmore") is returned first in the XML. Seems like the empty homepageURL is overwriting it.

Comment 4

[Dave Townsend [:mossop]]

Looks like a bug on our side then. Should be as simple as doing an empty check before assigning here http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonRepository.jsm#901

Not sure I'd call this blocking but if someone wants to whip up a quick patch and test I'd push to take it.

Comment 5

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

Attachment: patch

I think this was not caught in the current tests because the test for "use learnmore if homepage is not defined" did not have an empty <homepageURL/> in the XML. The real AMO feed does.

Pushed to try

Comment 6

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

Patch works in Fennec. Add-ons with a homepage will open it, else the learnmore AMO page is opened.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 514862 [details] [diff] [review]
patch

I think this is the wrong patch

Comment 8

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

Attachment: real patch

Here's the right one

Comment 9

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

The patch is passing on Try server

Comment 10

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

pushed:
http://hg.mozilla.org/mozilla-central/rev/e8facc64c88e

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Kevin or Aaron, can one of you please verify this fix? Thanks.

Comment 12

[Kevin Brosnan [Ex-Mozilla]]

Verified - Firefox Mobile nightly 20110304042129

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Kevin, who can add this manual test to Litmus?