Last Comment Bug 788769 - The update function in dbg-script-actors.js should also copy getters and setters to the target object
: The update function in dbg-script-actors.js should also copy getters and sett...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: Firefox 18
Assigned To: Anton Kovalyov (:anton)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-05 14:15 PDT by Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
Modified: 2012-09-22 00:35 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to copy getters/setters alongside normal properties. (711 bytes, patch)
2012-09-20 15:19 PDT, Anton Kovalyov (:anton)
nfitzgerald: feedback+
Details | Diff | Review
Patch to copy getters/setters alongside normal properties. (740 bytes, patch)
2012-09-20 16:48 PDT, Anton Kovalyov (:anton)
past: review+
Details | Diff | Review

Description Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-05 14:15:59 PDT
Using a combination of getOwnPropertyDescriptor and defineProperty should do the trick.

Possibly a good first bug. If we think so, I can mentor it.
Comment 1 Panos Astithas [:past] 2012-09-06 01:21:32 PDT
Does this mean that the patch for bug 755661 will fail to copy the threadActor getter to the SourceActor prototype?
Comment 2 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-06 11:23:43 PDT
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.
Comment 3 Anton Kovalyov (:anton) 2012-09-20 15:19:54 PDT
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?
Comment 4 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-20 16:21:14 PDT
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.
Comment 5 Anton Kovalyov (:anton) 2012-09-20 16:43:24 PDT
> 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, {
Comment 6 Anton Kovalyov (:anton) 2012-09-20 16:48:56 PDT
Created attachment 663214 [details] [diff] [review]
Patch to copy getters/setters alongside normal properties.
Comment 7 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-20 17:02:17 PDT
Comment on attachment 663214 [details] [diff] [review]
Patch to copy getters/setters alongside normal properties.

Nice!
Comment 8 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-20 17:06:18 PDT
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! >_<
Comment 9 Panos Astithas [:past] 2012-09-20 23:43:53 PDT
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.
Comment 10 Panos Astithas [:past] 2012-09-21 01:02:00 PDT
https://hg.mozilla.org/integration/fx-team/rev/c90c90509736
Comment 11 Tim Taubert [:ttaubert] 2012-09-22 00:35:24 PDT
https://hg.mozilla.org/mozilla-central/rev/c90c90509736

Note You need to log in before you can comment on or make changes to this bug.