Closed Bug 817551 Opened 11 years ago Closed 11 years ago

The window controller (for undo/redo) doesn't work when Inspector is undocked

Categories

(DevTools :: Inspector, defect, P1)

x86
All
defect

Tracking

(firefox20 fixed)

RESOLVED FIXED
Firefox 21
Tracking Status
firefox20 --- fixed

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Priority: P1 → P2
Raising to P1 due to pseudo-dependency from Bug 820343.
Priority: P2 → P1
working on this
Assignee: nobody → paul
Attached patch v1 (obsolete) — Splinter Review
Optimizer, can I ask you for an unofficial review?
Attachment #699230 - Flags: review?(scrapmachines)
Comment on attachment 699230 [details] [diff] [review]
v1

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

Works fine now.

::: browser/devtools/framework/toolbox.xul
@@ +15,5 @@
>  
>  <window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +
> +  <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
> +

Nice.. I was thinking that instead of Cu.import-ing gDevTools.jsm in the patch for Bug 820735, I should also do the same ?
Attachment #699230 - Flags: review?(scrapmachines) → review+
Attachment #699230 - Flags: review?(dcamp)
Comment on attachment 699230 [details] [diff] [review]
v1

This looks ok to me, but I'm not totally confident reviewing it.  Tagging gavin, but I bet there are other people that could confidently review it...
Attachment #699230 - Flags: review?(dcamp) → review?(gavin.sharp)
This bug's a little light on "the problem was _____" and "this patch fixes it because ____" :)

Rather than adding to toolbox.dtd, you should probably just include editMenuOverlay.dtd? Or maybe that already happens via the editMenuOverlay.xul overlay and you can just omit those changes - not sure whether that works with overlays (though if it didn't work, the existing overlay would probably have been broken I guess).
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> This bug's a little light on "the problem was _____" and "this patch fixes
> it because ____" :)

Context: the markup view in the inspector (the DOM tree) supports undo/redo shortcuts. When the devtools are undocked, these don't work anymore. This is because of the absence of the the undo/redo keys and commands.

What I did in this patch is to move the undo/redo keys and commands inside the toolbox window. Docked or undocked, we use the same keys and commands now.

> Rather than adding to toolbox.dtd, you should probably just include
> editMenuOverlay.dtd? Or maybe that already happens via the
> editMenuOverlay.xul overlay and you can just omit those changes - not sure
> whether that works with overlays (though if it didn't work, the existing
> overlay would probably have been broken I guess).

Let me try that.
Attached patch patch v2Splinter Review
Attachment #699230 - Attachment is obsolete: true
Attachment #699230 - Flags: review?(gavin.sharp)
Attachment #699614 - Flags: review?(gavin.sharp)
Comment on attachment 699614 [details] [diff] [review]
patch v2

I don't understand the source-editor-overlay.xul change, but this seems fine.
Attachment #699614 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #10)
> Comment on attachment 699614 [details] [diff] [review]
> patch v2
> 
> I don't understand the source-editor-overlay.xul change, but this seems fine.

It was no-op in the first place.
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/96b24952d65d

This will need an uplift in Aurora.
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/96b24952d65d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
Comment on attachment 699614 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature (toolbox)
User impact if declined: can't undo in the devtools window (but we could before)
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low (not much code)
String or UUID changes made by this patch: no
Attachment #699614 - Flags: approval-mozilla-aurora?
Attachment #699614 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [land-in-aurora]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.