738642 - Finalized marketplace icon for about:home

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Frank Yan (:fryn)]

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.

Comment 1

[Frank Yan (:fryn)]

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+.

Comment 2

[Jason Smith [:jsmith]]

(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).

Comment 3

[Jason Smith [:jsmith]]

(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.

Comment 4

[Jason Smith [:jsmith]]

See the URL. Looks like there's a proposal created by John Slater for marketplace icons.

Comment 5

[John Slater]

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

Comment 6

[Frank Yan (:fryn)]

Attachment: patch

Just an image replacement.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 8

[Frank Yan (:fryn)]

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. :)

Comment 9

[Marco Castelluccio [:marco]]

(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?

Comment 10

[Jason Smith [:jsmith]]

Note - This should not be approved for mozilla aurora, as the web apps integration into desktop feature is moving to FF 15.

Comment 11

[Frank Yan (:fryn)]

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.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/d976e55df66e

Comment 13

[Justin Dolske [:Dolske]]

(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 14

[Justin Dolske [:Dolske]]

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

[Triage Comment]

Comment 15

[Jason Smith [:jsmith]]

(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.

Comment 16

[Jason Smith [:jsmith]]

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.

Comment 17

[Frank Yan (:fryn)]

(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.

Comment 18

[Jason Smith [:jsmith]]

(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).

Comment 19

[Jason Smith [:jsmith]]

See https://wiki.mozilla.org/Web_Apps_integration for the feature details on why its tied to the critical path in this feature.

Comment 20

[Frank Yan (:fryn)]

(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.

Comment 21

[Jason Smith [:jsmith]]

(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 22

[Justin Dolske [:Dolske]]

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.

Comment 23

[Frank Yan (:fryn)]

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?

Comment 24

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/b73f34824b3e
https://hg.mozilla.org/releases/mozilla-beta/rev/a8aaf619dcef

Comment 25

[Paul Silaghi, QA [:pauly]]

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

Comment 26

[Jason Smith [:jsmith]]

Verified on FF 14 Aurora and FF 15 Nightly.