785044 - Replace the ThreadActor's private debugger object and its getter with a public property

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

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

No need for this complication. See also bug 783393 comment 48.

Comment 1

[Amod [:AMoz]]

Sir,
I would like to contribute to this bug, Will you explain the bug in detail ?

Comment 2

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

(In reply to Amod from comment #1)
> Sir,
> I would like to contribute to this bug, Will you explain the bug in detail ?

Yes, thank you. In toolkit/devtools/debugger/server/dbg-script-actors.js there is a _dbg property on the ThreadActor prototype as well as a dbg getter for that property. This distinction has never been of much use in practice, so for simplicity, we would like to just have a plain dbg property on the prototype, without a getter. 

This is pretty much a search-and-replace job, but you will also need to run the xpcshell tests and the mochitests to be sure nothing is broken after this change. The locations of the tests are:

toolkit/devtools/debugger/tests/unit/
browser/devtools/debugger/test/

Pop in the #devtools channel in IRC (I'm past) if you need any help with the above.

Comment 3

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

To expand on what Panos has said, to actually run the tests you can use the following commands (after you have rebuilt Firefox with your changes):

To run the xpcshell tests:

$ make -C $OBJ/toolkit/devtools/debugger/tests/ xpcshell-tests

To run the mochitests:

$ TEST_PATH=browser/devtools/debugger/test/ make -C obj/ mochitest-browser-chrome

Comment 4

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

Where $OBJ is your object directory.

(Mine is simply "obj", which is why that second command is "make -C obj/". I forgot to change that to "$OBJ" when pasting.)

Comment 5

[WangJun]

Is getter used in any function? I have not found any function that uses get dbg()

Comment 6

[Amod [:AMoz]]

(In reply to Panos Astithas [:past] from comment #2)
> (In reply to Amod from comment #1)
> > Sir,
> > I would like to contribute to this bug, Will you explain the bug in detail ?
> 
> Yes, thank you. In toolkit/devtools/debugger/server/dbg-script-actors.js
> there is a _dbg property on the ThreadActor prototype as well as a dbg
> getter for that property. This distinction has never been of much use in
> practice, so for simplicity, we would like to just have a plain dbg property
> on the prototype, without a getter. 
> 
> This is pretty much a search-and-replace job, but you will also need to run
> the xpcshell tests and the mochitests to be sure nothing is broken after
> this change. The locations of the tests are:
> 
> toolkit/devtools/debugger/tests/unit/
> browser/devtools/debugger/test/
> 
> Pop in the #devtools channel in IRC (I'm past) if you need any help with the
> above.


Yes Sir, there is a line where get dbg() is used and in the body dbg property is used using 'this' keyword..so after that what exactly should be done...and how to recognize whether the property is public or private ?

Comment 7

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

(In reply to WangJun from comment #5)
> Is getter used in any function? I have not found any function that uses get
> dbg()

Getters are triggered from plain property accesses, like "this.dbg".

(In reply to Amod from comment #6)
> Yes Sir, there is a line where get dbg() is used and in the body dbg
> property is used using 'this' keyword..so after that what exactly should be
> done...and how to recognize whether the property is public or private ?

As I said in comment 2, you should replace all instances of this._dbg with this.dbg and then run the tests to make sure everything still works as expected. We prefix private properties with an underscore, so |_foo| is considered a private property, whereas |foo| is considered public.

Comment 8

[Anton Kovalyov (:anton)]

Attachment: Patch to replace _dbg property and dbg getter with a dbg property

I ran both xpcshell tests and mochi tests, they all passed. I also find/grep'ed toolkit/ and browser/devtools directory to see if _dbg was ever used as a public property but didn't notice any cases.

Let me know if I missed anything.

Comment 9

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

Comment on attachment 663166 [details] [diff] [review]
Patch to replace _dbg property and dbg getter with a dbg property

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

Looks like you got them all, thanks!

Comment 10

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

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

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/48ed2f6d569f