Finalized marketplace icon for about:home

VERIFIED FIXED in Firefox 13

Status

()

Firefox
General
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: fryn, Assigned: fryn)

Tracking

Trunk
Firefox 15
Points:
---

Firefox Tracking Flags

(blocking-kilimanjaro:+, firefox13 verified, firefox14 verified)

Details

(Whiteboard: [qa!], URL)

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Before the marketplace button in about:home can be displayed to users, we need a finalized icon.

This doesn't block the landing of the patch in bug 731546, but we shouldn't set the hidden pref browser.aboutHome.apps to true until we have an icon.
(Assignee)

Updated

6 years ago
Summary: Land finalized marketplace icon for about:home → Finalized marketplace icon for about:home

Updated

5 years ago
Blocks: 731054
(Assignee)

Comment 1

5 years ago
I will be getting the icon shortly from shorlander via martell.
Then I'll upload a patch and request approval for aurora and beta once it gets r+.
Assignee: nobody → fryn
Status: NEW → ASSIGNED
(In reply to Frank Yan (:fryn) from comment #1)
> I will be getting the icon shortly from shorlander via martell.
> Then I'll upload a patch and request approval for aurora and beta once it
> gets r+.

FYI - This should not be uplifted to Aurora. Web apps integration into desktop is targeted for FF 14 right now (possible it could move even to FF 15, but that's still up in the air).
(In reply to Jason Smith [:jsmith] from comment #2)
> (In reply to Frank Yan (:fryn) from comment #1)
> > I will be getting the icon shortly from shorlander via martell.
> > Then I'll upload a patch and request approval for aurora and beta once it
> > gets r+.
> 
> FYI - This should not be uplifted to Aurora. Web apps integration into
> desktop is targeted for FF 14 right now (possible it could move even to FF
> 15, but that's still up in the air).

And when I mean Aurora I meant Beta. Don't uplift to beta.

Updated

5 years ago
blocking-kilimanjaro: --- → +
See the URL. Looks like there's a proposal created by John Slater for marketplace icons.

Comment 5

5 years ago
That was actually created by Sean Martell, which brings us back to comment #1...it should all be the same work.
(Assignee)

Comment 6

5 years ago
Created attachment 620831 [details] [diff] [review]
patch

Just an image replacement.
Attachment #620831 - Flags: review?(jwein)
Comment on attachment 620831 [details] [diff] [review]
patch

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

I assume the image is the wanted one and it has been optimized already.
Attachment #620831 - Flags: review?(jwein) → review+
(Assignee)

Comment 8

5 years ago
Comment on attachment 620831 [details] [diff] [review]
patch

[Approval Request Comment]
User impact if declined: No marketplace icon on about:home!
Testing completed (on m-c, etc.): Locally
Risk to taking this patch: None
String changes made by this patch: None

(In reply to Marco Bonardo [:mak] from comment #7)
> I assume the image is the wanted one and it has been optimized already.

Yes to both. I checked. :)
Attachment #620831 - Flags: approval-mozilla-aurora?
(In reply to Frank Yan (:fryn) from comment #8)
> [Approval Request Comment]
> User impact if declined: No marketplace icon on about:home!

Do we want a marketplace icon on about:home also if we don't support native installation on Aurora?
Note - This should not be approved for mozilla aurora, as the web apps integration into desktop feature is moving to FF 15.
(Assignee)

Comment 11

5 years ago
Comment on attachment 620831 [details] [diff] [review]
patch

(In reply to Marco Bonardo [:mak] from comment #7)
> Review of attachment 620831 [details] [diff] [review]

Thanks for the review, Marco. :)

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

(In reply to Jason Smith [:jsmith] from comment #10)
> Note - This should not be approved for mozilla aurora, as the web apps
> integration into desktop feature is moving to FF 15.

I see. I was not informed of that until now. Thanks.
Attachment #620831 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d976e55df66e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
(In reply to Jason Smith [:jsmith] from comment #10)
> Note - This should not be approved for mozilla aurora, as the web apps
> integration into desktop feature is moving to FF 15.

Hmm, I'm not sure that matters. The previous push for this, at least, was to have the ability to promote (eg to developers) Marketplace, even if the browser-integration pieces are still underway.

Since it's just an image swap, and the display if pref'd off, let's just go ahead an land this on Aurora. Even if plans change to not enable it, it doesn't hurt to be prepared.
Comment on attachment 620831 [details] [diff] [review]
patch

[Triage Comment]
Attachment #620831 - Flags: approval-mozilla-aurora+
(In reply to Justin Dolske [:Dolske] from comment #13)
> (In reply to Jason Smith [:jsmith] from comment #10)
> > Note - This should not be approved for mozilla aurora, as the web apps
> > integration into desktop feature is moving to FF 15.
> 
> Hmm, I'm not sure that matters. The previous push for this, at least, was to
> have the ability to promote (eg to developers) Marketplace, even if the
> browser-integration pieces are still underway.
> 
> Since it's just an image swap, and the display if pref'd off, let's just go
> ahead an land this on Aurora. Even if plans change to not enable it, it
> doesn't hurt to be prepared.

Eh, don't see the value of doing this still. It actually does matter btw, as marketplace is setup to only allow app installations on firefox builds where web apps installations are allowed. Once bug 750936 lands to disable FF 14 support for webapps, then we'll be officially be moving to FF 15. You should check with Jen and Myk on this one before giving the approval for aurora. I just question if there's even value to doing it in the first place.

Updated

5 years ago
Attachment #620831 - Flags: approval-mozilla-aurora+
Please answer the question in comment 9 before making a decision for approval for aurora or not. Approving when there is a clear disagreement is not wise, as people are not in agreement here.
(Assignee)

Comment 17

5 years ago
(In reply to Jason Smith [:jsmith] from comment #16)

I don't understand your objection.
This bug is not about making the marketplace icon visible.
It's about making sure we have the right marketplace icon in the tree.
(In reply to Frank Yan (:fryn) from comment #17)
> (In reply to Jason Smith [:jsmith] from comment #16)
> 
> I don't understand your objection.
> This bug is not about making the marketplace icon visible.
> It's about making sure we have the right marketplace icon in the tree.

I think my counterargument to Justin's comments was that there isn't value to uplifting to Aurora to fix it in Aurora, as we don't plan to enable it there anyways given that this feature is tied to the web apps integration into desktop feature that was recently re-targeted for FF 15. This will result in Beta (13) and Aurora (14)'s code tree for this to follow the same logic for this link to marketplace feature, which already is disabled behind a pref. There's no value uplifting this (i.e. resolution for FF 14 = Won't Fix).
See https://wiki.mozilla.org/Web_Apps_integration for the feature details on why its tied to the critical path in this feature.
(Assignee)

Comment 20

5 years ago
(In reply to Jason Smith [:jsmith] from comment #18)
We are simply trying to ensure that, regardless of the decision the apps team and various stakeholders make about the target for web apps integration into desktop, we (the Firefox desktop team) have the right code ready for you guys, even if you change your mind and decide to target it for FF 14 again. There is effectively zero risk to uplifting this to aurora and beta, and the upside is that, if you change your mind, this will be ready for you to go.

If we don't take this icon onto aurora and beta, I'm going to remove the useless pref from those branches, since there is no way we're going to allow that pref to be flipped to display a placeholder "X" icon in a hotfix add-on. You're also going to be wasting a lot of people's time if you backtrack on this.

As I explained in comment 17, comment 9 is irrelevant to the approval decision. It's fine for you to bring up what you believe to be issues, but please don't clear "+" flags that people have legitimately set.
(In reply to Frank Yan (:fryn) from comment #20)
> (In reply to Jason Smith [:jsmith] from comment #18)
> We are simply trying to ensure that, regardless of the decision the apps
> team and various stakeholders make about the target for web apps integration
> into desktop, we (the Firefox desktop team) have the right code ready for
> you guys, even if you change your mind and decide to target it for FF 14
> again. There is effectively zero risk to uplifting this to aurora and beta,
> and the upside is that, if you change your mind, this will be ready for you
> to go.

I agree on the zero risk. As a side note, we should be having the firefox desktop team and apps team a bit more synced up, so we can avoid the confusion such as what happened on this bug a bit more (I'm a QA on both). This will likely be critical for k9o, as there will be apps firefox desktop features coming down the pipeline (e.g. new tab).

> 
> If we don't take this icon onto aurora and beta, I'm going to remove the
> useless pref from those branches, since there is no way we're going to allow
> that pref to be flipped to display a placeholder "X" icon in a hotfix
> add-on. You're also going to be wasting a lot of people's time if you
> backtrack on this.

Okay, the pref explanation actually does make sense to uplift this then, as then if someone set the pref, then the button would be setup correctly.

> 
> As I explained in comment 17, comment 9 is irrelevant to the approval
> decision. It's fine for you to bring up what you believe to be issues, but
> please don't clear "+" flags that people have legitimately set.

As I stated above, the rationale for clearing the + field was because there was a disagreement that was not addressed. Next time, let's make sure to address the disagreement earlier rather than later (I brought this up way back in 4/26 in comment 2). Remember that I have to test this to make sure that the implementation works and makes sense (I'm on the QA on it), so it's important I understand what's going on and that it makes sense.

The rationale now makes sense to uplift to aurora and beta. Thanks for addressing the concerns.
Comment on attachment 620831 [details] [diff] [review]
patch

Resetting cleared flag. I don't understand why there's any disagreement about a simple image swap.
Attachment #620831 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 23

5 years ago
Comment on attachment 620831 [details] [diff] [review]
patch

We should put this on beta for the same reasons as for aurora.
Dolske, thanks for the approval for aurora. Could you formally approve it for beta?
Attachment #620831 - Flags: approval-mozilla-beta?
Attachment #620831 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b73f34824b3e
https://hg.mozilla.org/releases/mozilla-beta/rev/a8aaf619dcef
status-firefox13: --- → fixed
status-firefox14: --- → fixed

Updated

5 years ago
Whiteboard: [qa+]

Updated

5 years ago
No longer blocks: 731054
Marketplace icon is displayed in about:home when browser.aboutHome.apps pref is set on true.
Verified fixed on FF 13b3:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:13.0) Gecko/20100101 Firefox/13.0
status-firefox13: fixed → verified
Verified on FF 14 Aurora and FF 15 Nightly.
Status: RESOLVED → VERIFIED
status-firefox14: fixed → verified
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.