Last Comment Bug 763932 - [devtb] Add the right buttons to the Developer Toolbar
: [devtb] Add the right buttons to the Developer Toolbar
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Firefox 16
Assigned To: Paul Rouget [:paul]
:
:
Mentors:
: 762921 (view as bug list)
Depends on: 764625
Blocks: 745773 764555 devtools.jsm
  Show dependency treegraph
 
Reported: 2012-06-12 07:50 PDT by Paul Rouget [:paul]
Modified: 2012-06-26 11:05 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove responsive mode - v0.1 (2.50 KB, patch)
2012-06-15 11:33 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
add StyleEditor - implement StyleEditor Manager - v0.1 (12.98 KB, patch)
2012-06-15 11:34 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
style editor - v1 (14.75 KB, patch)
2012-06-17 04:34 PDT, Paul Rouget [:paul]
jwalker: review+
Details | Diff | Splinter Review
remove responsive mode - v1 (2.50 KB, patch)
2012-06-17 04:35 PDT, Paul Rouget [:paul]
cedricv: review+
Details | Diff | Splinter Review
v2 (21.44 KB, patch)
2012-06-21 07:59 PDT, Paul Rouget [:paul]
jwalker: review+
Details | Diff | Splinter Review
v2.1 (20.71 KB, patch)
2012-06-22 08:05 PDT, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Splinter Review
v2.2 (20.62 KB, patch)
2012-06-25 02:37 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
[in-fx-team] v2.3 (20.63 KB, patch)
2012-06-25 10:37 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2012-06-12 07:50:04 PDT
gcli / Console / Inspector / Style Editor / Debugger / DevTools menubutton
Comment 1 Paul Rouget [:paul] 2012-06-15 11:33:25 PDT
Created attachment 633598 [details] [diff] [review]
remove responsive mode - v0.1
Comment 2 Paul Rouget [:paul] 2012-06-15 11:34:26 PDT
Created attachment 633599 [details] [diff] [review]
add StyleEditor - implement StyleEditor Manager - v0.1
Comment 3 Paul Rouget [:paul] 2012-06-15 11:36:41 PDT
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] 2012-06-17 04:29:21 PDT
I won't work on the devtools menu in this bug. See bug 765564.
Comment 5 Paul Rouget [:paul] 2012-06-17 04:34:12 PDT
Created attachment 633885 [details] [diff] [review]
style editor - v1
Comment 6 Paul Rouget [:paul] 2012-06-17 04:35:32 PDT
Created attachment 633886 [details] [diff] [review]
remove responsive mode - v1
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-06-17 04:53:43 PDT
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] (needinfo me or ping on irc) 2012-06-17 04:55:54 PDT
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] 2012-06-18 21:31:13 PDT
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 10 Paul Rouget [:paul] 2012-06-19 08:57:21 PDT
*** Bug 762921 has been marked as a duplicate of this bug. ***
Comment 11 Paul Rouget [:paul] 2012-06-20 03:04:58 PDT
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] 2012-06-21 07:59:01 PDT
Created attachment 635312 [details] [diff] [review]
v2
Comment 13 Paul Rouget [:paul] 2012-06-21 08:03:26 PDT
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] 2012-06-21 08:06:54 PDT
https://tbpl.mozilla.org/?tree=Try&rev=f38673a3531b
Comment 15 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-06-22 07:11:04 PDT
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] 2012-06-22 08:05:32 PDT
Created attachment 635759 [details] [diff] [review]
v2.1

thanks Joe. fixed
Comment 17 Paul Rouget [:paul] 2012-06-22 09:09:49 PDT
Comment on attachment 635759 [details] [diff] [review]
v2.1

I need a browser review for this.
Comment 18 Dão Gottwald [:dao] 2012-06-22 09:19:54 PDT
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] 2012-06-25 02:37:45 PDT
Created attachment 636257 [details] [diff] [review]
v2.2

Addressed Dao's comments
Comment 22 Panos Astithas [:past] 2012-06-25 07:12:51 PDT
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] 2012-06-25 10:37:25 PDT
Created attachment 636382 [details] [diff] [review]
[in-fx-team] v2.3
Comment 24 Paul Rouget [:paul] 2012-06-25 14:04:47 PDT
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] 2012-06-25 14:05:16 PDT
(I just use an weakmap instead. I love weakmaps)
Comment 26 Mihai Sucan [:msucan] 2012-06-26 03:48:27 PDT
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] 2012-06-26 11:05:37 PDT
https://hg.mozilla.org/mozilla-central/rev/d9e78d7def12

Note You need to log in before you can comment on or make changes to this bug.