RDP, new packet type: getObjectsProperties

RESOLVED FIXED in Firefox 26

Status

DevTools
Debugger
P3
normal
RESOLVED FIXED
6 years ago
10 days ago

People

(Reporter: Honza, Assigned: mhordecki)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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
Blocks: 800200
Priority: -- → P3
(Assignee)

Comment 1

5 years ago
It looks like this packet type doesn't exist anymore. I'm closing the issue for now.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
This packet type has never existed, this bug is a request to implement it.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Comment 3

5 years ago
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.
(Reporter)

Comment 5

5 years ago
(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
(Assignee)

Comment 6

5 years ago
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)

Updated

5 years ago
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)
(Assignee)

Comment 8

5 years ago
Created attachment 776774 [details] [diff] [review]
New packet type implementation.
(Assignee)

Comment 9

5 years ago
Also, the corresponding pull request can be found at https://github.com/jimblandy/DebuggerDocs/pull/12 .
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 12

5 years ago
Created attachment 777488 [details] [diff] [review]
Patch v2
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+

Comment 14

5 years ago
(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)
(Assignee)

Comment 15

5 years ago
Created attachment 786522 [details] [diff] [review]
Patch v3

Attached a new patch with changes proposed by Panos.
Attachment #777488 - Attachment is obsolete: true
Attachment #786522 - Flags: review?(past)
(Assignee)

Comment 16

5 years ago
Created attachment 786527 [details] [diff] [review]
Patch v3.1

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)
(Assignee)

Comment 19

5 years ago
Created attachment 791052 [details] [diff] [review]
0001-Bug-795979-RDP-new-packet-type-getObjectsProperties.patch

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]

Comment 20

5 years ago
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
Last Resolved: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26

Updated

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