Closed Bug 898706 Opened 11 years ago Closed 8 years ago

[meta] Remove social FrameWorker

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set
normal

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.
Depends on: 899908
Depends on: 936302
Depends on: 936307
Blocks: 986998
Summary: [meta] Replace social FrameWorker with a real worker → [meta] Replace social FrameWorker with a service worker
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
Attached patch WIP: remove frameworker (obsolete) — Splinter Review
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)
Blocks: 1032398
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+
(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.
Attached patch remove worker (obsolete) — Splinter Review
updated patch, move to review
Assignee: nobody → mixedpuppy
Attachment #8718158 - Attachment is obsolete: true
Attachment #8730355 - Flags: review?(markh)
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)
Attached patch remove worker (obsolete) — Splinter Review
comments addressed as well as fixing a couple test issues.
Attachment #8730355 - Attachment is obsolete: true
Attachment #8731386 - Flags: review?(markh)
Blocks: 1074553
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+
Attached file remove worker
updated, carry forward r+
Attachment #8731386 - Attachment is obsolete: true
Attachment #8731738 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/32a2a01d3d2b3f3bd7aa9fd29d065f18c3bf4dd5
Bug 898706 remove frameworker and all associated code and tests, r=markh
Depends on: 1257746
https://hg.mozilla.org/mozilla-central/rev/32a2a01d3d2b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.