944836 - No longer possible to move the Firefox window when it happens to be busy

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I'm not quite sure where to file this, actually.  I ran into it because the Firefox window would randomly not move when I tried to move it, but here are quite reliable steps to reproduce:

1) Start Firefox.
2) Look up its PID using ps.
3) kill -STOP fxpid

EXPECTED RESULTS: Can still move the Firefox window.

ACTUAL RESULTS: Absolutely no response when you try to.

The regression range, unsurprisingly, is <http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=beddd6d4bcdf&tochange=ba9ecdea3a90>: this is a regression from there not being a real titlebar anymore with Australis.  In particular, toggling the "browser.tabs.drawInTitlebar" pref makes the problem go away.

This issue pops up any time the main event loop isn't serviced quite often enough, and it's driving me pretty insane.  If nothing else, right when a Firefox window hangs because it's doing something that takes too long is when I suddenly can't move it out of my way and get on with other things.

Comment 1

[:Gijs (out for now; he/him)]

Markus, on Windows we still defer some of these mouse events etc. to the native code... do we do that on OS X and/or is that possible and/or likely to help solve this?

Comment 2

[Markus Stange [:mstange]]

We actually do use a setup for this that's pretty similar to what we do on Windows. The problem is that even the native handling happens on the main thread, so when that's blocked, we're screwed either way.
In order to push this stuff off the main thread, we'd need something like a persistent draggable region stored somewhere. But that's hard to get.

Boris, do you usually drag the space above the tabs? Or draggable parts elsewhere in the titlebar/toolbar region?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Space above the tabs; there's not much else that's draggable there usually.

Comment 4

[Justin Dolske [:Dolske]]

Comment 2 kind of makes this sound like this is an unfortunate consequence of tabs-in-titlebar. Are we likely to find a fix/workaround for this? Or is this a CANTFIX?

Comment 5

[Benjamin Kerensa [:bkerensa]]

We will go ahead and track this.

Comment 6

[Justin Dolske [:Dolske]]

Marcus: is there a practical fix here, or is it a wontfix?

Comment 7

[Markus Stange [:mstange]]

Hard to say. If we can somehow communicate from the browser to the widget that the top 9 pixels will always be draggable, then widget could spawn a secondary thread that allows dragging in that strip while the main thread is blocked. But that wouldn't help us in the scenario where gdb is attached and all threads are blocked.

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Markus Stange [:mstange] from comment #7)
> Hard to say. If we can somehow communicate from the browser to the widget
> that the top 9 pixels will always be draggable, then widget could spawn a
> secondary thread that allows dragging in that strip while the main thread is
> blocked. But that wouldn't help us in the scenario where gdb is attached and
> all threads are blocked.

Markus, do you have cycles to pick this up in the next couple of weeks so we don't ship this? Front-end-wise this isn't really within our ability range... (even if we'll add UI for the pref mentioned in comment #0, we would obviously prefer if this got fixed and it wasn't necessary to toggle the titlebar on just to have usable DnD if Fx was busy)

Comment 9

[Markus Stange [:mstange]]

Yes.

Comment 10

[Markus Stange [:mstange]]

Attachment: part 1: add dragmargin attribute and nsIWidget::SetDragMargin API

For the main browser window, we'd set dragmargin="9,0,0,0" whenever tabs are in the titlebar.

An attribute isn't the nicest API for this... I'd have liked something like SetDraggableRegion that gets updated for example during display list building, where we'd determine the draggability of all XUL elements in the window under the same rules as currently used in http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/WindowDraggingUtils.jsm#29 , but I don't see a simple way of implementing something like that. So an attribute will need to suffice for now.

Comment 11

[Markus Stange [:mstange]]

Attachment: part 2: OS X implementation

Things work like this: As long as [window isMovableByWindowBackground] == YES, an NSView is draggable iff it and all its ancestor NSViews return YES for [view mouseDownCanMoveWindow].

That means we can't override with YES from the inside; NO always overrides all YESs. So if we want to be draggable at all times in the top, say, 9 pixels, that strip must never be covered by an NSView that can return NO for mouseDownCanMoveWindow. So we create a new child NSView that covers everything except for the margin, and that can dynamically return YES or NO depending on where the mouse is. But due to its location it never overrides draggability in the margin.

Comment 12

[Markus Stange [:mstange]]

I haven't yet written the JavaScript code that sets dragmargin="9,0,0,0" in concert with the chromemargin attribute.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8367977 [details] [diff] [review]
part 2: OS X implementation

I suspect this may cause trouble with plugins, especially in HiDPI mode.  Have you tested with plugins?

Comment 14

[Markus Stange [:mstange]]

Attachment: part 2: OS X implementation with WindowDragView

Right on. WindowDragViews of plugins never get setWindowIsMovable called, so they always return the default NO and make the window not draggable in the parts they (sometimes invisibly) cover.
I've updated the patch to call setWindowIsMovable:YES for plugin views.

What HiDPI problems do you have in mind?

Comment 15

[Steven Michaud [:smichaud] (Retired)]

> What HiDPI problems do you have in mind?

For example displaying the plugin at the wrong size or scaling.  You'd want to test both with and without HiDPI (perhaps by turning it off in FF's config settings).

Also, some plugins make assumptions about FF's internal workings -- in particular about the NSView hierarchy.  I'd strongly advise testing with all the major plugins (e.g. at least Flash, Quicktime, Java, Silverlight).

Try to think of some way to fix this bug (or work around it) that doesn't involve adding an extra NSView.  I'll do the same.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

> As long as [window isMovableByWindowBackground] == YES, an NSView is
> draggable iff it and all its ancestor NSViews return YES for [view
> mouseDownCanMoveWindow].

Do you know where this is implemented in OS code?  Once we know that,
and can look at the code in the Hopper disassembler, it'll (hopefully)
be easier to find less "violent" ways to work around it.

