Add a gcli command for black boxing sources in the debugger

RESOLVED FIXED in Firefox 25

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Blocks 1 bug)

unspecified
Firefox 25
x86
macOS
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

We should have a gcli command that allows you to black box sources in the debugger either by

* specifying them directly (http://example.com/jquery.js)

* with a regular expression (ember\-.+)

* or with a host (cdn.example.com)
Assignee: nobody → nfitzgerald
Posted patch wip part 2: add gcli command (obsolete) — Splinter Review
Works, but doesn't update checkbox state. Discussed with Victor the proper way to do this last week, just haven't implemented it yet.
(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> Created attachment 779906 [details] [diff] [review]
> wip part 2: add gcli command
> 
> Works, but doesn't update checkbox state. Discussed with Victor the proper
> way to do this last week, just haven't implemented it yet.

Might want to make that bug depending/blocker/something.
Depends on: 877686
Depends on: 897173
Attachment #779904 - Attachment is obsolete: true
Attachment #779906 - Attachment is obsolete: true
Attachment #779904 - Flags: review?(vporof)
Attachment #781309 - Flags: review?(vporof)
Duplicate of this bug: 897173
Comment on attachment 781309 [details] [diff] [review]
Part 0: add the ability to call Widget child methods (and add this for check)

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

::: browser/devtools/shared/widgets/SideMenuWidget.jsm
@@ +331,5 @@
>      }
>    },
>  
>    /**
> +   * Set the checkbox state for the item associate with the given node.

associate*d*

@@ +608,5 @@
>    get _orderedGroupElementsArray() this.ownerView._orderedGroupElementsArray,
>    get _orderedMenuElementsArray() this.ownerView._orderedMenuElementsArray,
> +  get _itemsByElement() { return this.ownerView._itemsByElement; },
> +
> +  _makeCheckbox: function (aAttachment) {

Document this method please (the conditions in which it's called, and the event that shall be emitted because of this).

@@ +702,5 @@
>    ownerView: null,
>    _target: null,
>    _container: null,
> +  _arrow: null,
> +  _checkbox: null

Nit: move _checkbox before _arrow since that's the order in which they're created and displayed.
Attachment #781309 - Flags: review?(vporof) → review+
Attachment #781310 - Flags: review?(vporof) → review+
Comment on attachment 781311 [details] [diff] [review]
Part 2: add gcli command, update checkboxes accordingly

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

::: browser/devtools/debugger/CmdDebugger.jsm
@@ +477,5 @@
> +
> +      const { promise, resolve, reject } = context.defer();
> +      const { activeThread } = dbg._controller;
> +
> +      activeThread.getSources(function ({ sources, error }) {

Why aren't you reusing the same sources in dbg._view.Sources here instead of asking for all of them again?

for (let sourceItem in dbg._view.Sources) {
  let sourceForm = sourceItem.attachment.source;
}

@@ +574,5 @@
> +
> +/**
> + * Converts a glob to a regular expression
> + */
> +function globToRegExp(glob) {

Do we really want to do this? Wouldn't it be safer to assume that people expect regex directly as argument? What do you think?

Sure, such globs are arguably easier to work with, but.. It's pretty funky.

::: browser/devtools/debugger/test/browser_dbg_cmd_blackbox.js
@@ +48,5 @@
> +      .then(waitForDebuggerSources)
> +      .then(testBlackBoxSource)
> +      .then(testUnBlackBoxSource)
> +      .then(testBlackBoxGlob)
> +      .then(testUnBlackBoxGlob)

<3

::: toolkit/devtools/client/dbg-client.jsm
@@ +1760,5 @@
>    get _activeThread() this._client.activeThread,
>    get isBlackBoxed() this._isBlackBoxed,
>    get actor() this._form.actor,
>    get request() this._client.request,
> +  get url() this._form.url,

Do we really need this if we have the source form? (I think it's safe to assume that if you need the source client you already know the url).
Attachment #781311 - Flags: review?(vporof) → review+
(In reply to Victor Porof [:vp] from comment #10)
> Comment on attachment 781311 [details] [diff] [review]
> Part 2: add gcli command, update checkboxes accordingly
> 
> Review of attachment 781311 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/CmdDebugger.jsm
> @@ +477,5 @@
> > +
> > +      const { promise, resolve, reject } = context.defer();
> > +      const { activeThread } = dbg._controller;
> > +
> > +      activeThread.getSources(function ({ sources, error }) {
> 
> Why aren't you reusing the same sources in dbg._view.Sources here instead of
> asking for all of them again?
> 
> for (let sourceItem in dbg._view.Sources) {
>   let sourceForm = sourceItem.attachment.source;
> }
> 

Good call.

> @@ +574,5 @@
> > +
> > +/**
> > + * Converts a glob to a regular expression
> > + */
> > +function globToRegExp(glob) {
> 
> Do we really want to do this? Wouldn't it be safer to assume that people
> expect regex directly as argument? What do you think?
> 
> Sure, such globs are arguably easier to work with, but.. It's pretty funky.

I think globs are the right way to go.

> 
> ::: browser/devtools/debugger/test/browser_dbg_cmd_blackbox.js
> @@ +48,5 @@
> > +      .then(waitForDebuggerSources)
> > +      .then(testBlackBoxSource)
> > +      .then(testUnBlackBoxSource)
> > +      .then(testBlackBoxGlob)
> > +      .then(testUnBlackBoxGlob)
> 
> <3

Thanks to Anton, who wrote the profiler cmd test and inspired this one. I have no idea how the other dbg command tests work.

> 
> ::: toolkit/devtools/client/dbg-client.jsm
> @@ +1760,5 @@
> >    get _activeThread() this._client.activeThread,
> >    get isBlackBoxed() this._isBlackBoxed,
> >    get actor() this._form.actor,
> >    get request() this._client.request,
> > +  get url() this._form.url,
> 
> Do we really need this if we have the source form? (I think it's safe to
> assume that if you need the source client you already know the url).

I think we should be moving away form using the packets directly at all, and towards only using clients so that we completely abstract over the protocol. A lofty goal that won't happen anytime soon, but I think we should make baby steps here and there.
No longer blocks: 895272
https://hg.mozilla.org/mozilla-central/rev/0c80222426fe
https://hg.mozilla.org/mozilla-central/rev/893b7d8e7b17
https://hg.mozilla.org/mozilla-central/rev/aa190a916dcc
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25

Updated

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