1053973 - [e10s] update Social.jsm OpenGraphBuilder

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Comment 1

[Chris Peterson [:cpeterson]]

Move old M2's low-priority bugs to M6 milestone.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: get page data via message manager

@markh, in socialmarks.xml you'll see where we pass on the target element of the context menu.  Fails per same reasons in bug 1039528

Comment 4

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8494878 [details] [diff] [review]
get page data via message manager

I see your pain here, but not sure what feedback I can offer.  I think the issue in bug 1039528 should be addressed in a more generic way that will help you here (and Mossop says he might look into that), but otherwise I don't really have much advice.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: fix opengraph data to work under e10s

context menu issue is moved to bug 1072669, and that test is separated out on it's own.

ttps://tbpl.mozilla.org/?tree=Try&rev=5890a4382a95

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: fix opengraph data to work under e10s

part of patch moved to bug 915547 to fix an e10s test.

new try https://tbpl.mozilla.org/?tree=Try&rev=d3d40a092b6a

Comment 7

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8516906 [details] [diff] [review]
fix opengraph data to work under e10s

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

Looks OK to me, but the promise problem in particular means I'd like another look.

::: browser/base/content/browser-social.js
@@ +297,5 @@
>    },
>  
> +  // retreive various meta data markup, such as OpenGraph, meta tags, etc. that
> +  // provide rich sharing data
> +  getPageData: function(browser) {

this looks suspect to me - if getPageData was ever called twice before the first response was received, it looks like the first promise will never resolve.

I'd be inclined to have the listener inline in getPageData (or, eg, a _promiseOneMessage helper given you use this pattern twice)

@@ +544,5 @@
>      });
>    }
>  }
>  
> +SocialShare = {  

trailing whitespace

::: browser/base/content/content.js
@@ +854,5 @@
> +      case "Social:GetPageData":
> +        sendAsyncMessage("Social:PageDataResult", this.og.getData(content.document));
> +        break;
> +      case "Social:GetMicrodata":
> +        // BUG 1072669, target use fails with e10s.  also see sender in browser-social.js

I'm a little confused by this comment (and the similar one in browser-social), even after reading that other bug.  If "target use fails" why are we passing it? At face value, the comment seems to imply simply "note that this doesn't work" :) I think these comments should be more descriptive about what the problem is and how it is being worked around.

::: browser/modules/Social.jsm
@@ +510,5 @@
>        endpointURL = endpointURL + "?" + str.join("&");
>      return endpointURL;
>    },
>  
> +  getData: function(aDocument, target) {

The new param should just be "document", or the second param should be renamed to "aTarget" - probably the former as a leading 'a' typically isn't used here.

(and ditto for all other touched functions below)

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond [:markh] from comment #7)

> ::: browser/base/content/browser-social.js
> @@ +297,5 @@
> >    },
> >  
> > +  // retreive various meta data markup, such as OpenGraph, meta tags, etc. that
> > +  // provide rich sharing data
> > +  getPageData: function(browser) {
> 
> this looks suspect to me - if getPageData was ever called twice before the
> first response was received, it looks like the first promise will never
> resolve.
> 
> I'd be inclined to have the listener inline in getPageData (or, eg, a
> _promiseOneMessage helper given you use this pattern twice)

agreed, I'll rework that code.

> ::: browser/base/content/content.js
> @@ +854,5 @@
> > +      case "Social:GetPageData":
> > +        sendAsyncMessage("Social:PageDataResult", this.og.getData(content.document));
> > +        break;
> > +      case "Social:GetMicrodata":
> > +        // BUG 1072669, target use fails with e10s.  also see sender in browser-social.js
> 
> I'm a little confused by this comment (and the similar one in
> browser-social), even after reading that other bug.  If "target use fails"
> why are we passing it? At face value, the comment seems to imply simply
> "note that this doesn't work" :) I think these comments should be more
> descriptive about what the problem is and how it is being worked around.

I looked at this further today.  I believe that bug 1072669 is only a test issue.  In the real world, "target" ends up being sent from the child browser via Content:Click message (and manual testing works fine), whereas the test is calling getElementById on the child document, and the test fails with that.  I'll need to rework the test somehow.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: fix opengraph data to work under e10s

cleaned up message listeners

Comment 10

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/85378234c56c

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/85378234c56c