Closed Bug 1031019 Opened 5 years ago Closed 5 years ago

Add a Firefox Marketplace button

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 35
Iteration:
35.3
Tracking Status
firefox35 + verified
relnote-firefox --- 35+

People

(Reporter: clouserw, Assigned: Gavin)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1048519
No longer blocks: desktop-marketplace
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Flags: qe-verify?
Flags: firefox-backlog+
Marco - 
When you have a sec, is there a target date for this (as well as bug 1031018?)

Thx!!
Caitlin
Flags: needinfo?(mmucci)
Flags: needinfo?(mmucci) → needinfo?(gavin.sharp)
I'll land the patches by end of next week.
Flags: needinfo?(gavin.sharp)
Flags: qe-verify? → qe-verify+
Depends on: 1070202
Attached patch WIP (no icons/CSS) (obsolete) — Splinter Review
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?
Attachment #8492420 - Flags: feedback?(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: gavin@gavinsharp.com] 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.
Attachment #8492420 - Flags: feedback?(mconley) → feedback+
Also, I'm pretty sure every time somebody refers to it as "The Hamburger Button", shorlander dies a little bit inside. :)
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?
Flags: needinfo?(mmucci)
Flags: needinfo?(gavin.sharp)
This will land on Nightly soon after bug 1070202 is resolved.
Flags: needinfo?(mmucci)
Flags: needinfo?(gavin.sharp)
Attached patch patchSplinter Review
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.
Attachment #8492420 - Attachment is obsolete: true
Attachment #8496247 - Flags: review?(jaws)
Attachment #8496247 - Flags: review?(gijskruitbosch+bugs)
What's the status of the Apps button on about:home? We've had that hidden since March 2012 :)
Flags: needinfo?(gavin.sharp)
Don't know! Want to file another bug?
Flags: needinfo?(gavin.sharp)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #11)
> Don't know! Want to file another bug?

https://bugzilla.mozilla.org/show_bug.cgi?id=738646
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
Attachment #8496247 - Flags: review?(jaws)
Attachment #8496247 - Flags: review?(gijskruitbosch+bugs)
Attachment #8496247 - Flags: review+
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.
Attachment #8496247 - Flags: review-
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! :-)
Attachment #8496247 - Flags: review- → review+
Summary: Add Firefox Marketplace to the hamburger menu → Add a Firefox Marketplace button
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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
https://hg.mozilla.org/mozilla-central/rev/81be82b617b9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Iteration: --- → 35.3
QA Contact: bogdan.maris
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.
Status: RESOLVED → VERIFIED
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.
Already verified in Firefox 35 per comment 20.
You need to log in before you can comment on or make changes to this bug.