Comment 17

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #16)
> > As long as [window isMovableByWindowBackground] == YES, an NSView is
> > draggable iff it and all its ancestor NSViews return YES for [view
> > mouseDownCanMoveWindow].
> 
> Do you know where this is implemented in OS code?  Once we know that,
> and can look at the code in the Hopper disassembler, it'll (hopefully)
> be easier to find less "violent" ways to work around it.

I called [mWindowDragView setWindowIsMovable:YES] in a loop and took a profile. It looks like -[NSFrameView _resetDragMargins] does most of the work. It recursively calls -[NSView _regionForOpaqueDescendants:forMove:] to get the region in which mouseDownCanMoveWindow is NO (I think?) and passes the results in some way to _NSAddRectsToWindow which calls all kinds of window server communication functions, among them CGSAddButtonRegion, _CGSSetWindowRegionInline, CGSClearButtonRegion, _CGSDeleteWindowRegion and CGSAddActivationRegion.

(In reply to Steven Michaud from comment #15)
> > What HiDPI problems do you have in mind?
> 
> For example displaying the plugin at the wrong size or scaling.  You'd want
> to test both with and without HiDPI (perhaps by turning it off in FF's
> config settings).

I'll test, but I don't expect any scaling-related bugs.

> Also, some plugins make assumptions about FF's internal workings -- in
> particular about the NSView hierarchy.

Uh oh. :(
Can you point me to bugs that are evidence of this?

> I'd strongly advise testing with all
> the major plugins (e.g. at least Flash, Quicktime, Java, Silverlight).
> 
> Try to think of some way to fix this bug (or work around it) that doesn't
> involve adding an extra NSView.  I'll do the same.

Overriding -[NSView _regionForOpaqueDescendants:forMove:] might work. But we'd have to use the private CGSRegion building functions. I'd rather keep using supported methods when possible.

One thing I can definitely do is not to add the view unconditionally to every ChildView, but instead only to ChildViews that have setDragMargin or updateWindowDraggableStateOnMouseMove called on them (i.e. view's that are their window's mainChildView). Then we only modify our own views and no plugin views.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

>> Also, some plugins make assumptions about FF's internal workings -- in
>> particular about the NSView hierarchy.
>
> Uh oh. :(
> Can you point me to bugs that are evidence of this?

Not off the top of my head.

But I know I've seen it in the past.  And I *think* I've actually commented on it in a bug ... though it may be difficult to find that comment again.

> I called [mWindowDragView setWindowIsMovable:YES] in a loop and took a profile. ...

Thanks for the info.  I'll dig around myself this afternoon to see what I can find, if anything.

Comment 19

[Markus Stange [:mstange]]

Attachment: alternative part 2: OS X implementation with _regionForOpaqueDescendants override

This is definitely a bit simpler and a real alternative in my mind.

Comment 20

[Markus Stange [:mstange]]

Attachment: alternative part 2: OS X implementation with _regionForOpaqueDescendants override

fixed a typo in the unmovableRect calculation

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8367972 [details] [diff] [review]
part 1: add dragmargin attribute and nsIWidget::SetDragMargin API

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

::: xpfe/appshell/src/nsXULWindow.cpp
@@ +1399,5 @@
> +    float cssToDevice = mWindow->GetDefaultScale().scale;
> +    mWindow->SetDragMargin(nsIntMargin(margins.top * cssToDevice,
> +                                       margins.right * cssToDevice,
> +                                       margins.bottom * cssToDevice,
> +                                       margins.left * cssToDevice));

Ugh, there's a bunch of code duplication for these XUL attributes :-(.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8367972 [details] [diff] [review]
part 1: add dragmargin attribute and nsIWidget::SetDragMargin API

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

::: xpfe/appshell/src/nsXULWindow.cpp
@@ +1399,5 @@
> +    float cssToDevice = mWindow->GetDefaultScale().scale;
> +    mWindow->SetDragMargin(nsIntMargin(margins.top * cssToDevice,
> +                                       margins.right * cssToDevice,
> +                                       margins.bottom * cssToDevice,
> +                                       margins.left * cssToDevice));

There is one problem here which is that dynamic changes to the page scale factor won't work. I think that could actually be a problem when the window moves between screens with different HiDPI modes?

Comment 23

[Steven Michaud [:smichaud] (Retired)]

> As long as [window isMovableByWindowBackground] == YES, an NSView is
> draggable iff it and all its ancestor NSViews return YES for [view
> mouseDownCanMoveWindow].

This doesn't appear to be true.

As best I can tell, the crucial code is in -[NSWindow sendEvent:], and it unconditionally allows a window to be dragged if -[NSView mouseDownCanMoveWindow] returns true on NSWindow._lastLeftHit.  In this case the code doesn't even call -[NSWindow isMovableByWindowBackground].  Since our main ChildView object now covers the entire browser window, NSWindow._lastLeftHit is usually that ChildView.  But its value gets changed if you click on one of the titlebar widget buttons -- so for example if you click on the yellow minimize button it gets changed to that.  (And it gets changed back to the ChildView when you subsequently click elsewhere in the browser window -- even elsewhere on the titlebar.)

But if the Firefox process is busy, then (perhaps) a call to -[ChildView mouseDownCanMoveWindow] won't return quickly enough.  So (perhaps) it's best to have it return true unconditionally (as Markus' alternative patch does).

The reason my last paragraph is so tentative is that it's a mystery to me how a process's windows can still be moveable while the process is stopped.  I need to do more testing, with and without Markus' alternative patch.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

The dragging code in -[NSWindow sendEvent:] calls _NSDragWindowRelativeToMouseDown().

Comment 25

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #23)
> > As long as [window isMovableByWindowBackground] == YES, an NSView is
> > draggable iff it and all its ancestor NSViews return YES for [view
> > mouseDownCanMoveWindow].
> 
> This doesn't appear to be true.
> 
> As best I can tell, the crucial code is in -[NSWindow sendEvent:], and it
> unconditionally allows a window to be dragged if -[NSView
> mouseDownCanMoveWindow] returns true on NSWindow._lastLeftHit.

This is completely new to me. Very interesting!
We should find all callers of -[NSView mouseDownCanMoveWindow] somehow.

> But if the Firefox process is busy, then (perhaps) a call to -[ChildView
> mouseDownCanMoveWindow] won't return quickly enough.

If it's busy, we don't even get to -[NSWindow sendEvent:] in time (because the main thread is busy elswhere), so there's nothing that calls -[ChildView mouseDownCanMoveWindow]. Right?

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Thinking a bit more about this, there must be two code paths wrt window dragging -- one that's used when the process is running, and another that's used when the process is hung/stopped.  What I said in comment #23 presumably only concerns what happens when the process is running and not hung.

I have a hunch about what happens when the process is stopped/hung.  I'll try it out and report back.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

I tried out my hunch and it worked as expected:  There are two places in current code where -[NSWindow setMovableByWindowBackground:] is called.  If you change them to always turn on that value, the window is draggable from everywhere *whether or not the process is stopped*.

What I think that means is that, for the "second code path" (unlike the first), the value set by -[NSWindow setMovableByWindowBackground:] *on the window server* is the only one that matters.

Comment 28

[Steven Michaud [:smichaud] (Retired)]

> What I think that means is that, for the "second code path" (unlike
> the first), the value set by -[NSWindow
> setMovableByWindowBackground:] *on the window server* is the only
> one that matters.

Actually there must be more to it than that.  When FF 26 is
hung/stopped, its browser window is movable by the titlebar but not by
the rest of the window.  But since FF code can't be called when it's
hung, something more than just the return value of -[NSWindow
setMovableByWindowBackground:] must be "registered" at the window
server -- maybe the size of the window's "draggable region"?

I'll dig further into this tomorrow.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

There's a _resetDragMargins method for NSThemeFrame and friends.  I'll bet it's important.  It (indirectly) calls CGS... methods, which I think talk directly to the window server.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

_NSDragWindowRelativeToMouseDown() isn't called, in Firefox or Safari, even when the process(es) aren't hung!  And when I break in gdb in the middle of a drag, I don't see anything interesting in the stack.

So I think window dragging must be implemented entirely on the window server, and that we can only control it indirectly.

I still think -[NSWindow setMovableByWindowBackground:] and -[NSThemeFrame _resetDragMargins] are important, though.  We need to look further into how they work.

Comment 31

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #30)
> _NSDragWindowRelativeToMouseDown() isn't called, in Firefox or Safari, even
> when the process(es) aren't hung!  And when I break in gdb in the middle of
> a drag, I don't see anything interesting in the stack.

OK, this matches my findings and feels more familiar.

> So I think window dragging must be implemented entirely on the window
> server,

Well, yes, that was my initial assumption.

> and that we can only control it indirectly.

Yes, by making _resetDragMargins send the right draggable region to the window server, by returning the right values from mouseDownCanMoveWindow or _regionForOpaqueDescendants.

(In reply to Steven Michaud from comment #29)
> There's a _resetDragMargins method for NSThemeFrame and friends.  I'll bet
> it's important.  It (indirectly) calls CGS... methods, which I think talk
> directly to the window server.

I concur. And we cause _resetDragMargins to be called (and a new draggable region to be calculated and communicated to the window server) by changing the value of [window movableByWindowBackground], or by changing the NSView geometry.

In comment #11, when I said:

> Things work like this: As long as [window isMovableByWindowBackground] ==
> YES, an NSView is draggable iff it and all its ancestor NSViews return YES
> for [view mouseDownCanMoveWindow].

I meant "When the draggable region that will be communicated to the window server is assembled, a view is considered draggable iff ...".

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8368061 [details] [diff] [review]
alternative part 2: OS X implementation with _regionForOpaqueDescendants override

Now that I understand the background (how the OS handles window dragging), I like this quite well.  But as best I can tell mWindowIsMovable no longer serves any purpose.  Our windows are *always* movable/draggable -- the only thing that might change is the size of the "draggable region".

So we should be able to get rid of both mWindowIsMovable and -[ChildView updateWindowDraggableStateOnMouseMove:].

The only remaining thing that might cause trouble is the timing of Gecko's calls to nsIWidget::SetDragMargin().  Ideally this should be called once, when a window is created.  And if the drag margin can change dynamically (can it?, should it?), it should be called again only when it actually does change.

If we let this be called dynamically and often (say if we link it to mouse move events), it'd be easy for the drag margin to be set inappropriately when FF "hangs" (temporarily) and we can no longer handle mouse moved events.

The key constraint here (and with any bug concerning window dragging) is that, because window dragging is outside of our direct control, we can't keep fiddling with its state from moment to moment.  We need to inform the window server of a window's drag region as soon as we know what it will be, and as soon as it changes, but no more often than that.  That minimizes the chance that FF will "hang" with the drag region set incorrectly.

Comment 33

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #32)
> Comment on attachment 8368061 [details] [diff] [review]
> alternative part 2: OS X implementation with _regionForOpaqueDescendants
> override
> 
> Now that I understand the background (how the OS handles window dragging), I
> like this quite well.  But as best I can tell mWindowIsMovable no longer
> serves any purpose.  Our windows are *always* movable/draggable -- the only
> thing that might change is the size of the "draggable region".

Yes, and the region depends on the value of mWindowIsMovable.

Even with the drag margins, the margins aren't the only part of the window that should be draggable. The empty parts of the tab bar, and the parts of the toolbar around the toolbar buttons, should be draggable as well. But we can't get this region ahead-of-time. nsIWidget::SetDragMargin only tells us about the margin. The other parts we must sample on every mouse move event. Whenever the mouse is over a draggable pixel, we tell the window server that the whole window is draggable.

> The only remaining thing that might cause trouble is the timing of Gecko's
> calls to nsIWidget::SetDragMargin().  Ideally this should be called once,
> when a window is created.  And if the drag margin can change dynamically
> (can it?, should it?), it should be called again only when it actually does
> change.

The part 1 patch implements this - nsIWidget::SetDragMargin is called whenever the dragmargin XUL attribute changes.

> If we let this be called dynamically and often (say if we link it to mouse
> move events), it'd be easy for the drag margin to be set inappropriately
> when FF "hangs" (temporarily) and we can no longer handle mouse moved events.

Yes, that is a problem. When Firefox starts hanging while the mouse is over the content area, Firefox toolbars are not draggable during the hang (but the drag margins now are!). When Firefox starts hanging while the mouse is over a toolbar, even the content area will be draggable for the duration of the hang.

I think we can live with that problem. It's not perfect, but we've had that behavior for about a year now, I think. And getting the complete draggable region ahead-of-time looks really tricky to implement, at least to me.

Comment 34

[Markus Stange [:mstange]]

To sum up, the approach in this bug is a compromise. When Firefox starts hanging while the mouse was over a non-draggable part of the window, Firefox couldn't be dragged at all (ever since the title bar went away with Australis). And with the patches in this bug (plus a JS patch that will actually set the dragmargin attribute), at least the drag margins stay draggable.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8368061 [details] [diff] [review]
alternative part 2: OS X implementation with _regionForOpaqueDescendants override

I forgot to mention that -[NSWindow setMovableByWindowBackground:] calls -[NSFrameView _resetDragMargins], which in turn calls -[NSView _regionForOpaqueDescendants:forMove:].  So all we need to do to inform the window server of the drag margin's current state is:

1) Make sure ChildView.mDragMargin is set up correctly
2) Call -[NSWindow setMovableByWindowBackground:NO] and then -[NSWindow setMovableByWindowBackground:YES].

The change of state should ensure that -[NSWindow setMovableByWindowBackground:] actually calls -[NSFrameView _resetDragMargins].

I also forgot to ask about plugin ChildViews.  Your current patch seems to ignore them.  But even though a plugin will never overlap a window's draggable region, a plugin's ChildView object might.  However I'm not sure it's worth our trouble to worry about this.

Comment 36

[Steven Michaud [:smichaud] (Retired)]

>> Things work like this: As long as [window
>> isMovableByWindowBackground] == YES, an NSView is draggable iff it
>> and all its ancestor NSViews return YES for [view
>> mouseDownCanMoveWindow].
>
> I meant "When the draggable region that will be communicated to the
> window server is assembled, a view is considered draggable iff ...".

This is more correct than I first realized, but it's still slightly
off.  Looking at the assembly code for -[NSView
_regionForOpaqueDescendants:forMove:], I find that it returns the
union of the "opaque regions" of all its opaque ancestors.  What I
call "opaque region" is the return value of -[NSView _opaqueRect],
which by default (for NSViews that return true for isOpaque) is the
value of -[NSView visibleRect].

-[NSFrameView _resetDragMargins] subtracts this region from its own
draggable region.

Comment 37

[Steven Michaud [:smichaud] (Retired)]

opaque ancestors -> opaque descendants

Comment 38

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8368061 [details] [diff] [review]
alternative part 2: OS X implementation with _regionForOpaqueDescendants override

(In reply to comment #33)

> When Firefox starts hanging while the mouse is over the content
> area, Firefox toolbars are not draggable during the hang (but the
> drag margins now are!).

I'd have guessed that when FF starts hanging while the mouse is over
the content area, *no* part of the window is draggable.

But I misread your patch!

unmovableRect = NSZeroRect;

I read this exactly the wrong way, and took it to mean that the entire
window is unmovable.

You should add a comment to _regionForOpaqueDescendants: that explains
what you're up to.  But otherwise I'm now happy with the patch as it
stands.

You should also get rid of this: 

+@class WindowDragView;

Comment 39

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8368061 [details] [diff] [review]
alternative part 2: OS X implementation with _regionForOpaqueDescendants override

One thing I forgot to mention:  There's code in -[NSView _drawRect...] (an undocumented method called from -[NSView drawRect:]) that calls -[NSView _regionForOpaqueDescendants:forMove:].

I know we no longer use drawRect: for actual drawing.  But it's conceivable that this fact will cause us trouble down the road, and we should look out for it.

If it *does* cause trouble, we should be able to work around it by making sure that -[ChildView _regionForOpaqueDescendants:forMove:] only makes changes when called from -[BaseWindow/ToolbarWindow setMovableByWindowBackground:].

Comment 40

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #39)
> One thing I forgot to mention:  There's code in -[NSView _drawRect...] (an
> undocumented method called from -[NSView drawRect:]) that calls -[NSView
> _regionForOpaqueDescendants:forMove:].

Do you know whether it calls it with forMove: set to YES or NO? The override in the patch only goes into effect when forMove is YES.

Comment 41

[Steven Michaud [:smichaud] (Retired)]

> -[NSView _drawRect...]

-[NSView _drawRect:(NSRect)rect clip:(BOOL)flag];

> Do you know whether it calls it with forMove: set to YES or NO?

As best I can tell (from looking at the assembly code) it gets called with forMove: set to NO.  So in that case we're safe.

Comment 42

[Markus Stange [:mstange]]

Attachment: part 2, with comments

Comment 43

[Markus Stange [:mstange]]

Attachment: part 2, only calling [self updateWindowDraggableState] when mWindowIsMovable changes

Interdiff to the last version:

diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
--- a/widget/cocoa/nsChildView.mm
+++ b/widget/cocoa/nsChildView.mm
@@ -4685,19 +4685,21 @@ NSEvent* gLastDragMouseDownEvent = nil;
   if (!theEvent || !mGeckoChild) {
     return;
   }
 
   // We assume later on that sending a hit test event won't cause widget destruction.
   WidgetMouseEvent hitTestEvent(true, NS_MOUSE_MOZHITTEST, mGeckoChild,
                                 WidgetMouseEvent::eReal);
   [self convertCocoaMouseEvent:theEvent toGeckoEvent:&hitTestEvent];
-  mWindowIsMovable = mGeckoChild->DispatchWindowEvent(hitTestEvent);
-
-  [self updateWindowDraggableState];
+  BOOL movable = mGeckoChild->DispatchWindowEvent(hitTestEvent);
+  if (movable != mWindowIsMovable) {
+    mWindowIsMovable = movable;
+    [self updateWindowDraggableState];
+  }
 }
 
 - (void)updateWindowDraggableState
 {
   // Trigger update to the window server.
   [[self window] setMovableByWindowBackground:NO];
   [[self window] setMovableByWindowBackground:YES];
 }

Comment 44

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8369491 [details] [diff] [review]
part 2, only calling [self updateWindowDraggableState] when mWindowIsMovable changes

Looks fine to me.  And thanks for the comment.

Comment 45

[Markus Stange [:mstange]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> Comment on attachment 8367972 [details] [diff] [review]
> part 1: add dragmargin attribute and nsIWidget::SetDragMargin API
> 
> Review of attachment 8367972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpfe/appshell/src/nsXULWindow.cpp
> @@ +1399,5 @@
> > +    float cssToDevice = mWindow->GetDefaultScale().scale;
> > +    mWindow->SetDragMargin(nsIntMargin(margins.top * cssToDevice,
> > +                                       margins.right * cssToDevice,
> > +                                       margins.bottom * cssToDevice,
> > +                                       margins.left * cssToDevice));
> 
> There is one problem here which is that dynamic changes to the page scale
> factor won't work. I think that could actually be a problem when the window
> moves between screens with different HiDPI modes?

True enough. It's not a problem with the OS X implementation because the widget stores the margin internally as screen points, which always match CSS pixels currently, but the need to do this is not obvious from the nsIWidget API.
Do you know of a good place that's notified when the CSS-to-device-pixel ratio changes, and that's not that far away from nsXULWindow? I don't really want to pollute nsPresContext::AppUnitsPerDevPixelChanged() with attribute parsing...
Alternatively, we could just say that the attribute value is in screen points, remove all conversions and just pipe the value as-is through nsIWidget::SetDragMargin. Would you be happy with that?

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes to making the attribute value be in screen points.

Comment 47

[Markus Stange [:mstange]]

Attachment: part 1, with margin in screen points

Comment 48

[Markus Stange [:mstange]]

Attachment: part 2, without the device pixel -> screen point scaling

Comment 49

[Markus Stange [:mstange]]

Hmm, looks like there is still a problem with part 2. In windows where we don't draw in the titlebar, the titlebar is no longer draggable as soon as I've moved a toolbar inside the window.

Comment 50

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #49)
> as soon as I've moved a toolbar inside the window.

That should have been:
as soon as I've moved the mouse over a toolbar inside the window.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8370209 [details] [diff] [review]
part 1, with margin in screen points

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

::: widget/nsIWidget.h
@@ +1000,5 @@
>      /**
> +     * Set the drag margins on this window. Currently only supported on Mac.
> +     * This will make the window draggable in the supplied margins even while
> +     * the process is hung.
> +     * aMargin is in screen points and relative to the window's client area.

"screen coordinates"

@@ +1002,5 @@
> +     * This will make the window draggable in the supplied margins even while
> +     * the process is hung.
> +     * aMargin is in screen points and relative to the window's client area.
> +     */
> +    virtual void SetDragMargin(const nsIntMargin& aMargin) {}

The other APIs that take screen coordinates take doubles. I think it's OK to not use that here though.

Comment 52

[Markus Stange [:mstange]]

Attachment: part 1, fixed comment

Comment 53

[Markus Stange [:mstange]]

Attachment: part 2, also works for windows with titlebars

Last revision, I hope. The problem was that we returned the region relative to our view's bounds, but it's actually interpreted relative to the window. So when we were marking our view as non-draggable we were also making the titlebar non-draggable (and instead leaving a 22px high strip at the bottom of the window draggable).
The only changes compared to the previous patch are at the end of -[ChildView _regionForOpaqueDescendants:forMove:].

Comment 54

[Markus Stange [:mstange]]

Attachment: part 3: Set the dragmargin attribute at appropriate times

Not sure if I caught all cases, but this seems to work, with one exception: When the titlebar is visible and a lightweight theme is applied, the titlebar is not draggable. The case where I set dragmargin="22,0,0,0" doesn't seem to be hit. That's not a regression compared to pre-Australis, but it is a regression compared to pre-Firefox 19 (from bug 647216). Mike, can you think of any quick fix for that case?

Comment 55

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8372232 [details] [diff] [review]
part 2, also works for windows with titlebars

This time I got the right one :-)

Comment 56

[Mike Conley (:mconley) (:⚙️)]

Sorry for the delay - I'll be reviewing this today.

Comment 57

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8372236 [details] [diff] [review]
part 3: Set the dragmargin attribute at appropriate times

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

This doesn't account for the following case:

1) Start Firefox with tabs in titlebar
2) Add a lightweight theme
3) Enter customize mode
4) Disable tabs in titlebar
5) Exit customize mode
6) Attach debugger
7) Attempt to drag window

I *think* we might need to modify browser/base/content/browser-customization.js to call updateTitlebarDisplay when exiting customize mode... I hope that doesn't regress CART though...

Comment 58

[Sylvestre Ledru [:Sylvestre]]

Markus, the 29 beta is coming. Do you think you will be able to land your patches soon? (in order to have some time to test your changes). Thanks

Comment 59

[Markus Stange [:mstange]]

Attachment: part 3, v2: Set the dragmargin attribute at appropriate times

This should work. I tried to make the attribute handling a little less confusing, not sure if I succeeded...

This is how the end result should work:

lightweight themes off + toggling tabsintitlebar:
 - chromemargin: removed <-> "0,-1,-1,-1"
 - dragmargin: removed <-> "9,0,0,0"

lightweight themes on + toggling tabsintitlebar:
 - chromemargin: "0,-1,-1,-1" in both cases
 - dragmargin: "22,0,0,0," <-> "9,0,0,0"

tabsintitlebar off + toggling lightweight themes:
 - chromemargin: removed <-> "0,-1,-1,-1"
 - dragmargin: removed <-> "22,0,0,0"

tabsintitlebar on + toggling lightweight themes:
 - chromemargin: "0,-1,-1,-1" in both cases
 - dragmargin: "9,0,0,0," in both cases

Before this patch, the root.setAttribute("chromemargin-nonlwtheme", root.getAttribute("chromemargin")) part was executed when *de*activating a lightweight theme. I think that wasn't intended, so I've moved it to the activating branch. And this change means that we can remove the "chromemargin-nonlwtheme" mode in browser.js instead of setting it to "", if I understand everything correctly.

One more note: The line document.documentElement.setAttribute("dragmargin", "22,0,0,0"); in browser.js is only executed when toggling the browser.tabs.drawInTitlebar pref from about:config, while a lightweight theme is in use. It's not executed when toggling the pref using the button in customize mode, because customize mode temporarily deactivates lightweight themes.

Comment 60

[Markus Stange [:mstange]]

Comment on attachment 8386875 [details] [diff] [review]
part 3, v2: Set the dragmargin attribute at appropriate times

Dão should probably look over this as well.

Comment 61

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8386875 [details] [diff] [review]
part 3, v2: Set the dragmargin attribute at appropriate times

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

Sorry for the slow review! This works really well, and passes all of my local testing. Kudos! I also really like the clearer documentation you've added.

If / when this patch lands, I wonder if that means we can choose (at some point) to _always_ draw in the titlebar, regardless of whether the tabs are supposed to be in there. That would greatly reduce the complexity of some of our titlebar CSS.

::: toolkit/modules/LightweightThemeConsumer.jsm
@@ +144,4 @@
>      if (stateChanging) {
> +      // These are the attribute values we usually need in lightweight theme mode, for
> +      // applications that don't draw in the title bar on their own.
> +      let attributes = {

This should be a constant.

Comment 62

[Dão Gottwald [:dao]]

Comment on attachment 8386875 [details] [diff] [review]
part 3, v2: Set the dragmargin attribute at appropriate times

So the space above the tab bar is set as a 9px padding in CSS. Unfortunately, hard-coding that same number in XUL lets content make assumptions about the theme that could be wrong (think userChrome.css or third-party themes).

Comment 63

[Markus Stange [:mstange]]

Any suggestions for remedies? Should I remove the browser.xul change and let updateTitlebarDisplay() get the value from the computed style?

Comment 64

[Sylvestre Ledru [:Sylvestre]]

If we want to see that bug fixed for 29, I think we will need to have a patch pretty soon. Thanks

Comment 65

[Dão Gottwald [:dao]]

Markus said there'd be a way to fix this without XUL/JS hackery. This seems preferable to me, even though it will be too risky for Firefox 29.

Comment 66

[Markus Stange [:mstange]]

Attachment: part 1: Add CSS property -moz-window-dragging: drag | no-drag | inherit.

Initially I wanted the display list building code to detect draggable toolbars automatically, based on their -moz-appearance value. But that wouldn't work for lightweight themes. Detecting non-draggable controls inside of toolbars based on the display items they build would also have been very brittle.

I think the most robust approach is to add a new CSS property that can be used for indicating draggability. This patch calls it -moz-window-dragging.

Webkit apparently has a very similar property that's used by Webkit-based desktop apps: -webkit-app-region. It has the same drag | no-drag values. Dbaron, would you prefer -moz-app-region over -moz-window-dragging? This property will only be usable by XUL root documents, so cross-browser compatibility is not an issue.

Comment 67

[Markus Stange [:mstange]]

Attachment: part 2: Build a draggable region for the window based on the -moz-window-dragging property.

Comment 68

[Markus Stange [:mstange]]

Attachment: part 3: Implement nsChildView::UpdateWindowDraggingRegion.

This patch is very similar to attachment 8372232 [details] [diff] [review], which you have already reviewed before. The only differences is that this time we actually have the complete draggable region and don't need mWindowIsMovable, and we don't need to the window's draggability state on mouse move.

Comment 69

[Markus Stange [:mstange]]

Attachment: part 4: Add -moz-window-dragging CSS styles.

Comment 70

[Markus Stange [:mstange]]

Comment on attachment 8489626 [details] [diff] [review]
part 3: Implement nsChildView::UpdateWindowDraggingRegion.

r?roc for the ns(Int)Region::operator!= addition that slipped in here

Comment 71

[Dão Gottwald [:dao]]

Comment on attachment 8489627 [details] [diff] [review]
part 4: Add -moz-window-dragging CSS styles.

>+#titlebar {
>+  -moz-window-dragging: drag;
>+}

browser/base/content/browser.css still sets -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox"). How will these two things interact?

>-statusbar:not([nowindowdrag="true"]) {
>-  -moz-binding: url("chrome://global/content/bindings/general.xml#statusbar-drag");
>-}

This binding is now unused.

>+toolbarbutton,
>+button,
>+textbox,
>+searchbar,
>+tab,
>+menu,
>+radio,
>+tree {
>+  -moz-window-dragging: no-drag;
> }

Is this list of elements comprehensive? Where does it come from?

>+toolbar[customizing="true"] {
>+  -moz-window-dragging: no-drag;
>+}

How about using :not([customizing="true"]) for -moz-window-dragging: drag?

Comment 72

[Markus Stange [:mstange]]

(In reply to Dão Gottwald [:dao] from comment #71)
> Comment on attachment 8489627 [details] [diff] [review]
> part 4: Add -moz-window-dragging CSS styles.
> 
> >+#titlebar {
> >+  -moz-window-dragging: drag;
> >+}
> 
> browser/base/content/browser.css still sets -moz-binding:
> url("chrome://global/content/bindings/general.xml#windowdragbox"). How will
> these two things interact?

The windowdragbox binding only does something when it receives a MozMouseHitTest event, and with the Cocoa changes in part 3 we'll no longer send that event on Mac.

> >-statusbar:not([nowindowdrag="true"]) {
> >-  -moz-binding: url("chrome://global/content/bindings/general.xml#statusbar-drag");
> >-}
> 
> This binding is now unused.

thanks

> >+toolbarbutton,
> >+button,
> >+textbox,
> >+searchbar,
> >+tab,
> >+menu,
> >+radio,
> >+tree {
> >+  -moz-window-dragging: no-drag;
> > }
> 
> Is this list of elements comprehensive? Where does it come from?

It's probably not comprehensive, much like the whitelist in WindowDraggingUtils.jsm. That one was also incrementally adjusted until everybody was happy with the resulting behavior, IIRC.
I assembled the list by looking through the contents of the toolbars in various Firefox windows. (Except for tree, that one should probably be removed again, hopefully nobody puts trees in toolbars.)

> >+toolbar[customizing="true"] {
> >+  -moz-window-dragging: no-drag;
> >+}
> 
> How about using :not([customizing="true"]) for -moz-window-dragging: drag?

Good idea.

Comment 73

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8489622 [details] [diff] [review]
part 1: Add CSS property -moz-window-dragging: drag | no-drag | inherit.

>Bug 944836 - Add CSS property -moz-window-dragging: drag | no-drag | inherit. r=dbaron

Don't write out the "| inherit".



It would be good to make this chrome-only, so it's not exposed to the Web.
I don't see any disadvantage of doing so.  We'd need to add a mechanism
for that, but I don't think it should be too hard.  (The existing
CSS_PROPERTY_ALWAYS_ENABLED_IN_CHROME_OR_CERTIFIED_APP should lead to
the relevant code, although in this case we'd want just chrome and not
certified apps.)

Doing the chrome-only bit would be a separate patch, though.  Are you
willing to take that on?  (I wonder if it would help if I landed parts
of bug 837211.)

I don't particularly care about the name.

r=dbaron

Comment 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #73)
> It would be good to make this chrome-only, so it's not exposed to the Web.
> I don't see any disadvantage of doing so.  We'd need to add a mechanism
> for that, but I don't think it should be too hard.  (The existing
> CSS_PROPERTY_ALWAYS_ENABLED_IN_CHROME_OR_CERTIFIED_APP should lead to
> the relevant code, although in this case we'd want just chrome and not
> certified apps.)

Why don't we just use CSS_PROPERTY_ALWAYS_ENABLED_IN_CHROME_OR_CERTIFIED_APP?

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8489626 [details] [diff] [review]
part 3: Implement nsChildView::UpdateWindowDraggingRegion.

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

::: widget/cocoa/nsChildView.mm
@@ +4933,5 @@
> +
> +  nsIntRegion opaqueRegion;
> +  opaqueRegion.Sub(boundingRect, mGeckoChild->GetDraggableRegion());
> +
> +  std::vector<CGRect> rects;

nsTArray please

@@ +4944,5 @@
> +    rects.push_back([self convertToFlippedWindowCoordinates:rect]);
> +  }
> +
> +  CGSRegionObj region;
> +  CGSNewRegionWithRectList(rects.data(), rects.size(), &region);

Probably should break this out into a helper function that converts an nsIntRegion to a CGSRegionObj.

Comment 76

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #74)
> Why don't we just use CSS_PROPERTY_ALWAYS_ENABLED_IN_CHROME_OR_CERTIFIED_APP?

Two reasons:

 (1) it's designed for properties with a pref that's default-off (i.e., will-change).  I suppose we could add a bogus default-off pref, but it seems like a separate bit would be useful for the distinct case of a permanent situation.

 (2) I don't think we'd want it in B2G certified apps (but we do want will-change there).

Comment 77

[Markus Stange [:mstange]]

Attachment: part 3: Implement nsChildView::UpdateWindowDraggingRegion.

with roc's comments addressed

Comment 78

[Markus Stange [:mstange]]

Attachment: part 4: Add -moz-window-dragging CSS styles.

Comment 79

[Markus Stange [:mstange]]

Attachment: part 2: Build a draggable region for the window based on the -moz-window-dragging property.

I made two changes to nsDisplayListBuilder::AdjustWindowDraggingRegion: I added an IsForPainting() check, and I made it clip the frame's border box to mDirtyRect. The latter change makes a difference when the tab bar overflows: Before this change, tabs that were scrolled off to the side would prevent dragging in areas of the toolbar outside the tab scrollbox.

Comment 80

[Markus Stange [:mstange]]

Attachment: part 4: Add -moz-window-dragging CSS styles. r=dao

I added "splitter" to the dragging blacklist, for the splitter between the URL bar and the search field.

Comment 81

[Markus Stange [:mstange]]

Attachment: part 5: Make the CSS properties -moz-window-shadow and -moz-window-dragging chrome-only.

Something like this? I still need to fix the problem that executing document.defaultView.getComputedStyle(document.documentElement, "") on a regular web page gives an object that still contains "-moz-window-dragging", at index 189. (But at least '"MozWindowDragging" in computedStyle' is false.)
And hopefully fixing that will also fix the fact that the inspector panel of the devtools stays empty with this patch.

If you can tell me where to look, maybe I'll be able to fix it sooner. :)

Comment 82

[Markus Stange [:mstange]]

Attachment: part 5: Make the CSS properties -moz-window-shadow and -moz-window-dragging chrome-only.

Got it; nsComputedStyleMap::Entry::IsEnabled was bypassing the chrome_only check.

However, this still breaks the devtools inspector.

Comment 83

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8489919 [details] [diff] [review]
part 3: Implement nsChildView::UpdateWindowDraggingRegion.

Looks fine to me.

Comment 84

[Markus Stange [:mstange]]

Attachment: part 5: Make the CSS properties -moz-window-shadow and -moz-window-dragging chrome-only.

Devtools were failing because inDOMUtils::GetCSSPropertyNames was including the chrome-only properties in the returned list and subsequently refusing to accept them in inDOMUtils::GetCSSValuesForProperty (which calls nsCSSProps::LookupProperty(aProperty, nsCSSProps::eEnabledForAllContent)).

I've now converted all callers of the one-argument form of nsCSSProps::IsEnabled to the two-argument form. For some of them I'm not sure if ignoring chrome-only properties won't cause problems. But those problems would then probably already exist for will-change in chrome.

Comment 85

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8490052 [details] [diff] [review]
part 2: Build a draggable region for the window based on the -moz-window-dragging property.

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

::: layout/base/nsDisplayList.cpp
@@ +1073,5 @@
> +void
> +nsDisplayListBuilder::AdjustWindowDraggingRegion(nsIFrame* aFrame)
> +{
> +  if (IsForPainting() && !IsInSubdocument() && !IsInTransform()) {
> +    nsRect borderBox = aFrame->GetRectRelativeToSelf().Intersect(mDirtyRect);

Mmm good catch. But I think you should use the display item clip here. mDirtyRect is not reliable; in general it's only a conservative estimate of the rectangle.

Comment 86

[Markus Stange [:mstange]]

The problem with using the clip rect is that it gets temporarily reset for e.g. nsDiaplaySVGEffects contents. The tab center part has a clip path applied to it, so when AdjustWindowDraggingRegion is called for it, we don't know that we need to clip it.
Maybe we can just use the intersection of dirty rect and clip rect?

Comment 87

[Markus Stange [:mstange]]

Attachment: part 2: Build a draggable region for the window based on the -moz-window-dragging property.

This uses the intersection of mDirtyRect and the current clip, and adds a long comment explaining the rationale.

Another solution I thought of was to ignore, for the purposes of the window dragging region, all frames that have SVG effects applied to them. But the problem with that is that the URL bar is wrapped in an hbox with a clip-path. So that approach would make the URL bar draggable, which we definitely do not want.

Comment 88

[Markus Stange [:mstange]]

Comment on attachment 8490147 [details] [diff] [review]
part 5: Make the CSS properties -moz-window-shadow and -moz-window-dragging chrome-only.

Here are the test failures from this patch:
https://tbpl.mozilla.org/php/getParsedLog.php?id=48314418&tree=Try

David, can I pull the chrome-only thing out to a new bug and land the other patches without it? It's probably going to need a few more iterations, the way things look.

Comment 89

[Markus Stange [:mstange]]

Attachment: Remove test that uses MozMouseHitTest on a statusbar.

Comment 90

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8491057 [details] [diff] [review]
part 2: Build a draggable region for the window based on the -moz-window-dragging property.

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

Looks OK.

We could support a better visible region by hooking into FrameLayerBuilder and using the regions it computes, but I don't mind whether we do that or not.

Comment 91

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8490147 [details] [diff] [review]
part 5: Make the CSS properties -moz-window-shadow and -moz-window-dragging chrome-only.

In the EnabledState enum, could you write the values as
(1<<0), (1<<1), (1<<2) instead of 0x01, 0x02, 0x04?

(That said, I don't think it even makes sense for this to be a bitfield;
I think it should just be a sequence.  But that should probably be
a separate bug.)

>-    if ((aEnabled & eEnabledInChromeOrCertifiedApp) &&
>+    if (((aEnabled & eEnabledInChrome) ||
>+         (aEnabled & eEnabledInCertifiedApp)) &&

Just use (aEnabled & (eEnabledInChrome | eEnabledInCertifiedApp))

r=dbaron with that

Comment 92

[Markus Stange [:mstange]]

see comment 88

Comment 93

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Markus Stange [:mstange] from comment #88)
> David, can I pull the chrome-only thing out to a new bug and land the other
> patches without it? It's probably going to need a few more iterations, the
> way things look.

Yes, that's ok.

Comment 94

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Although I think the test failures result from having the values in property_database.js but not supporting them in the testing environment.  That said, maybe we should have a way to flip to having support for chrome-only properties in tests so that we can test them.

Comment 95

[Markus Stange [:mstange]]

Comment on attachment 8490147 [details] [diff] [review]
part 5: Make the CSS properties -moz-window-shadow and -moz-window-dragging chrome-only.

I moved this patch to bug 1069192.

Comment 96

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa6432518ad1
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c9e801f66aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/08f2238d29ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/71a8e9cb15c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/14e633eeb251

Comment 97

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aa6432518ad1
https://hg.mozilla.org/mozilla-central/rev/1c9e801f66aa
https://hg.mozilla.org/mozilla-central/rev/08f2238d29ee
https://hg.mozilla.org/mozilla-central/rev/71a8e9cb15c5
https://hg.mozilla.org/mozilla-central/rev/14e633eeb251

Comment 98

[Jeff Muizelaar [:jrmuizel]]

FWIW, I noticed some time being spent in AdjustWindowDraggingRegion inside region stuff. Should we be expecting expensive regions in this path?

Comment 99

[Markus Stange [:mstange]]

The region should be rather simple - it'll look like you'd expect it to look. But we might be calling the region methods a large number of times, since we call it for every nsIFrame in the frame tree of the browser chrome, even for frames whose draggability is the same as their parent's.

Comment 100

[Jeff Muizelaar [:jrmuizel]]

(In reply to Markus Stange [:mstange] from comment #99)
> The region should be rather simple - it'll look like you'd expect it to
> look. But we might be calling the region methods a large number of times,
> since we call it for every nsIFrame in the frame tree of the browser chrome,
> even for frames whose draggability is the same as their parent's.

Can we avoid calling it so many times?

Comment 101

[Markus Stange [:mstange]]

Maybe, I've filed bug 1118029.

Comment 102

[Markus Stange [:mstange]]

This was backed out on release for Firefox 35.0.1 due to bug 1104036 and bug 1122942.