Closed Bug 877686 Opened 11 years ago Closed 11 years ago

Add UI to toggle the blackboxing of specific sources

Categories

(DevTools :: Debugger, defect, P2)

Product:
DevTools
Component:
Debugger
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 25

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

v2.2
47.20 KB, patch
Details | Diff | Splinter Review 
I'm imagining this as a context menu item available when you right click on a source in the left pane, "blackbox this source" or something.
No longer blocks: 877682
Depends on: 877682
Assignee: nobody → nfitzgerald
Attached patch wip 1 (obsolete) — — Splinter Review 
Adds checkboxes to SideMenuWidget
Attachment #763767 - Flags: feedback?(vporof)
Comment on attachment 763767 [details] [diff] [review]
wip 1

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

::: browser/devtools/debugger/debugger-controller.js
@@ +1102,5 @@
>  
>    /**
> +   * TODO FITZGEN
> +   */
> +  blackBox: function(aSource, aBlackBoxFlag) {

Nit nit nit: can you please move this function after _handleTabNavigation and friends, like near getText? It keeps the flow bro, but that's just me being crazy.

::: browser/devtools/debugger/debugger-panes.js
@@ +106,5 @@
>    addSource: function(aSource, aOptions = {}) {
>      let url = aSource.url;
>      let label = SourceUtils.getSourceLabel(url.split(" -> ").pop());
>      let group = SourceUtils.getSourceGroup(url.split(" -> ").pop());
> +    let {staged} = aOptions;

Nit: a bit unnecessary IMHO.

@@ +112,5 @@
>      // Append a source item to this container.
> +    let sourceItem = this.push([url, label, group], {
> +      staged: staged, /* stage the item to be appended later? */
> +      checked: !aSource.isBlackBoxed,
> +      onCheck: (aChecked) => DebuggerController.Sources.blackBox(aSource, !aChecked),

This is where the fun starts.

::: browser/devtools/shared/widgets/SideMenuWidget.jsm
@@ +593,5 @@
> +
> +  if (aCheckboxFlag) {
> +    let checkbox = this._checkbox = this.document.createElement("checkbox");
> +    if (aOptions.checked) {
> +      checkbox.setAttribute("checked", true);

You can just checkbox.setAttribute("checked", aOptions.checked) to remove the extra conditional indenting.

@@ +598,5 @@
> +    }
> +    if (aOptions.onCheck) {
> +      let callback = aOptions.onCheck;
> +      checkbox.addEventListener("command", function () {
> +        callback(checkbox.checked);

Instead of callbacks and turtles all the way down, you should do:
ViewHelpers.dispatchEvent(checkbox, "check", checkbox.checked);

and in debugger-panes, when you add a "select" event listener:
this.node.addEventListener("check", myAwesomeHandler, false);

function myAwesomeHandler(e) {
  let sourceItem = this.getItemForElement(e.target);
  let sourceClient = sourceItem.attachment.source;
  let checked = e.detail;
}

(it should work, if it doesn't, I probably know why. but it should work)

@@ +601,5 @@
> +      checkbox.addEventListener("command", function () {
> +        callback(checkbox.checked);
> +      }, false);
> +    }
> +    this._container.insertBefore(checkbox, this._container.firstChild);

You should really just add this stuff in the conditional above. insertBefore after rendering the item causes a reflow if I'm not mistaken. Either add a helper func or just write it twice, it's only a few lines.

::: browser/devtools/shared/widgets/ViewHelpers.jsm
@@ +615,5 @@
>     *          - relaxed: true if this container should allow dupes & degenerates
>     *          - attachment: some attached primitive/object for the item
>     *          - attributes: a batch of attributes set to the displayed element
>     *          - finalize: function invokde when the item is untangled (removed)
> +   *          - checked: true if the item's checkbox should be checked

This is so so so dirty. Not all widgets implement this "checked" thing, and these are generic methods that need to work with all widgets.

You should simply pass the |checked| option inside the attachment, since that is generic and was already passed to the SideMenuWidget's insertItemAt.

@@ +616,5 @@
>     *          - attachment: some attached primitive/object for the item
>     *          - attributes: a batch of attributes set to the displayed element
>     *          - finalize: function invokde when the item is untangled (removed)
> +   *          - checked: true if the item's checkbox should be checked
> +   *          - onCheck: callback every time this item's checkbox is toggled

Remove this (because of dispatchEvent).

@@ +1390,5 @@
>     *          - relaxed: true if this container should allow dupes & degenerates
>     *          - attributes: a batch of attributes set to the displayed element
>     *          - finalize: function when the item is untangled (removed)
> +   *          - checked: true if the item's checkbox should be checked
> +   *          - onCheck: callback for everytime the checkbox is toggled

KILL THEM WITH FIRE.
Attachment #763767 - Flags: feedback?(vporof) → feedback+
(In reply to Victor Porof [:vp] from comment #2)
> 
> function myAwesomeHandler(e) {
>   let sourceItem = this.getItemForElement(e.target);
>   ...
> }

Ok. Haven't tested this, but getItemForElement(e.target) *may* work because the checkbox isn't a child of side-menu-widget-item-contents. Not sure if this affects you or not, but I'll look into this.
Comment on attachment 763767 [details] [diff] [review] [diff] [review]
wip 1

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

::: browser/devtools/debugger/debugger-panes.js
@@ +112,5 @@
>      // Append a source item to this container.
> +    let sourceItem = this.push([url, label, group], {

This used to be [label, url, group].
One more thing: since you added checkboxes to the widget, which have moz-user-select: normal by default (are keyboard accessible), this confuses keyboard navigation (pressing UP/DOWN etc.).

To fix this, you'll need to make sure that the next time an item is focused, it'll be the Expected Thing™. So, _focusChange in ViewHelpers.jsm needs a tiny change to account for that. Here's how it looks like after I made it working again:

> _focusChange: function(aDirection) {
>   let commandDispatcher = this._commandDispatcher;
>   let prevFocusedElement = commandDispatcher.focusedElement;
>   let currFocusedElement;
>
>   do {
>     commandDispatcher.suppressFocusScroll = true;
>     commandDispatcher[aDirection]();
>     currFocusedElement = commandDispatcher.focusedElement;
>
>     // Make sure the newly focused item is a part of this container.
>     // If the focus goes out of bounds, revert the previously focused item.
>     if (!this.getItemForElement(currFocusedElement)) {
>       prevFocusedElement.focus();
>       return false;
>     }
>   } while (!WIDGET_FOCUSABLE_NODES.has(currFocusedElement.tagName));
>
>   // Focus remained within bounds.
>   return true;
> }
(In reply to Victor Porof [:vp] from comment #5)

..where those nodes are
> const WIDGET_FOCUSABLE_NODES = new Set(["vbox", "hbox"]);
Priority: -- → P2
Attached patch v1 (obsolete) — — Splinter Review 
https://tbpl.mozilla.org/?tree=Try&rev=8c2ec8c026d1

* getItemForElement(e.target) doesn't work, have a work around but it isn't super pretty. Land now and fix later, or fix now?

* Getting weird failure messages when I run all mochitests, but not when I run my new ones individually: 

 5:26.95 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_blackboxing_blackboxme.js | Exception thrown at chrome://mochikit/content/browser-test.js:436 - TypeError: this.currentTest.scope.test is not a function
 5:26.95 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_blackboxing_one.js | Exception thrown at chrome://mochikit/content/browser-test.js:436 - TypeError: this.currentTest.scope.test is not a function
 5:26.95 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_blackboxing_three.js | Exception thrown at chrome://mochikit/content/browser-test.js:436 - TypeError: this.currentTest.scope.test is not a function
 5:26.95 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_blackboxing_two.js | Exception thrown at chrome://mochikit/content/browser-test.js:436 - TypeError: this.currentTest.scope.test is not a function

Suspect I'm not cleaning up some global state properly, but haven't pinned it down yet.
Attachment #763767 - Attachment is obsolete: true
Attachment #774370 - Flags: review?(vporof)
Comment on attachment 774370 [details] [diff] [review]
v1

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #7)

Quite a heroic and clean patch, Nick. Honestly, thank you :) r+ with the comments below addressed, and the fix for getItemForElement.

The only thing that really bothers me is that whenever I now open the debugger I have a million eyes looking down at me from the sources widget. I'm worried that I might have nightmares because of that :) But in all seriousness, somehow I doubt that users will understand that "If I poke the eye with my mouse cursor, this source will be blackboxed!". IIRC same thing happened with the source editor where people didn't really figure out that clicking the eye disables the stylesheet, so I think it's even more pertinent here since "eyes" don't really tell me anything related to blackboxing. Let's talk about this a bit.

> * getItemForElement(e.target) doesn't work, have a work around but it isn't
> super pretty. Land now and fix later, or fix now?

This one's easy to fix, let's take care of it now. Basically, you're creating the checkbox as a sibling of the deepmost child known as the "view", which isn't caught when walking the nodes chain in getItemForElement (ViewHelpers.jsm). Here's a diff that would make your life easier:

-      let item = this._itemsByElement.get(aElement);
+      let item =
+        this._itemsByElement.get(aElement.nextElementSibling) ||
+        this._itemsByElement.get(aElement.previousElementSibling) ||
+        this._itemsByElement.get(aElement);

> * Getting weird failure messages when I run all mochitests, but not when I
> run my new ones individually: 
> 

I'll need to take a closer look at this, but see if these still persist once you remove the ugly hacks you mention and perform the change above.

::: browser/devtools/debugger/debugger-controller.js
@@ +603,5 @@
>        let frameLocation = NetworkHelper.convertToUnicode(unescape(url));
>        let frameTitle = StackFrameUtils.getFrameTitle(frame);
>  
> +      if (isBlackBoxed) {
> +        if (previousBlackBoxed == url) {

What do you think is better? Showing the first frame from a blackboxed source or the last one? It looks like you're showing the first one. I also think this is a good idea, since it allows you to go in there and add a breakpoint if you want to.

@@ +1025,5 @@
>      window.dispatchEvent(document, "Debugger:AfterSourcesAdded");
>    },
>  
>    /**
> +   * Set the black boxed status of the given source.

If the source was already blackboxed, I guess nothing should actually happen, right? However, it looks like we're going to clear and fill frames if some sloppy consumer blackboxes an already blackboxed source.

Not something that you should really care about for now, what I just wrote seems somewhat not plausible, but just putting it out there.

@@ +1033,5 @@
> +   * @param bool aBlackBoxFlag
> +   *        True to black box the source, false to un-black box it.
> +   */
> +  blackBox: function(aSource, aBlackBoxFlag) {
> +    const sourceClient = this.activeThread.source(aSource);

Why const?

@@ +1036,5 @@
> +  blackBox: function(aSource, aBlackBoxFlag) {
> +    const sourceClient = this.activeThread.source(aSource);
> +    sourceClient[aBlackBoxFlag ? "blackBox" : "unblackBox"](function({ error }) {
> +      if (error) {
> +        return void Cu.reportError(error);

Can you also please add dumpn here? Cu.reportError doesn't show up in stdout afaik. See previous occurences of "let msg = ".

@@ +1038,5 @@
> +    sourceClient[aBlackBoxFlag ? "blackBox" : "unblackBox"](function({ error }) {
> +      if (error) {
> +        return void Cu.reportError(error);
> +      }
> +      window.dispatchEvent(document, "Debugger:BlackBoxChange", sourceClient);

I really want to remove those DOM events. They're giving me stomachaches.

::: browser/devtools/debugger/debugger-panes.js
@@ +113,5 @@
>      // Append a source item to this container.
>      this.push([label, url, group], {
>        staged: aOptions.staged, /* stage the item to be appended later? */
>        attachment: {
> +        checked: !aSource.isBlackBoxed,

How about checkboxState, to be in harmony with checkboxTooltip?

@@ +114,5 @@
>      this.push([label, url, group], {
>        staged: aOptions.staged, /* stage the item to be appended later? */
>        attachment: {
> +        checked: !aSource.isBlackBoxed,
> +        checkboxTooltip: "Toggle black boxing",

Need to localize this.
Once you do, please also cache the string since this function is likely to be called a lot.

@@ +652,5 @@
> +  _onSourceCheck: function({ detail }) {
> +    let { value, checked } = detail;
> +    let item = this.getItemByValue(value);
> +    DebuggerController.SourceScripts.blackBox(item.attachment.source,
> +                                              !checked);

Nit: you can safely keep all of this on the same line, I don't really care.

::: browser/devtools/debugger/debugger-toolbar.js
@@ +488,5 @@
> +      STACK_FRAMES_SOURCE_URL_MAX_LENGTH,
> +      STACK_FRAMES_SOURCE_URL_TRIM_SECTION);
> +
> +    if (aIsBlackBoxed) {
> +      container.className += " dbg-stackframe-black-boxed";

Use clasList please.

From what I see, you're not actually styling this differently based on the dbg-stackframe-black-foxed class. Maybe it'd be a good idea to do so? Like using a different text color or background.

::: browser/devtools/debugger/test/browser_dbg_blackboxing-01.js
@@ +31,5 @@
> +}
> +
> +function testBlackBoxSource() {
> +  once(gDebugger, "Debugger:SourceShown", function () {
> +    const checkbox = gDebugger.document.querySelector(".side-menu-widget-item-checkbox");

Const?

@@ +36,5 @@
> +    ok(checkbox, "Should get the checkbox for black boxing the source");
> +    ok(checkbox.checked, "Should not be black boxed by default");
> +
> +    once(gDebugger, "Debugger:BlackBoxChange", function (event) {
> +      const sourceClient = event.detail;

?

@@ +49,5 @@
> +}
> +
> +function testBlackBoxReload() {
> +  once(gDebugger, "Debugger:SourceShown", function () {
> +    const checkbox = gDebugger.document.querySelector(".side-menu-widget-item-checkbox");

I wish I understood what you're trying to do with all these consts. I don't mind them though.

@@ +59,5 @@
> +
> +  gDebuggee.location.reload();
> +}
> +
> +function once(target, event, callback) {

<3

::: browser/devtools/shared/widgets/SideMenuWidget.jsm
@@ +27,5 @@
>   *        The element associated with the widget.
> + * @param Object aOptions
> + *        - showArrows: Specifies if items in this container should display
> + *          horizontal arrows.
> + *        - showCheckboxes: Specifies if items in this containes should display

s/containes/container

@@ +556,5 @@
> +    checkbox.setAttribute("tooltiptext", aAttachment.checkboxTooltip);
> +    checkbox.addEventListener("command", function () {
> +      ViewHelpers.dispatchEvent(checkbox, "check", {
> +        checked: checkbox.checked,
> +        value: aTooltip,

Eeek. I guess you don't really need to send out the value if you make the changes I suggested in getItemByElement.

::: browser/themes/osx/devtools/debugger.css
@@ +18,5 @@
>  #sources-pane + .devtools-side-splitter {
>    -moz-border-start-color: transparent;
>  }
>  
> +#sources .side-menu-widget-item-checkbox {

Don't prepend #sources here for now, it's unlikely that we'll ever have another .side-menu-widget-item-checkbox anytime soon.

@@ +24,5 @@
> +  padding: 0;
> +  margin: 0 -4px 0 4px;
> +}
> +
> +#sources .side-menu-widget-item-checkbox > .checkbox-check {

Ditto.

@@ +32,5 @@
> +  background-repeat: no-repeat;
> +  background-clip: content-box;
> +  background-position: -24px 0;
> +  width: 24px;
> +  height: 24px;

All of this should be in widgets.css, since checkboxes can be used with a sidemenuwidget in any tool.

Also, once you start adding breakpoints, things start to look a bit funky IMHO with a L-shaped blue selection. Fixable in a followup, but I'd like that to get figured out.

@@ +35,5 @@
> +  width: 24px;
> +  height: 24px;
> +}
> +
> +#sources .side-menu-widget-item-checkbox[checked] > .checkbox-check {

2xDitto.

@@ +37,5 @@
> +}
> +
> +#sources .side-menu-widget-item-checkbox[checked] > .checkbox-check {
> +  background-position: 0 0;
> +}

You'll need to add this css to windows and linux as well.
Attachment #774370 - Flags: review?(vporof) → review+
(In reply to Victor Porof [:vp] from comment #8)
> 
> This one's easy to fix, let's take care of it now. Basically, you're
> creating the checkbox as a sibling of the deepmost child known as the
> "view", which isn't caught when walking the nodes chain in getItemForElement
> (ViewHelpers.jsm). Here's a diff that would make your life easier:

Sorry, these should be the other way around. First try to get a thing for the element, then the siblings. Otherwise, actual sibling views will yield incorrect items.

-      let item = this._itemsByElement.get(aElement);
+      let item =
+        this._itemsByElement.get(aElement) ||
+        this._itemsByElement.get(aElement.nextElementSibling) ||
+        this._itemsByElement.get(aElement.previousElementSibling);
Attached patch v2 (obsolete) — — Splinter Review 
Updated based on review comments.

Black boxing tests are still passing individually, but have those errors when running the whole suite of tests.

Try is apparently down again, so no push.

(In reply to Victor Porof [:vp] from comment #8)
> > * Getting weird failure messages when I run all mochitests, but not when I
> > run my new ones individually: 
> > 
> 
> I'll need to take a closer look at this, but see if these still persist once
> you remove the ugly hacks you mention and perform the change above.

Still there, would really appreciate the help, thanks :)

> @@ +1025,5 @@
> >      window.dispatchEvent(document, "Debugger:AfterSourcesAdded");
> >    },
> >  
> >    /**
> > +   * Set the black boxed status of the given source.
> 
> If the source was already blackboxed, I guess nothing should actually
> happen, right? However, it looks like we're going to clear and fill frames
> if some sloppy consumer blackboxes an already blackboxed source.
> 
> Not something that you should really care about for now, what I just wrote
> seems somewhat not plausible, but just putting it out there.

This is true, however I don't think we can rely on SourceClient.prototype.isBlackBoxed because if someone else creates another source client for the same source actor and changes the black boxed state, that isn't reflected in the first source client. Maybe if we have a weak map pool of clients and always return the same client for the same source...

> 
> @@ +1033,5 @@
> > +   * @param bool aBlackBoxFlag
> > +   *        True to black box the source, false to un-black box it.
> > +   */
> > +  blackBox: function(aSource, aBlackBoxFlag) {
> > +    const sourceClient = this.activeThread.source(aSource);
> 
> Why const?

Because the binding is never reassigned, or in other words: it is constant! :)


> @@ +1038,5 @@
> > +    sourceClient[aBlackBoxFlag ? "blackBox" : "unblackBox"](function({ error }) {
> > +      if (error) {
> > +        return void Cu.reportError(error);
> > +      }
> > +      window.dispatchEvent(document, "Debugger:BlackBoxChange", sourceClient);
> 
> I really want to remove those DOM events. They're giving me stomachaches.

Do you want me to do something else instead?

> From what I see, you're not actually styling this differently based on the
> dbg-stackframe-black-foxed class. Maybe it'd be a good idea to do so? Like
> using a different text color or background.

No, but I am using it in testing, and the black boxed frames do look different because they have less stuff on them. See the black-box-me.js frame here: http://i.imgur.com/FvmcDtl.png

> 
> ::: browser/devtools/debugger/test/browser_dbg_blackboxing-01.js
> @@ +31,5 @@
> > +}
> > +
> > +function testBlackBoxSource() {
> > +  once(gDebugger, "Debugger:SourceShown", function () {
> > +    const checkbox = gDebugger.document.querySelector(".side-menu-widget-item-checkbox");
> 
> Const?

Yup, it doesn't change (alternatively, is constant) ;)

> 
> @@ +36,5 @@
> > +    ok(checkbox, "Should get the checkbox for black boxing the source");
> > +    ok(checkbox.checked, "Should not be black boxed by default");
> > +
> > +    once(gDebugger, "Debugger:BlackBoxChange", function (event) {
> > +      const sourceClient = event.detail;
> 
> ?

Booya grandma

> 
> @@ +49,5 @@
> > +}
> > +
> > +function testBlackBoxReload() {
> > +  once(gDebugger, "Debugger:SourceShown", function () {
> > +    const checkbox = gDebugger.document.querySelector(".side-menu-widget-item-checkbox");
> 
> I wish I understood what you're trying to do with all these consts. I don't
> mind them though.

Trying to have the js engine enforce things that I already expect about the code. If I'm never re-assigning it, it should be constant, and if it isn't I'd like to know loudly.

> @@ +59,5 @@
> > +
> > +  gDebuggee.location.reload();
> > +}
> > +
> > +function once(target, event, callback) {
> 
> <3

(◡ ‿ ◡ ✿)

> @@ +556,5 @@
> > +    checkbox.setAttribute("tooltiptext", aAttachment.checkboxTooltip);
> > +    checkbox.addEventListener("command", function () {
> > +      ViewHelpers.dispatchEvent(checkbox, "check", {
> > +        checked: checkbox.checked,
> > +        value: aTooltip,
> 
> Eeek. I guess you don't really need to send out the value if you make the
> changes I suggested in getItemByElement.

Yup, I knew it was bad. Fixed in this new patch.

> @@ +32,5 @@
> > +  background-repeat: no-repeat;
> > +  background-clip: content-box;
> > +  background-position: -24px 0;
> > +  width: 24px;
> > +  height: 24px;
> 
> All of this should be in widgets.css, since checkboxes can be used with a
> sidemenuwidget in any tool.

Yeah, but this styling is specific to the use here because it is trying to make the checkbox look like an eyeball rather than a checkbox.

> 
> Also, once you start adding breakpoints, things start to look a bit funky
> IMHO with a L-shaped blue selection. Fixable in a followup, but I'd like
> that to get figured out.

I didn't like it at first, but I kinda do like it. It makes sense that the eye is for the source and the breakpoints because when you black box a source, the existing breakpoints won't be stopped at either.
Attachment #774370 - Attachment is obsolete: true
Attachment #776059 - Flags: review?(vporof)
(In reply to Nick Fitzgerald [:fitzgen] from comment #10)
> 
> Black boxing tests are still passing individually, but have those errors
> when running the whole suite of tests.
> 

I'll take a look.

> 
> > @@ +1025,5 @@
> > >      window.dispatchEvent(document, "Debugger:AfterSourcesAdded");
> > >    },
> > >  
> > >    /**
> > > +   * Set the black boxed status of the given source.
> > 
> > If the source was already blackboxed, I guess nothing should actually
> > happen, right? However, it looks like we're going to clear and fill frames
> > if some sloppy consumer blackboxes an already blackboxed source.
> > 
> > Not something that you should really care about for now, what I just wrote
> > seems somewhat not plausible, but just putting it out there.
> 
> This is true, however I don't think we can rely on
> SourceClient.prototype.isBlackBoxed because if someone else creates another
> source client for the same source actor and changes the black boxed state,
> that isn't reflected in the first source client. Maybe if we have a weak map
> pool of clients and always return the same client for the same source...
> 

Don't worry about it for now, I'm over-exaggerating.

> > 
> > @@ +1033,5 @@
> > > +   * @param bool aBlackBoxFlag
> > > +   *        True to black box the source, false to un-black box it.
> > > +   */
> > > +  blackBox: function(aSource, aBlackBoxFlag) {
> > > +    const sourceClient = this.activeThread.source(aSource);
> > 
> > Why const?
> 
> Because the binding is never reassigned, or in other words: it is constant!
> :)
> 

Hipster.

> 
> > @@ +1038,5 @@
> > > +    sourceClient[aBlackBoxFlag ? "blackBox" : "unblackBox"](function({ error }) {
> > > +      if (error) {
> > > +        return void Cu.reportError(error);
> > > +      }
> > > +      window.dispatchEvent(document, "Debugger:BlackBoxChange", sourceClient);
> > 
> > I really want to remove those DOM events. They're giving me stomachaches.
> 
> Do you want me to do something else instead?
> 

Nope, I was just voicing my frustration with the old APIs. I'll take care of those myself soon.

> > From what I see, you're not actually styling this differently based on the
> > dbg-stackframe-black-foxed class. Maybe it'd be a good idea to do so? Like
> > using a different text color or background.
> 
> No, but I am using it in testing, and the black boxed frames do look
> different because they have less stuff on them. See the black-box-me.js
> frame here: http://i.imgur.com/FvmcDtl.png
> 

I still think it could be nice to further distinguish them, but it's your call.

> 
> > @@ +32,5 @@
> > > +  background-repeat: no-repeat;
> > > +  background-clip: content-box;
> > > +  background-position: -24px 0;
> > > +  width: 24px;
> > > +  height: 24px;
> > 
> > All of this should be in widgets.css, since checkboxes can be used with a
> > sidemenuwidget in any tool.
> 
> Yeah, but this styling is specific to the use here because it is trying to
> make the checkbox look like an eyeball rather than a checkbox.
> 

You're right.

> > 
> > Also, once you start adding breakpoints, things start to look a bit funky
> > IMHO with a L-shaped blue selection. Fixable in a followup, but I'd like
> > that to get figured out.
> 
> I didn't like it at first, but I kinda do like it. It makes sense that the
> eye is for the source and the breakpoints because when you black box a
> source, the existing breakpoints won't be stopped at either.

No argument there, the checkbox placement is okay, however IMHO the blue L shaped focus styling looks more like an artifact or accident, than something that was planned and Looks Good™.
(In reply to Victor Porof [:vp] from comment #8)
> 
> The only thing that really bothers me is that whenever I now open the
> debugger I have a million eyes looking down at me from the sources widget.
> I'm worried that I might have nightmares because of that :) But in all
> seriousness, somehow I doubt that users will understand that "If I poke the
> eye with my mouse cursor, this source will be blackboxed!". IIRC same thing
> happened with the source editor where people didn't really figure out that
> clicking the eye disables the stylesheet, so I think it's even more
> pertinent here since "eyes" don't really tell me anything related to
> blackboxing. Let's talk about this a bit.
> 

Any comment about this?
Comment on attachment 776059 [details] [diff] [review]
v2

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

See my comments above.
Attachment #776059 - Flags: review?(vporof) → review+
(In reply to Victor Porof [:vp] from comment #11)
> > > Also, once you start adding breakpoints, things start to look a bit funky
> > > IMHO with a L-shaped blue selection. Fixable in a followup, but I'd like
> > > that to get figured out.
> > 
> > I didn't like it at first, but I kinda do like it. It makes sense that the
> > eye is for the source and the breakpoints because when you black box a
> > source, the existing breakpoints won't be stopped at either.
> 
> No argument there, the checkbox placement is okay, however IMHO the blue L
> shaped focus styling looks more like an artifact or accident, than something
> that was planned and Looks Good™.

I think maybe making the  first breakpoint have a border radius on the top left corner could help. I'll file a follow up once this lands.

(In reply to Victor Porof [:vp] from comment #12)
> (In reply to Victor Porof [:vp] from comment #8)
> > 
> > The only thing that really bothers me is that whenever I now open the
> > debugger I have a million eyes looking down at me from the sources widget.
> > I'm worried that I might have nightmares because of that :) But in all
> > seriousness, somehow I doubt that users will understand that "If I poke the
> > eye with my mouse cursor, this source will be blackboxed!". IIRC same thing
> > happened with the source editor where people didn't really figure out that
> > clicking the eye disables the stylesheet, so I think it's even more
> > pertinent here since "eyes" don't really tell me anything related to
> > blackboxing. Let's talk about this a bit.
> > 
> 
> Any comment about this?

I think that the eyeball styling is better than the plain checkbox. On the mailing list, there seemed to be agreement that it made sense to have some kind of checkbox thingy next to each source to toggle black boxing. Would you prefer to move it to a context menu? I'm not a huge fan of tucking things away in context menus in general, though...
(In reply to Nick Fitzgerald [:fitzgen] from comment #14)
> 
> I think that the eyeball styling is better than the plain checkbox. On the
> mailing list, there seemed to be agreement that it made sense to have some
> kind of checkbox thingy next to each source to toggle black boxing. Would
> you prefer to move it to a context menu? I'm not a huge fan of tucking
> things away in context menus in general, though...

Maybe having an all-eyes-visibility-toggler would be nice?
When would you want to black box every source? That would make the debugger kind of useless.

I'm also working on a gcli command to toggle the black boxing of many sources at a time here: https://bugzilla.mozilla.org/show_bug.cgi?id=892605
(In reply to Nick Fitzgerald [:fitzgen] from comment #16)
> When would you want to black box every source? That would make the debugger
> kind of useless.
> 

You're not getting me. I'm suggesting a toggle (a menu item, whatever) that makes all the checkboxes visible. They should be hidden by default.
Ah, ok. That's a good idea, but I think it is something that can really be a follow up, no? We should probably flesh out the idea some more (because I can't think of a nice way to enter "show me the black boxing eyes" mode) and I don't want it to stop this from landing.
(In reply to Nick Fitzgerald [:fitzgen] from comment #18)
> Ah, ok. That's a good idea, but I think it is something that can really be a
> follow up, no? We should probably flesh out the idea some more (because I
> can't think of a nice way to enter "show me the black boxing eyes" mode) and
> I don't want it to stop this from landing.

Yes, definitely, don't let it block this. File some followups and land ahoy!
Backed out: https://hg.mozilla.org/integration/fx-team/rev/a58e072e2ae7

(D'oh forgot about the issue with running the whole suite... sigh)
Whiteboard: [fixed-in-fx-team]
(In reply to Nick Fitzgerald [:fitzgen] from comment #21)
> Backed out: https://hg.mozilla.org/integration/fx-team/rev/a58e072e2ae7
> 
> (D'oh forgot about the issue with running the whole suite... sigh)

Shit, me too. Let me take a look at this.
While you guys are at it and relanding it back, please add a border: 0 to the eyes. They look ugly on Windows where checkbox with -moz-appearance: none gets a groovy border by default
(In reply to Victor Porof [:vp] from comment #22)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #21)
> > Backed out: https://hg.mozilla.org/integration/fx-team/rev/a58e072e2ae7
> > 
> > (D'oh forgot about the issue with running the whole suite... sigh)
> 
> Shit, me too. Let me take a look at this.

Fixed it! My new auxillary test files were being run as tests because they were named browser_dbg_... and the cryptic error message was because they weren't defining "test" functions for the test runner to call!
(In reply to Victor Porof [:vp] from comment #22)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #21)
> > Backed out: https://hg.mozilla.org/integration/fx-team/rev/a58e072e2ae7
> > 
> > (D'oh forgot about the issue with running the whole suite... sigh)
> 
> Shit, me too. Let me take a look at this.

JavaScript files that start with browser_ are automatically considered to be test files. Simply rename them :)
Mid-air!
Whiteboard: [fixed-in-fx-team]
Attached patch v2.1 (obsolete) — — Splinter Review 
Rebased, included the changes from bug 895774 to stop the crashing.

https://tbpl.mozilla.org/?tree=Try&rev=79bce0f13e3d
Attachment #776059 - Attachment is obsolete: true
Attached patch v2.2 — — Splinter Review 
https://tbpl.mozilla.org/?tree=Try&rev=cf1b337067ab

Separated the js engine patch from this one since it changed a bunch.
Attachment #779478 - Attachment is obsolete: true
Comment on attachment 779494 [details] [diff] [review]
v2.2

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

::: browser/devtools/debugger/debugger-panes.js
@@ +36,5 @@
>      dumpn("Initializing the SourcesView");
>  
> +    this.widget = new SideMenuWidget(document.getElementById("sources"), {
> +      showCheckboxes: true
> +    });

You forgot to also add showArrows here.

Also, the profiler also uses the sidemenuwidget and should be changed to use this new API. Currently, it looks a bit broken because of this :)

Followup please.
https://hg.mozilla.org/mozilla-central/rev/73f2e30e0154
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25
From IRC:

nick: "which api do you want the profiler to use?"
When the profiler initializes the sidemenuwidget it needs to pass `{ showArrows: true }`, not just `true`.

nick: "also, it didn't use showArrows before"
That's because the param defaulted to true.
Depends on: 905136
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: