Closed
Bug 872605
Opened 12 years ago
Closed 11 years ago
re-activation to update provider manifest needs ux
Categories
(Firefox Graveyard :: SocialAPI, defect)
Tracking
(firefox24 verified, firefox25 verified)
VERIFIED
FIXED
Firefox 25
People
(Reporter: florian, Assigned: mixedpuppy)
References
Details
Attachments
(1 file, 5 obsolete files)
10.81 KB,
patch
|
mixedpuppy
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Steps to reproduce: 1. Have a social provider that has an "Activate" button that installs a provider without workerURL. 2. Update the code of the activate button, to include a workerURL pointing on a file that doesn't exist on the server. 3. Click the Activate button again. Expected result: - Some notification that the social provider has been updated. - The social provider is reloaded (and shown as broken). Actual result: - The preference is updated, but there's no visible feedback. - After a restart, the social provider is broken.
Comment 1•12 years ago
|
||
Here's a reproduction that doesn't involve an error state and will be encountered by a lot of people. (This reproduction can be done whe the https://mozsocial.cliqz.com/ provider) 1. Using Firefox 21, install a current provider, which won't have a shareURL 2. Upgrade to Firefox 23 (Aurora) 3. Reactivate the provider, which now has a shareURL 4. Zero feedback to user (Error) 5. New shareURL not used (Error) 6. Worker not reloaded (Error) Note that even if you turn on/off the provider nothing is reloaded. You need to reload the browser before seeing the changes.
Assignee | ||
Comment 2•11 years ago
|
||
This allows a provider to update the manifest and have it take affect without restarting firefox. If the provider is the current provider, it is completely reloaded. The "undo" panel is shown again, which isn't quite appropriate, but we wont be able to change strings (ie. we should uplift this to fx23)
Assignee: nobody → mixedpuppy
Attachment #763720 -
Flags: review?(gavin.sharp)
Comment 3•11 years ago
|
||
Having "addBuiltinProvider" do this auto-magically doesn't feel right - seems like there should be a more explicit API for this. But I admit losing track of how this code all works - maybe addBuiltinProvider is the right place and it just needs a better name.
Assignee | ||
Updated•11 years ago
|
Attachment #763720 -
Flags: review?(gavin.sharp) → review?(mhammond)
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #3) > Having "addBuiltinProvider" do this auto-magically doesn't feel right - > seems like there should be a more explicit API for this. But I admit losing > track of how this code all works - maybe addBuiltinProvider is the right > place and it just needs a better name. Right now this is the most direct route for refreshing the active providers, and since we really should have a way for providers to update in the short term (fx23) I'd like to go this way. I do think we need some refactoring in SocialService.
Assignee | ||
Comment 6•11 years ago
|
||
Comment on attachment 763720 [details] [diff] [review] upgrade manifest from reactivation not quite working yet, will have to examine this more.
Attachment #763720 -
Attachment is obsolete: true
Attachment #763720 -
Flags: review?(mhammond)
Assignee | ||
Comment 7•11 years ago
|
||
The previous patch didn't fix browser_social_multiprovider.js. That test was using activateFromOrigin which expects the pref to exist now (and a comment within activateFromOrigin alludes to that), however, the setup for that test uses runSocialTestWithProvider which does not add the prefs. The primary change in addBuiltinProvider just makes that a hard requirement in this situation. I also added a fix to activateFromOrigin, but can remove it if it is a big deal.
Attachment #763955 -
Flags: review?(mhammond)
Comment 8•11 years ago
|
||
(In reply to edA-qa mort-ora-y from comment #1) > Here's a reproduction that doesn't involve an error state and will be > encountered by a lot of people. (This reproduction can be done whe the > https://mozsocial.cliqz.com/ provider) > 1. Using Firefox 21, install a current provider, which won't have a shareURL > 2. Upgrade to Firefox 23 (Aurora) At this point, what state is the provider in? I'm guessing it is broken? > 3. Reactivate the provider, which now has a shareURL And this step requires the user to manually navigate to the provider's site, locate a page with the activation button, then click it, after which things work again? If that's correct, it sounds like a bigger problem than what this patch is solving. ie, I don't have any real objection to activation also doing an "upgrade", but it sounds like we actually want to avoid the browser/provider being broken after step 2 and avoid step 3 completely?
Comment 9•11 years ago
|
||
After Step 2 our provider still works but it is lacking a share button. It is however not hard to imagine that some providers would completely break at this point. I'd also prefer the user doesn't have to go back to your site to do an upgrade manually. If there a "reloadManifest" API of some kind I could use that -- since I'll be able to detect on my own when it's outdated. Alternately, if this is to behave more like an AddOn, you may wish to have a standard way of detecting that it is outdated. However, unlike an addon there is no way to keep running an outdated version since the pages/worker are always reloaded at start, thus getting the newer version but old manifest.
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to Mark Hammond (:markh) from comment #8) > (In reply to edA-qa mort-ora-y from comment #1) > > Here's a reproduction that doesn't involve an error state and will be > > encountered by a lot of people. (This reproduction can be done whe the > > https://mozsocial.cliqz.com/ provider) > > 1. Using Firefox 21, install a current provider, which won't have a shareURL > > 2. Upgrade to Firefox 23 (Aurora) > > At this point, what state is the provider in? I'm guessing it is broken? no. the only new feature in the manifest is the shareurl, which in a normal case would never be loaded until the user goes to the share button and chooses that providers. We simply are not reloading the provider instances when an update occurs. This patch is necessary regardless of how the manifest pref gets updated (ie. user clicking activate button, or new api for worker) > > 3. Reactivate the provider, which now has a shareURL > > but it sounds like we actually want to avoid the browser/provider > being broken after step 2 and avoid step 3 completely? They are not.
Comment 11•11 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #10) > > At this point, what state is the provider in? I'm guessing it is broken? > > no. the only new feature in the manifest is the shareurl, So the provider is broken WRT share functionality. And the only way that can be fixed is for the user to navigate to the activation page. That still sounds sub-optimal and "broken" to me. Are there other possibilities worth discussing that don't force the user to navigate to a specific page to take action?
Assignee | ||
Comment 12•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=102a5a53163e
Attachment #763955 -
Attachment is obsolete: true
Attachment #763955 -
Flags: review?(mhammond)
Attachment #764990 -
Flags: review?(mhammond)
Assignee | ||
Comment 13•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=7f552b1aaafe
Attachment #764990 -
Attachment is obsolete: true
Attachment #764990 -
Flags: review?(mhammond)
Attachment #765178 -
Flags: review?(mhammond)
Comment 14•11 years ago
|
||
Comment on attachment 765178 [details] [diff] [review] fix updating manifests, add worker api for self-update Review of attachment 765178 [details] [diff] [review]: ----------------------------------------------------------------- Looking good, but there are a few tweaks necessary IMO. ::: browser/base/content/test/social/browser_addons.js @@ +280,5 @@ > + let activationURL = manifest2.origin + "/browser/browser/base/content/test/social/social_activate.html" > + addTab(activationURL, function(tab) { > + let doc = tab.linkedBrowser.contentDocument; > + let installFrom = doc.nodePrincipal.origin; > + Services.prefs.setCharPref("social.whitelist", installFrom); this pref should be reset ::: browser/base/content/test/social/social_worker.js @@ +151,5 @@ > case "share-data-message": > if (testPort) > testPort.postMessage({topic:"got-share-data-message", result: event.data.result}); > break; > + case "worker.update": it looks like manifest.get doesn't have test coverage - maybe just call that and bump the version number by 1 - that would avoid duplicating the manifest here too. ::: browser/modules/Social.jsm @@ +161,5 @@ > // An engine change caused by adding/removing a provider should notify > if (topic == "provider-added" || topic == "provider-removed") { > this._updateProviderCache(data); > Services.obs.notifyObservers(null, "social:providers-changed", null); > + } else if (topic == "provider-update") { I think we tend to prefer an early return here - ie, return in the first block and drop the "else" @@ +165,5 @@ > + } else if (topic == "provider-update") { > + // a provider has self-updated its manifest, we need to update our > + // cache and possibly reload if it was the current provider. > + let provider = data; > + let doReload = this.provider.origin == provider.origin; I'm not sure this check is necessary - currently the provider *must* be current as that's the only way the call can be made in the first-place? And later, if we change that, it might end up being the case that we need to reload the provider even if they aren't current. A better check might just be "if (provider.enabled) {... toggle to false then back to true...)? @@ +256,5 @@ > // provider has already been activated - we still get called back with it. > SocialService.addBuiltinProvider(origin, function(provider) { > if (provider) { > // No need to activate again if we're already active > + if (provider != this.provider) this looks wrong to me. The old code would not callback if the provider was already current - and thus would not display the activation prompt. But it *would* set the new provider current, which IIUC, was the motivation for this change. This new code always makes the callback, so always displays the activation prompt (and that is the only thing the callback does) So I don't think this change is necessary for this patch. If it is, I don't think there is test coverage of the new behaviour (and for that matter, I guess there isn't complete test coverage of the old behaviour wrt the activation prompt either, or tests would fail). Or I'm just wrong about something :)
Attachment #765178 -
Flags: review?(mhammond) → feedback+
Assignee | ||
Comment 15•11 years ago
|
||
feedback added https://tbpl.mozilla.org/?tree=Try&rev=155e9b7c6ac6
Attachment #765178 -
Attachment is obsolete: true
Attachment #765189 -
Flags: review?(mhammond)
Comment 16•11 years ago
|
||
Comment on attachment 765189 [details] [diff] [review] fix updating manifests, add worker api for self-update Review of attachment 765189 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/Social.jsm @@ +172,5 @@ > + this._updateProviderCache(providers); > + Services.obs.notifyObservers(null, "social:providers-changed", null); > + // if we need a reload, do it now > + if (provider.enabled) { > + this.enabled = false; I'd really prefer toggling provider.enabled here rather than Social.enabled if possible
Attachment #765189 -
Flags: review?(mhammond) → review+
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to Mark Hammond (:markh) from comment #16) > Comment on attachment 765189 [details] [diff] [review] > fix updating manifests, add worker api for self-update > > Review of attachment 765189 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/modules/Social.jsm > @@ +172,5 @@ > > + this._updateProviderCache(providers); > > + Services.obs.notifyObservers(null, "social:providers-changed", null); > > + // if we need a reload, do it now > > + if (provider.enabled) { > > + this.enabled = false; > > I'd really prefer toggling provider.enabled here rather than Social.enabled > if possible That would only reload the worker, whereas we need to reload all the frames. Considering an manifest update could change the sidebar url, we need to reload everything. We do need to fix the reload situation so we don't have to toggle social.enabled. I've added a followup bug 885200 to do something about reload.
Assignee | ||
Comment 18•11 years ago
|
||
seems the last try hit leaks from intermittent failure in bug 858948, trying again to see if that cleared up. https://tbpl.mozilla.org/?tree=Try&rev=02d336727824
Assignee | ||
Comment 19•11 years ago
|
||
try three, see if the binding of the callbacks I added caused the issue. https://tbpl.mozilla.org/?tree=Try&rev=99e509969eeb
Assignee | ||
Comment 20•11 years ago
|
||
try four, remove another references that looks circular https://tbpl.mozilla.org/?tree=Try&rev=dc43abc091e2
Assignee | ||
Comment 21•11 years ago
|
||
fixed leak by adding a getter for the manifest rather than keeping reference. Only minor changes, carrying forward r+ https://tbpl.mozilla.org/?tree=Try&rev=3bbcf8003c82
Attachment #765189 -
Attachment is obsolete: true
Attachment #766478 -
Flags: review+
Assignee | ||
Comment 22•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9661a68f0bff
Assignee | ||
Comment 23•11 years ago
|
||
Comment on attachment 766478 [details] [diff] [review] fix updating manifests, add worker api for self-update [Approval Request Comment] Bug caused by (feature/regressing bug #): web install of manifests User impact if declined: users wont have updated features supported by social providers (e.g. an existing provider adding support for share) Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): low String or IDL/UUID changes made by this patch: none
Attachment #766478 -
Flags: approval-mozilla-aurora?
Comment 24•11 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a257161ff235 - your tests weren't particularly fond of it, both simple little mozSocial is undefineds like https://tbpl.mozilla.org/php/getParsedLog.php?id=24503203&tree=Mozilla-Inbound and more severe things like https://tbpl.mozilla.org/php/getParsedLog.php?id=24504005&tree=Mozilla-Inbound
Updated•11 years ago
|
Attachment #766478 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 25•11 years ago
|
||
now that bug 860421 is fixed, try again: https://tbpl.mozilla.org/?tree=Try&rev=b4f85f7f1394
Assignee | ||
Comment 26•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dd428369503
Comment 27•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8dd428369503
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Assignee | ||
Comment 28•11 years ago
|
||
Comment on attachment 766478 [details] [diff] [review] fix updating manifests, add worker api for self-update [Approval Request Comment] Bug caused by (feature/regressing bug #): web install of manifests User impact if declined: users wont have updated features supported by social providers (e.g. an existing provider adding support for share) Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): low String or IDL/UUID changes made by this patch: none
Attachment #766478 -
Flags: approval-mozilla-aurora?
Comment 29•11 years ago
|
||
Comment on attachment 766478 [details] [diff] [review] fix updating manifests, add worker api for self-update low risk patch giving users ability to make use of updated features which are now supported by social
Attachment #766478 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 30•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/f2b49f275dff
status-firefox24:
--- → fixed
status-firefox25:
--- → fixed
Comment 31•11 years ago
|
||
Florian and edA-qa, please check that this works as expected for you with tomorrow's Firefox Nightly and Aurora builds.
Flags: needinfo?(florian)
Flags: needinfo?(em)
Depends on: 886227
Comment 32•11 years ago
|
||
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #31) > Florian and edA-qa, please check that this works as expected for you with > tomorrow's Firefox Nightly and Aurora builds. I'm not clear on exactly what was changed. Is there an API which triggers the reload, or is it automatic/manual, or what triggers it?
Comment 33•11 years ago
|
||
(In reply to edA-qa mort-ora-y from comment #32) > I'm not clear on exactly what was changed. Is there an API which triggers > the reload, or is it automatic/manual, or what triggers it? Please see the attached patch, but the short version is that the worker can send 2 new messages - 'social.manifest-get' to fetch the current manifest (so the worker can see what version of the manifest is held) and 'social.manifest-set' (to update the manifest if the worker determines it is necessary).
Comment 34•11 years ago
|
||
I've implemented what seems like a reasonable solution using this approach (and confirmed it works). - on 'social.initialize' I post social.manifest-get - on 'social.manifest' I load our manifest and compare a version field - if our manifest is newer I call 'social.manifest.set' - this in turn appears to cause a reload of worker/sidebar I confirmed a request to load the new worker/sidebar but didn't actually check if the new worker is actually used. I just assume it is.
Flags: needinfo?(em)
Comment 35•11 years ago
|
||
I'm calling this verified. Thanks for your help edA-qa mort-ora-y.
Status: RESOLVED → VERIFIED
Flags: needinfo?(florian)
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•