Closed Bug 917072 Opened 11 years ago Closed 11 years ago

move black box eyeball into sources toolbar

Categories

(DevTools :: Debugger, defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

The black box eyeball shouldn't be next to every single source anymore, it should be a toggle-able button in the sources toolbar.
How would you target individual sources? (don't say via the commandline).
The selected source would be the one black boxed (or un-black boxed).
So I need to first wait for jquery to load and be displayed, to be able to blackbox it? A bit sad. But I guess this would be fine after having a blackbox source pragma, or blackboxing minified sources by default.
Well, we can't currently black box a source until it is loaded in the sources list, this doesn't change that. You don't have to wait for the whole source text to be loaded.

Unfortunately, it does add one extra click: select source then click eyeball, instead of hover over source and click eyeball. I don't see a way we can keep adding features that require UI without either making the UI really crowded / maximalistic, or by hiding things until you are in their context (which often requires one extra click, like it does here).
Assignee: nobody → nfitzgerald
Priority: -- → P2
Haven't updated tests to work with the new button, or the non OSX CSS.

The black box eyeball is a little big for the button, and there is like 1 pixel on each side being cut off. I don't think there is something like background-size for list-style-image, so I will probably have to find the smaller eyeball that shorlander sent me.
Would really like to land this before uplift! :)

Deleted the old browser_dbg_blackboxing-06.js because it no longer makes sense. Moved 07 to 06.

Using smaller eyeyball icon image (although it is displayed as the same size it was before, because we were scaling it down with CSS). Can't continue to scale in CSS because toolbarbuttons use list-style-image for their icons, not background-image.

Simplified a lot of test stuff by moving shared code into head.js.
Attachment #819269 - Attachment is obsolete: true
Attachment #819270 - Attachment is obsolete: true
Attachment #819271 - Attachment is obsolete: true
Attachment #820791 - Flags: review?(vporof)
Comment on attachment 820791 [details] [diff] [review]
bug-917072-bb-sources-toolbar.patch

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

::: browser/devtools/debugger/debugger-panes.js
@@ +709,2 @@
>     */
> +  updateButtonState: function() {

Rename this to updateToolbarButtonsState ('cuz they're more than 1).

@@ +734,5 @@
>      DebuggerView.Filtering.target = this;
>    },
>  
>    /**
>     * The check listener for the sources container.

This comment is now incorrect.

::: browser/devtools/debugger/debugger.xul
@@ +321,5 @@
>      <hbox id="debugger-widgets" flex="1">
>        <vbox id="sources-pane">
>          <vbox id="sources" flex="1"/>
>          <toolbar id="sources-toolbar" class="devtools-toolbar">
> +          <hbox id="sources-controls">

Nit: the extra hbox is premature at this point, but I wouldn't necessarily remove it.

::: browser/devtools/debugger/test/browser_dbg_blackboxing-01.js
@@ +26,5 @@
>    });
>  }
>  
>  function testBlackBoxSource() {
> +  const bbButton = getBlackBoxButton(gPanel);

bee bee buttun

::: browser/devtools/debugger/test/head.js
@@ +537,5 @@
> +
> +  if (aSource) {
> +    aPanel.panelWin.DebuggerView.Sources.selectedValue = aSource;
> +    ensureSourceIs(aPanel, aSource, true)
> +      .then(clickBlackBoxButton);

Nit: .then would look better on the same line IMO.

::: browser/locales/en-US/chrome/browser/devtools/debugger.dtd
@@ +33,5 @@
>    -  the button that opens up an options context menu for the debugger UI. -->
>  <!ENTITY debuggerUI.optsButton.tooltip  "Debugger Options">
>  
> +<!-- LOCALIZATION NOTE (debuggerUI.sources.blackBoxTooltip): This is the tooltip for the
> +button that black boxes the selected source. -->

Uber nit: localization comment doesn't follow the other comments' pattern. (Move the 'the' on the next line and fix indentation).

::: browser/themes/linux/devtools/debugger.css
@@ -50,5 @@
> -  background-size: 32px 16px;
> -  background-position: -16px 0;
> -  width: 16px;
> -  height: 16px;
> -  border: 0;

WOOHOOOO!!

@@ +32,3 @@
>  }
>  
> ++#sources .side-menu-widget-item-contents.black-boxed {

There's a + at the beginning of this line. U rebase by hand bro?

@@ +35,4 @@
>    color: #888;
>  }
>  
> +#sources .side-menu-widget-item-contents.black-boxed  > .dbg-breakpoint {

Is .side-menu-widget-item-contents still necessary? (Ditto on the above selector).

::: browser/themes/osx/devtools/debugger.css
@@ +37,4 @@
>    color: #888;
>  }
>  
> +#sources .side-menu-widget-item-contents.black-boxed  > .dbg-breakpoint {

Ditto.

::: browser/themes/windows/devtools/debugger.css
@@ +35,4 @@
>    color: #888;
>  }
>  
> +#sources .side-menu-widget-item-contents.black-boxed  > .dbg-breakpoint {

Ditto.
Attachment #820791 - Flags: review?(vporof) → review+
Attachment #820887 - Flags: review?(past)
Attachment #820887 - Flags: review?(past) → review+
https://hg.mozilla.org/mozilla-central/rev/2ed8270e713f
https://hg.mozilla.org/mozilla-central/rev/01672d68f16a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.