Choose either close or onClosed as transport closing event

RESOLVED FIXED in Firefox 51

Status

DevTools
General
P3
normal
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: jryans, Assigned: jsnajdr)

Tracking

unspecified
Firefox 51

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

In bug 1126274, an event API was added to observe a transport.  One part of this is an event on transport close.

Some transports emit "close" and others emit "onClosed".  We should choose one and apply it everywhere.

AFAIK, the only consumer of this API is the RDP Inspector add-on.  At the moment, it does not appear to actually do anything in response to this particular event.
> Some transports emit "close" and others emit "onClosed".  We should choose one and apply it everywhere.
Both works for me.

As soon as this is fixed I can adapt the code in RDPi.

Honza
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 years ago
Wow, after some time fighting with git-cinnabar, my first MozReview request :)

One thing I'm not sure how to handle: a duplicate version of transport.js file is bundled inside devtools/client/debugger/new/bundle.js

James, is there any process in place how to transfer changes in files like this into the debugger.html repo? This particular bugfix doesn't matter, but there might be others that do.
Assignee: nobody → jsnajdr
Flags: needinfo?(jlong)
There is no process yet, things like this don't affect us because it only affects the consumer of this transport client, and we use our own websocket thing. The only changes that really affect us are devtools/shared/client/main.js. Because the amount of changes to that file are small we've just been manually checking if it's been changed. The long-term goal is to be able to simply copy over that and other files, but we need to de-chromify them first. We should have a process in place soon.
Flags: needinfo?(jlong)
(Reporter)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8779270 [details]
Bug 1290599 - Part 1: Improve names of events fired by debugger transport

https://reviewboard.mozilla.org/r/70296/#review67762

Thanks for taking a look at this!

