Closed Bug 763932 Opened 12 years ago Closed 12 years ago

[devtb] Add the right buttons to the Developer Toolbar

Categories

(DevTools :: General, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 16

People

(Reporter: paul, Assigned: paul)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file, 7 obsolete files)

gcli / Console / Inspector / Style Editor / Debugger / DevTools menubutton
Depends on: 754481
Blocks: 745773
Summary: Add the right buttons to the Developer Toolbar → [devtb] Add the right buttons to the Developer Toolbar
Assignee: nobody → paul
Attached patch remove responsive mode - v0.1 (obsolete) — Splinter Review
Attachment #633599 - Flags: feedback?(cedricv)
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.
Depends on: 764625
I won't work on the devtools menu in this bug. See bug 765564.
No longer depends on: 754481
Attached patch style editor - v1 (obsolete) — Splinter Review
Attachment #633599 - Attachment is obsolete: true
Attachment #633599 - Flags: feedback?(cedricv)
Attached patch remove responsive mode - v1 (obsolete) — Splinter Review
Attachment #633598 - Attachment is obsolete: true
Attachment #633885 - Flags: review?(jwalker)
Attachment #633886 - Flags: review?(cedricv)
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.
Attachment #633885 - Flags: review?(jwalker) → review+
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?
Blocks: devtools.jsm
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/.
Attachment #633886 - Flags: review?(cedricv) → review+
Status: NEW → ASSIGNED
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.
Attached patch v2 (obsolete) — Splinter Review
Attachment #633885 - Attachment is obsolete: true
Attachment #633886 - Attachment is obsolete: true
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.
Attachment #635312 - Flags: review?(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.
Attachment #635312 - Flags: review?(jwalker) → review+
Attached patch v2.1 (obsolete) — Splinter Review
thanks Joe. fixed
Attachment #635312 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
Whiteboard: [land-in-fx-team]
Comment on attachment 635759 [details] [diff] [review]
v2.1

I need a browser review for this.
Attachment #635759 - Flags: review?(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/
Attachment #635759 - Flags: review?(dao) → review+
Attached patch v2.2 (obsolete) — Splinter Review
Addressed Dao's comments
Attachment #635759 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
Whiteboard: [land-in-fx-team]
Blocks: 764555
Attachment #636257 - Attachment is obsolete: true
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
(I just use an weakmap instead. I love weakmaps)
Whiteboard: [land-in-fx-team]
Comment on attachment 636382 [details] [diff] [review]
[in-fx-team] v2.3

https://hg.mozilla.org/integration/fx-team/rev/d9e78d7def12
Attachment #636382 - Attachment description: v2.3 → [in-fx-team] v2.3
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d9e78d7def12
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.