1031019 - Add a Firefox Marketplace button

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Wil Clouser [:clouserw]]

This is bug 1031018's cousin.  The same there would be a link in the Tools menu for Apps we should put one in the hamburger menu, right next to the Add-ons link.

Comment 1

[Wil Clouser [:clouserw]]

The URL for this is https://marketplace.firefox.com/discovery/

Comment 2

[Caitlin Galimidi [:c8o]]

Marco - 
When you have a sec, is there a target date for this (as well as bug 1031018?)

Thx!!
Caitlin

Comment 3

[:Gavin Sharp [email: [e-mail]]]

I'll land the patches by end of next week.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: WIP (no icons/CSS)

This is missing the icons and styling, but otherwise works. This is built on top of the patch for bug 1031018.

Some notes:
- specifying the "label" property explicitly in CustomizableWidgets.jsm is apparently optional, but I went for being explicit (I got confused by entries that didn't have it explicitly)
- the tooltiptext could use some wordsmithing (just "Discover Apps" for now)
- I added this to BrowserUITelemetry. I noticed some other buttons (like the loop button) arent't here. I assume they should be?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8492420 [details] [diff] [review]
WIP (no icons/CSS)

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

In terms of adding a button, you've hit all of the right places (modulo icons/styling, as you mentioned).

(In reply to :Gavin Sharp [email: [e-mail]] from comment #4)
> Created attachment 8492420 [details] [diff] [review]
> WIP (no icons/CSS)
> 
> This is missing the icons and styling, but otherwise works. This is built on
> top of the patch for bug 1031018.
> 
> Some notes:
> - specifying the "label" property explicitly in CustomizableWidgets.jsm is
> apparently optional, but I went for being explicit (I got confused by
> entries that didn't have it explicitly)

This is fine, and actually preferred for the default "button" type of widget you're creating. There are some more complicated widgets (like zoom, edit, loop) that allow the implementer to override a bunch of defaults and custom build a bunch of things, thereby side-stepping some of the built-in niceness - so that's why you're not seeing it in some cases.

> - I added this to BrowserUITelemetry. I noticed some other buttons (like the
> loop button) arent't here. I assume they should be?

Yes - or even better, we could expose the default placements for things in CustomizableUI so that we only have a single place where we have to maintain that list. Short-term fix is to add the items you've mentioned, though.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Also, I'm pretty sure every time somebody refers to it as "The Hamburger Button", shorlander dies a little bit inside. :)

Comment 7

[Caitlin Galimidi [:c8o]]

Team - Thanks so much for the work on this.

Gavin and Marco - 

What's the target date for this in Nightly?
Anything else you need from the Marketplace team?

Comment 8

[:Gavin Sharp [email: [e-mail]]]

This will land on Nightly soon after bug 1070202 is resolved.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Flagging Gijs/Jared since this looks a lot like bug 1069300 which they both recently looked at. Figure out amongst yourselves who wants to actually review (deathmatch).

This is built on top of attachment 8496235 [details] [diff] [review] (toolbar/menubar image asset update) and attachment 8492422 [details] [diff] [review] (for actual button logic). It hasn't yet been rebased on top of bug 1069300 but that should be trivial.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

What's the status of the Apps button on about:home? We've had that hidden since March 2012 :)

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Don't know! Want to file another bug?

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #11)
> Don't know! Want to file another bug?

https://bugzilla.mozilla.org/show_bug.cgi?id=738646

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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

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

::: browser/modules/BrowserUITelemetry.jsm
@@ +95,5 @@
>      "feed-button",
>      "email-link-button",
>      "sync-button",
>      "tabview-button",
> +    "web-apps-button"

nit, please add a comma at the end to make updating the blame will have less churn with the next change

Comment 14

[:Gijs (he/him)]

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

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

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +924,5 @@
>      }
> +  }, {
> +    id: "web-apps-button",
> +    label: "web-apps-button.label",
> +    tooltiptext: "web-apps-button.tooltiptext",

Nit: as Mike said, can omit both of these.

@@ +925,5 @@
> +  }, {
> +    id: "web-apps-button",
> +    label: "web-apps-button.label",
> +    tooltiptext: "web-apps-button.tooltiptext",
> +    defaultArea: CustomizableUI.AREA_PANEL,

This is not enough. You should add this to the list in CustomizableUI.jsm.

And after that, tests about placeholders will fail, so they will need updating. Run ./mach mochitest-browser browser/components/customizableui/ to see which/how.

And after that, this won't be added to the placements for existing users who have customized their panels. Is that OK? If not, need to add a version on the spec (like for loop) and increment the version at the top of CustomizableUI.jsm.

Comment 15

[:Gijs (he/him)]

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

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

So it seems I misunderstood, and this doesn't need to be in the panel just yet... so r+, but please remove the defaultArea line and file a bug on investigating that not doing what it says on the tin. Thanks! :-)

Comment 16

[:Gavin Sharp [email: [e-mail]]]

https://tbpl.mozilla.org/?tree=Try&rev=33b303886ab0

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #16)
> https://tbpl.mozilla.org/?tree=Try&rev=33b303886ab0

Canceled that one, and pushed the version rebased on top of bug 1069300:

https://tbpl.mozilla.org/?tree=Try&rev=1212c3904cf7

Comment 18

[:Gavin Sharp [email: [e-mail]]]

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/81be82b617b9

Comment 20

[Bogdan Maris, Desktop QA]

I did some exploratory testing around this implementation and found one issues (bug 1077278). Testing was done using latest Nightly on Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.9.5. 
Marking this as verified based on my testing.

Comment 21

[Matthew N. [:MattN]]

Release Note Request (optional, but appreciated)
[Why is this notable]: With bug 1031018, the Firefox Marketplace is promoted in desktop Firefox
[Suggested wording]: Access the Firefox Marketplace from the Tools menu and optional toolbar button.
[Links (documentation, blog post, etc)]: https://marketplace.firefox.com/discovery/

Tracking requested to make sure the release of these Marketplace entry points is coordinated with marketing, the marketplace team, etc.

Comment 22

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Already verified in Firefox 35 per comment 20.