Closed Bug 1257723 Opened 4 years ago Closed 4 years ago

move more event handling to content script

Categories

(Firefox Graveyard :: SocialAPI, defect)

46 Branch
defect
Not set

Tracking

(firefox48 fixed, firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

Attachments

(1 file, 3 obsolete files)

several dom events (load, visibility, close, etc) that are needed by social need to move into the content script to support remote=true on the social frames.
Blocks: 1074553
Assignee: nobody → mixedpuppy
Attachment #8731966 - Flags: review?(markh)
Comment on attachment 8731966 [details] [diff] [review]
move event handling to content script

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

LGTM with the message names changed.

::: browser/base/content/browser-social.js
@@ +357,5 @@
>                          { template: "about:socialerror?mode=compactInfo&origin=%{origin}" });
>    },
>  
> +  get messageManager() {
> +    return this.iframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.messageManager;

I think we should keep the comment here, as otherwise people will assume we just didn't know about the obvious way to get it.

@@ +517,5 @@
> +    return this.iframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.messageManager;
> +  },
> +
> +  receiveMessage: function(aMessage) {
> +    dump("received message "+aMessage.name+"\n");

dump should die

@@ +884,3 @@
>        } else {
> +        // if the document has not loaded, delay until it is
> +        if (sbrowser.contentDocument.readyState != "complete") {

it looks like this is still a CPOW reference, so there's probably scope to improve this further, but I guess I'm not too bothered - it's still an improvement on what we had.

::: browser/base/content/social-content.js
@@ +22,3 @@
>  
>  var gDOMContentLoaded = false;
> +addEventListener("DOMContentLoaded", function(event) {

I think we should use a Social: prefix on these messages too - if content.js ends up growing the same message names things will either break or get very confusing.

@@ +53,5 @@
>      "detail": JSON.parse(event.detail)
>    });
>  });
>  
> +addMessageListener("OpenGraphData", (message) => {

ditto here I think.
Attachment #8731966 - Flags: review?(markh) → review+
some minor additions based on prepending Social: to messages.

not handling CPOW issues in this bug, I expect there will be a few more steps/bugs towards making it possible to remote the social frames.
Attachment #8731966 - Attachment is obsolete: true
Attachment #8733162 - Flags: review?(markh)
Comment on attachment 8733162 [details] [diff] [review]
move event handling to content script

I'm going to have revert the DOMContentLoaded name change, and have a followup bug on hello to make that change.  Hello has this scattered throughout its code and tests.
Attachment #8733162 - Flags: review?(markh)
reverting to original patch with a couple of the comments addressed, so carrying forward that r+.
Attachment #8733162 - Attachment is obsolete: true
Attachment #8733206 - Flags: review+
Backed out in https://hg.mozilla.org/integration/fx-team/rev/fda36d04418de60f78844a51240ae89e38c20582

Hard to tell what was failure from this and what was failure from bug 1253604 in platforms/suites we just didn't get around to running on its push, but I think that at least https://treeherder.mozilla.org/logviewer.html#?job_id=8982341&repo=fx-team and the browser_aboutHome_activation.js part of https://treeherder.mozilla.org/logviewer.html#?job_id=8982364&repo=fx-team (where the browser_share.js part is what I backed out bug 1253604 for) were from this one.
revert share to using load event (rather than DOMContentLoaded), some functionality depends on that and now it passes tests again.
Attachment #8733206 - Attachment is obsolete: true
Attachment #8750394 - Flags: review?(markh)
Blocks: 1253604
Comment on attachment 8750394 [details] [diff] [review]
move event handling to content script

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

Looks fine to me
Attachment #8750394 - Flags: review?(markh) → review+
https://hg.mozilla.org/mozilla-central/rev/5467a8d9d844
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment on attachment 8750394 [details] [diff] [review]
move event handling to content script

Approval Request Comment
[Feature/regressing bug #]:bug 1253604
[User impact if declined]:share can be broken by share providers using onbeforeunload
[Describe test coverage new/current, TreeHerder]:existing tests
[Risks and why]: This particular patch is a step towards being able to support remote frames for socialapi, however it fixes some logic that bug 1253604 needs to be a complete fix.  Would like to get this into release earlier by uplift to aurora, but larger than what we'd want in beta.
[String/UUID change made/needed]:none
Attachment #8750394 - Flags: approval-mozilla-aurora?
Comment on attachment 8750394 [details] [diff] [review]
move event handling to content script

Support for share features after page reload. 
We should verify this on aurora after it lands along with the fix from bug 1253604.
Attachment #8750394 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1274595
Depends on: 1327939
Depends on: 1327952
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.