Closed Bug 828778 Opened 7 years ago Closed 7 years ago

find alternative way for providers to specify activation notification icon

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set

Tracking

(firefox20- wontfix, firefox21+ verified, firefox22 verified)

VERIFIED FIXED
Firefox 22
Tracking Status
firefox20 - wontfix
firefox21 + verified
firefox22 --- verified

People

(Reporter: Gavin, Assigned: mixedpuppy)

References

Details

Attachments

(1 file, 1 obsolete file)

The social activation notification currently uses a hardcoded Facebook icon (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/social-icon.png?force=1). We need to not have provider images in m-c like that, and need a solution that scales to handle multiple providers.
Worst case we could probably just could include this in the "manifest" as a data URI: (though that'd be a bit gross since this is a large-ish image).

Ideally, it might be nice to have the icon be specified as metadata on the providers "ActivateSocialFeature" event.
If we end up using the addons tab configuration in bug 815546, we will need a larger image there as well.
Blocks: 755126
I don't think using "ActivateSocialFeature" to get the image will work well for bug 755126, which may get viewed prior to any activation.  For builtin providers, we'll need the image handy, so it will need to be in the manifest.  Either accepting a url and the load delay (plus possible breakage when offline), or having it encoded will be necessary.  As ugly as it is, I vote for encoded in the manifest.
bug 755126 adds icon32URL and icon64URL to the manifest.  This works really well for that, however it does bloat our manifest.  IMO it is still the best approach to avoid empty icons if offline.

Opinions on that?
This is fine with me! In an ideal scenario we'd have something to load these icons and permanently cache them, but that becomes way more complicated and there's no reason to not start with something simple that works well. We can revisit that later if it makes sense to do so.

The only concern is to have a sensible handling for manifests that doesn't specify these icons (i.e. we must at least accept them, possibly with some standard icons).
(In reply to :Felipe Gomes from comment #5)
> The only concern is to have a sensible handling for manifests that doesn't
> specify these icons (i.e. we must at least accept them, possibly with some
> standard icons).

Everything, so far, falls back to using just iconURL, so that is the minimum requirement.  Right now I have a half dozen providers, only one sets iconURL32/64, but they all continue to work as expected, just don't look as nice in UI.
(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> bug 755126 adds icon32URL and icon64URL to the manifest.  This works really
> well for that, however it does bloat our manifest.  IMO it is still the best
> approach to avoid empty icons if offline.
> 
> Opinions on that?

I think the direction is good, but instead of inventing our own icon attributes, we should reuse the same format apps use - https://developer.mozilla.org/en-US/docs/Apps/Manifest - so something like:

  icons: {32: "blah://...",
          64: ...
       }

Re the "ActivateSocialFeature" event - it possibly makes sense to allow the manifest URL to be specified as part of this event.  If the URL is missing, we assume a builtin manifest (ie, FB etc would not need to supply it) - that at least gives us a sane installation story and allows us to defer the discovery story.
(In reply to Mark Hammond (:markh) from comment #7)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> > bug 755126 adds icon32URL and icon64URL to the manifest.  This works really
> > well for that, however it does bloat our manifest.  IMO it is still the best
> > approach to avoid empty icons if offline.
> > 
> > Opinions on that?
> 
> I think the direction is good, but instead of inventing our own icon
> attributes, we should reuse the same format apps use -
> https://developer.mozilla.org/en-US/docs/Apps/Manifest - so something like:

We should follow any mechanics that make sense for the addonmanager since much of our discovery/install/blocklist/configuration is based around that.

>   icons: {32: "blah://...",
>           64: ...
>        }

I know there is a way to have multiple icons in addons, not sure how it maps to what we're doing, but regardless we could define icons that way and map them in AddonWrapper.

> Re the "ActivateSocialFeature" event - it possibly makes sense to allow the
> manifest URL to be specified as part of this event.  If the URL is missing,
> we assume a builtin manifest (ie, FB etc would not need to supply it) - that
> at least gives us a sane installation story and allows us to defer the
> discovery story.

I like doing the manifesturl from a discovery aspect, but it is more work.  The current patch for this does make it optional to pass data if you are builtin.  If you are builtin and pass data, it will override the builtin defaults.
Assignee: nobody → mixedpuppy
Attached patch activation icon patch (obsolete) — Splinter Review
This patch moves the icon into the manifest and uses that in the activation panel.  This is also compatible with the approach in the addon manager and install bugs.
Attachment #715153 - Flags: review?(gavin.sharp)
Comment on attachment 715153 [details] [diff] [review]
activation icon patch

uber-nit: use " instead of ' for string literals in browser-social? (helps grepping for strings when we're consistent, " is much more common in browser front-end code)
Attachment #715153 - Flags: review?(gavin.sharp) → review+
changes quotes on string literals, carry forward r+ from gavin.

A short try push at https://tbpl.mozilla.org/?tree=Try&rev=a8e82b876427 but IMO ready for checkin.
Attachment #715153 - Attachment is obsolete: true
Attachment #715475 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/54ea22cb33e1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Not sure we should uplift this all the way to 20 (original tracking added), but 21 kind of needs it.
(In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> Not sure we should uplift this all the way to 20 (original tracking added),
> but 21 kind of needs it.

Would you mind providing context as to why FF21 needs this patch but FF20 may not? Email to release-mgmt@mozilla.com would be great, if the explanation inappropriate for bug comments.
Comment on attachment 715475 [details] [diff] [review]
activation icon patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: fx21 hsa multiprovider support, however any provider that is activated shows the facebook icon in the info panel that is shown upon activation.  
Testing completed (on m-c, etc.): on m-c for a while now
Risk to taking this patch (and alternatives if risky): only affects social, and only the ui during activation of a provider.
String or UUID changes made by this patch: none
Attachment #715475 - Flags: approval-mozilla-aurora?
Comment on attachment 715475 [details] [diff] [review]
activation icon patch

Low risk patch needed to support multiprovider for social.Approving for uplift
Attachment #715475 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Marking this verified fixed based on my Multi-provider testing over the last couple of months. I've not encountered a regression caused by this bug.
Status: RESOLVED → VERIFIED
Just a little house cleaning, this should be wontfix for Fx20 now.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.