851546 - Create an Options panel for the toolbox

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Girish Sharma [:Optimizer]]

This options panel will contain many options related to the toolbox and DevTools on a whole.

Some brainstormed ideas are at : https://etherpad.mozilla.org/toolbox-options-panel

I am already on it and tools and buttons customization is complete. will attach a patch shortly.

In short, you will be able to do : 
gDevTools.showToolbox(target, "options").then(function(targetIframe) {...});

Comment 1

[Girish Sharma [:Optimizer]]

Attachment: WIP 001

Just wanted to know if I am in the right direction.

Things completed:
 - An options panel which goes with the tool id "options" and can be selected/opened it via normal gDevTools APIs.
 - Tools options with ability to enable disable any tool or button.
 - Toggle the two prefs (chrome.enabled and debugger.remote-enabled)

Next step:
 - Make is more accessible
 - Write tests
 - Fix the issue where debugger does not resume when removed while paused.
 - Better styling.

To answer a simple question of why did I not write a tools definition for optionsPanel and simply register it to gDevTools:

 - It will create keys into the main browser window along with the menu items for options tool. It is not a tool and thus should not have anything outside of toolbox.
 - Someone might override that tool with a same named actual tool. (though chances are less)

There are ways to avoid both of the above, but then the tool id "options" will have to have special meaning in many places other than Toolbox.jsm (for ex. gDevToolsBrowser would have to create key and menus for every tool other than "options"), and we will have to make sure that no other tool with id "options" gets registered via gDevTools.registerTool method, which is equally tricky as right now gDevTools does not know whether the tool is being registered by firefox or not.

I find the approach followed in the patch a better solution as things are confined to Toolbox.jsm only.

Comment 2

[Girish Sharma [:Optimizer]]

Attachment: Screenshot of the patch on Windows

Comment 3

[Paul Rouget [:paul]]

Can you use a sidemenu instead of the splitview?

(Good work!)

Comment 4

[Girish Sharma [:Optimizer]]

