347452 - add native resize animation support to nsIDOMChromeWindow

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, enhancement)

Description

[Josh Aas]

I'd like to propose adding native resize animation support to nsIDOMChromeWindow. This would allow chrome windows to resize themselves using different animation techniques. Any platform that does not implement a given animation technique should just do a normal resize when the unimplemented technique is requested. That way we can implement animation techniques for different platforms as it becomes necessary and as we have the resources to do so.

The basic animation, which we should implement first, is the type used by Mac OS X preference windows (see the System Preferences window when switching between pref panes). We can use that immediately in the Firefox preferences on Mac OS X to provide smooth animation without a lot of nasty js code (see the js for animation in toolkit/content/widgets/preferences.xml). Right now the animation isn't very smooth under Carbon widgets, and it is horrible under Cocoa widgets. Due to the way Cocoa window origins work, there isn't a good way to animate the resize without using the native Mac OS X window animation API, which the new API I'm proposing allows us to do.

Comment 1

[Josh Aas]

Attachment: fix v1.0 (Mac OS X only)

This fix works well, but it does not include API stubs for platforms other than Mac OS X Cocoa widgets. This is the API support only, it does not include any consumer code.

Comment 2

[Josh Aas]

Attachment: example consumer (prefs) v1.0

This causes preferences to become a consumer of the new API. This is clearly not the best way to write the consumer code for preferences widgets, but it is simple and enough to work as a good example of native resize animation API usage.

Comment 3

[Josh Aas]

Attachment: fix v1.0 + fade option

This is the same as v1.0, it just includes an additional animation type, fade, as an example of other things we could do. This is also Cocoa widgets only.

Comment 4

[Mark Mentovai]

Comment on attachment 232222 [details] [diff] [review]
fix v1.0 (Mac OS X only)

>Index: dom/public/idl/base/nsIDOMChromeWindow.idl>+  
>+  const unsigned short ANIMATION_OFF = 1;
>+  const unsigned short ANIMATION_SLIDE = 2;

I think these should be named RESIZE_ANIMATION_*, and RESIZE_ANIMATION_OFF should be 0.

>Index: dom/src/base/nsGlobalWindow.cpp

> NS_IMETHODIMP
>+nsGlobalChromeWindow::SetAnimatedResize(PRUint16 aAnimation)
[...]
>+  widget->SetAnimatedResize(aAnimation);
>+  return NS_OK;

return widget->SetAnimatedResize(aAnimation);

Same comment applies to:

>+NS_IMETHODIMP
>+nsGlobalChromeWindow::GetAnimatedResize(PRUint16* aAnimation)
[...]
>+  widget->GetAnimatedResize(aAnimation);
>+  return NS_OK;

>Index: widget/src/cocoa/nsChildView.mm

