901733 - Add "--invert" flag to the "dbg blackbox" command

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

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

This way you could (for example) use the browser debugger and black box all the sources that aren't in the debugger itself by issuing the command "dbg blackbox --invert --glob *debugger*".

Comment 1

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

The code for the "dbg blackbox" command is inside browser/devtools/debugger/CmdDebugger.jsm, this would involve adding a new param "invert" here[0] and changing the list of sources we queue up to black box here[1].

[0]: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/CmdDebugger.jsm#448

[1]: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/CmdDebugger.jsm#486

Comment 2

[Andrei Oprea [:andreio]]

I'll be working on this bug. Thanks.

Comment 3

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

From irc:

17:03 < andreio> fitzgen: hei. I've added the flag and it works, one problem though is with the description. I've added it in glicommands.properties and i'm using lookup("InvertDesc") to retrieve it but for some reason it wont work

Note that the lookup function isn't the same as gcli.lookup. It is adding the L10N prefix for each command[0]. This means you will have to add two new entries (one for the "dbg blackbox" command and one for the "dbg unblackbox" command): dbgBlackBoxInvertDesc and dbgUnBlackBoxInvertDesc.

Sorry I forgot about that little bit of trickiness, I should have warned you.

[0] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/CmdDebugger.jsm#441

Comment 4

[Andrei Oprea [:andreio]]

Attachment: dbg.invert

dbg unblackbox tests fail

Comment 5

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

Comment on attachment 787120 [details] [diff] [review]
dbg.invert

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

Awesome! Thanks a bunch for this patch, this is really close :)

See inline notes below.

::: browser/devtools/debugger/CmdDebugger.jsm
@@ +493,2 @@
>            || args.source && source.url == args.source;
> +        return (args.invert) ? !value : value;

Nit: no need to wrap the conditional in parenthesis.

