Closed
Bug 784893
Opened 12 years ago
Closed 12 years ago
Make nsITCPSocketEvent look more like a normal object
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: sicking, Assigned: fzzzy)
References
Details
Attachments
(1 file, 1 obsolete file)
From bug 733573 comment 192:
If the reason is that we can't implement DOMEvents in JS, then we should make sure that nsITCPSocketEvent looks as much as possible as nsIDOMEvent. That would mean renaming "source" to "target", and making sure that the "type" property returns strings like "open", "error", "data", "drain", "close". I.e. the "on"-prefix should not be included in .type.
Comment 1•12 years ago
|
||
nsITCPSocketEvent should be implemented using event generator.
For now change
readonly attribute jsval data;
to
readonly attribute nsIVariant data;
Add [noscript] initTCPSocketEvent method and dictionary for constructor
and add the idl to http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/event_impl_gen.conf.in?force=1
The event interface should be in its own .idl file.
That way you get an event which can be created using event ctors:
var e = new TCPSocketEvent("foobarname", { socket: yoursocket, data: yourdata});
No need to write any C++. The event generator takes care of cycle collecting member variables
when needed.
Comment 2•12 years ago
|
||
Comment 3•12 years ago
|
||
I'm experiencing a regression on the contents of the data array when actually run in content, so I think we should add a mochitest for this fix that does actual network access to be really sure that the event is good. We might be able to just have mochitest talk to our built-in HTTP server as the test?
Assignee | ||
Comment 4•12 years ago
|
||
Sounds like we would have to implement a simple http client on top of tcpsocket if we want to talk to the built in httpd? This shouldn't be too hard if it just makes a lot of assumptions about http and sticks to doing http 1.0 with Connection: close.
Comment 5•12 years ago
|
||
All we need to do is make sure that ondata returns something that looks valid. I was thinking just "GET / HTTP/1.0\n\n" and making sure that the result has ASCII-looking values. If the values are undefined or remain initialized to \u0000, that is no good. Implementing an http client would be total overkill.
Assignee | ||
Comment 6•12 years ago
|
||
Ok, right. That's pretty much what I meant anyway; enough of an http client to make the server produce a response :-)
Comment 7•12 years ago
|
||
A potentially related issue to switching to a proper event object is using dispatchEvent or something that will properly catch and report exceptions thrown by the event handler in such a way so that window.onerror gets the error and so that there is a full stack trace. The current errors that end up looking like the following example are fairly sucky:
[Exception... "'TypeError: index is undefined' when calling method: [nsIStreamListener::onDataAvailable]" nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)" location: "<unknown>" data: no] [XPConnect JavaScript]
I think the XPConnect lossage case will still likely need to exist for the xpcshell case/tests, but having proper errors in content would be a real nice thing.
Reporter | ||
Comment 8•12 years ago
|
||
I think the only thing that we have to do right now is to fix the issues mentioned in comment 0. Anything else will be easier to do once we have proper support for implementing EventTargets in JS. And those changes will be backwards compatible enough that we can do them without worrying about breaking content.
Comment 9•12 years ago
|
||
Just implement nsITCPSocketEvent using codegenerator.
Oh, and I hope http://mxr.mozilla.org/mozilla-central/source/dom/network/interfaces/nsIDOMTCPSocket.idl?mark=194-198#191 is wrong. Event types don't start with on-
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #9)
> Just implement nsITCPSocketEvent using codegenerator.
>
> Oh, and I hope
> http://mxr.mozilla.org/mozilla-central/source/dom/network/interfaces/
> nsIDOMTCPSocket.idl?mark=194-198#191 is wrong. Event types don't start with
> on-
The types currently start with on- but this bug is about fixing that, as Jonas mentioned in the first comment on this bug.
Using the codegenerator to get the right behavior is definitely the way this bug will be fixed.
Reporter | ||
Comment 11•12 years ago
|
||
Olli: Can you take this bug. Donovan won't have time to learn the code generator stuff in time for this to make B2Gv1 so otherwise we'll have to just do the simple changes mentioned in comment 0.
Comment 12•12 years ago
|
||
Ok, I'll add support for TCPConnectEvent, but someone needs to test this stuff.
I don't know how to run any b2g stuff, so I'll test this first without #ifdefs and add then
hopefully the right #ifdef.
Comment 13•12 years ago
|
||
Actually, since .target can't point to the right object, and 'this' handling would be wrong,
I'll just wait for someone to make TCPSocket a real eventtarget.
Reporter | ||
Comment 14•12 years ago
|
||
Ok, can we then simply do the renaming from comment 0. That way we are much less likely to break content once we fix this stuff up in the future. That was the whole point of this bug in the first place.
Donovan: would you be able to do this?
Assignee | ||
Comment 16•12 years ago
|
||
- Switched .socket attribute name to .target
- Changed the .type strings to "open", "data" etc instead of "onopen", "ondata"
Tests pass. The tests showed me how to fix the IPC code too, so Horray! Tests!
Attachment #672379 -
Flags: review?(jonas)
Reporter | ||
Comment 17•12 years ago
|
||
Comment on attachment 672379 [details] [diff] [review]
Rename the event's .socket attribute to .target; remove "on" prefix from event names
Review of attachment 672379 [details] [diff] [review]:
-----------------------------------------------------------------
Yay! Thanks!
Attachment #672379 -
Flags: review?(jonas) → review+
Reporter | ||
Comment 18•12 years ago
|
||
And thanks for jumping on this so quickly!
Reporter | ||
Updated•12 years ago
|
Attachment #672379 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 19•12 years ago
|
||
Do I need to push this to try or add checkin-needed to the whiteboard? I'm still a little new to bugzilla..
Comment 20•12 years ago
|
||
If I understand things properly, then:
If you have L3 then you need to push to mozilla-inbound, and if it's sucessful there, push to mozilla-aurora.
If you don't have L3, then you should add [checkin-needed] and [needs-checkin-aurora] to the whiteboard.
Assignee | ||
Comment 21•12 years ago
|
||
This is the exact same patch with the hg metadata lines at the beginning.
Attachment #672379 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Whiteboard: checkin-needed needs-checkin-aurora
Updated•12 years ago
|
Keywords: checkin-needed
Whiteboard: checkin-needed needs-checkin-aurora → [checkin to inbound and aurora]
Reporter | ||
Comment 22•12 years ago
|
||
Do we have any mochitests or other tests that are running automatically on try that cover this code? If so, definitely push to try.
Assignee | ||
Comment 23•12 years ago
|
||
Yes, there are mochitests and xpcshell tests.
I tried to push to try but my ssh key is still not set up properly or something.
Comment 24•12 years ago
|
||
Having a commit message on the patch you want landed is greatly appreciated. Please do so in the future. Also, I was unaware that Jonas is able to approve patches for landing on aurora? Jonas, I don't want to land this on aurora without being sure that the release drivers are OK with it. Can you clarify?
https://hg.mozilla.org/integration/mozilla-inbound/rev/67f8d99d80fb
Finally, should this have a test?
Reporter | ||
Comment 25•12 years ago
|
||
I was honestly a bit surprised myself, but I assumed I was given the bugzilla bits for the same reason that I was given bits to mark basecamp-blocking+ (which currently has the same effect).
I'll check with drivers and set back the approval flag if they confirm.
Comment 26•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 27•12 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #25)
> I'll check with drivers and set back the approval flag if they confirm.
Any updates?
Reporter | ||
Comment 28•12 years ago
|
||
Thanks for reminding me. I had lost track of which bug this question had been asked.
I just checked with Lukas and he said that this was ok for patches that we want for B2G and that are safe for desktop/fennec. Which is the case here (I wouldn't have approved it otherwise).
Thanks for checking on this to make sure that all the ducks are in a row!
Reporter | ||
Comment 29•12 years ago
|
||
Comment on attachment 672497 [details] [diff] [review]
Rename the event's .socket attribute to .target; remove "on" prefix from event names
Please make sure to mark this as status-firefox18:fixed when landing this.
Donovan: We really should be having tests for this. Would you mind writing some up? Ideally we would have mochitests which test the full implementation of the API, for example by connecting to the mochitest http server and simulating a simple HTTP request.
Attachment #672497 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 30•12 years ago
|
||
There's already a full test suite of xpcshell tests which test the lower level stuff, and mochitests which test the permissions stuff.
Reporter | ||
Comment 31•12 years ago
|
||
But apparently nothing that tests the API itself?
Good to know though that most of this stuff is tested. Definitely makes it a lower priority to write any remaining tests.
Comment 32•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/2f0e4a2982fb
Going to call this in-testsuite+ based on comment 30. Jonas, feel free to change it back if you feel otherwise.
Updated•12 years ago
|
Component: DOM: Mozilla Extensions → DOM
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•