>+NS_IMETHODIMP nsChildView::SetAnimatedResize(PRUint16 aAnimation)
>+{
>+  return NS_OK;

NS_ERROR_NOT_IMPLEMENTED?

Same comment applies to:

>+NS_IMETHODIMP nsChildView::GetAnimatedResize(PRUint16* aAnimation)
>+{
>+  return NS_OK;

>Index: widget/src/cocoa/nsCocoaWindow.h
>   PRPackedBool         mIsResizing;     // we originated the resize, prevent infinite recursion
>   PRPackedBool         mWindowMadeHere; // true if we created the window, false for embedding
>-  PRPackedBool         mVisible;        // Whether or not we're visible.
>+  PRPackedBool         mVisible;        // whether or not we're visible.
>   PRPackedBool         mSheetNeedsShow; // if this is a sheet, are we waiting to be shown?
>+  PRPackedBool         mAnimation;      // whether or not we should animate resizes

PRUint16, not PRPackedBool.  Because it's a wider type, move the declaration above the PRPackedBool declarations.

>Index: widget/src/cocoa/nsCocoaWindow.mm
> , mWindow(nil)
> , mDelegate(nil)
> , mSheetWindowParent(nil)
> , mPopupContentView(nil)
> , mIsResizing(PR_FALSE)
> , mWindowMadeHere(PR_FALSE)
> , mVisible(PR_FALSE)
> , mSheetNeedsShow(PR_FALSE)
>+, mAnimation(nsIDOMChromeWindow::ANIMATION_OFF)

This will need to move up too.

>+    if (mAnimation == nsIDOMChromeWindow::ANIMATION_SLIDE) {
>+      [[mWindow contentView] setHidden:YES];
>+      [mWindow setFrame:newFrame display:YES animate:YES];
>+      [[mWindow contentView] setHidden:NO];

What's with the setHidden business?

>+    }
>+    else {
>+      [mWindow setFrame:newFrame display:NO];
>+    }

Could we just do this?

[mWindow setFrame:newFrame
          display:YES
          animate:(mAnimation == nsIDOMChromeWindow::ANIMATION_SLIDE)];

Does this call block until the animated resize is complete?  One way to check would be to override [(NSWindow) animationResizeTime:].  I'd prefer non-blocking.

Your ANIMATION_FADE code from the later patch was blocking for sure.  In Carbon, you can do non-blocking window transitions by sticking management into the event loop.  You make a single call that takes care of this for you.  Would it be possible to do that here?  Also, 0.2s is standard for a fade-in or fade-out transition.  I'm not sure if 0.3s to do both would seem too long or too short.  It sounds about right, but you might want to check against other fades.

Consumers of this API might want a way to determine if animated resizes are supported or not.  It may be enough for the prefs code to fetch  or set the animatedResize property in a try block, and fall back to the JS that steps through the sizes if animatedResize isn't implemented.  This strategy relies on throwing NS_ERROR_NOT_IMPLEMENTED from nsIWidget::Resize on platforms (or for widgets) that don't implment native resize.  You should add @throws (and @return) to the comments in nsIDOMChromeWindow and nsIWidget to make this clear, if this is the way you'd like to go.

Comment 5

[Josh Aas]

Attachment: fix v1.1 (Mac OS X only)

This addresses Mark's comments and fixes up some of the math in the fading.

I'm not sure why we'd want this to be non-blocking. I wouldn't want somebody dealing with the window size while the window is in an odd transitional state.

Comment 6

[Josh Aas]

Attachment: example consumer (prefs) v1.1

Comment 7

[Mark Mentovai]

We'd want non-blocking because it's evil for the main thread to go to sleep.  Ideally, the window would be internally resized to the new size immediately when the resize call returns, so it's harmless to draw into the window, but the effect would take place asynchronously over time.  If consumers need to know when the effect is complete, they should be notified.

Comment 8

[Josh Aas]

I order to deal with the blocking situation for now I suggest just removing the FADE option from the patch. We don't actually need that I was just throwing it in there to show another option.

Comment 9

[Mark Mentovai]

Comment on attachment 233739 [details] [diff] [review]
fix v1.1 (Mac OS X only)

OK, I guess we could go with this.  Let's leave the fade stuff out for now.

Comment 10

[Mark Mentovai]

Comment on attachment 233739 [details] [diff] [review]
fix v1.1 (Mac OS X only)

OK, I guess we could go with this.  Let's leave the fade stuff out for now.

Comment 11

[Josh Aas]

Attachment: fix v1.1.1

doesn't have Fade option in it

Comment 12

[Josh Aas]

Attachment: example consumer (prefs), reference only (doesn't work)

This patch doesn't work. It is for reference only.

Comment 13

[Josh Aas]

Attachment: fix v1.1.2

updated so it applies/builds on trunk

Comment 14

[Josh Aas]

Attachment: fix v1.1.3

Updated to current trunk.

Comment 15

[Josh Aas]

Comment on attachment 260416 [details] [diff] [review]
fix v1.1.3

Requesting sr, but I won't land this until a consumer patch is ready.

Comment 16

[Josh Aas]

There is a patch in bug 355177 that works well using this API.

Comment 17

[Stuart Parmenter]

Comment on attachment 260416 [details] [diff] [review]
fix v1.1.3

this looks ok to me but should have jst or jonas or someone familiar with dom/chrome/etc review it

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 260416 [details] [diff] [review]
fix v1.1.3

Looks good to me.

Comment 19

[Josh Aas]

landed on trunk with stub NS_ERROR_NOT_IMPLEMENTED in nsBaseWidget for platforms that don't implement native resize animation

Comment 20

[Asaf Romano (gone)]

This was backed-out in bug 393117.