::: devtools/shared/transport/transport.js:657
(Diff revision 1)
>  
>      /**
>       * Close the transport.
>       */
>      close: function () {
> -      this.emit("close");
> +      this.emit("onClosed");

Looking at all usages of `emit` in the transports, my impression of the name convention was that the event name is meant to be the method name on the transport.  See examples like `send` as the event name.  I agree with your commit message that the naming is a bit unusual compared to the DOM APIs.

I think at a minimum, it would be better to go the other way, and change this event to `close` everywhere, since the method is named `close`.

Since Honza has already chimed in and seems fine updating RDP if needed, maybe it would be good to also clean up the `on*` names, since they do feel odd as far as event names go.  Perhaps `onBulkPacket` becomes `bulk-packet`?  Up to you whether to tackle this part.
Attachment #8779270 - Flags: review?(jryans) → review-
(Assignee)

Comment 7

2 years ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> I think at a minimum, it would be better to go the other way, and change
> this event to `close` everywhere, since the method is named `close`.
> 
> Since Honza has already chimed in and seems fine updating RDP if needed,
> maybe it would be good to also clean up the `on*` names, since they do feel
> odd as far as event names go.  Perhaps `onBulkPacket` becomes `bulk-packet`?
> Up to you whether to tackle this part.

OK, I will do a complete cleanup of the transport events. Another cleanup worth considering is removing the "hooks" API, which is somewhat duplicate, and replacing it with event listeners. This way, there will be only one way how to receive events from the transport. What do you think about that?
Flags: needinfo?(jryans)
(Reporter)

Comment 8

2 years ago
(In reply to Jarda Snajdr [:jsnajdr] from comment #7)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> > I think at a minimum, it would be better to go the other way, and change
> > this event to `close` everywhere, since the method is named `close`.
> > 
> > Since Honza has already chimed in and seems fine updating RDP if needed,
> > maybe it would be good to also clean up the `on*` names, since they do feel
> > odd as far as event names go.  Perhaps `onBulkPacket` becomes `bulk-packet`?
> > Up to you whether to tackle this part.
> 
> OK, I will do a complete cleanup of the transport events. Another cleanup
> worth considering is removing the "hooks" API, which is somewhat duplicate,
> and replacing it with event listeners. This way, there will be only one way
> how to receive events from the transport. What do you think about that?

If you are up for it, it does seem like a nice clean up as well, but it will be more involved and should be done in a separate bug.  The reason it's more involved is that some add-ons make use of the hooks API[1].  From the search results, at least Firebug and Valence would be impacted (the B2G simulators are spurious, they just happen to contain client files they don't use).

[1]: https://dxr.mozilla.org/addons/search?q=transport.hooks&redirect=true (use LDAP to login)
Flags: needinfo?(jryans)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
mozreview-review-reply
Comment on attachment 8779270 [details]
Bug 1290599 - Part 1: Improve names of events fired by debugger transport

https://reviewboard.mozilla.org/r/70296/#review67762

> Looking at all usages of `emit` in the transports, my impression of the name convention was that the event name is meant to be the method name on the transport.  See examples like `send` as the event name.  I agree with your commit message that the naming is a bit unusual compared to the DOM APIs.
> 
> I think at a minimum, it would be better to go the other way, and change this event to `close` everywhere, since the method is named `close`.
> 
> Since Honza has already chimed in and seems fine updating RDP if needed, maybe it would be good to also clean up the `on*` names, since they do feel odd as far as event names go.  Perhaps `onBulkPacket` becomes `bulk-packet`?  Up to you whether to tackle this part.

I switched all the events to DOM convention - lowercase without any separators:
- send
- startbulksend
- packet
- bulkpacket
- close

I also added a new unit test (xpcshell) that tests the events.

I discovered several other ugly things when working on this:
- running the xpcshell tests shows "head_dbg.js:84: TypeError: DebuggerServer.xpcInspector is undefined" - probably some long-gone property?
- if the transport doesn't have any hooks set, you get JS exceptions. Some hook invocations check if this.hooks is defined, some don't
- the "close" event gets fired twice in both socket and local transport - once when you call transport.close(), second time when the other side sends close back to you
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
Comment on attachment 8780116 [details]
Bug 1290599 - Part 2: Fix the browser_toolbox_transport_events.js test

I didn't notice the browser_toolbox_transport_events.js test before it failed on try. Submitted a fix. The new xpcshell is now mostly redundant :(
(Reporter)

Comment 16

2 years ago
mozreview-review
Comment on attachment 8779270 [details]
Bug 1290599 - Part 1: Improve names of events fired by debugger transport

https://reviewboard.mozilla.org/r/70296/#review68344

What about the event names in your new websocket-transport.js?
Attachment #8779270 - Flags: review?(jryans) → review-
(Reporter)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8780116 [details]
Bug 1290599 - Part 2: Fix the browser_toolbox_transport_events.js test

https://reviewboard.mozilla.org/r/70916/#review68346

Looks good, thanks for updating!
Attachment #8780116 - Flags: review?(jryans) → review+
(Reporter)

Comment 18

2 years ago
mozreview-review
Comment on attachment 8780058 [details]
Bug 1290599 - Part 3: New unit test for debugger transport events

https://reviewboard.mozilla.org/r/70880/#review68350

Seems nice to have this test, xpcshell is nicer to work with.
Attachment #8780058 - Flags: review?(jryans) → review+
(Reporter)

Comment 19

2 years ago
mozreview-review-reply
Comment on attachment 8779270 [details]
Bug 1290599 - Part 1: Improve names of events fired by debugger transport

https://reviewboard.mozilla.org/r/70296/#review67762

> I switched all the events to DOM convention - lowercase without any separators:
> - send
> - startbulksend
> - packet
> - bulkpacket
> - close
> 
> I also added a new unit test (xpcshell) that tests the events.
> 
> I discovered several other ugly things when working on this:
> - running the xpcshell tests shows "head_dbg.js:84: TypeError: DebuggerServer.xpcInspector is undefined" - probably some long-gone property?
> - if the transport doesn't have any hooks set, you get JS exceptions. Some hook invocations check if this.hooks is defined, some don't
> - the "close" event gets fired twice in both socket and local transport - once when you call transport.close(), second time when the other side sends close back to you

>I discovered several other ugly things when working on this:
- running the xpcshell tests shows "head_dbg.js:84: TypeError: DebuggerServer.xpcInspector is undefined" - probably some long-gone property?

Looks like `xpcInspector` was changed to be a module you require[1].  Feel free to update the head file if you like!

>- if the transport doesn't have any hooks set, you get JS exceptions. Some hook invocations check if this.hooks is defined, some don't

Feel free to add more guards if you like.

>- the "close" event gets fired twice in both socket and local transport - once when you call transport.close(), second time when the other side sends close back to you

Yes, it would probably be more correct to only do the closing work once.  We could check at the beginning of close() if it's already done and return early without emitting or anything else if so.

[1]: https://dxr.mozilla.org/mozilla-central/search?q=xpcInspector&case=true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 23

2 years ago
mozreview-review
Comment on attachment 8779270 [details]
Bug 1290599 - Part 1: Improve names of events fired by debugger transport

https://reviewboard.mozilla.org/r/70296/#review68366

Looks good, thanks for working on this!
Attachment #8779270 - Flags: review?(jryans) → review+
So, if I understand correctly, in order to keep backward compatibility, any Add-on can just listen to both version of an event name: the old one and the new one, correct?

Honza
Flags: needinfo?(jsnajdr)
(Assignee)

Comment 25

2 years ago
(In reply to Jan Honza Odvarko [:Honza] from comment #24)
> So, if I understand correctly, in order to keep backward compatibility, any
> Add-on can just listen to both version of an event name: the old one and the
> new one, correct?

Exactly, just attach the same listener to both old and new event name.
Flags: needinfo?(jsnajdr)
(Assignee)

Comment 26

2 years ago
Going to checkin. I'll file followup bugs for the issues mentioned in comment 11.
Keywords: checkin-needed

Comment 27

2 years ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/51600f3f15c2
Part 1: Improve names of events fired by debugger transport r=jryans
https://hg.mozilla.org/integration/autoland/rev/a1ab21952abc
Part 2: Fix the browser_toolbox_transport_events.js test r=jryans
https://hg.mozilla.org/integration/autoland/rev/8fd2f35a58fa
Part 3: New unit test for debugger transport events r=jryans
Keywords: checkin-needed

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/51600f3f15c2
https://hg.mozilla.org/mozilla-central/rev/a1ab21952abc
https://hg.mozilla.org/mozilla-central/rev/8fd2f35a58fa
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.