795979 - RDP, new packet type: getObjectsProperties

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

In order to reduce number of client server round-trips, it would be useful to get properties for more objects at once - getObjectsProperties.

(there is currently only getObjectPropeties packet type).

Honza

Comment 1

[Mike Hordecki [:mhordecki] (UTC+2)]

It looks like this packet type doesn't exist anymore. I'm closing the issue for now.

Comment 2

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

This packet type has never existed, this bug is a request to implement it.

Comment 3

[Mike Hordecki [:mhordecki] (UTC+2)]

I was talking about the getObjectProperties one - there is no mention of it in the code, at least I can't find any.

Comment 4

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

That is true, Honza was referring to the DebuggerClient's getPrototypeAndProperties method that sends a prototypeAndProperties protocol request.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Panos Astithas [:past] from comment #4)
> That is true, Honza was referring to the DebuggerClient's
> getPrototypeAndProperties method that sends a prototypeAndProperties
> protocol request.
That's correct

Honza

Comment 6

[Mike Hordecki [:mhordecki] (UTC+2)]

As far as implementation goes, I thinking about adding a method to ThreadActor that takes a list of actor names. What do you guys think? I'm just getting my feet wet in devtools.

Comment 7

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

Things we need:

* Proposed spec (pull request to https://github.com/jimblandy/DebuggerDocs would be nice)

* Handler on the server (ThreadActor)

* Requester on the client (ThreadClient, toolkit/devtools/client/dbg-client.js)

Comment 8

[Mike Hordecki [:mhordecki] (UTC+2)]

Attachment: New packet type implementation.

Comment 9

[Mike Hordecki [:mhordecki] (UTC+2)]

Also, the corresponding pull request can be found at https://github.com/jimblandy/DebuggerDocs/pull/12 .

Comment 10

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

Comment on attachment 776774 [details] [diff] [review]
New packet type implementation.

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

Did a quick pass, but I'll play with it more tomorrow.

::: toolkit/devtools/client/dbg-client.jsm
@@ +1543,5 @@
> +  getPrototypesAndProperties: DebuggerClient.requester({
> +    type: "prototypesAndProperties",
> +    actors: args(0)
> +  }, {
> +    telemetry: "PROTOTYPEANDPROPERTIES"

Typo: PROTOTYPESANDPROPERTIES (you are missing an 'S'). You also need to add this to Histograms.json (2 entries like the rest of the requests).

Comment 11

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

Jim should also sign off on the protocol spec change.

Comment 12

[Mike Hordecki [:mhordecki] (UTC+2)]

Attachment: Patch v2

Comment 13

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

Comment on attachment 777488 [details] [diff] [review]
Patch v2

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

Just a few nits, but this looks good, if Jim approves the protocol spec change.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=956fd0f47891

::: toolkit/devtools/client/dbg-client.jsm
@@ +1537,5 @@
> +  /**
> +   * Request the prototype and own properties of mutlipleObjects.
> +   *
> +   * @param aOnResponse function Called with the request's response.
> +   * @param actors [string] List of actor ID of the queried objects.

Nit: move the description beneath the type info, as we do in other comments in this file.

::: toolkit/devtools/server/actors/script.js
@@ +1351,5 @@
> +      if (!actor) {
> +        return { from: this.actorID,
> +                 error: "noSuchActor" };
> +      }
> +      let handler = actor.requestTypes["prototypeAndProperties"];

actor.onPrototypeAndProperties would be clearer.

::: toolkit/devtools/server/tests/unit/test_objectgrips-08.js
@@ +4,5 @@
> +var gDebuggee;
> +var gClient;
> +var gThreadClient;
> +
> +function run_test()

Add a brief comment explaining the purpose of the test please.

Comment 14

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #11)
> Jim should also sign off on the protocol spec change.

Looks good; merged the pull request.

Comment 15

[Mike Hordecki [:mhordecki] (UTC+2)]

Attachment: Patch v3

Attached a new patch with changes proposed by Panos.

Comment 16

[Mike Hordecki [:mhordecki] (UTC+2)]

Attachment: Patch v3.1

Submitted a wrong thing.

Comment 17

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

Try push: https://tbpl.mozilla.org/?tree=Try&rev=6f63082fdd42

Comment 18

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

Comment on attachment 786527 [details] [diff] [review]
Patch v3.1

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

Cancelling review until test failures are taken care of.

Comment 19

[Mike Hordecki [:mhordecki] (UTC+2)]

Attachment: 0001-Bug-795979-RDP-new-packet-type-getObjectsProperties.patch

Test failures should be corrected now.

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=5d67a721cf67

Comment 20

[Paul Rouget [:paul]]

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

Comment 21

[Ed Morley [:emorley]]

Was backed out (https://hg.mozilla.org/integration/fx-team/rev/60610dbd2cf0) on suspicion of causing orange, but this was actually due to another landing, so have relanded:
remote:   https://hg.mozilla.org/integration/fx-team/rev/092dd07390a1

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/092dd07390a1