892605 - Add a gcli command for black boxing sources in the debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

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

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)

Comment 1

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

Attachment: part 1: use events on the thread client instead of dom events

Comment 2

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

Attachment: 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.

Comment 3

[Victor Porof [:vporof][:vp]]

(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.

Comment 4

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

https://tbpl.mozilla.org/?tree=Try&rev=55fcbe094fd3

Comment 5

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

Attachment: Part 0: add the ability to call Widget child methods (and add this for check)

Comment 6

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

Attachment: part 1: use events on the thread client instead of dom events

Comment 7

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

Attachment: Part 2: add gcli command, update checkboxes accordingly

Comment 9

[Victor Porof [:vporof][:vp]]

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.

Comment 10

[Victor Porof [:vporof][:vp]]

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).

Comment 11

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

(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.

Comment 12

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

https://hg.mozilla.org/integration/fx-team/rev/0c80222426fe
https://hg.mozilla.org/integration/fx-team/rev/893b7d8e7b17
https://hg.mozilla.org/integration/fx-team/rev/aa190a916dcc

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0c80222426fe
https://hg.mozilla.org/mozilla-central/rev/893b7d8e7b17
https://hg.mozilla.org/mozilla-central/rev/aa190a916dcc