937197 - add a "requestTypes" RDP request

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Every actor should accept "requestTypes" requests and return a packet of the form

  { from: actorID, requestTypes: [...] }

where requestTypes is an array of string request types.

This will help us with feature detection when dealing with slightly different versions of the protocol, and helps make our protocol a little more self documenting.

We shouldn't need to actually implement onRequestTypes for every actor, though. The server's main onPacket method could handle this for everyone: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#969

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

More info for any one who wants to pick up this bug:

General info on how to hack on our tools: https://wiki.mozilla.org/DevTools/Hacking

The test suite you'll be running: https://wiki.mozilla.org/DevTools/Hacking#xpcshell_Tests

You'll need to add a new xpcshell test:

1. Create a file "test_requestTypes.js" in toolkit/devtools/server/tests/unit/

2. Add it to the xpcshell.ini in the same directory

3. Do an incremental build

4. Run the tests again to see if it passes/fails

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Clarification: the request packet form should be:

  { to: actorID, type "requestTypes" }

Also, you should be able to get a list of an actor's request types by doing:

  Object.keys(actor.requestTypes)

Comment 3

[Peiyong Lin[:lpy](UTC+8)]

Hello Nick, I am willing to try to fix this bug. So I pick it up.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Great! If you have any questions, feel free to ask me either on this bug or on irc!

Here is an overview of our remote debugging protocol (RDP): https://wiki.mozilla.org/Remote_Debugging_Protocol

Comment 5

[Peiyong Lin[:lpy](UTC+8)]

Thank you.

Comment 6

[Peiyong Lin[:lpy](UTC+8)]

Hi Nick. Should I run the requestTypes for all acotrs? Or I just need to pick on to test?

Comment 7

[Peiyong Lin[:lpy](UTC+8)]

Oh, sorry. I mean whether I should run the requestTypes test for all actors.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

You don't need to test this for every actor, but you should probably test a couple different ones. Maybe the ThreadActor and the ObjectActor.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Testing an actor that uses protocol.js would be good as well. Not sure if we have any that can be tested with xpcshell tests, but chrome mochitests will work.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hi Peiyong!

Are you still actively working on this? I only ask because there hasn't been much movement here lately and I have another contributor who expressed interest in this particular bug. If you are still working on this, a quick confirmation of that fact would be awesome. If you aren't, then I will re-assign to the other contributor.

Thanks!

Comment 11

[Peiyong Lin[:lpy](UTC+8)]

I am sorry. I am not working on this bug.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Peiyong Lin[:lpy](UTC-8) from comment #11)
> I am sorry. I am not working on this bug.

No problem :) Thanks for unassigning yourself.

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: rev0 - handle requestTypes RDP request for any existent RDP actor, added success test case (xpcshell-test unit)

In this first revision of the patch:

- add "requestTypes RDP request" for any existent actor (handled in the toolkit/devtools/server/main.js module just after the check for actor existency)
- add a new xpcshell-test unit (test_requestTypes.js), currently it checks the success test-case on the chrome debugger actor

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8355820 [details] [diff] [review]
rev0 - handle requestTypes RDP request for any existent RDP actor, added success test case (xpcshell-test unit)

Review of attachment 8355820 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! Thanks, Luca.

r+ with small comments below addressed.

::: toolkit/devtools/server/tests/unit/test_requestTypes.js
@@ +10,5 @@
> +  var calls = [];
> +
> +  calls.push(test_existent_actor(aClient, anActor));
> +
> +  promise.all.apply(null, calls).then(() => {

Should be able to just do:

  promise.all(calls).then(...)

@@ +22,5 @@
> +{
> +  let deferred = promise.defer();
> +
> +  aClient.request({ to: anActor, type: "requestTypes" }, function (aResponse) {
> +    do_check_eq(typeof aResponse.requestTypes.length, "number");

What is the point of checking that |length| is a number? Making sure that |requestTypes| is an array? If that is what we are going for, I'd replace the check with:

  Array.isArray(aResponse.requestTypes)

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: rev1 - applied suggested tweaks to test_requestTypes.js xpcshell test in rev0

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Luca Greco from comment #15)
> Created attachment 8356169 [details] [diff] [review]
> rev1 - applied suggested tweaks to test_requestTypes.js xpcshell test in rev0

Thanks!

In the future, when I say "r+ with blah blah", that means you don't need to flag me for review again. You can just upload the new patch and add the "checkin-needed" keyword.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/05d387f1dc78

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/05d387f1dc78