The default bug view has changed. See this FAQ.

[devtb] Add the right buttons to the Developer Toolbar

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

Trunk
Firefox 16
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

5 years ago
gcli / Console / Inspector / Style Editor / Debugger / DevTools menubutton
(Assignee)

Updated

5 years ago
Depends on: 754481
Blocks: 745773
Summary: Add the right buttons to the Developer Toolbar → [devtb] Add the right buttons to the Developer Toolbar
(Assignee)

Updated

5 years ago
Assignee: nobody → paul
(Assignee)

Comment 1

5 years ago
Created attachment 633598 [details] [diff] [review]
remove responsive mode - v0.1
(Assignee)

Comment 2

5 years ago
Created attachment 633599 [details] [diff] [review]
add StyleEditor - implement StyleEditor Manager - v0.1
(Assignee)

Updated

5 years ago
Attachment #633599 - Flags: feedback?(cedricv)
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 764625
(Assignee)

Comment 4

5 years ago
I won't work on the devtools menu in this bug. See bug 765564.
No longer depends on: 754481
(Assignee)

Comment 5

5 years ago
Created attachment 633885 [details] [diff] [review]
style editor - v1
(Assignee)

Updated

5 years ago
Attachment #633599 - Attachment is obsolete: true
Attachment #633599 - Flags: feedback?(cedricv)
(Assignee)

Comment 6

5 years ago
Created attachment 633886 [details] [diff] [review]
remove responsive mode - v1
(Assignee)

Updated

5 years ago
Attachment #633598 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #633885 - Flags: review?(jwalker)
(Assignee)

Updated

5 years ago
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?
(Assignee)

Updated

5 years ago
Blocks: 765573
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
(Assignee)

Updated

5 years ago
Duplicate of this bug: 762921
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Comment 12

5 years ago
Created attachment 635312 [details] [diff] [review]
v2
(Assignee)

Updated

5 years ago
Attachment #633885 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #633886 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
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)
(Assignee)

Comment 14

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=f38673a3531b
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+
(Assignee)

Comment 16

5 years ago
Created attachment 635759 [details] [diff] [review]
v2.1

thanks Joe. fixed
(Assignee)

Updated

5 years ago
Attachment #635312 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
(Assignee)

Comment 17

5 years ago
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+
(Assignee)

Comment 19

5 years ago
Created attachment 636257 [details] [diff] [review]
v2.2

Addressed Dao's comments
(Assignee)

Updated

5 years ago
Attachment #635759 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/e51851dba7a2
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 21

5 years ago
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
...
Whiteboard: [fixed-in-fx-team]
(Assignee)

Updated

5 years ago
Blocks: 764555
The two changesets are in m-c, too:
https://hg.mozilla.org/mozilla-central/rev/e51851dba7a2
https://hg.mozilla.org/mozilla-central/rev/cf6d687f0784
(Assignee)

Comment 23

5 years ago
Created attachment 636382 [details] [diff] [review]
[in-fx-team] v2.3
(Assignee)

Updated

5 years ago
Attachment #636257 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
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
(Assignee)

Comment 25

5 years ago
(I just use an weakmap instead. I love weakmaps)
(Assignee)

Updated

5 years ago
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

Updated

5 years ago
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d9e78d7def12
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
You need to log in before you can comment on or make changes to this bug.