Closed
Bug 828778
Opened 11 years ago
Closed 11 years ago
find alternative way for providers to specify activation notification icon
Categories
(Firefox Graveyard :: SocialAPI, defect)
Firefox Graveyard
SocialAPI
Tracking
(firefox20- wontfix, firefox21+ verified, firefox22 verified)
VERIFIED
FIXED
Firefox 22
People
(Reporter: Gavin, Assigned: mixedpuppy)
References
Details
Attachments
(1 file, 1 obsolete file)
11.03 KB,
patch
|
mixedpuppy
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
If we end up using the addons tab configuration in bug 815546, we will need a larger image there as well.
Assignee | ||
Comment 3•11 years ago
|
||
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.
Assignee | ||
Comment 4•11 years ago
|
||
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?
Comment 5•11 years ago
|
||
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).
Assignee | ||
Comment 6•11 years ago
|
||
(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.
Comment 7•11 years ago
|
||
(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.
Assignee | ||
Comment 8•11 years ago
|
||
(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 | ||
Updated•11 years ago
|
Assignee: nobody → mixedpuppy
Assignee | ||
Updated•11 years ago
|
tracking-firefox20:
--- → ?
Reporter | ||
Updated•11 years ago
|
Assignee | ||
Comment 9•11 years ago
|
||
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)
Reporter | ||
Comment 10•11 years ago
|
||
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+
Assignee | ||
Comment 11•11 years ago
|
||
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+
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/54ea22cb33e1
Comment 13•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/54ea22cb33e1
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Assignee | ||
Comment 14•11 years ago
|
||
Not sure we should uplift this all the way to 20 (original tracking added), but 21 kind of needs it.
status-firefox20:
--- → affected
status-firefox21:
--- → affected
status-firefox22:
--- → fixed
tracking-firefox21:
--- → ?
Comment 15•11 years ago
|
||
(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.
Updated•11 years ago
|
Assignee | ||
Comment 16•11 years ago
|
||
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 17•11 years ago
|
||
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+
Assignee | ||
Comment 18•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/48b4df4b72e9
Updated•11 years ago
|
Comment 19•11 years ago
|
||
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.
Comment 20•11 years ago
|
||
Just a little house cleaning, this should be wontfix for Fx20 now.
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•