If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

move more event handling to content script

RESOLVED FIXED in Firefox 48

Status

()

Firefox
SocialAPI
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

(Blocks: 1 bug)

46 Branch
Firefox 49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1074553
(Assignee)

Comment 1

2 years ago
Created attachment 8731966 [details] [diff] [review]
move event handling to content script
Assignee: nobody → mixedpuppy
Attachment #8731966 - Flags: review?(markh)
(Assignee)

Comment 2

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d0fa4b1e8a3
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+
(Assignee)

Comment 4

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f29aa9101cbc
(Assignee)

Comment 5

2 years ago
Created attachment 8733162 [details] [diff] [review]
move event handling to content script

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)
(Assignee)

Comment 6

2 years ago
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)
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=216ba4c7116e
(Assignee)

Comment 8

2 years ago
Created attachment 8733206 [details] [diff] [review]
move event handling to content script

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+
(Assignee)

Comment 9

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5305f90e1e5
(Assignee)

Comment 10

a year ago
https://hg.mozilla.org/integration/fx-team/rev/5fdb0bf1527a446f946d3585371f4da0ab33c447
Bug 1257723 move to using messagemanager for some events, r=markh
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.
(Assignee)

Comment 12

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cfc234a262b
(Assignee)

Comment 13

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d96db96f50ce
(Assignee)

Comment 14

a year ago
Created attachment 8750394 [details] [diff] [review]
move event handling to content script

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)
(Assignee)

Updated

a year ago
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+
(Assignee)

Comment 16

a year ago
https://hg.mozilla.org/integration/fx-team/rev/5467a8d9d844fff2b8916999ebd1f4e90289e2d1
Bug 1257723 move to using messagemanager for some events, r=markh

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5467a8d9d844
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
(Assignee)

Comment 18

a year ago
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+

Comment 20

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/f25acdea677c
status-firefox48: --- → fixed
(Assignee)

Updated

a year ago
Depends on: 1274595

Updated

9 months ago
Depends on: 1327939

Updated

9 months ago
Depends on: 1327952
You need to log in before you can comment on or make changes to this bug.