Closed Bug 1290599 Opened 8 years ago Closed 8 years ago

Choose either close or onClosed as transport closing event

Categories

(DevTools :: General, defect, P3)

Product:
DevTools
Component:
General
Type:
defect

Tracking

(firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: jryans, Assigned: jsnajdr)

Details

Attachments

(3 files)

Bug 1290599 - Part 1: Improve names of events fired by debugger transport
58 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1290599 - Part 3: New unit test for debugger transport events
58 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1290599 - Part 2: Fix the browser_toolbox_transport_events.js test
58 bytes, text/x-review-board-request
jryans
: review+
Details 
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
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)
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-
(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)
(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 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 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 :(
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-
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+
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+
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 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)
(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)
Going to checkin. I'll file followup bugs for the issues mentioned in comment 11.
Keywords: checkin-needed
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: