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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Comment 1

[Paul Rouget [:paul]]

The problem comes from this change: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/InspectorPanel.jsm#398

Comment 2

[Joe Walker [:jwalker]]

Raising to P1 due to pseudo-dependency from Bug 820343.

Comment 3

[Paul Rouget [:paul]]

working on this

Comment 4

[Paul Rouget [:paul]]

Attachment: v1

Optimizer, can I ask you for an unofficial review?

Comment 5

[Girish Sharma [:Optimizer]]

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 ?

Comment 6

[Dave Camp (: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...

Comment 7

[:Gavin Sharp [email: [e-mail]]]

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).

Comment 8

[Paul Rouget [:paul]]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 9

[Paul Rouget [:paul]]

Attachment: patch v2

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 699614 [details] [diff] [review]
patch v2

I don't understand the source-editor-overlay.xul change, but this seems fine.

Comment 11

[Paul Rouget [:paul]]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 12

[Paul Rouget [:paul]]

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

This will need an uplift in Aurora.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/96b24952d65d

Comment 14

[Paul Rouget [:paul]]

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

Comment 15

[Paul Rouget [:paul]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e38bafecbe41