RDP, new packet type: getObjectsProperties

RESOLVED FIXED in Firefox 26

Status

defect
P3
normal
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: Honza, Assigned: mhordecki)

Tracking

Trunk
Firefox 26
x86
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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
Priority: -- → P3
It looks like this packet type doesn't exist anymore. I'm closing the issue for now.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
This packet type has never existed, this bug is a request to implement it.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
I was talking about the getObjectProperties one - there is no mention of it in the code, at least I can't find any.
That is true, Honza was referring to the DebuggerClient's getPrototypeAndProperties method that sends a prototypeAndProperties protocol request.
(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
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.
Assignee: nobody → mhordecki
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)
Also, the corresponding pull request can be found at https://github.com/jimblandy/DebuggerDocs/pull/12 .
Attachment #776774 - Flags: review?(past)
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).
Jim should also sign off on the protocol spec change.
Status: REOPENED → ASSIGNED
Flags: needinfo?(jimb)
Posted patch Patch v2 (obsolete) — Splinter Review
Attachment #776774 - Attachment is obsolete: true
Attachment #776774 - Flags: review?(past)
Attachment #777488 - Flags: review?(past)
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.
Attachment #777488 - Flags: review?(past) → review+
(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.
Flags: needinfo?(jimb)
Posted patch Patch v3 (obsolete) — Splinter Review
Attached a new patch with changes proposed by Panos.
Attachment #777488 - Attachment is obsolete: true
Attachment #786522 - Flags: review?(past)
Posted patch Patch v3.1 (obsolete) — Splinter Review
Submitted a wrong thing.
Attachment #786522 - Attachment is obsolete: true
Attachment #786522 - Flags: review?(past)
Attachment #786527 - Flags: review?(past)
Blocks: 902512
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.
Attachment #786527 - Flags: review?(past)
Test failures should be corrected now.

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=5d67a721cf67
Attachment #786527 - Attachment is obsolete: true
Attachment #791052 - Flags: review?(past)
Attachment #791052 - Flags: review?(past) → review+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/e039f5fc18d0
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
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
https://hg.mozilla.org/mozilla-central/rev/092dd07390a1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.