Note: There are a few cases of duplicates in user autocompletion which are being worked on.

The update function in dbg-script-actors.js should also copy getters and setters to the target object

RESOLVED FIXED in Firefox 18

Status

()

Firefox
Developer Tools: Debugger
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: fitzgen, Assigned: anton)

Tracking

unspecified
Firefox 18
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Using a combination of getOwnPropertyDescriptor and defineProperty should do the trick.

Possibly a good first bug. If we think so, I can mentor it.
Does this mean that the patch for bug 755661 will fail to copy the threadActor getter to the SourceActor prototype?
Yes, which is why I am using this._threadActor in the source method. This is a really easy bug to fix, but I have just been trying to track down these mochitest failures on the patches for bug 755661.
(Assignee)

Updated

5 years ago
Assignee: nobody → anton
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
Created attachment 663181 [details] [diff] [review]
Patch to copy getters/setters alongside normal properties.

Attached patch contains a very trivial change. Two questions, though:

1. Was it intentional that you didn't check for hasOwnProperty? If yes, I can change the patch to either copy inherited properties directly (aTarget[key] = aNewAttrs[key]) or climb up the prototype tree looking for that property.

2. This function, update, is not exported (as far as I can tell from dbg-server.jsm only DebuggerServer is) so I can't really write a unit test for that. Or is there a way to unit test unexported functions?
Attachment #663181 - Flags: feedback?(nfitzgerald)
Comment on attachment 663181 [details] [diff] [review]
Patch to copy getters/setters alongside normal properties.

Looks good to me!

> 1. Was it intentional that you didn't check for hasOwnProperty? If yes, I
> can change the patch to either copy inherited properties directly
> (aTarget[key] = aNewAttrs[key]) or climb up the prototype tree looking for
> that property.

I just figured it wasn't that big of a deal, since we can be fairly sure no one is messing with Object.prototype in the chrome context. I suppose we should check hasOwnProperty, though.

> 2. This function, update, is not exported (as far as I can tell from
> dbg-server.jsm only DebuggerServer is) so I can't really write a unit test
> for that. Or is there a way to unit test unexported functions?

Not sure if there is a way to unit test unexported functions, but what you can do is verify that the getters/setters on objects that use |update| in the prototype actually work. I believe there are some broken getters/setters on LongStringActor's prototype right now.
Attachment #663181 - Flags: feedback?(nfitzgerald) → feedback+
(Assignee)

Comment 5

5 years ago
> Not sure if there is a way to unit test unexported functions, but what
> you can do is verify that the getters/setters on objects that use 
> |update| in the prototype actually work. I believe there are some broken 
> getters/setters on LongStringActor's prototype right now.

I don't see LongStringActor using |update|. ObjectActor uses it but it doesn't have any getters/setters in the source object.

fx-team $ cd toolkit/devtools
devtools $ find . -name "*.js" -exec grep -nH --color "update(" {} \;
debugger/server/dbg-script-actors.js:1127:function update(aTarget, aNewAttrs) {
debugger/server/dbg-script-actors.js:1154:update(ObjectActor.prototype, {
(Assignee)

Comment 6

5 years ago
Created attachment 663214 [details] [diff] [review]
Patch to copy getters/setters alongside normal properties.
Attachment #663181 - Attachment is obsolete: true
Attachment #663214 - Flags: review?(nfitzgerald)
Comment on attachment 663214 [details] [diff] [review]
Patch to copy getters/setters alongside normal properties.

Nice!
Attachment #663214 - Flags: review?(nfitzgerald) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
I'm not a module peer, so although I gave r+ and approve of the patch, it isn't ready to land until a module peer gives it r+.

Sorry for the confusion! >_<
Whiteboard: [land-in-fx-team]
(Assignee)

Updated

5 years ago
Attachment #663214 - Flags: review+ → review?(past)
Comment on attachment 663214 [details] [diff] [review]
Patch to copy getters/setters alongside normal properties.

Looks solid. I'm not worried about testing, since this code path will be taken by other tests, particularly if Nick remembers to change the patch in bug 755661 to use this.threadActor in the source method.
Attachment #663214 - Attachment is patch: true
Attachment #663214 - Flags: review?(past) → review+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/c90c90509736
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c90c90509736
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
You need to log in before you can comment on or make changes to this bug.