After reloading, the blackbox styles aren't applied to sources in the debugger UI

RESOLVED FIXED in Firefox 33

Status

P3
normal
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: vporof, Assigned: piyushwaradpande, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 33
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
STR:

1. Go to http://todomvc.com/architecture-examples/backbone/
2. Add a breakpoint on line 31 in app-view.js
3. Blackbox that source
4. Reload

Now, the source is indeed blackboxed and the brekapoint didn't trigger, however the breakpoints are still displayed in the UI and the source item isn't grayed out anymore.
(Reporter)

Comment 1

5 years ago
Nick, is this a recent regression? I think this might have worked properly at some point, but I may be wrong.
Flags: needinfo?(nfitzgerald)
Yeah this used to work; browser_dbg_blackboxing-01.js tests for exactly this case, sad to see that it didn't catch the error.
Flags: needinfo?(nfitzgerald)
https://wiki.mozilla.org/DevTools/Hacking

Ah this is actually a pretty good first bug. We just need to add the "black-boxed" class to the source element if the source is black boxed in addSource[0]. Make sure to get a source client[1] instead of relying directly on the source form, as that is the real source of truth as sources are black boxed on and off. We should update the test[2] to check for the existence of that css class as well.

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#134
[1] |this.activeThread.source(aSource)| should give you a |SourceClient|
[2] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_blackboxing-01.js#42
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js]
Priority: -- → P3

Comment 4

5 years ago
I would like to work on this one. :)
Go for it :)
Assignee: nobody → hequmania
(Reporter)

Comment 6

4 years ago
Need any help with this Henri?

Comment 7

4 years ago
Hi, Henri - Can you confirm you're still working on this?
Flags: needinfo?(hequmania)

Comment 8

4 years ago
Hi, Sorry I've been really busy for the couple last months. :/

I haven't started working on this yet but I would still like to.
Flags: needinfo?(hequmania)

Comment 9

4 years ago
I'm going to deassign this, so that it shows up as available to other contributors; you're welcome to work on it, and once you've got a first try at a patch ready to go, I'd be happy to reassign it to you.
Assignee: hequmania → nobody
(Assignee)

Comment 10

4 years ago
I tried applying the style as follows:

if (aSource.isBlackBoxed) {
    contents.classList.add("black-boxed");
}

This applies the correct style to the filename in the debugger pane, but the checkbox and the line number don't hide[0] as they should 


[0] http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/debugger.inc.css#110
(In reply to pwaradpande from comment #10)
> I tried applying the style as follows:
> 
> if (aSource.isBlackBoxed) {
>     contents.classList.add("black-boxed");
> }
> 
> This applies the correct style to the filename in the debugger pane, but the
> checkbox and the line number don't hide[0] as they should 
> 
> 
> [0]
> http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/
> debugger.inc.css#110

Sorry for the delayed response, you're on the right path :)

You are applying the class to the <label> element, when it needs to go on the label's containing <vbox> which has class "side-menu-widget-item-contents". If you use the browser toolbox[0] to manually add the class to the label's containing vbox, everything works as expected.

Unfortunately, that vbox isn't created until after you call `push`, so you have to do something like this:

  const item = this.push(...);

  if (gThreadClient.source(aSource).isBlackBoxed) {
    // add the class to item.target
  }

We use gThreadClient.source(...) because it produces a source client whose `isBlackBoxed` getter will stay in sync with whatever black boxing changes may happen. I'm not sure that is an actual issue in this case, but it is good to be vigilant and always use it.

You will also need a test. I'd suggest copying one of the existing browser/devtools/debugger/test/browser_dbg_blackboxing-*.js tests and modifying it. It should:

1. Load a document with two scripts:

  // first script
  function a() {
    b();
  }

  // second script
  function b() {
    debugger;
  }

2. Set a breakpoint in the first script on the line with the call to `b`.

3. Black box the first script containing a.

4. Set up a promise that resolves on the next pause and then refresh.

5. When that promise resolves, verify that you are paused in the second script and b's debugger statement (as opposed to the black boxed breakpoint).

6. Verify that the first script's source item target has the black-boxed css rule and that the breakpoint item is hidden.

For more information about writing devtools mochitests, see [1].

[0] https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
[1] https://wiki.mozilla.org/DevTools/mochitests_coding_standards
Alternatively, instead of writing a new test, you can just modify this one[0] to also check that the css class is applied properly and the breakpoints are hidden.

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_blackboxing-01.js#42
Mentor: nfitzgerald
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js] → [good first bug][lang=js]
Are you still working on this?
Flags: needinfo?(pwaradpande)
(Assignee)

Comment 14

4 years ago
Well, I have the fix ready. I verified it and it works as expected.

I am stuck at the point of writing test case for the fix. I couldn't find any documentation about what exactly is happening with gDebugee, gTab etc. and what components I need to import. I also had a look at other test files, but I was doing a trial and error. 

Any pointer you could provide for the test case?
(In reply to pwaradpande from comment #14)
> Well, I have the fix ready. I verified it and it works as expected.
> 
> I am stuck at the point of writing test case for the fix. I couldn't find
> any documentation about what exactly is happening with gDebugee, gTab etc.
> and what components I need to import. I also had a look at other test files,
> but I was doing a trial and error. 
> 
> Any pointer you could provide for the test case?

See comment 12. All you should have to do is verify that not only is the button checked, but the css class is applied to the element: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_blackboxing-01.js#42

If you need further clarification, feel free to ping me on irc.
Assignee: nobody → pwaradpande
Status: NEW → ASSIGNED
Flags: needinfo?(pwaradpande)
You shouldn't need to make a new test, just modify that one I linked.
(Assignee)

Comment 17

4 years ago
Created attachment 8451999 [details] [diff] [review]
Adding 'black-boxed' class to the blackboxed source after reload
Attachment #8451999 - Flags: review?(nfitzgerald)
Comment on attachment 8451999 [details] [diff] [review]
Adding 'black-boxed' class to the blackboxed source after reload

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

Looks good!

Here is a try push**: https://tbpl.mozilla.org/?tree=Try&rev=bed24e6610c5

If it comes back green, we will add the "checkin-needed" keyword and one of the sheriffs will commit it (the new version of the patch with requested change below) for us!

** Runs all the tests on our various supported platforms.

::: browser/devtools/debugger/test/browser_dbg_blackboxing-01.js
@@ +43,2 @@
>      ok(bbButton.checked, "Should still be black boxed.");
> +    ok(selectedSource.classList.contains("black-boxed"), "'black-boxed' class should still be applied");

Nitpicking: please put the second parameter on a newline so that this line isn't so long.
Attachment #8451999 - Flags: review?(nfitzgerald) → review+
Try push looks good to me, as soon as you upload the revised patch, we can add "checkin-needed"!
(Assignee)

Comment 20

4 years ago
Created attachment 8452450 [details] [diff] [review]
patch2.patch

Revised patch as per Comment 18
Attachment #8451999 - Attachment is obsolete: true
Attachment #8452450 - Flags: review?(nfitzgerald)
Comment on attachment 8452450 [details] [diff] [review]
patch2.patch

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

Just FYI, once you get a review+ with a request for small changes, you don't need to ask for review again.
Attachment #8452450 - Flags: review?(nfitzgerald) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/8252ef806389
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8252ef806389
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 33

Updated

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