763932 - [devtb] Add the right buttons to the Developer Toolbar

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Paul Rouget [:paul]]

gcli / Console / Inspector / Style Editor / Debugger / DevTools menubutton

Comment 1

[Paul Rouget [:paul]]

Attachment: remove responsive mode - v0.1

Comment 2

[Paul Rouget [:paul]]

Attachment: add StyleEditor - implement StyleEditor Manager - v0.1

Comment 3

[Paul Rouget [:paul]]

To integrate the style editor, I needed to implement a StyleEditor Manager that keeps track of opened editor.

The button is checked only of the page is associated with a StyleEditor instance.

Comment 4

[Paul Rouget [:paul]]

I won't work on the devtools menu in this bug. See bug 765564.

Comment 5

[Paul Rouget [:paul]]

Attachment: style editor - v1

Comment 6

[Paul Rouget [:paul]]

Attachment: remove responsive mode - v1

Comment 7

[Joe Walker [:jwalker]]

Comment on attachment 633885 [details] [diff] [review]
style editor - v1

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

Please could you take a look at GcliCommands.jsm:273 - I suspect the implementation of the 'edit foo' might do with a tweak.
I have looked at StyleEditorManager and it looks OK to me, but I have made the assumption that Cedricv will be taking a deeper look.

Comment 8

[Joe Walker [:jwalker]]

Actually, second thoughts - we've had a ton of trouble with button state.
shared/test/browser_toolbar_basic.js does some button state testing. Perhaps we should add something in there to check on this?

Comment 9

[Cedric Vivier [:cedricv]]

Comment on attachment 633886 [details] [diff] [review]
remove responsive mode - v1

r+, works great but I'm not browser/ peer and those two patches only touch browser/.

Comment 11

[Paul Rouget [:paul]]

Sorry, I messed up with the review flag. I wanted Cedric to look at the style editor patch and Joe to look at the Responsive Mode patch.

I will make sure the command is ok, add some test as suggested by Joe, then try tests.
I will then re-ask for reviews.

Comment 12

[Paul Rouget [:paul]]

Attachment: v2

Comment 13

[Paul Rouget [:paul]]

Comment on attachment 635312 [details] [diff] [review]
v2

So I have updated the tests and fixed a missing "return" in the initial patch.
The gcli command will still work.

Cedric already looked at the Style Editor code.

Comment 14

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=f38673a3531b

Comment 15

[Joe Walker [:jwalker]]

Comment on attachment 635312 [details] [diff] [review]
v2

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

::: browser/devtools/styleeditor/styleeditor.xul
@@ +128,5 @@
>  let chrome = new StyleEditorChrome(chromeRoot, contentWindow);
>  chrome.selectStyleSheet(args.selectedStyleSheet, args.line, args.col);
>  window.styleEditorChrome = chrome;
> +]]>
> +</xul:script>

Did you mean to change this? It's the only change in this file. I'm not sure it's actually a problem or anything.

Comment 16

[Paul Rouget [:paul]]

Attachment: v2.1

thanks Joe. fixed

Comment 17

[Paul Rouget [:paul]]

Comment on attachment 635759 [details] [diff] [review]
v2.1

I need a browser review for this.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 635759 [details] [diff] [review]
v2.1

>+  toggle: function SE_toggle()
>+  {
>+    let contentWindow = gBrowser.selectedBrowser.contentWindow;
>+    this.StyleEditorManager.toggleEditor(contentWindow);

this.StyleEditorManager.toggleEditor(gBrowser.contentWindow);

>+  refreshCommand: function SEM_refreshCommand() {
>+    let contentWindow = this.chromeWindow.gBrowser.selectedBrowser.contentWindow;

let contentWindow = this.chromeWindow.gBrowser.contentWindow;

>+  listenToTabs: function SEM_listenToTabs() {
>+    let win = this.chromeWindow;
>+    let tabs = win.gBrowser.tabContainer;
>+
>+    let bound_refreshCommand = this.refreshCommand.bind(this);
>+    tabs.addEventListener("TabSelect", bound_refreshCommand, true);
>+
>+    win.addEventListener("DOMWindowClose", function onClose(aEvent) {
>+      tabs.removeEventListener("TabSelect", bound_refreshCommand, true);
>+      win.removeEventListener("DOMWindowClose", onClose, false);
>+    }, false);
>+  },

s/DOMWindowClose/unload/

Comment 19

[Paul Rouget [:paul]]

Attachment: v2.2

Addressed Dao's comments

Comment 20

[Paul Rouget [:paul]]

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

Comment 21

[Paul Rouget [:paul]]

Backed out: https://hg.mozilla.org/integration/fx-team/rev/cf6d687f0784

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

Comment 22

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

The two changesets are in m-c, too:
https://hg.mozilla.org/mozilla-central/rev/e51851dba7a2
https://hg.mozilla.org/mozilla-central/rev/cf6d687f0784

Comment 23

[Paul Rouget [:paul]]

Attachment: [in-fx-team] v2.3

Comment 24

[Paul Rouget [:paul]]

The Map was holding a refence to the editors and the contentWindows. Fixed in attachment 636382 [details] [diff] [review]

And it's green: https://tbpl.mozilla.org/?tree=Try&rev=8d0a5ef977ac

Comment 25

[Paul Rouget [:paul]]

(I just use an weakmap instead. I love weakmaps)

Comment 26

[Mihai Sucan [:msucan]]

Comment on attachment 636382 [details] [diff] [review]
[in-fx-team] v2.3

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

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

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