Closed
Bug 898706
Opened 11 years ago
Closed 8 years ago
[meta] Remove social FrameWorker
Categories
(Firefox Graveyard :: SocialAPI, defect)
Firefox Graveyard
SocialAPI
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: markh, Assigned: mixedpuppy)
References
Details
(Keywords: dev-doc-complete, site-compat)
Attachments
(1 file, 3 obsolete files)
It has always been an aspirational goal to replace our Frameworker with a real worker - ideally a real shared worker. We might as well start tracking what we depend on to make this happen.
Assignee | ||
Updated•9 years ago
|
Summary: [meta] Replace social FrameWorker with a real worker → [meta] Replace social FrameWorker with a service worker
Assignee | ||
Comment 1•8 years ago
|
||
if a provider needs a worker, they have several to choose from. most do not.
Summary: [meta] Replace social FrameWorker with a service worker → [meta] Remove social FrameWorker
Assignee | ||
Comment 2•8 years ago
|
||
WIP to remove frameworker, looking for feedback. Notes: - Providers can use shared or service workers as needed. Some functionality will have to be considered (ie. WorkerAPI) but most if not all of it is unnecessary. - new event handler on social content to handle ambient notifications. This is TBD. - "profile" is gone, most real functionality around this was removed long ago, it was kept for compat. - Disabling providers in private windows is removed as there is no longer a way to cross that divide. services workers simply dont work in private windows. This means share would now work in PB, users would have to log in again (expected and desirable). - More changes/deprecation possible post this bug - BD will have to discuss changes with a couple partners prior to scheduling this to land, I don't expect push back from the few that actually used frameworkers.
Attachment #8718158 -
Flags: feedback?(markh)
Reporter | ||
Comment 3•8 years ago
|
||
Comment on attachment 8718158 [details] [diff] [review] WIP: remove frameworker Review of attachment 8718158 [details] [diff] [review]: ----------------------------------------------------------------- Less is more! LGTM ::: browser/base/content/test/social/browser_share.js @@ +40,5 @@ > + let frameScript = "data:,(" + function frame_script() { > + addEventListener("OpenGraphData", function (aEvent) { > + sendAsyncMessage("sharedata", aEvent.detail); > + }, true, true); > + }.toString() + ")();"; I wonder if .toSource() is better here, and whether we should call encodeURI() on the string? (same comment in a number of other tests too) @@ +228,5 @@ > "html": "The Raspberry Pi is a credit-card sized computer that plugs into your TV and a keyboard. It's a capable little PC which can be used for many of the things that your desktop PC does, like spreadsheets, word-processing and games. It also plays high-definition video. We want to see it being used by kids all over the world to learn programming." > } > ], > "url": ["https://example.com/"], > + "price": ["£29.95"], it looks a little like your editor changed the encoding of this file - maybe unicode literals would be better? ::: browser/base/content/test/social/browser_social_chatwindowfocus.js @@ -20,5 @@ > // to be true. > EventUtils.synthesizeMouseAtCenter(button, {}, sidebarDoc.defaultView); > } > > -function openChatViaSidebarMessage(port, data, callback) { how would a serviceworker request a chat to be opened in this new world? ::: browser/base/content/test/social/microformats.html @@ +6,5 @@ > <h2 class="fn">Raspberry Pi</h2> > <img class="photo" src="https://example.com/someimage.jpg" /> > <p class="description">The Raspberry Pi is a credit-card sized computer that plugs into your TV and a keyboard. It's a capable little PC which can be used for many of the things that your desktop PC does, like spreadsheets, word-processing and games. It also plays high-definition video. We want to see it being used by kids all over the world to learn programming.</p> > <a class="url" href="https://example.com/">More info about the Raspberry Pi</a> > + <p class="price">£29.95</p> ditto here re encodings (but TBH I'm not sure if you are just *fixing* the encoding - I'm not sure what bugzilla does) ::: toolkit/components/social/SocialService.jsm @@ +502,5 @@ > } > }, > > _manifestFromData: function(type, data, installOrigin) { > + let featureURLs = ['sidebarURL', 'shareURL', 'statusURL', 'markURL']; I wonder if we should Cu.reportError() if we get a provider with a workerURL for diagnostics (not sure here is the best place though)
Attachment #8718158 -
Flags: feedback?(markh) → feedback+
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Mark Hammond [:markh] from comment #3) > > ], > > "url": ["https://example.com/"], > > + "price": ["£29.95"], > > it looks a little like your editor changed the encoding of this file - maybe > unicode literals would be better? hrm, I think I switched those to utf-8 encoding (intentionally) but I'll verify. > ::: browser/base/content/test/social/browser_social_chatwindowfocus.js > > > > -function openChatViaSidebarMessage(port, data, callback) { > > how would a serviceworker request a chat to be opened in this new world? Hello is the only use of chat windows, I think it's fine to deprecate opening them from a worker, deal with it if we get an actual use case.
Assignee | ||
Comment 6•8 years ago
|
||
updated patch, move to review
Assignee: nobody → mixedpuppy
Attachment #8718158 -
Attachment is obsolete: true
Attachment #8730355 -
Flags: review?(markh)
Assignee | ||
Comment 7•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79dbd143662b
Reporter | ||
Comment 8•8 years ago
|
||
Comment on attachment 8730355 [details] [diff] [review] remove worker Review of attachment 8730355 [details] [diff] [review]: ----------------------------------------------------------------- This looks great, but I think we need to make a change to social-content.js as having Social.jsm in both processes sounds wrong. ::: browser/base/content/browser-social.js @@ -286,5 @@ > if (browser == SocialShare.iframe && Services.prefs.getBoolPref("social.share.activationPanelEnabled")) { > options = { bypassContentCheck: true, bypassInstallPanel: true }; > } > > - // If we are in PB mode, we silently do nothing (bug 829404 exists to it looks like we should close this bug once this lands (I wonder what other bugs are assigned to Boriss we should also close? :) ::: browser/base/content/social-content.js @@ +10,5 @@ > > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/Services.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "Social", > + "resource:///modules/Social.jsm"); This import seems suspect - Social.jsm has state, but now that it's imported in both parent and child processes there seems a risk the state will end up different between the 2 processes. Eg, when a provider is uninstalled (which IIUC happens in the parent), how will the child know the provider no longer exists? @@ +38,5 @@ > var gHookedWindowCloseForPanelClose = false; > > +addEventListener("Social:Notification", function(event) { > + let frame = docShell.chromeEventHandler; > + let origin = frame.getAttribute("origin"); So I think here you need to send a messageManager to the parent and have it make the call on the provider it has access to. ::: browser/base/content/test/social/browser_social_status.js @@ +46,5 @@ > + sendAsyncMessage("visibility", "hidden"); > + }, false); > + addMessageListener("socialTest-sendEvent", function(msg) { > + let data = msg.data; > + dump("***** msg data is "+JSON.stringify(msg.data)+"\n"); kill this dump ::: browser/base/content/test/social/microformats.html @@ +6,5 @@ > <h2 class="fn">Raspberry Pi</h2> > <img class="photo" src="https://example.com/someimage.jpg" /> > <p class="description">The Raspberry Pi is a credit-card sized computer that plugs into your TV and a keyboard. It's a capable little PC which can be used for many of the things that your desktop PC does, like spreadsheets, word-processing and games. It also plays high-definition video. We want to see it being used by kids all over the world to learn programming.</p> > <a class="url" href="https://example.com/">More info about the Raspberry Pi</a> > + <p class="price">å£29.95</p> this encoding change still looks strange - bugzilla shows the old version correctly having the pound symbol. Like I said before, using unicode literals probably makes more sense (ie, "\xa3") ::: browser/base/content/test/social/social_sidebar.html @@ +11,3 @@ > </script> > </head> > + <body\> stray "\" here.
Attachment #8730355 -
Flags: review?(markh)
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=735eace69eeb
Assignee | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a22f5acabf4
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca09d0f49e4d
Assignee | ||
Comment 13•8 years ago
|
||
comments addressed as well as fixing a couple test issues.
Attachment #8730355 -
Attachment is obsolete: true
Attachment #8731386 -
Flags: review?(markh)
Reporter | ||
Comment 14•8 years ago
|
||
Comment on attachment 8731386 [details] [diff] [review] remove worker Review of attachment 8731386 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/social-content.js @@ +9,5 @@ > var {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components; > > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/Services.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "Social", We can remove this new import now. ::: browser/base/content/test/social/browser_social_marks.js @@ +150,5 @@ > ok(btn, "got a mark button"); > + let ourTab; > + // > + //let mm = getGroupMessageManager("social"); > + //mm.addMessageListener("visibility", function handler(msg) { we should probably kill this commented block if it isn't necessary.
Attachment #8731386 -
Flags: review?(markh) → review+
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5762be8b4930
Assignee | ||
Comment 16•8 years ago
|
||
updated, carry forward r+
Attachment #8731386 -
Attachment is obsolete: true
Attachment #8731738 -
Flags: review+
Assignee | ||
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/32a2a01d3d2b3f3bd7aa9fd29d065f18c3bf4dd5 Bug 898706 remove frameworker and all associated code and tests, r=markh
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/32a2a01d3d2b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment 19•8 years ago
|
||
Documentation updated: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API/Service_worker_API_reference https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API/Manifest https://developer.mozilla.org/en-US/Firefox/Releases/48
Keywords: dev-doc-complete
Comment 20•8 years ago
|
||
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/social-service-worker-api-has-been-removed/
Keywords: site-compat
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
•