The toolbox toolbar contains 3 buttons for launching tilt, scratchpad and responsive mode. Besides scratchpad which can be opened multiple times, the other two can be either enabled or not. Using type=checkbox in the button would make their state evident.

This completely works..... except for the fact that the event handlers added via .on are not removed on toolbox closing. So, memory will leak when you reopen toolbox. I could not find a way to call the .off method with the exact function, as for that, a mapping would be required between tab and the handler. If no other option is there, then I will do that only :|

I think I figured a way to do this without creating a map. Will submit if it works out. Feedback still welcomed :)

Comment on attachment 693596 [details] [diff] [review] WIP 90% Review of attachment 693596 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/DeveloperToolbar.jsm @@ +108,5 @@ > + button.setAttribute("checked", > + command.state.isChecked(tab)? true: false); > + }; > + command.state.onChange(tab, onChange); > + onChange(); We'll need to call offChange somewhere. I'm wondering if this is better than exposing something that is an event emitter? ::: browser/devtools/tilt/CmdTilt.jsm @@ +179,4 @@ > exec: function(args, context) { > let chromeWindow = context.environment.chromeDocument.defaultView; > let Tilt = TiltManager.getTiltForBrowser(chromeWindow); > + Added spaces

Thanks Optimizer - I wonder if we shouldn't give Victor a chance to take a look since this effects Tilt.

(In reply to Joe Walker [:joe_walker] [:jwalker] from comment #3) > We'll need to call offChange somewhere. > I'm wondering if this is better than exposing something that is an event > emitter? Exactly the name I thought to call the .off's . But the problem was that I needed to hold reference to the handler that was called while calling .on's Maybe Event Emitter can have a method to remove all the handlers (without needing the exact handler) using some key/id (as we don't want to remove *all* handler, their might be some handlers added by someone else)

(In reply to Girish Sharma [:Optimizer] from comment #5) > (In reply to Joe Walker [:joe_walker] [:jwalker] from comment #3) > > We'll need to call offChange somewhere. > > I'm wondering if this is better than exposing something that is an event > > emitter? > > Exactly the name I thought to call the .off's . But the problem was that I > needed to hold reference to the handler that was called while calling .on's Do you mean the Toolbox will need to hold a reference to the things to 'offChange' on? If so, that shouldn't be a problem. > Maybe Event Emitter can have a method to remove all the handlers (without > needing the exact handler) using some key/id (as we don't want to remove > *all* handler, their might be some handlers added by someone else) OTOH, if you expose an EventEmitter, then this is handled by the EventEmitter. I'm a little unclear what the problem is.

The problem is : The commond is a single reference, kind of singleton, and we call the onChange method of the same command again and again whenever the toolbox is opened. The differentiating parameter is the target.tab which allows us to add the .on method to different tilt and responsiveUI. Now lets say I open toolbox in tab1, thus doing the following: tiltForTab1.on("change", handler1); and then I open it for tab2, thus doing: tiltForTab2.on("change", handler2); now when I close toolbox on tab1, My command needs to have a reference to handler2. In this patch, I modify the handler inside of onChange method, thus the reference that is with the toolbox is not valid, so to properly call the offChange method, I needed to save the handler that I generate from inside the onChange method in a Map of tab vs handler. But now I think I will not generate a handler inside the onChange method, instead make the same changes from inside the createButtons method itself, so that the handler passed to .on("change" is the same. About an API to do this easily in EventEmitter : something like: tilt.off("change") will remove all the handlers added via tilt.on("change", handlerX)

(contd..) but we might want to remove selective handlers (still, without reference to those handlers) so the API would be like do : tilt.on(event, handler, uniqID); and when we do tilt.off(event, null, uniqID) (or simply off(event, uniqID) ) then all the handlers added via on with the id as uniqID will be removed, without the need of the exact handler. This can further be extended to something like : tilt.off(uniqID) -> removes all the handlers for all the events added using uniqID.

Tilt-related changes look ok so far.

Optimizer - do you have a reference to the etherpad - I think the layout that we had there would work?

This works. The approach used in this patch is similar to that in the etherpad only. I little simpler. Also fixed some stray vars.

Comment on attachment 693935 [details] [diff] [review] Patch v1.0 I'll let Joe review this. r+ for the responsive mode changes (test pass, code looks right).

Comment on attachment 693935 [details] [diff] [review] Patch v1.0 Review of attachment 693935 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: browser/devtools/shared/DeveloperToolbar.jsm @@ +106,5 @@ > + let tab = target.tab; > + let onChange = function(event, eventTab) { > + if (eventTab == tab) { > + button.setAttribute("checked", > + command.state.isChecked(tab)? true: false); I think we should do: if (command.state.isChecked(tab)) { button.setAttribute("checked", "true"); } else { button.clearAttribute("checked"); } Because this is XML and because people write css like toolbarbutton[checked] @@ +112,5 @@ > + }; > + command.state.onChange(tab, onChange); > + onChange(null, tab); > + document.defaultView.addEventListener("unload", function() { > + command.state.offChange(tab, onChange); I think we should probably use toolbox.on("destroy", ....) rather than a window event. ::: browser/devtools/tilt/CmdTilt.jsm @@ +46,5 @@ > buttonClass: "command-button devtools-toolbarbutton", > tooltipText: gcli.lookup("tiltToggleTooltip"), > hidden: true, > + state: { > + isChecked: function(aTab) { In these 3 functions I think the parameter should be a target rather than a tab. This goes for all the commands, obviously.

(In reply to Joe Walker [:joe_walker] [:jwalker] from comment #13) > I think we should do: > if (command.state.isChecked(tab)) { > button.setAttribute("checked", "true"); > } else { > button.clearAttribute("checked"); > } > > Because this is XML and because people write css like toolbarbutton[checked] by clearAttribute, you mean removeAttribute .. right ? > I think we should probably use toolbox.on("destroy", ....) rather than a > window event. So should I get the toolbox reference using the target ? > ::: browser/devtools/tilt/CmdTilt.jsm > @@ +46,5 @@ > > buttonClass: "command-button devtools-toolbarbutton", > > tooltipText: gcli.lookup("tiltToggleTooltip"), > > hidden: true, > > + state: { > > + isChecked: function(aTab) { > > In these 3 functions I think the parameter should be a target rather than a > tab. This goes for all the commands, obviously. noted.

(In reply to Girish Sharma [:Optimizer] from comment #14) > (In reply to Joe Walker [:joe_walker] [:jwalker] from comment #13) > > I think we should do: > > if (command.state.isChecked(tab)) { > > button.setAttribute("checked", "true"); > > } else { > > button.clearAttribute("checked"); > > } > > > > Because this is XML and because people write css like toolbarbutton[checked] > > by clearAttribute, you mean removeAttribute .. right ? Yes. > > I think we should probably use toolbox.on("destroy", ....) rather than a > > window event. > > So should I get the toolbox reference using the target ? There isn't a target.toolbox, so I think you'll need to pass it in. Thanks.

New component triage. Filter on HORSE MASKS.

Fixed as per discussion and comments.

Verified as fixed on the latest Nightly and Aurora - the state of the buttons (3D View and Responsive Design mode) is now evident. Verified on Windows 7, Ubuntu 12.10 and Mac OS X 10.8.2: Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20130210 Firefox/21.0 (20130210031150) Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20130210 Firefox/20.0 (20130210042017) Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20130211 Firefox/21.0 (20130211031055) Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20130210 Firefox/20.0 (20130210042017) Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20130211 Firefox/21.0 (20130211031055) Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20130211 Firefox/20.0 (20130211042016)