::: browser/devtools/debugger/test/browser_dbg_cmd_blackbox.js
@@ +126,5 @@
>      });
>  }
>  
> +function testBlackBoxInvert() {
> +  return cmd("dbg blackbox --invert *blackboxing_t*.js", 2,

So the way that the dbg [un]blackbox commands work is that you can either pass in a whole URL directly (such as "dbg blackbox http://example.com/foo.js") or you can use globs to match all of the sources whose URL matches the glob (such as "dbg blackbox *.min.js").

Right now, you are passing in a glob pattern (the "*"s make it a glob pattern) without using the "--glob" flag. Because you don't use the "--glob" flag, we are trying to find a source with the exact URL "*blackboxing_t*.js", which doesn't exist.

Also, your conditions are backwards in the test: BLACKBOXME_URL and BLACKBOXONE_URL should be checked (because they don't match the glob and we are using invert), while BLACKBOXTWO_URL and BLACKBOXTHREE_URL should not change blackbox state (because they match the glob and we are using invert). BLACKBOXTWO_URL and BLACKBOXTHREE_URL are already checked and should remain that way.

To fix the tests, add the "--glob" flag before your glob pattern.

@@ +147,5 @@
> +    .then(function () {
> +      ok(getBlackBoxCheckbox(BLACKBOXME_URL).checked,
> +         "blackboxme should be un-black boxed because it does not match the glob");
> +      ok(getBlackBoxCheckbox(BLACKBOXONE_URL).checked,
> +         "blackboxone should be un-black boxed because it does not match the glob");

These tests should be

  ok(!getBlackBoxCheckbox...

not

  ok(getBlackBoxCheckbox...

because we BLACKBOXME_URL and BLACKBOXONE_URL don't match the glob, but we are using the "--invert" flag so they should be the ones getting un-black boxed.

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +428,5 @@
>  dbgBlackBoxGlobDesc=Black box all sources that match this glob (for example: "*.min.js")
>  
> +# LOCALIZATION NOTE (dbgBlackBoxGlobDesc) A very short string used to describe the
> +# 'invert' parameter to the 'dbg blackbox' command.
> +dbgBlackBoxInvertDesc=Black box all sources except for the ones that matches this glob

Nit: the LOCALIZATION NOTE (...) doesn't match the property, please update it.

@@ +470,5 @@
>  dbgUnBlackBoxErrorDesc=Error stopping black boxing:
>  
> +# LOCALIZATION NOTE (dbgBlackBoxGlobDesc) A very short string used to describe the
> +# 'invert' parameter to the 'dbg blackbox' command.
> +dbgUnBlackBoxInvertDesc=Stop black boxing the sources that do not match this glob

Ditto.

Comment 6

[Andrei Oprea [:andreio]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #5)
> Comment on attachment 787120 [details] [diff] [review]
> dbg.invert
> 
> Review of attachment 787120 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome! Thanks a bunch for this patch, this is really close :)
> 
> See inline notes below.
> 
> ::: browser/devtools/debugger/CmdDebugger.jsm
> @@ +493,2 @@
> >            || args.source && source.url == args.source;
> > +        return (args.invert) ? !value : value;
> 
> Nit: no need to wrap the conditional in parenthesis.
> 
> ::: browser/devtools/debugger/test/browser_dbg_cmd_blackbox.js
> @@ +126,5 @@
> >      });
> >  }
> >  
> > +function testBlackBoxInvert() {
> > +  return cmd("dbg blackbox --invert *blackboxing_t*.js", 2,
> 
> So the way that the dbg [un]blackbox commands work is that you can either
> pass in a whole URL directly (such as "dbg blackbox
> http://example.com/foo.js") or you can use globs to match all of the sources
> whose URL matches the glob (such as "dbg blackbox *.min.js").
> 
> Right now, you are passing in a glob pattern (the "*"s make it a glob
> pattern) without using the "--glob" flag. Because you don't use the "--glob"
> flag, we are trying to find a source with the exact URL
> "*blackboxing_t*.js", which doesn't exist.
> 
> Also, your conditions are backwards in the test: BLACKBOXME_URL and
> BLACKBOXONE_URL should be checked (because they don't match the glob and we
> are using invert), while BLACKBOXTWO_URL and BLACKBOXTHREE_URL should not
> change blackbox state (because they match the glob and we are using invert).
> BLACKBOXTWO_URL and BLACKBOXTHREE_URL are already checked and should remain
> that way.
> 

This is because the *.checked & !*.checked is confusing for me. 
If *.checked == true does it mean it's black boxed or not.
Looking at testBlackBoxSource() it runs the black box cmd and checkes for !checkbox.checked so is it safe to assume !*.checked == blackboxed ? 

> To fix the tests, add the "--glob" flag before your glob pattern.
> 
> @@ +147,5 @@
> > +    .then(function () {
> > +      ok(getBlackBoxCheckbox(BLACKBOXME_URL).checked,
> > +         "blackboxme should be un-black boxed because it does not match the glob");
> > +      ok(getBlackBoxCheckbox(BLACKBOXONE_URL).checked,
> > +         "blackboxone should be un-black boxed because it does not match the glob");
> 
> These tests should be
> 
>   ok(!getBlackBoxCheckbox...
> 
> not
> 
>   ok(getBlackBoxCheckbox...
> 
> because we BLACKBOXME_URL and BLACKBOXONE_URL don't match the glob, but we
> are using the "--invert" flag so they should be the ones getting un-black
> boxed.
> 
> ::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
> @@ +428,5 @@
> >  dbgBlackBoxGlobDesc=Black box all sources that match this glob (for example: "*.min.js")
> >  
> > +# LOCALIZATION NOTE (dbgBlackBoxGlobDesc) A very short string used to describe the
> > +# 'invert' parameter to the 'dbg blackbox' command.
> > +dbgBlackBoxInvertDesc=Black box all sources except for the ones that matches this glob
> 
> Nit: the LOCALIZATION NOTE (...) doesn't match the property, please update
> it.
> 
> @@ +470,5 @@
> >  dbgUnBlackBoxErrorDesc=Error stopping black boxing:
> >  
> > +# LOCALIZATION NOTE (dbgBlackBoxGlobDesc) A very short string used to describe the
> > +# 'invert' parameter to the 'dbg blackbox' command.
> > +dbgUnBlackBoxInvertDesc=Stop black boxing the sources that do not match this glob
> 
> Ditto.

Comment 7

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

(In reply to andrei.br92 from comment #6)
> This is because the *.checked & !*.checked is confusing for me. 
> If *.checked == true does it mean it's black boxed or not.
> Looking at testBlackBoxSource() it runs the black box cmd and checkes for
> !checkbox.checked so is it safe to assume !*.checked == blackboxed ? 

Sorry about that confusion!

When the checkbox is checked, that means that it is not black boxed.

When the checkbox is not checked, that means it is black boxed.

Comment 8

[Andrei Oprea [:andreio]]

Attachment: dbg.invert

Comment 9

[Andrei Oprea [:andreio]]

regarding the tests testUnBlackBoxInvert failed 1. BLACKBOXONE_URL does not change state. It remains false before and after the command although it does change state when using blackbox cmd.

Regarding the conditions:
1. testBlackBoxInvert()

!getBlackBoxCheckbox(BLACKBOXME_URL)
!getBlackBoxCheckbox(BLACKBOXONE_URL)

- the cmd blackboxes everything except for what the matches the query so ME & ONE should be blackboxed. THREE & TWO should be not black boxed so checked. Also shouldn't this test be the exact opposite (in terms of conditions) to testBlackBoxGlob because adding the --invert flag should have the opposite effect. And that is what I did.

2. testUnBlackBoxInvert()

- the cmd will black box things that do not match the query. In this case ME & ONE should be checked (opposite of testUnBlackBoxGlob())

Comment 10

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

Hello again! Sorry, this bug is getting a bit more involved than I had anticipated and most of our good-first-bugs. You're doing awesome, though!

* It looks like you've not attached the full diff from fx-team, so I can't apply the patch locally and try and help debug from my end. Please upload the whole patch instead of the incremental diff from your last patch.

* If we "dbg blackbox *.min.js", it will NOT un black box anything that doesn't match, they will simply stay the same. Same should apply when we are using "--invert" and  to "dbg unblackbox".

* The checkboxes (really each source's black box state) are maintaining state through the whole test, so if it gets checked in one function, it will still be checked in the next function unless something specifically un-checks it.

* Acknowledging the last point, this means that for each function we should have the following state:

  - initially, all are checked
  - after testBlackBoxSource, only BLACKBOXME_URL is not checked
  - after testUnBlackBoxSource, all are checked again
  - after testBlackBoxGlob, BLACKBOXTWO_URL and BLACKBOXTHREE_URL are not checked
  - after testUnBlackBoxGlob, all are checked again

and with your new tests:

  - after testBlackBoxInvert, BLACKBOXONE_URL and BLACKBOXME_URL shouldn't be checked
  - after testUnBlackBoxInvert, all should be checked again

Which appears to be what you have in your tests (and I think I said it backwards before, sorry!)

-----

If you can upload the whole diff, I can play around with it and see whats up. You might also want to flip the prefs to enable the browser debugger and see if you can't figure out what's up. See the instructions here: https://developer.mozilla.org/en-US/docs/Debugging_JavaScript#JavaScript_Debugger_.28Venkman.29

Comment 11

[Andrei Oprea [:andreio]]

Attachment: dbg.invert.update

Adding full diff patch

Comment 12

[Andrei Oprea [:andreio]]

Attachment: bug901733.dbg.invert.patch

Comment 13

[Andrei Oprea [:andreio]]

Attachment: dbg.invert

Comment 14

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

Attachment: bug-901733-add-invert-flag.patch

I fixed the test so that it passes now, it was a pretty small but subtle change. I figured it out by enabling logging of the Remote Debugging Protocol by flipping the boolean here[0]. This is a nice little trick to remember in the future :)

Anyways, when I looked at the packets sent over the protocol, what I saw was that we were actually (un-)black boxing 3 sources at a time with the glob pattern and the invert flag in your new tests. I forgot that the html page has an inline script, and so it will show up in the debugger too. D'oh!

I also updated the LOCALIZATION NOTE(...) comments to reflect the correct property, and re-worded them just a little bit :)

So the process now is that you would ask me for review by editing the details of the uploaded patch and changing the "review" flag to "review ? [e-mail]". I have already been working with you on this patch closely, and I'm familiar with the changes, so I am "r+"ing it now. That means I'm giving it a positive review so that it can land in fx-team, incubate for a day or so, get merged into mozilla-central, and then show up in the next Nightly build.

Before pushing any non-trivial change to fx-team, we like to run a TryServer[1] push first. This runs the tests on all of our various platforms, just to make sure that there isn't some platform specific bug you couldn't catch locally in the change. It will take a few hours for all the tests to come back. Once they do come back, and assuming they all pass, I will push it to fx-team and your first patch will have landed! Woo!

Here is the try push: https://tbpl.mozilla.org/?tree=Try&rev=1d09023f5aee

---------------

[0] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/head.js#15

[1] https://wiki.mozilla.org/ReleaseEngineering/TryServer

Comment 15

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

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

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/b1d22ac6214d