Closed
Bug 1257723
Opened 8 years ago
Closed 8 years ago
move more event handling to content script
Categories
(Firefox Graveyard :: SocialAPI, defect)
Tracking
(firefox48 fixed, firefox49 fixed)
RESOLVED
FIXED
Firefox 49
People
(Reporter: mixedpuppy, Assigned: mixedpuppy)
References
Details
Attachments
(1 file, 3 obsolete files)
27.78 KB,
patch
|
markh
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•8 years ago
|
||
Assignee: nobody → mixedpuppy
Attachment #8731966 -
Flags: review?(markh)
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d0fa4b1e8a3
Comment 3•8 years ago
|
||
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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f29aa9101cbc
Assignee | ||
Comment 5•8 years ago
|
||
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•8 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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=216ba4c7116e
Assignee | ||
Comment 8•8 years ago
|
||
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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5305f90e1e5
Assignee | ||
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5fdb0bf1527a446f946d3585371f4da0ab33c447 Bug 1257723 move to using messagemanager for some events, r=markh
Comment 11•8 years ago
|
||
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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cfc234a262b
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d96db96f50ce
Assignee | ||
Comment 14•8 years ago
|
||
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)
Comment 15•8 years ago
|
||
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•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5467a8d9d844fff2b8916999ebd1f4e90289e2d1 Bug 1257723 move to using messagemanager for some events, r=markh
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5467a8d9d844
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 18•8 years 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 19•8 years ago
|
||
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•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/f25acdea677c
status-firefox48:
--- → fixed
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•