(If you are talking about Victor's sidemenuwidget) I thought of using it, but it was too complicated and for 3 static entries with no grouping, I though it will be overkill. (Not to forget that by default there will be only 2 entries)

So basically, those three are just custom radio elements in a radiogroup.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

The problem is that the splitview is going away soon.

Comment 6

[Girish Sharma [:Optimizer]]

(In reply to Panos Astithas [:past] from comment #5)
> The problem is that the splitview is going away soon.

I just need the splitview-active class from split view, and I am sure, sidemenu also have similar classes, I can simply use them instead. How does it sound ?

Comment 7

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

(In reply to Girish Sharma [:Optimizer] from comment #6)
> (In reply to Panos Astithas [:past] from comment #5)
> > The problem is that the splitview is going away soon.
> 
> I just need the splitview-active class from split view, and I am sure,
> sidemenu also have similar classes, I can simply use them instead. How does
> it sound ?

The SideMenuWidget was designed exactly for these kind of cases, and grouping is optional. It'd be best to use it, so if the widget improves (keyboard navigation etc.) the this panel will also improve for free.

If you have any questions about the API (or you think it's too cumbersome, file a bug). However, if you take a look at the documentation, it's just "insertItemAt(index, string)" and "get/set selectedItem", so I think it should be very easy to use.

Comment 8

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #7)
> (In reply to Girish Sharma [:Optimizer] from comment #6)
> > (In reply to Panos Astithas [:past] from comment #5)
> > > The problem is that the splitview is going away soon.
> > 
> > I just need the splitview-active class from split view, and I am sure,
> > sidemenu also have similar classes, I can simply use them instead. How does
> > it sound ?
> 
> The SideMenuWidget was designed exactly for these kind of cases, and
> grouping is optional. It'd be best to use it, so if the widget improves
> (keyboard navigation etc.) the this panel will also improve for free.
> 
> If you have any questions about the API (or you think it's too cumbersome,
> file a bug). However, if you take a look at the documentation, it's just
> "insertItemAt(index, string)" and "get/set selectedItem", so I think it
> should be very easy to use.

API wise, its okay. But the main thing is that I don't want to make the creation of the side panel a dynamic thing, that too when there are only 2 items present. I get keyboard navigation right now for free as I use radio and radiogroups. I will eventually use the styles used by your side menu widget, so that even if splitview is gone, it does not changes anything.

I would certainly have used sidemenuwidget if this was a big thing, or it had some major functionality. For now, its just 3 radios, I do not want to complicate things :)

Comment 9

[Girish Sharma [:Optimizer]]

Attachment: WIP 007

Enough for today(night).

Adds:

 - Panel Icons.
 - Appearance panel items.
 - Connect panel screen (basically the connect screen in toolbox)
 - Live Toolbox Tab style changing.
 - Central system to notify each and every tool for a pref change. For ex. Responsive UI can do gDevTools.on("pref-changed", foo) and enable/disable the floating scrollbars if that is the pref being changed.


Next step:

 - Style the new connect panel, and remove(?) the previous connect screen.
 - Hook responsive UI and devtools ui to use floating toolbar pref.
 - Fix some style nits and use sidemenuwidget's styles.
 - Write tests.
 - Get strings verified.

Comment 10

[Girish Sharma [:Optimizer]]

Attachment: screenshot showing the three panes

Look! (no icon in tabs!)

Comment 11

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

(In reply to Girish Sharma [:Optimizer] from comment #8)
> 
> I would certainly have used sidemenuwidget if this was a big thing, or it
> had some major functionality. For now, its just 3 radios, I do not want to
> complicate things :)

I'd assert that you do complicate things. You handle (a lot of!) markup yourself. You apply classes yourself. The radio circles shouldn't be visible, so you'll need to hack them out. Plus some other functionality differences that may always arise.

I tried to apply the patch on latest fx-team and nothing shows up on OS X.

I definitely like the panel, we certainly need it and I'm glad that you started working on it. But let's try not to end up with separate implementations of the same thing.

Comment 12

[Girish Sharma [:Optimizer]]

Attachment: WIP 008

Added one missed style rule on linux and osx.

Comment 13

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #11)
> (In reply to Girish Sharma [:Optimizer] from comment #8)
> > 
> > I would certainly have used sidemenuwidget if this was a big thing, or it
> > had some major functionality. For now, its just 3 radios, I do not want to
> > complicate things :)
> 
> I'd assert that you do complicate things. You handle (a lot of!) markup
> yourself. You apply classes yourself. The radio circles shouldn't be

As per handling markup, again, its all in xul as static (atleast the sidebar).
And I only add 2 keys. nothing more. If things start getting complicated, I will definitely switch to sidemenuwidget.

And I already have plans to move the applying class manually into css, as its pretty straight forward.

> visible, so you'll need to hack them out. Plus some other functionality

No longer present.

> differences that may always arise.

Again, please keep in mind that this is a static list of 3 (by default 2) items only. And that is the only reason I reverted from using sidemenuwidget to keep things simple.

> I tried to apply the patch on latest fx-team and nothing shows up on OS X.

Can you try WIP 008 ?

> I definitely like the panel, we certainly need it and I'm glad that you
> started working on it. But let's try not to end up with separate
> implementations of the same thing.

Yes, if you guys really think that the static panel should be dynamically created with the use of sidemenuwidget (even if there are only 3 entries, and no functionality is required at all) I can do that.

Comment 14

[Girish Sharma [:Optimizer]]

Before starting to style connect screen, I wanted to ask a few

Comment 15

[Girish Sharma [:Optimizer]]

Screw Tab Enter!

Before starting to style connect screen, I wanted to ask a few questions :

- Should the connect screen be only shown inside the options panel, and not as a standalone page (like its right now).
- Will the menu entry (in app menu and tools menu) for Connect screen still be there ?
- If the answer to first question is no, then the style of the standalone connect screen would have to be overwritten by toolbox (via toolbox.css), otherwise, I would simply change the styles in connect.css

Comment 16

[Girish Sharma [:Optimizer]]

Attachment: patch v0.1

Okay, Everything that can work is working except one thing. (Debugger resuming properly).

Things that remain : 
 - Decide what to do with the Connect menu entry in the Web Developer Menu.
 - Theme switching mechanism's backend. (Since it depends on other things)
 - Tests.

New screenshots coming.

Paul, waiting for your feedback and info now :)

Comment 17

[Girish Sharma [:Optimizer]]

Attachment: Screenshots of patch v0.1

Screenshot of patch v0.1 in action

Comment 18

[Paul Rouget [:paul]]

Comment on attachment 726284 [details] [diff] [review]
patch v0.1

I won't have time to look at that. Joe will help you Optimizer.

We need to define what this panel should show, make sure it behaves well with the theme switching mechanism (see bug 836233), agree on the disable/enable mechanism and figure out what we do for the connection screen (connecting and listening).

Comment 19

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

Comment on attachment 726284 [details] [diff] [review]
patch v0.1

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

::: browser/devtools/framework/toolbox-options.xul
@@ +10,5 @@
> +<?xml-stylesheet href="chrome://browser/content/devtools/framework/toolbox.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://browser/skin/devtools/common.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://browser/skin/devtools/toolbox.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://browser/skin/devtools/splitview.css"?>
> +<?xml-stylesheet href="chrome://browser/content/splitview.css"?>

Don't use splitview.css

Comment 20

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #19)
 
> Don't use splitview.css

I just need three styles from there, Should I copy them over to toolbox.css then ?

PS : I tried utilizing the classes of sidemenuwidget, they were not giving me the same results and too many modifications were required, making the radiogroups unnecessarily complex.

Comment 21

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

(In reply to Girish Sharma [:Optimizer] from comment #20)
> (In reply to Victor Porof [:vp] from comment #19)
>  
> > Don't use splitview.css
> 
> I just need three styles from there, Should I copy them over to toolbox.css
> then ?
> 

No.

> PS : I tried utilizing the classes of sidemenuwidget, they were not giving
> me the same results and too many modifications were required, making the
> radiogroups unnecessarily complex.

How were they not giving you the same results? They're mostly an identical copy, with different class names.

Comment 22

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #21)
> 
> > PS : I tried utilizing the classes of sidemenuwidget, they were not giving
> > me the same results and too many modifications were required, making the
> > radiogroups unnecessarily complex.
> 
> How were they not giving you the same results? They're mostly an identical
> copy, with different class names.

No they are not, the whole DOM is different in your case. In sidemenywideget, each entry is like :
<hbox>
  <vbox>
  // Content goes here
  </vbox>
  <hbox/> // An empty div to display the image for the splitter having the center arrow
</hbox>

and what splitview works on is :

<anytag>
  // Content goes here
</anytag>

content for me here is :

<radio>
  <label/>
  <img/>
</radio>

Now then I tried to convert my content to fit in your DOM structure:

<hbox>
  <vbox>
    <radio>
      <label/>
    </radio>
  </vbox>
  <hbox/>
</hbox>

but due to radio's internal style properties, it was not giving the same result. I was able to achieve the same style by overriding almost all of the radio's internal styles, but that resulted in 10lines of !important styles.

Let me upload a patch that uses the sideMenuWidget instead of my current approach, and let Joe decide on which one to use.

Comment 23

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

(In reply to Girish Sharma [:Optimizer] from comment #22)
> (In reply to Victor Porof [:vp] from comment #21)
> 
> No they are not, the whole DOM is different in your case.

An additional box is there because the arrow needed to be on top of internal content, so a background image wouldn't have worked. Normally this wouldn't matter, but in the debugger's case (breakpoints etc.) it made a difference.

> but due to radio's internal style properties, it was not giving the same
> result.

I'm curious as to why the radio's internal style was overridden with splitview and not with sidemenuwidget. From your explanation above, I understand that with splitview you'll need:

<hbox>
  <!-- start content -->
  <radio>
    <label/>
    <img/>
  </radio>
  <!-- end content -->
<hbox>

vs.:

<hbox>
  <vbox>
    <!-- start content -->
    <radio>
      <label/>
      <img/>
    </radio>
    <!-- end content -->
  </vbox>
  <hbox/>
</hbox>

I think 10 lines of !important styles shouldn't normally be necessary just because of an extra box. Feel free to touch widgets.css if necessary, maybe you found a way to make it more reliable.

Remember, postponing the change here would just mean more work later on to remove splitview :)

Comment 24

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #23)
> (In reply to Girish Sharma [:Optimizer] from comment #22)
> > (In reply to Victor Porof [:vp] from comment #21)
> > 
> > No they are not, the whole DOM is different in your case.
> 
> An additional box is there because the arrow needed to be on top of internal
> content, so a background image wouldn't have worked. Normally this wouldn't
> matter, but in the debugger's case (breakpoints etc.) it made a difference.

Yes, I know, there is is very helpful as the content on one item can take up n number of rows.

> > but due to radio's internal style properties, it was not giving the same
> > result.
> 
> I'm curious as to why the radio's internal style was overridden with
> splitview and not with sidemenuwidget. From your explanation above, I
> understand that with splitview you'll need:
> 
> <hbox>
>   <!-- start content -->
>   <radio>
>     <label/>
>     <img/>
>   </radio>
>   <!-- end content -->
> <hbox>

No, simply like :

<radio>
  <label/>
  <img/>
</radio>

(see the toolbox-options.xul in the patch)

> vs.:
> 
> <hbox>
>   <vbox>
>     <!-- start content -->
>     <radio>
>       <label/>
>       <img/>
>     </radio>
>     <!-- end content -->
>   </vbox>
>   <hbox/>
> </hbox>
> 
> I think 10 lines of !important styles shouldn't normally be necessary just
> because of an extra box. Feel free to touch widgets.css if necessary, maybe
> you found a way to make it more reliable.

No Idea what you mean by reliable, but maybe those extra boxes are unnecessary, and we can simply do away with something like :

<box>
  <vbox>
    // file name and breakpoints
  </vbox>
</box>

for the sidemenuwidget.

The outer box will have the background position right, center and the image as the arrow image.

The vbox's first child will have the style of side-menu-item(-or-group) and rest of the childs will have style of the breakpoints. Let me hack on something for that, but until then, the rules in widgets.css are unusable by options-toolbox.xul. I am happy to change to widgets.css's styles once these changes are made, but I don't think that waiting on those changes (if at all possible) should be the right choice for landing this options panel.
I am happy to change to widgets.css afterwards.

Comment 25

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

(In reply to Girish Sharma [:Optimizer] from comment #24)
> 
> No, simply like :
> 
> <radio>
>   <label/>
>   <img/>
> </radio>
> 

Well, by that definition you can certainly shove one outer box out of a sidemenu item structure as well :)

> <box>
>   <vbox>
>     // file name and breakpoints
>   </vbox>
> </box>
>

You don't need to care about breakpoints, they're not directly part of the sidemenuwidget. And the structure you're proposing is pretty much exactly what we have already. But we can certainly talk about it more later on.

> I am happy to change to widgets.css afterwards.

I'm fine with that. It would be extremely helpful, thank you.

Comment 26

[Paul Rouget [:paul]]

> I tried utilizing the classes of sidemenuwidget, they were not giving me the same results and too many modifications were required,

My 2c (and not reading everything because OMG jetlag) but it would be great to be able to re-use sidemenu here. If sidemenu don't meet our needs in this basic case, let's fix sidemenu.

Comment 27

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #26)
> > I tried utilizing the classes of sidemenuwidget, they were not giving me the same results and too many modifications were required,
> 
> My 2c (and not reading everything because OMG jetlag) but it would be great
> to be able to re-use sidemenu here. If sidemenu don't meet our needs in this
> basic case, let's fix sidemenu.

Well, if you directly use the SideMenuWidget.jsm to create the items dynamically, then it can be re-used, but without that , trying to use just the styles on this simple case is impossible.

I am working on changing to using the jsm to generate the menu items to see how much difference it is.

Comment 28

[Girish Sharma [:Optimizer]]

Attachment: patch v0.2 using SideMenuWidget

Using SideMenuWidget now.

Also added getter and setter for selectedIndex and fixed a bug where the item was not getting removed properly in SideMenuWidget.jsm. Added r? for Victor for that.

But still requiring only feedback from Joe right now.

Comment 29

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

(In reply to Girish Sharma [:Optimizer] from comment #28)

Thanks! Splinter is showing empty diffs in many files, like for example the firefox.js or Toolbox.jsm, so it's a bit hard to review properly. What happened, did you change the way you're creating patches?

Comment 30

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #29)
> (In reply to Girish Sharma [:Optimizer] from comment #28)
> 
> Thanks! Splinter is showing empty diffs in many files, like for example the
> firefox.js or Toolbox.jsm, so it's a bit hard to review properly. What
> happened, did you change the way you're creating patches?

Weird, firefox.js has changes, and I can see them using the diff viewer but not using splinter.

Comment 31

[Girish Sharma [:Optimizer]]

Attachment: patch v0.2.1 using SideMenuWidget

May be this works.
But it has some additional useless white space removal from firefox.js file (my editor seems to be extra smart)

Comment 32

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

Comment on attachment 726867 [details] [diff] [review]
patch v0.2.1 using SideMenuWidget

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

If you're using sublime, change trim_trailing_white_space_on_save to false in your preferences.

Remove #side-menu-widget, #side-menu-widget > *:first-child and .splitview-nav-container code from osx toolbox.css, leftovers probably, you don't need those rules right?

SideMenuWidget usage looks good otherwise. Thanks!

Comment 33

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

(In reply to Girish Sharma [:Optimizer] from comment #28)

Btw, I see you're using a splitter. You need this:

#your-container + .devtools-side-splitter {
  -moz-border-start-color: transparent;
}

to avoid two vertical black lines (one from the widget and one from the splitter). Maybe that's in there already on other platforms, but I'm seeing the splitter on OS X.

Comment 34

[Joe Walker [:jwalker]]

(CCing Stephen Horlander our UX guy)

I like it, and thanks for your work on this. If it feels painful to get this in, please keep at it. Things like this have a big impact and are hard to get right, but it's worth the effort.

I think we should consider putting the gear icon on the LHS of the toolbar or at least nearer the WebConsole|Inspector|Debugger|etc tabs because it acts like a tab.

Also, I'm concerned that 'connect' isn't a setting, so it seems counter-intuitive to put it in the settings dialog. Maybe we should have a button for it that is disabled by default (most users won't need this ever, but the ones that do will need it all the time)?

Comment 35

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #34)
> (CCing Stephen Horlander our UX guy)
> 
> I like it, and thanks for your work on this. If it feels painful to get this
> in, please keep at it. Things like this have a big impact and are hard to
> get right, but it's worth the effort.
> 
> I think we should consider putting the gear icon on the LHS of the toolbar
> or at least nearer the WebConsole|Inspector|Debugger|etc tabs because it
> acts like a tab.

I was also thinking that, but all other gear like buttons were on RHS (LHS for mac) so I put it like that only. I am okay with both, but it might appear an odd one out on Windows

> Also, I'm concerned that 'connect' isn't a setting, so it seems
> counter-intuitive to put it in the settings dialog. Maybe we should have a
> button for it that is disabled by default (most users won't need this ever,
> but the ones that do will need it all the time)?

By default, the menu item for Connect is not shown. Only if you have the pref "devtools.debugger.remote-enabled" as true (via about:config, or the options panel itself), the item is shown.

In one sense, I can see how the connect screen acts as a setting. It allows us to convert the target of the open toolbox from the current tab (or window) to something remote. (Does it make sense ? )

Comment 36

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #35)
> (In reply to Joe Walker [:jwalker] from comment #34)
> > (CCing Stephen Horlander our UX guy)
> > 
> > I like it, and thanks for your work on this. If it feels painful to get this
> > in, please keep at it. Things like this have a big impact and are hard to
> > get right, but it's worth the effort.
> > 
> > I think we should consider putting the gear icon on the LHS of the toolbar
> > or at least nearer the WebConsole|Inspector|Debugger|etc tabs because it
> > acts like a tab.
> 
> I was also thinking that, but all other gear like buttons were on RHS (LHS
> for mac) so I put it like that only. I am okay with both, but it might
> appear an odd one out on Windows

Is this in force in enough places that people are going to be confused? Can you give me some examples?

> > Also, I'm concerned that 'connect' isn't a setting, so it seems
> > counter-intuitive to put it in the settings dialog. Maybe we should have a
> > button for it that is disabled by default (most users won't need this ever,
> > but the ones that do will need it all the time)?

> In one sense, I can see how the connect screen acts as a setting. It allows
> us to convert the target of the open toolbox from the current tab (or
> window) to something remote. (Does it make sense ? )

It does, but you could use the same argument to make a case that virtually anything is a setting. Current URL? setting of the page. Current tab? Setting of the window, etc.

I think the minimum distinction is that settings are things that persist in some way, and the current target (i.e. the thing that connect changes) doesn't persist.

Comment 37

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #36)
> (In reply to Girish Sharma [:Optimizer] from comment #35)
> > (In reply to Joe Walker [:jwalker] from comment #34)
> > > (CCing Stephen Horlander our UX guy)
> > > 
> > > I like it, and thanks for your work on this. If it feels painful to get this
> > > in, please keep at it. Things like this have a big impact and are hard to
> > > get right, but it's worth the effort.
> > > 
> > > I think we should consider putting the gear icon on the LHS of the toolbar
> > > or at least nearer the WebConsole|Inspector|Debugger|etc tabs because it
> > > acts like a tab.
> > 
> > I was also thinking that, but all other gear like buttons were on RHS (LHS
> > for mac) so I put it like that only. I am okay with both, but it might
> > appear an odd one out on Windows
> 
> Is this in force in enough places that people are going to be confused? Can
> you give me some examples?

The debugger has the options cog wheel on the right on Windows vs on left on Mac. The Dock Buttons on the toolbox are on right in Windows vs on left in Mac.

> > > Also, I'm concerned that 'connect' isn't a setting, so it seems
> > > counter-intuitive to put it in the settings dialog. Maybe we should have a
> > > button for it that is disabled by default (most users won't need this ever,
> > > but the ones that do will need it all the time)?
> 
> > In one sense, I can see how the connect screen acts as a setting. It allows
> > us to convert the target of the open toolbox from the current tab (or
> > window) to something remote. (Does it make sense ? )
> 
> It does, but you could use the same argument to make a case that virtually
> anything is a setting. Current URL? setting of the page. Current tab?
> Setting of the window, etc.

Heh.

> I think the minimum distinction is that settings are things that persist in
> some way, and the current target (i.e. the thing that connect changes)
> doesn't persist.

Fair enough. I will move the connect menu out of the panel.

Comment 38

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #37)
> ...
> The debugger has the options cog wheel on the right on Windows vs on left on
> Mac. The Dock Buttons on the toolbox are on right in Windows vs on left in
> Mac.

Would it be easy to do a quick mock-up so we could have a look? That would make it easy for shorlander to opine.

> Fair enough. I will move the connect menu out of the panel.

Before you do, let's let others have a chance to weigh in. If we don't get any argument with this plan in the next day, let's do it.

Thanks.

Comment 39

[Girish Sharma [:Optimizer]]

Comment on attachment 726288 [details]
Screenshots of patch v0.1

There might be some string changes, and/or removal of connect menu item, and thus the connection screen

Comment 40

[Panos Astithas (he/him) [:past] (please ni?)]

My personal preferences are for a 'Connect' button instead of an options tab and LHS for the options gear icon/tab. I wouldn't mind RHS and right next to the tools, but I think it would seem kinda lonely and out of place there.

(In reply to Girish Sharma [:Optimizer] from comment #37)
> The debugger has the options cog wheel on the right on Windows vs on left on
> Mac. The Dock Buttons on the toolbox are on right in Windows vs on left in
> Mac.

The first part is not actually true, the debugger gear icon is on the right everywhere.

Comment 41

[Girish Sharma [:Optimizer]]

(In reply to Panos Astithas [:past] from comment #40)
> My personal preferences are for a 'Connect' button instead of an options tab
> and LHS for the options gear icon/tab. I wouldn't mind RHS and right next to
> the tools, but I think it would seem kinda lonely and out of place there.
> 
> (In reply to Girish Sharma [:Optimizer] from comment #37)
> > The debugger has the options cog wheel on the right on Windows vs on left on
> > Mac. The Dock Buttons on the toolbox are on right in Windows vs on left in
> > Mac.
> 
> The first part is not actually true, the debugger gear icon is on the right
> everywhere.

Ah, my bad. You are right.

Comment 42

[Girish Sharma [:Optimizer]]

I guess, I can go ahead and remove Connect screen from Options panel ?

Comment 43

[Joe Walker [:jwalker]]

Yes, thanks Optimizer.

Comment 44

[Girish Sharma [:Optimizer]]

Attachment: patch v0.3

Based this bug on top of Paul's work in bug 836233 to add theme switching capabilities on the go.

remaining things :
 - tests

some questions :
 - Now that there is a UI for switching both theme and floating scrollbars, should we enable floating scrollbars on light theme as well ? That way, user can choose the best suited combination of (light | dark) theme + (native | floating) scrollbars. And the floating scrollbars look absolutely fine on ligth background too. (screenshots coming)
 - Any other preference to be added in the panel ?

I will also add a comparison screenshot of the toolbox on Windows with the options panel cogwheel on the left vs on the right, so that we can decide the place.

Comment 45

[Girish Sharma [:Optimizer]]

Attachment: new screenshots

New screenshots as the options panel now have only two items.

Comment 46

[Girish Sharma [:Optimizer]]

Attachment: floating scrollbars on light theme

They look okay-ish, but we can certainly darken the color in light mode :)

Comment 47

[Girish Sharma [:Optimizer]]

Attachment: Options button on left on windows

Screenshot of options button on the left of tabs on Windows.

Comment 48

[Paul Rouget [:paul]]

(In reply to Girish Sharma [:Optimizer] from comment #46)
> Created attachment 727865 [details]
> floating scrollbars on light theme
> 
> They look okay-ish, but we can certainly darken the color in light mode :)

You're not supposed to use floating scrollbars in light mode. Only dark mode for now.

Comment 49

[Joe Walker [:jwalker]]

I've just been having a play, and I like it lots, thanks.

Here's what I think we need:
- The 'gear' icon should probably have a divider on the left as well as the right.
  It is after all a type of tab, and the other tabs have dividers both sides.
  Perhaps there is styling that we can apply to the toolbarbutton?
- Perhaps the 'Developer Tools installed by add-ons' should be hidden if there are
  none
- Scrollbars - I have to resize my window because I can't scroll the options area

And I think we should continually seeking to avoid adding options where we can. Everything option that we add is extra cognitive load that makes people feel stupider and less able to do their jobs. So I think we should make sure there is a real demand for:
- allowing people to change their tab style (chrome icons are above their labels
  so we don't have quite the same screen real estate justification)
- allowing people to alter the toolbox buttons

Please could you split out these 2 into a separate patch so we can check that people really need them.

Awesome work, thanks!

Comment 50

[Joe Walker [:jwalker]]

Also I would love to have Shorlander/Paul (and Optimizer, obviously) comment on my thoughts in comment 49. Does it make sense?

Comment 51

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #49)
> I've just been having a play, and I like it lots, thanks.
> 
> Here's what I think we need:
> - The 'gear' icon should probably have a divider on the left as well as the
> right.
>   It is after all a type of tab, and the other tabs have dividers both sides.
>   Perhaps there is styling that we can apply to the toolbarbutton?
So we should keep it on the left of the tabs for Windows too ?

> - Perhaps the 'Developer Tools installed by add-ons' should be hidden if
> there are
>   none
Agreed.

> - Scrollbars - I have to resize my window because I can't scroll the options
> area
Already done locally.

> And I think we should continually seeking to avoid adding options where we
> can. Everything option that we add is extra cognitive load that makes people
> feel stupider and less able to do their jobs. So I think we should make sure
> there is a real demand for:
> - allowing people to change their tab style (chrome icons are above their
> labels
>   so we don't have quite the same screen real estate justification)
I didn't get the bracket part, but letting them decide whether to have text only tabs, or icon only tabs can be useful in case of too many tabs on a small screen. Whether their is a need - I don't think so, atleast not now.
The latest chromium show text only tabs by default, with an option to show the icons too.

> - allowing people to alter the toolbox buttons
> 
> Please could you split out these 2 into a separate patch so we can check
> that people really need them.
> 
> Awesome work, thanks!

Paul was also suggesting that we do not allow people to disable floating scrollbars in responsive mode.

So that would leave the panel with only 4 option sets : Default tools, tools by add-ons, under the hood settings and the theme switch options.

2 different panes for only these many options seems wastage.

Comment 52

[Paul Rouget [:paul]]

I can't apply the patch:

RuntimeError: File "devtools/toolbox-options-tools.png" not found in /Users/paul/mozilla/src/browser/themes/osx, /Users/paul/mozilla/obj/browser/themes/osx

Comment 53

[Paul Rouget [:paul]]

Joe said:
> And I think we should continually seeking to avoid adding options where we can.

I totally agree with that. In the Appearance section, only the theme option is actually needed.

Also, for now, I think we should not have 2 sections (Tools / Appearance). Let's keep it simple and just have one section (no side menu then).

We will add sections once we will have too many options (even though I hope we won't ever need to do that).

Comment 54

[Paul Rouget [:paul]]

Could toolbox-options.xul be an html page instead of a xul page? Not mandatory, but I tend to use HTML where I can.

Comment 55

[Paul Rouget [:paul]]

Disregard comment 52

Comment 56

[Girish Sharma [:Optimizer]]

So here's my understanding of what is desired finally :

- only one page (no side menus)
- options for tools, theme switch and the two prefs (chrome.enabled and remote-enabled) only. [no button/floating scrollbars/tab style options]
- convert the xul to xhtml [optional]

I am still confused on whether the options button is required on the right or on left (on linux and windows) ?

Please clear my last doubt, so that I can attach the final patch along with tests.

Comment 57

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #56)
> So here's my understanding of what is desired finally :
> 
> - only one page (no side menus)
> - options for tools, theme switch and the two prefs (chrome.enabled and
> remote-enabled) only. [no button/floating scrollbars/tab style options]
> - convert the xul to xhtml [optional]

I think that's right.
It seems possible (or even likely) that we'll need to put the side menus back in when we get more options, but right now there doesn't seem to be much point.

> I am still confused on whether the options button is required on the right
> or on left (on linux and windows) ?

So my opinion is that it should be on the left. It's the only thing that we can't turn off so it's the most permanent. The RHS is where least permanent things (i.e panels supplied by addons) live. So conceptually the tabs are sorted by permanence.

> Please clear my last doubt, so that I can attach the final patch along with
> tests.

I hope that helps.

I sense some frustration that we keep changing things. Which I can understand, but I think UI design is hard, and taking a look and having a play, and then deciding that it's not quite right, is vital to the process of getting a good design, so please stick at it.

Comment 58

[Girish Sharma [:Optimizer]]

Attachment: patch v0.4

Updated with all the changes required. This should be ready for review. Tests coming in a separate patch.

(I tried to do things in html, but the workarounds were too much, no radio groups were the most hitting things.)

New screenshots coming .

Comment 59

[Girish Sharma [:Optimizer]]

Attachment: latest screenshot

Screenshot of latest patch.

- Scrollbars are working
- When toolbox is in side, all the options come in one vertical line.
- The options button is on the left of the tabs, (even in Windows)

Comment 60

[Girish Sharma [:Optimizer]]

Attachment: patch v0.5 with tests

- Moves the checkbox population logic out of Toolbox.jsm into toolbox-options.js 
 - Adds tests for opening of options panel and the tools and preferences.

 try at : https://tbpl.mozilla.org/?tree=Try&rev=4122255fd127

just theme switching mechanism testing is left and I am not even sure if this should be covered as a part of this bug. Maybe I will open up a new bug to strongly test the theme switching mechanism and all its corner cases ..

Comment 61

[Girish Sharma [:Optimizer]]

Attachment: patch v0.6

added missing toolbox-options.xul

try at : https://tbpl.mozilla.org/?tree=Try&rev=f4718ee135b2

Comment 62

[Joe Walker [:jwalker]]

Comment on attachment 729840 [details] [diff] [review]
patch v0.4

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

Things changed since I worked on this, but I think these comments are still relevant.

::: browser/devtools/framework/Toolbox.jsm
@@ +557,4 @@
>  
>      // and select the right iframe
>      let deck = this.doc.getElementById("toolbox-deck");
> +    // offset by 1 due to options panel

Ug. How hard would it be to have the options panel be a tool like the other tools?

(Don't rush out and do it unless it's a no-brainer, I'm just asking at this stage)

@@ +611,5 @@
>        }
> +      else if (panel) {
> +        let boundLoad = function() {
> +          panel.removeEventListener("DOMContentLoaded", boundLoad, true);
> +          this.emit("select", id);

Could you factor out these 3 lines (and the duplicates above) into a new function?

::: browser/devtools/framework/gDevTools.jsm
@@ +122,5 @@
>      let tools = new Map();
> +    let disabledTools = [];
> +    try {
> +      disabledTools = JSON.parse(Services.prefs.getCharPref("devtools.toolbox.disabledTools"));
> +    } catch(ex) {}

I think we should report this error somewhere rather than just throwing it away.

::: browser/devtools/framework/toolbox-options.js
@@ +4,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> +
> +window.addEventListener("load", function onLoad() {
> +  window.removeEventListener("load", onLoad);

Are we likely to get more than one load event?

::: browser/devtools/shared/theme-switching.js
@@ +4,5 @@
> +      let display = window.getComputedStyle(document.documentElement).display; // Save display value
> +      document.documentElement.style.display = "none";
> +      window.getComputedStyle(document.documentElement).display; // Flush
> +      document.documentElement.style.display = display; // Restore
> +    } catch(ex) {}

Please could you add a comment explaining why you need to catch and ignore

::: browser/themes/linux/devtools/toolbox.css
@@ +240,5 @@
> +
> +#options-panel {
> +  background-image: url("chrome://browser/skin/newtab/noise.png");
> +  overflow-y: auto;
> +  display: block;

I think 'display' belongs in theme CSS. Probably the same goes with 'float' and 'overflow-y' too.

https://wiki.mozilla.org/DevTools/CSSTips

::: browser/themes/windows/devtools/toolbox.css
@@ +140,5 @@
> +  -moz-image-region: rect(0px 32px 16px 16px);
> +}
> +
> +#toolbox-tab-options > image {
> +  opacity: 1 !important;

Please could you explain why you need !important? Isn't there a better way?
If not, perhaps we need a comment explaining why it's needed.

Comment 63

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #62)
> Comment on attachment 729840 [details] [diff] [review]
> patch v0.4
> 
> Review of attachment 729840 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Things changed since I worked on this, but I think these comments are still
> relevant.
> 
> ::: browser/devtools/framework/Toolbox.jsm
> @@ +557,4 @@
> >  
> >      // and select the right iframe
> >      let deck = this.doc.getElementById("toolbox-deck");
> > +    // offset by 1 due to options panel
> 
> Ug. How hard would it be to have the options panel be a tool like the other
> tools?

This implementation also adds it to the list of tools, but by indirect methods to avoid:
- Menu items being created
- tool named "options" being overwritten
- hard coded bypassing in toolbox-options.js to not allow to disable "options" tool.
- a separate optionsPanel.jsm and an object in ToolsDefinition.jsm.

While this implementation also has some hardcoded code to bypass options, they all reside in Toolbox.jsm (as options panel is a thing belonging to toolbox) and are very minimal.

I initially started off with that approach only, when I found too much hardcoding, so I shifted to this approach.

> (Don't rush out and do it unless it's a no-brainer, I'm just asking at this
> stage)
> 
> @@ +611,5 @@
> >        }
> > +      else if (panel) {
> > +        let boundLoad = function() {
> > +          panel.removeEventListener("DOMContentLoaded", boundLoad, true);
> > +          this.emit("select", id);
> 
> Could you factor out these 3 lines (and the duplicates above) into a new
> function?

Sure.

> ::: browser/devtools/framework/gDevTools.jsm
> @@ +122,5 @@
> >      let tools = new Map();
> > +    let disabledTools = [];
> > +    try {
> > +      disabledTools = JSON.parse(Services.prefs.getCharPref("devtools.toolbox.disabledTools"));
> > +    } catch(ex) {}
> 
> I think we should report this error somewhere rather than just throwing it
> away.

Good idea. Someone could have changed it manually, he must know.

> ::: browser/devtools/framework/toolbox-options.js
> @@ +4,5 @@
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> > +
> > +window.addEventListener("load", function onLoad() {
> > +  window.removeEventListener("load", onLoad);
> 
> Are we likely to get more than one load event?

It seemed nice to remove a listener when not needed :)

> ::: browser/devtools/shared/theme-switching.js
> @@ +4,5 @@
> > +      let display = window.getComputedStyle(document.documentElement).display; // Save display value
> > +      document.documentElement.style.display = "none";
> > +      window.getComputedStyle(document.documentElement).display; // Flush
> > +      document.documentElement.style.display = display; // Restore
> > +    } catch(ex) {}
> 
> Please could you add a comment explaining why you need to catch and ignore

For some reason, document.documentElement was undefined for me at some times, thus the first line there throwing.

> ::: browser/themes/linux/devtools/toolbox.css
> @@ +240,5 @@
> > +
> > +#options-panel {
> > +  background-image: url("chrome://browser/skin/newtab/noise.png");
> > +  overflow-y: auto;
> > +  display: block;
> 
> I think 'display' belongs in theme CSS. Probably the same goes with 'float'
> and 'overflow-y' too.
> 
> https://wiki.mozilla.org/DevTools/CSSTips

ITYM content css ?

> ::: browser/themes/windows/devtools/toolbox.css
> @@ +140,5 @@
> > +  -moz-image-region: rect(0px 32px 16px 16px);
> > +}
> > +
> > +#toolbox-tab-options > image {
> > +  opacity: 1 !important;
> 
> Please could you explain why you need !important? Isn't there a better way?
> If not, perhaps we need a comment explaining why it's needed.

err. I actually don't need this .. will remove.

Comment 64

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #63)
> > Ug. How hard would it be to have the options panel be a tool like the other
> > tools?
> 
> This implementation also adds it to the list of tools, but by indirect
> methods to avoid:
> - Menu items being created
> - tool named "options" being overwritten
> - hard coded bypassing in toolbox-options.js to not allow to disable
> "options" tool.
> - a separate optionsPanel.jsm and an object in ToolsDefinition.jsm.
> 
> While this implementation also has some hardcoded code to bypass options,
> they all reside in Toolbox.jsm (as options panel is a thing belonging to
> toolbox) and are very minimal.
> 
> I initially started off with that approach only, when I found too much
> hardcoding, so I shifted to this approach.

OK, makes sense.

> > ::: browser/themes/linux/devtools/toolbox.css
> > @@ +240,5 @@
> > > +
> > > +#options-panel {
> > > +  background-image: url("chrome://browser/skin/newtab/noise.png");
> > > +  overflow-y: auto;
> > > +  display: block;
> > 
> > I think 'display' belongs in theme CSS. Probably the same goes with 'float'
> > and 'overflow-y' too.
> > 
> > https://wiki.mozilla.org/DevTools/CSSTips
> 
> ITYM content css ?

I do, yeah.

Thanks.

Comment 65

[Girish Sharma [:Optimizer]]

Attachment: patch v0.7

Comments addressed.

Comment 66

[Joe Walker [:jwalker]]

Comment on attachment 730858 [details] [diff] [review]
patch v0.7

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

Sorry it took a while.

::: browser/devtools/shared/theme-switching.js
@@ +16,5 @@
> +    document.documentElement.style.display = display; // Restore
> +  }
> +
> +  function switchTheme(theme, oldTheme) {
> +    let winUtils = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);

Minor nit: please could you wrap at 80 columns.

Comment 67

[Girish Sharma [:Optimizer]]

Attachment: patch v0.8

Carrying forward Joe's r+
I am concluding that all the strings are okay :)

Should I wait for ui-r+ from Stephen before marking it [land-in-fx-team] ?

Comment 68

[Joe Walker [:jwalker]]

Stephen - might you have time for this in the next week?
If not, I suggest we land anyway - both Paul and I have taken a good look.

Comment 69

[Girish Sharma [:Optimizer]]

Dave said that I can land this since his devtools loader patch got backed out. Please someone either land this before bug 855914 lands in fx-team, or wait for the updated patch.

Comment 70

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 71

[Panos Astithas (he/him) [:past] (please ni?)]

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

Backed out for test failures, e.g.:

https://tbpl.mozilla.org/php/getParsedLog.php?id=21804072&tree=Fx-Team

Comment 72

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

Comment on attachment 733464 [details] [diff] [review]
patch v0.8

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

::: browser/devtools/framework/toolbox-options.xul
@@ +11,5 @@
> +<?xml-stylesheet rel="stylesheet" href="chrome://browser/skin/devtools/toolbox.css" type="text/css"?>
> +
> +<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +  <script type="application/javascript;version=1.8" src="toolbox-options.js"></script>
> +  <hbox flex="1" style="overflow: auto">

Inline styles? Also, why overflow this? You overflow #options-panel in toolbox.css, which is also an only child, which is also flexed!

::: browser/devtools/framework/toolbox.css
@@ +15,5 @@
>  }
> +
> +#options-panel {
> +  overflow-y: auto;
> +  display: block;

Why display: block?

::: browser/devtools/shared/theme-switching.js
@@ +21,5 @@
> +  function switchTheme(theme, oldTheme) {
> +    let winUtils = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                         .getInterface(Ci.nsIDOMWindowUtils);
> +    if (oldTheme && theme != oldTheme) {
> +      let old_theme_url = Services.io.newURI(DEVTOOLS_SKIN_URL + oldTheme +

Mixing camelCase and underscores?

@@ +27,5 @@
> +      try {
> +        winUtils.removeSheet(old_theme_url, window.AUTHOR_SHEET);
> +      } catch(ex) {}
> +    }
> +    let theme_url = Services.io.newURI( +

Why is there a + here? If |theme| is a string it will be turned into a NaN. Am I missing something? Has anyone looked at this?

@@ +50,5 @@
> +
> +  function handlePrefChange(event, data) {
> +    switch(data.pref) {
> +      case "devtools.theme":
> +        switchTheme(data.newValue, data.oldValue);

Switch statement with a single case?

::: browser/locales/en-US/chrome/browser/devtools/toolbox.dtd
@@ +11,5 @@
> +<!ENTITY options.context.label         "Advanced settings. (Requires a browser restart)">
> +<!ENTITY options.enableChrome.label    "Enable Chrome debugging.">
> +<!ENTITY options.enableChrome.tooltip  "Turning this option on will allow you to use various developer tools in browser context.">
> +<!ENTITY options.enableRemote.label    "Enable Remote Debugging.">
> +<!ENTITY options.enableRemote.tooltip  "Turning this option on will allow the developer tools to debug remote Firefox instance like Firefox OS.">

Not even a single localization note in this file?

Comment 73

[Panos Astithas (he/him) [:past] (please ni?)]

I played with the patch a bit while testing the build before the backout, and I have a few nits that should be very easy to address:

- we don't end sentences with a period in option panels
- "Requires a browser restart" shouldn't start with a capital, after the period is removed
- chrome debugging and remote debugging have inconsistent capitalization. I vote for no caps at all
- the word Theme shouldn't be capitalized
- it would be more helpful to the user if the order of the tools corresponded to the order of the tabs when all are enabled

Comment 74

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof [:vp] from comment #72)
> @@ +27,5 @@
> > +      try {
> > +        winUtils.removeSheet(old_theme_url, window.AUTHOR_SHEET);
> > +      } catch(ex) {}
> > +    }
> > +    let theme_url = Services.io.newURI( +
> 
> Why is there a + here? If |theme| is a string it will be turned into a NaN.
> Am I missing something? Has anyone looked at this?

This is probably what caused the oranges.

Comment 75

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #72)
> Comment on attachment 733464 [details] [diff] [review]
> patch v0.8
> 
> Review of attachment 733464 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/framework/toolbox-options.xul
> @@ +11,5 @@
> > +<?xml-stylesheet rel="stylesheet" href="chrome://browser/skin/devtools/toolbox.css" type="text/css"?>
> > +
> > +<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> > +  <script type="application/javascript;version=1.8" src="toolbox-options.js"></script>
> > +  <hbox flex="1" style="overflow: auto">
> 
> Inline styles? Also, why overflow this? You overflow #options-panel in
> toolbox.css, which is also an only child, which is also flexed!

Without this, scrollbars will not appears even on overflow. So when you decrease the height of toolbox, the content will simply crop off.

> ::: browser/devtools/framework/toolbox.css
> @@ +15,5 @@
> >  }
> > +
> > +#options-panel {
> > +  overflow-y: auto;
> > +  display: block;
> 
> Why display: block?

For it to be responsive when the toolbox is docked to side.

> ::: browser/devtools/shared/theme-switching.js
> @@ +21,5 @@
> > +  function switchTheme(theme, oldTheme) {
> > +    let winUtils = window.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                         .getInterface(Ci.nsIDOMWindowUtils);
> > +    if (oldTheme && theme != oldTheme) {
> > +      let old_theme_url = Services.io.newURI(DEVTOOLS_SKIN_URL + oldTheme +
> 
> Mixing camelCase and underscores?

This is inherited from original file. I wanted to use the same styling. I can change this if this is too much bothering.

> @@ +27,5 @@
> > +      try {
> > +        winUtils.removeSheet(old_theme_url, window.AUTHOR_SHEET);
> > +      } catch(ex) {}
> > +    }
> > +    let theme_url = Services.io.newURI( +
> 
> Why is there a + here? If |theme| is a string it will be turned into a NaN.
> Am I missing something? Has anyone looked at this?

I messed up in the latest patch. It is straight forward. Missing DEVTOOLS_SKIN_URL before +


> ::: browser/locales/en-US/chrome/browser/devtools/toolbox.dtd
> @@ +11,5 @@
> > +<!ENTITY options.context.label         "Advanced settings. (Requires a browser restart)">
> > +<!ENTITY options.enableChrome.label    "Enable Chrome debugging.">
> > +<!ENTITY options.enableChrome.tooltip  "Turning this option on will allow you to use various developer tools in browser context.">
> > +<!ENTITY options.enableRemote.label    "Enable Remote Debugging.">
> > +<!ENTITY options.enableRemote.tooltip  "Turning this option on will allow the developer tools to debug remote Firefox instance like Firefox OS.">
> 
> Not even a single localization note in this file?

Will add along with addressing Panos's requests too.

Comment 76

[Girish Sharma [:Optimizer]]

(In reply to Panos Astithas [:past] from comment #73)
> I played with the patch a bit while testing the build before the backout,
> and I have a few nits that should be very easy to address:
> 
> - we don't end sentences with a period in option panels
> - "Requires a browser restart" shouldn't start with a capital, after the
> period is removed
> - chrome debugging and remote debugging have inconsistent capitalization. I
> vote for no caps at all
> - the word Theme shouldn't be capitalized

Okay. I was actually looking for some help with strings all along :)

> - it would be more helpful to the user if the order of the tools
> corresponded to the order of the tabs when all are enabled

May be in a follow up ? Right now, there is a lot of inconsistency in the ordering. Like when you disable a tool from options panel , and add it back without closing the toolbox, it appears towards the very end. The same goes when an tool is registered while toolbox is open. May be we can fix all that in the follow up.

Comment 77

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

Attachment: offset

Is this the intended look of the options button? The extra margin on the end looks a bit funky.

Comment 78

[Girish Sharma [:Optimizer]]

(In reply to Panos Astithas [:past] from comment #73)
> I played with the patch a bit while testing the build before the backout,
> and I have a few nits that should be very easy to address:
> 
> - we don't end sentences with a period in option panels

What about in tooltips ?

> - chrome debugging and remote debugging have inconsistent capitalization. I
> vote for no caps at all
even the 'e' in "enable" should be small ?

Comment 79

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #77)
> Created attachment 737413 [details]
> offset
> 
> Is this the intended look of the options button? The extra margin on the end
> looks a bit funky.

What extra margin are you talking about ?
The docking buttons have 14px margin, so does the options cog wheel. I see a 2px difference in the left and right border sizes, if that is what you are talking about. But then that is a special case on Mac where the docking buttons are on the left. All these styles are inherited as I am not applying any margin to the button explicitly.

Comment 80

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

(In reply to Girish Sharma [:Optimizer] from comment #79)
> 
> What extra margin are you talking about ?

I think the white-ish border on the left shouldn't be there. The fact that some buttons on OS X are on the left doesn't implicitly mean styling issues shouldn't be dealt with. That's why we have different theme files for each platform :)

Comment 81

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #80)
> (In reply to Girish Sharma [:Optimizer] from comment #79)
> > 
> > What extra margin are you talking about ?
> 
> I think the white-ish border on the left shouldn't be there. The fact that
> some buttons on OS X are on the left doesn't implicitly mean styling issues
> shouldn't be dealt with. That's why we have different theme files for each
> platform :)

That is an already present right border on the docking button. Joe suggested that we make the options panel a part of the tabs instead of the buttons. Thus the border is separating buttons with tabs. That border should already be present even without the patch :)

Comment 82

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

Attachment: before the patch

Comment 83

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #82)
> Created attachment 737423 [details]
> before the patch

I see now, its this extra border-left style :

#toolbox-tabs {
  margin: 0;
  border-left: 1px solid hsla(206,37%,4%,.45);
}

applied on the tabs bar just on Mac. Fixed locally.

Comment 84

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Girish Sharma [:Optimizer] from comment #76)
> (In reply to Panos Astithas [:past] from comment #73)
> > - it would be more helpful to the user if the order of the tools
> > corresponded to the order of the tabs when all are enabled
> 
> May be in a follow up ? Right now, there is a lot of inconsistency in the
> ordering. Like when you disable a tool from options panel , and add it back
> without closing the toolbox, it appears towards the very end. The same goes
> when an tool is registered while toolbox is open. May be we can fix all that
> in the follow up.

I'm OK with that.

(In reply to Girish Sharma [:Optimizer] from comment #78)
> (In reply to Panos Astithas [:past] from comment #73)
> > - we don't end sentences with a period in option panels
> 
> What about in tooltips ?

Tooltips don't have periods either. Basically whenever a phrase is used in a context as a caption or label, the period is omitted. When in doubt, look at what is the common theme across other Firefox components.

> > - chrome debugging and remote debugging have inconsistent capitalization. I
> > vote for no caps at all
> even the 'e' in "enable" should be small ?

No, the first letter in labels is always capitalized.

Comment 85

[Girish Sharma [:Optimizer]]

Attachment: patch v0.9

Asking Panos and Victor too, for the changes requested. I think any one review is also enough too :) . Fixed the Mac OS X border issue too. Fixed the oranges caused by missing variable.

Comment 86

[Girish Sharma [:Optimizer]]

Green try : https://tbpl.mozilla.org/?tree=Try&rev=59ccefc0ee70

Comment 87

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 737505 [details] [diff] [review]
patch v0.9

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

Yep, looks good to me!

Comment 88

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

Comment on attachment 737505 [details] [diff] [review]
patch v0.9

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

::: browser/devtools/framework/gDevTools.jsm
@@ +122,5 @@
>      let tools = new Map();
> +    let disabledTools = [];
> +    try {
> +      disabledTools = JSON.parse(Services.prefs.getCharPref("devtools.toolbox.disabledTools"));
> +    } catch(ex) {}

Maybe if bad things happen here it'd be wise to reset the pref to []. Followup if you think it's important.

Comment 89

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 90

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 91

[Girish Sharma [:Optimizer]]

Comment on attachment 729848 [details]
latest screenshot

Clearing ui-review. Too late for that now :)

Comment 92

[Lukas Blakk [:lsblakk] use ?needinfo]

Added to the 23.0 release notes as:

NEW
Options panel created for Web Developer Toolbox

any questions or concerns should be directed to release-mgmt.