Status

defect
RESOLVED FIXED
7 years ago
4 months ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

unspecified
Firefox 17
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx16])

Attachments

(1 attachment, 6 obsolete attachments)

Assignee

Description

7 years ago
we need to inject navigator.mozSocial on the social content panels/sidebar, this bug is to port over the code from the addon.
Assignee

Updated

7 years ago
Blocks: 770695
Assignee

Comment 1

7 years ago
Posted patch mozSocialAPI.patch (obsolete) — Splinter Review
mozSocial api implementation
Attachment #641650 - Flags: feedback?(gavin.sharp)
Assignee

Comment 2

7 years ago
Posted patch mozSocialAPI.patch (obsolete) — Splinter Review
Attachment #641650 - Attachment is obsolete: true
Attachment #641650 - Flags: feedback?(gavin.sharp)
Attachment #642036 - Flags: feedback?(gavin.sharp)
Posted patch modified patch (obsolete) — Splinter Review
Shane: I made a couple of changes to the patch:
- Initialize in SocialService instead of browser code
- adjust the attachToWindow code to use createObjectIn/makeObjectPropsNormal/etc. stuff from bug 634156 rather than sandbox insertion
- add a test (in browser/ since it depends on browser-specific UI pieces (panel UI)
- other minor cleanup

mrbkap: can you review the attachToWindow function specifically? It's attempting to inject a function into content script that returns a chrome JS object, and I'm not sure I'm doing that properly.
Attachment #642036 - Attachment is obsolete: true
Attachment #642036 - Flags: feedback?(gavin.sharp)
Attachment #644157 - Flags: review?(mrbkap)
Attachment #644157 - Flags: review?(mixedpuppy)
(this is on top of my updated patch for bug 755136)
Assignee

Comment 5

7 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #3)
> Created attachment 644157 [details] [diff] [review]
> modified patch

I see an empty patch.
Posted patch patch (obsolete) — Splinter Review
Here's the real patch...
Attachment #644157 - Attachment is obsolete: true
Attachment #644157 - Flags: review?(mrbkap)
Attachment #644157 - Flags: review?(mixedpuppy)
Attachment #644195 - Flags: review?(mrbkap)
Attachment #644195 - Flags: review?(mixedpuppy)
Assignee

Comment 7

7 years ago
+
+    var containingBrowser = window.QueryInterface(Ci.nsIInterfaceRequestor)
+                                  .getInterface(Ci.nsIWebNavigation)
+                                  .QueryInterface(Ci.nsIDocShell)
+                                  .chromeEventHandler;
+
+    // If the containing browser isn't one of the social browsers, nothing to
+    // do here.
+    // XXX this is app-specific, might want some mechanism for consumers to
+    // whitelist other IDs/windowtypes  
+    if (!(containingBrowser.id == "social-sidebar-browser" ||
+          containingBrowser.id == "social-notification-browser") ||
+        containingBrowser.ownerDocument.documentElement.getAttribute("windowtype") != "navigator:browser") {
+      return;
+    }
+
+    let origin = containingBrowser.getAttribute("origin");
+    if (!origin) {
+      return;
+    }
+

preventing windowtype == navigator:browser will break the service/chat window (later patch) which uses the navigator:browser window via openDialog.  Having the origin attribute takes care of ensuring that we only attach onto social content.
(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> preventing windowtype == navigator:browser will break the service/chat
> window (later patch) which uses the navigator:browser window via openDialog.
> Having the origin attribute takes care of ensuring that we only attach onto
> social content.

OK, I'll remove that part. I wanted a more robust check than just checking the id/origin, but that's probably good enough.
Comment on attachment 644195 [details] [diff] [review]
patch

Review of attachment 644195 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Hopefully this sort of thing will get much easier with bug 760109.

::: toolkit/components/social/MozSocialAPI.jsm
@@ +89,5 @@
> +          port: "r"
> +        }
> +      };
> +    },
> +    __exposedProps__: {

mozSocialObj is never exposed directly to content, so it doesn't need __exposedProps__.
Attachment #644195 - Flags: review?(mrbkap) → review+
Posted patch updated patch (obsolete) — Splinter Review
I cleaned a few of the __exposedProps__ mistakes (addressing at least part of bug 773162), and simplified the code a little bit per discussion with mrbkap.
Attachment #644195 - Attachment is obsolete: true
Attachment #644195 - Flags: review?(mixedpuppy)
Attachment #644434 - Flags: review?(mixedpuppy)
Here is a new version of the same patch that doesn't report leaks while testing.  The only changes are the first 2 chunks (2 places where createAboutBlankContentViewer is called) and the removal of the code that skips the test on debug builds.  Not obsoleting the old patch to avoid stepping on toes.
Comment on attachment 644844 [details] [diff] [review]
version of the patch that doesn't report leaks

I'll split this off as a patch against bug 775779
Comment on attachment 644434 [details] [diff] [review]
updated patch

I'd be inclined to deprecate getWorker immediately (ie, add a cu.reportError when getWorker is called and add a .port property now), but given that can be done later and that we are in somewhat of a hurry, r+
Attachment #644434 - Flags: review?(mixedpuppy) → review+
Comment on attachment 644434 [details] [diff] [review]
updated patch

browser_frameworker test fails:

TEST-INFO | chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_frameworker.js | Console message: [JavaScript Error: "FrameWorker: Error handling client port control message: TypeError: this._clientWindow.wrappedJSObject is undefined
Attachment #644434 - Flags: review+ → review-
browser_frameworker test failed with the old patch.  This version changes only that failing test.
Attachment #644434 - Attachment is obsolete: true
Attachment #644844 - Attachment is obsolete: true
Applied a different fix, as described on IRC (use XPCNativeWrapper.unwrap instead of wrappedJSObject), landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/deaf2494a3a6
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Firefox 17
https://hg.mozilla.org/mozilla-central/rev/deaf2494a3a6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED

Updated

4 months ago
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.