373266 - Add support for Windows widget theme animations

Status: RESOLVED INCOMPLETE

(Core :: Widget: Win32, defect, P4)

Description

[Dan Dayon]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2

In all other applications in Vista, the scrollbar highlighting fades in quickly when you hover over it, and fades out slowly when the mouse leaves it. In Firefox, however, the highlighting is instant. There is no fading.

Reproducible: Always

Steps to Reproduce:
1. Hover mouse over the scrollbar.
2. Move mouse away from scrollbar.
Actual Results:  
Highlight appears instantly, and disappears instantly.

Expected Results:  
Highlight should fade in, and fade out.

Comment 1

[Dan Dayon]

Forgot to mention this: The up and down buttons don't highlight when you hover over the rest of the scrollbar. They do in every other application.

Comment 2

[Ria Klaassen (not reading all bugmail)]

Did you also test this in Firefox's -safe-mode?

Comment 3

[Doug Wright]

Can't say I'd previously noticed the difference myself, but it is there.

Comment 4

[Dan Dayon]

(In reply to comment #2)
> Did you also test this in Firefox's -safe-mode?
> 

Safe mode makes no difference.

Comment 5

[James Ross]

None of the native theme code in Firefox (which emulates all the OS' theme code) does any animations, at least on Windows. This applies to all the subtle hover and focus animations, which also include dialog buttons.

Comment 6

[AndrewM]

Is this really a bug? For me, the slow fading gets a bit annoying, and the current Firefox behavior is actually an improvement over the "native" Vista behavior.

Comment 7

[James Ross]

Whatever you think of the OS theme, it is and will continue to be a bug in Firefox (well, the core code behind Firefox) until it actually matches the native widgets. Whether someone will be bothered to fix it is another matter...

Comment 8

[AndrewM]

(In reply to comment #7)
> Whatever you think of the OS theme, it is and will continue to be a bug in
> Firefox (well, the core code behind Firefox) until it actually matches the
> native widgets. Whether someone will be bothered to fix it is another matter...

Yes, after thinking about I think I've changed my mind and I do agree that the ultimate goal for Firefox should be to look as "native" as possible on each platform.

And in relation to the issue that annoys me, I found a way to disable the fading that annoys me in Vista generally, so now Vista behaves more like Firefox does at the moment. It's under System Properties > Advanced > Performance Settings... > Custom > and then uncheck 'Animate controls and elements inside windows', for those who are interested :)

Comment 9

[Destian]

In addition to the already mentioned problem, when clicking on a blank area of the scrollbar the clicked area should highlight (darken actually) as it does in other Vista applications.

Comment 10

[Destian]

Furthermore, if aiming for a complete match, at it's smallest side, the scroller should lose the 3 indents.

I'm sorry, I don't know the terminology. 

Comment 11

[Destian]

At its smallest SIZE not SIDE. Sorry again.

Comment 12

[Martijn Wargers (dead)]

(In reply to comment #9)
> In addition to the already mentioned problem, when clicking on a blank area of
> the scrollbar the clicked area should highlight (darken actually) as it does in
> other Vista applications.

Destian, that's also a bug in older versions of Windows, so that's a general windows bug.
You might want to file a new bug on that, although, I vaguely remember something about a bug that I morphed into that issue.

Comment 13

[Alex Faaborg [:faaborg] (Firefox UX)]

this bug is eligible for bug 462080

Comment 14

[Daniel Jackson]

This is probably getting to far in to the finer detail, but one more parity issue at least on Vista (and I think also on XP) -- when you right-click anywhere on a standard Windows scrollbar (e.g. try Notepad), you get a context menu with the following entries:

  Scroll Here
  ---
  Top
  Bottom
  ---
  Page Up
  Page Down
  ---
  Scroll Up
  Scroll Down

...a bit useless I know, but it's there anyway. I didn't think this would really merit a new bug though.

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

For normal use this is completely insane, perhaps this is a limited mobility accessibility feature?

Comment 16

[klonos]

Confirming comment #14 issue on WinXP as well with latest nightlies of both firefox 3.6a1pre and thunderbird 3.1a1pre.

So not only Vista and not only firefox? Please someone with rights update the description of the bug.

Comment 17

[Alex Faaborg [:faaborg] (Firefox UX)]

>So not only Vista and not only firefox? Please someone with rights update the
>description of the bug.

Filed follow up bug 489921 since this bug was originally focused just on the widget animation aspects of scrolls bars on Vista.

Comment 18

[klonos]

thanx

Comment 19

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P1 - Polish issue that appears in the main window, or is something that the user may encounter several times a day.

scrolling is, well, pretty common :)

Comment 20

[Jim Mathies [:jimm]]

Attachment: wip

Still some work to do here:
- Cairo won't let me draw a gfxWindowsSurface onto a D2D surface, so I need to find an alternative solution in that case.
- This cover a vertical scroll thumb, I need to add support for the other components of the scrollbar (grip and arrow buttons and the horizontal scroll)
- I'd like to dupe some of the other animation widget bugs to this bug and add support for other widgets here, like buttons.
- code cleanup and testing.

Comment 22

[Jim Mathies [:jimm]]

generalizing this up.

Comment 23

[Jim Mathies [:jimm]]

Attachment: wip

scrollbars are complete, I have animation working on the gripper, buttons, and the base thumb track. Still have to look at D2D issues.

Comment 24

[Jim Mathies [:jimm]]

Attachment: test case

Comment 25

[Jim Mathies [:jimm]]

Attachment: wip

- D2D rendering issues fixed
- filed bug 724933 on a scrollbar bug I found while testing

Comment 26

[Jim Mathies [:jimm]]

Attachment: base v.1

Comment 27

[Jim Mathies [:jimm]]

Attachment: scrollbars v.1

Comment 28

[Jim Mathies [:jimm]]

Attachment: buttons v.1

Comment 29

[Jim Mathies [:jimm]]

Attachment: base v.1

This is the base patch for adding the ability to fade themed content in and out via the theme library. Comments in the code should explain what's going on. The code can also be used to implement throbbing effects for elements that need it.

Roc, have the time to look this over?

Comment 30

[Jim Mathies [:jimm]]

(Note, to apply these you'll also need the patch in bug 719983 applied.)

Comment 31

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

I probably won't get to this until next week.

Comment 32

[Jim Mathies [:jimm]]

Attachment: base v.2

Updates to address leaks in test runs. mAnimatedFadesList wasn't getting cleared properly in nsNativeTheme's destructor, so I switched to an observer that listens for xpcom shutdown. 

Roc, re not having time to get to this soon - these patches are built on bug 719983, which is dependent on bug 699247 which was backed out due to perf regressions, so no big hurry. If 699247 doesn't land in the next week I'll rebase without it.

Comment 33

[Jim Mathies [:jimm]]

Attachment: testfixes v.1

Ref test fix which was thrown off by a fading element background.

Comment 34

[Jim Mathies [:jimm]]

Attachment: base v.2

sorry for the review spam - fixed up some formatting and grammar.

Comment 35

[Jim Mathies [:jimm]]

Attachment: scrollbars v.2

Comment 36

[Jim Mathies [:jimm]]

Attachment: buttons v.2

Comment 37

[Jim Mathies [:jimm]]

Attachment: base v.2

This addresses a build issue on unix systems, and fixes a problem on linux where nsNativeThemeGTK had already implemented nsIObserver.

Comment 38

[Jim Mathies [:jimm]]

Comment on attachment 595938 [details] [diff] [review]
scrollbars v.2

I've got updated patches for scrollbars, buttons, radio and checkboxes, and toolbar buttons. I'll post those after the initial reviews are complete.

Comment 39

[Jim Mathies [:jimm]]

Try results:

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=313daee316dc

One more ref test for check boxes that needs some touching up, but other than that things are looking good.

Comment 40

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #39)
> Try results:
> 
> https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=313daee316dc
> 
> One more ref test for check boxes that needs some touching up, but other
> than that things are looking good.

check box problems fixed:

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=6cfc1d760d45

Comment 41

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

Comment on attachment 596254 [details] [diff] [review]
base v.2

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

::: widget/xpwidgets/nsNativeTheme.h
@@ +211,5 @@
> +   *
> +   * Note callers are responsible for switching fade transitions from
> +   * FADE_IN/FADE_IN_FINISHED to FADE_OUT through calls to QACRFF. Failing
> +   * to do so keeps content / fade data stored in mAnimatedFadesList until
> +   * the content's underlying frame is destroyed or the application closes.

Do you need to say something about how this will adjust the duration if the caller switches to FADE_OUT during FADE_IN or vice versa?

In fact it might be helpful to list out what the possible state transitions are.

@@ +235,5 @@
> +   *                   Value is passed to QueueAnimatedContentForRefresh
> +   *                   to trigger a refresh.
> +   * aMilliseconds     Total duration of the current fade transition. This
> +   *                   value is updated internally on creation and directional
> +   *                   changes. Should always be valid and constant.

This is confusing. Just remove the "this valid is updated internally..." comment.

@@ +237,5 @@
> +   * aMilliseconds     Total duration of the current fade transition. This
> +   *                   value is updated internally on creation and directional
> +   *                   changes. Should always be valid and constant.
> +   * aUserData         User data storage for state across the rendering of
> +   *                   individual frames, general use. Updated on every call.

It's unclear how this is used.

@@ +298,5 @@
> +
> +  // Fade data helpers
> +
> +  // Retrieves the FadeData object associated with this content, or null.
> +  FadeData* GetFade(nsIContent* aContent);

Can these be protected/hidden? Possibly even made into static functions inside nsNativeTheme.cpp?

Comment 42

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> Comment on attachment 596254 [details] [diff] [review]
> base v.2
> 
> Review of attachment 596254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/xpwidgets/nsNativeTheme.h
> @@ +211,5 @@
> > +   *
> > +   * Note callers are responsible for switching fade transitions from
> > +   * FADE_IN/FADE_IN_FINISHED to FADE_OUT through calls to QACRFF. Failing
> > +   * to do so keeps content / fade data stored in mAnimatedFadesList until
> > +   * the content's underlying frame is destroyed or the application closes.
> 
> Do you need to say something about how this will adjust the duration if the
> caller switches to FADE_OUT during FADE_IN or vice versa?


Sure I can document that behavior. In fact looking at the code in cpp, I should probably touch up the logic there so that the interval of an early fade-out matches the amount of time used to fade-in vs. resetting back to the base timeout period. (In practice though this is largely unnoticed - these intervals (at least on Windows) are in the 200msec range. With that short of an interval the number of callbacks is rarely greater than 5 calls. Any minor variance in total interval is hard to notice.

> @@ +237,5 @@
> > +   * aMilliseconds     Total duration of the current fade transition. This
> > +   *                   value is updated internally on creation and directional
> > +   *                   changes. Should always be valid and constant.
> > +   * aUserData         User data storage for state across the rendering of
> > +   *                   individual frames, general use. Updated on every call.
> 
> It's unclear how this is used.

This is just a general purpose value callers can store and pull between frames. I needed this for one transition (buttons) to store the base state the fade was initiated from. This could be expanded later to hold a larger data structure if need be. For the patches I wrote, all I needed was an int, and I only needed it in the case of one type of content element.

> 
> @@ +298,5 @@
> > +
> > +  // Fade data helpers
> > +
> > +  // Retrieves the FadeData object associated with this content, or null.
> > +  FadeData* GetFade(nsIContent* aContent);
> 
> Can these be protected/hidden? Possibly even made into static functions
> inside nsNativeTheme.cpp?

Protected yes, but not hidden. For example I use GetFade in one case in NativeThemeWin for finishing a fade when the content state switches from TS_HOVER to TS_ACTIVE (pushed) mid-fade - a common occurrence with elements like scrollbar thumbs.

Comment 43

[Jim Mathies [:jimm]]

One other note, when I started in on this I was originally thinking of breaking the base theme code up into two calls, rather than the single call  QueueAnimatedContentRefreshForFade. This would solve oddities like this in the api:

> +   * aMilliseconds     Total duration of the current fade transition. This
> +   *                   value is updated internally on creation and directional
> +   *                   changes. Should always be valid and constant.

But since QueueAnimatedContentForRefresh already existed and implemented this type of behavior, I decided to mimic it. I could still break this up up though if we think it would make use of this less confusing.

Comment 44

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #43)
> One other note, when I started in on this I was originally thinking of
> breaking the base theme code up into two calls, rather than the single call 
> QueueAnimatedContentRefreshForFade. This would solve oddities like this in
> the api:
> 
> > +   * aMilliseconds     Total duration of the current fade transition. This
> > +   *                   value is updated internally on creation and directional
> > +   *                   changes. Should always be valid and constant.
> 
> But since QueueAnimatedContentForRefresh already existed and implemented
> this type of behavior, I decided to mimic it. I could still break this up up
> though if we think it would make use of this less confusing.

..and in looking at the client code I can see why bent implemented QueueAnimatedContentForRefresh the way he did - having split calls would require the native client code to figure out if a fade was already active, and split the code path accordingly to make the two different calls. So maybe it's best to keep it as a single call.

Comment 45

[Jim Mathies [:jimm]]

Attachment: base v.3

Comment 46

[Jim Mathies [:jimm]]

Comment on attachment 597311 [details] [diff] [review]
base v.3

Updated per comments plus some other cleanup.

Comment 47

[Jim Mathies [:jimm]]

Attachment: render routines v.1

for reference, plus I'll post scrollbars code.

Comment 48

[Jim Mathies [:jimm]]

Attachment: scrollbars v.1

Comment 49

[Jim Mathies [:jimm]]

Attachment: render routines v.1

fixing up comment header typos in render routines v.1

Comment 50

[Jim Mathies [:jimm]]

Attachment: scrollbars v.1

Comment 51

[Mozilla RelEng Bot]

Try run for efa5576ce9ef is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=efa5576ce9ef
Results (out of 212 total builds):
    success: 177
    warnings: 20
    failure: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-efa5576ce9ef

Comment 52

[Jim Mathies [:jimm]]

Another try result will be coming up in an hour or two with no major changes and some additional cleanup / comment updates.

Comment 53

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

Comment on attachment 597311 [details] [diff] [review]
base v.3

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

::: widget/xpwidgets/nsNativeTheme.cpp
@@ +585,5 @@
> +  // content getting stuck in mAnimatedFadesList until shutdown, so we
> +  // warn loudly. Generally this should never happen.
> +
> +  if (aFadeDirection != FADE_IN && aFadeDirection != FADE_OUT) {
> +    NS_WARNING("Bad initial fade direction.");

Shouldn't this be an NS_ASSERTION? It's a bug if the caller got this wrong.

@@ +600,5 @@
> +          pFade->GetState() == FADE_IN_FINISHED) &&
> +         aFadeDirection == FADE_OUT) ||
> +        (pFade->GetState() == FADE_OUT && aFadeDirection == FADE_IN)) {
> +      if ((pFade->GetState() == FADE_IN && aFadeDirection == FADE_OUT) ||
> +          (pFade->GetState() == FADE_OUT && aFadeDirection == FADE_IN)) {

These expressions are way complicated. Can we simplify with some helper functions?

@@ +678,5 @@
> +  nsCOMPtr<nsIObserverService> obsSvc =
> +    mozilla::services::GetObserverService();
> +  nsresult rv = NS_ERROR_UNEXPECTED;
> +  if (obsSvc)
> +    rv = obsSvc->AddObserver(this, NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID, false);

{} around the if body

@@ +692,5 @@
> +    nsCOMPtr<nsIObserverService> obsSvc =
> +      mozilla::services::GetObserverService();
> +    nsresult rv = NS_ERROR_UNEXPECTED;
> +    if (obsSvc)
> +      rv = obsSvc->RemoveObserver(this, NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID);

{} around the if body

@@ +778,5 @@
> +nsNativeTheme::SetFadeUserData(nsIContent* aContent, PRUint32 aUserData)
> +{
> +  FadeData* pFade = GetFade(aContent);
> +  if (pFade)
> +    pFade->SetUserData(aUserData);

{} around the if body

@@ +786,5 @@
> +nsNativeTheme::CancelFade(nsIContent* aContent)
> +{
> +  if (aContent && mAnimatedFadesList.IsInitialized() &&
> +      mAnimatedFadesList.Get(reinterpret_cast<nsISupports*>(aContent)))
> +    mAnimatedFadesList.Remove(reinterpret_cast<nsISupports*>(aContent));

{} around the if body

@@ +794,5 @@
> +nsNativeTheme::FinishFadeIn(nsIContent* aContent)
> +{
> +  FadeData* pFade = GetFade(aContent);
> +  if (pFade)
> +    pFade->FadeInFinished();

{} around the if body

::: widget/xpwidgets/nsNativeTheme.h
@@ +273,5 @@
> +      mTimeout = aTimeout;
> +      mState = aState;
> +    }
> +
> +    PRUint32 GetTicks() {

Document this?

@@ +278,5 @@
> +      PRUint32 now = PR_IntervalToMilliseconds(PR_IntervalNow());
> +      if (now >= mTimeout) {
> +        if (mState == FADE_OUT)
> +          return 0;
> +        return 100;

TICK_MAX?

@@ +288,5 @@
> +        tick = (PRUint32)abs(tick - TICK_MAX);
> +      return tick;
> +    }
> +
> +    PRUint32 TimeoutUsed() {

Document this

@@ +331,2 @@
>   private:
> +  inline nsresult InitFadeList();

Where's the definition?

@@ +334,4 @@
>    PRUint32 mAnimatedContentTimeout;
>    nsCOMPtr<nsITimer> mAnimatedContentTimer;
>    nsAutoTArray<nsCOMPtr<nsIContent>, 20> mAnimatedContentList;
> +  nsClassHashtable<nsISupportsHashKey, FadeData> mAnimatedFadesList;

Document what the keys are.

Comment 54

[Mozilla RelEng Bot]

Try run for eee9580d929e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=eee9580d929e
Results (out of 209 total builds):
    success: 174
    warnings: 21
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-eee9580d929e

Comment 55

[Jim Mathies [:jimm]]

Attachment: base v.4

comments addressed.

Comment 56

[Jim Mathies [:jimm]]

Attachment: render routines v.1

Joe, is this something you'd be ok with reviewing?

Comment 57

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #56)
> Created attachment 597499 [details] [diff] [review]
> render routines v.1
> 
> Joe, is this something you'd be ok with reviewing?

Note the debug routines in WinUtils are just for testing purposes, so they really aren't critical. The important work here is in nsNativeThemeWin.

Comment 58

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

Comment on attachment 597426 [details] [diff] [review]
base v.4

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

Looks good. Just one thing: should we use TimeStamp here instead of PR_IntervalNow? We probably should if we can. It doesn't wrap and the API is a bit cleaner.

::: widget/xpwidgets/nsNativeTheme.cpp
@@ +624,5 @@
> +      // If timed out and it's a fade up, set state to finished. We keep the
> +      // fade data around until a corresponding fade out completes or the
> +      // underlying frame is destroyed.
> +      if (pFade->GetState() == FADE_IN ||
> +          pFade->GetState() == FADE_IN_FINISHED) {

IsFadeIn

@@ +632,5 @@
> +        if (!QueueAnimatedContentForRefresh(aContent, 1)) {
> +          NS_WARNING("QueueAnimatedContentForRefresh failed???");
> +          return false;
> +        }
> +      } else if (pFade->GetState() == FADE_OUT) {

IsFadeOut for consistency

@@ +651,5 @@
> +    }
> +    return true;
> +  }
> +
> +  // If we don't have a fade set put together a FadeData, store it in

set, put

@@ +727,5 @@
> +      // If this content has fade data associated with it, and the
> +      // frame has gone away, free the data and cancel the fade.
> +      if (mAnimatedFadesList.IsInitialized() &&
> +          mAnimatedFadesList.Get(mAnimatedContentList[index])) {
> +        mAnimatedFadesList.Remove(mAnimatedContentList[index]);

Why not just call Remove unconditionally instead of calling Get first?

@@ +795,5 @@
> +nsNativeTheme::CancelFade(nsIContent* aContent)
> +{
> +  if (aContent && mAnimatedFadesList.IsInitialized() &&
> +      mAnimatedFadesList.Get(reinterpret_cast<nsISupports*>(aContent))) {
> +    mAnimatedFadesList.Remove(reinterpret_cast<nsISupports*>(aContent));

Why not just call Remove unconditionally instead of calling Get first?

::: widget/xpwidgets/nsNativeTheme.h
@@ +296,5 @@
> +      PRUint32 tick = (PRUint32)ceil((((double)(now - mStartTime) /
> +        (double)(mTimeout - mStartTime)) * TICK_MAX));
> +      // we want ticks to ascend and descend according to the direction.
> +      if (mState == FADE_OUT)
> +        tick = (PRUint32)abs(tick - TICK_MAX);

{}

@@ +312,5 @@
> +      PRInt32 totalTime = mTimeout - mStartTime;
> +      if (used >= totalTime)
> +        return totalTime;
> +      else
> +        return used;

return NS_MIN(used, totalTime);

Comment 59

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #58)
> Comment on attachment 597426 [details] [diff] [review]
> base v.4
> 
> Review of attachment 597426 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Just one thing: should we use TimeStamp here instead of
> PR_IntervalNow? We probably should if we can. It doesn't wrap and the API is
> a bit cleaner.

Sorry, keep forgetting we added that. Will update.

Comment 60

[Jim Mathies [:jimm]]

Attachment: base v.5

comments addressed.

Comment 61

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

Comment on attachment 597817 [details] [diff] [review]
base v.5

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

::: widget/xpwidgets/nsNativeTheme.h
@@ +53,5 @@
> +#include "nsIObserver.h"
> +#include "mozilla/TimeStamp.h"
> +
> +using mozilla::TimeStamp;
> +using mozilla::TimeDuration;

You can't use "using" in header files. Add "typedef mozilla::TimeStamp TimeStamp;" etc to nsNativeTheme instead.

Comment 62

[Joe Drew (not getting mail)]

Comment on attachment 597499 [details] [diff] [review]
render routines v.1

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

I'm not qualified to review the GDIPlus part of this patch. Hopefully Brian is!

::: widget/windows/WinUtils.cpp
@@ +428,5 @@
> +  GetImageEncodersSize(&num, &size);
> +  if(size == 0)
> +    return false;
> +
> +  pImageCodecInfo = (ImageCodecInfo*)(malloc(size));

Either use moz_xmalloc or moz_malloc, depending on whether you want infallible or fallible allocation. And if you want this to be infallible (because size is not user-controlled), remove the null check. :)

::: widget/windows/nsNativeThemeWin.cpp
@@ +327,5 @@
> +
> +  // Create a new highlight theme background surface
> +  nsRefPtr<gfxWindowsSurface> surfAlpha =
> +    new gfxWindowsSurface(gfxIntSize(width, height),
> +                          gfxASurface::ImageFormatRGB24);

Is this meant to have an alpha channel? It currently doesn't.

@@ +368,5 @@
> +    roundedRect.Round();
> +    paintCtx->Clip(roundedRect);
> +    paintCtx->Translate(roundedRect.TopLeft());
> +  }
> +  paintCtx->SetOperator(gfxContext::OPERATOR_SOURCE);

Make sure you really want to use SOURCE, because it needs to be pixel-aligned and have no blending with the background (ie non-pixel-aligned/non-rectangular clip) to be fast.

Comment 63

[Jim Mathies [:jimm]]

(In reply to Joe Drew (:JOEDREW!) from comment #62)
> Comment on attachment 597499 [details] [diff] [review]
> render routines v.1
> 
> Review of attachment 597499 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not qualified to review the GDIPlus part of this patch. Hopefully Brian
> is!

They are just debug routines, so it's really not critical. I've been working with these a lot lately and they are working correctly for the type of use they are getting.

> 
> ::: widget/windows/WinUtils.cpp
> @@ +428,5 @@
> > +  GetImageEncodersSize(&num, &size);
> > +  if(size == 0)
> > +    return false;
> > +
> > +  pImageCodecInfo = (ImageCodecInfo*)(malloc(size));
> 
> Either use moz_xmalloc or moz_malloc, depending on whether you want
> infallible or fallible allocation. And if you want this to be infallible
> (because size is not user-controlled), remove the null check. :)

will do.

> ::: widget/windows/nsNativeThemeWin.cpp
> @@ +327,5 @@
> > +
> > +  // Create a new highlight theme background surface
> > +  nsRefPtr<gfxWindowsSurface> surfAlpha =
> > +    new gfxWindowsSurface(gfxIntSize(width, height),
> > +                          gfxASurface::ImageFormatRGB24);
> 
> Is this meant to have an alpha channel? It currently doesn't.

No, DrawThemeBackground doesn't support alpha.

paintCtx->SetOperator(gfxContext::OPERATOR_OVER);
paintCtx->SetSource(imageAlpha);
paintCtx->Paint(aAlpha);

This works as expected with the source being ImageFormatRGB24.

> 
> @@ +368,5 @@
> > +    roundedRect.Round();
> > +    paintCtx->Clip(roundedRect);
> > +    paintCtx->Translate(roundedRect.TopLeft());
> > +  }
> > +  paintCtx->SetOperator(gfxContext::OPERATOR_SOURCE);
> 
> Make sure you really want to use SOURCE, because it needs to be
> pixel-aligned and have no blending with the background (ie
> non-pixel-aligned/non-rectangular clip) to be fast.

I ran into some trouble with this until I snapped the clip and the position to a rounded rect. There was a bug on this a long time ago where code similar to this was added to gfxWindowsNativeDrawing. Once I rounded these off, the cairo Paint path simplified quite a bit and a number of drawing anomalies went away. So I eblieve I'm handing this correctly.

Comment 64

[Brian R. Bondy [:bbondy]]

Comment on attachment 597499 [details] [diff] [review]
render routines v.1

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

::: widget/windows/WinUtils.cpp
@@ +67,5 @@
> +#endif
> +
> +#ifndef min
> +#define min(a,b)            (((a) < (b)) ? (a) : (b))
> +#endif

I think we can just use std::max/min here from 
#include <algorithm>

@@ +70,5 @@
> +#define min(a,b)            (((a) < (b)) ? (a) : (b))
> +#endif
> +
> +#ifdef DEBUG
> +#include <GdiPlus.h>

All lowercase

@@ +412,5 @@
> +#ifdef DEBUG
> +
> +// Win xp and up
> +#pragma comment(lib, "gdiplus.lib")
> +static ULONG_PTR gGiplusToken = NULL;

Instead of a global variable for gGiplusToken just make it a static variable inside of SaveDCToPng

@@ +413,5 @@
> +
> +// Win xp and up
> +#pragma comment(lib, "gdiplus.lib")
> +static ULONG_PTR gGiplusToken = NULL;
> +static int gPngFileIndex = 0;

Instead of a global variable for gPngFileIndex just make it a static variable inside of SaveDCToPngIndexed

@@ +416,5 @@
> +static ULONG_PTR gGiplusToken = NULL;
> +static int gPngFileIndex = 0;
> +
> +static bool
> +GetEncoderClsid(const WCHAR* format, CLSID* pClsid)

The other added functions have a prefix of a on the parameters.

@@ +424,5 @@
> +  UINT size = 0;
> +
> +  ImageCodecInfo* pImageCodecInfo = NULL;
> +
> +  GetImageEncodersSize(&num, &size);

Can check for return value not Ok and do early return here.

@@ +432,5 @@
> +  pImageCodecInfo = (ImageCodecInfo*)(malloc(size));
> +  if(!pImageCodecInfo)
> +    return false;
> +
> +  GetImageEncoders(num, size, pImageCodecInfo);

Can check for return value not Ok and do early return here.

@@ +470,5 @@
> +  NS_WARN_IF_FALSE(memdc, "CreateCompatibleDC failed");
> +  if (!memdc)
> +    return;
> +  membit = CreateCompatibleBitmap(aHdc, aWidth, aHeight);
> +  NS_WARN_IF_FALSE(memdc, "CreateCompatibleBitmap failed");

You meant to check membit here.

@@ +472,5 @@
> +    return;
> +  membit = CreateCompatibleBitmap(aHdc, aWidth, aHeight);
> +  NS_WARN_IF_FALSE(memdc, "CreateCompatibleBitmap failed");
> +  if (!membit)
> +    return;

DeleteObject(memdc); on this error condition.

@@ +473,5 @@
> +  membit = CreateCompatibleBitmap(aHdc, aWidth, aHeight);
> +  NS_WARN_IF_FALSE(memdc, "CreateCompatibleBitmap failed");
> +  if (!membit)
> +    return;
> +  HBITMAP hOldBitmap =(HBITMAP) SelectObject(memdc, membit);

nit: SelectObject could fail so pls have return check here or below when restoring. "If an error occurs and the selected object is not a region, the return value is NULL. Otherwise, it is HGDI_ERROR."

@@ +474,5 @@
> +  NS_WARN_IF_FALSE(memdc, "CreateCompatibleBitmap failed");
> +  if (!membit)
> +    return;
> +  HBITMAP hOldBitmap =(HBITMAP) SelectObject(memdc, membit);
> +  BitBlt(memdc, 0, 0, aWidth, aHeight, aHdc, aXPos, aYPos, SRCCOPY);

nit: Maybe NS_WARN_IF_FALSE here.

@@ +484,5 @@
> +    NS_WARNING("GetEncoderClsid failed");
> +    return;
> +  }
> +
> +  DeleteFileW(aFilePath);

Since you already have an nsIFile you can just use Remove() on that, but if you'd rather this directly that's fine too.

@@ +487,5 @@
> +
> +  DeleteFileW(aFilePath);
> +  bitmap.Save(aFilePath, &clsid);
> +
> +  SelectObject(memdc, hOldBitmap);

nit: warn here if failure.

@@ +494,5 @@
> +}
> +
> +/* static */
> +void
> +WinUtils::SaveDCToPngIndexed(HDC aHdc, WCHAR* aDirectory, WCHAR* aAnnotation,

I think these strings can be const: LPCWSTR

@@ +499,5 @@
> +                             int aXPos, int aYPos, int aWidth, int aHeight)
> +{
> +  WCHAR filename[512];
> +  filename[0] = 0;
> +  wsprintf(filename, L"%s\\%s_image-%d.png", aDirectory, aAnnotation,

nit: StringCbPrintf

Comment 65

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #64)
> Comment on attachment 597499 [details] [diff] [review]
> render routines v.1
> 
> Review of attachment 597499 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/WinUtils.cpp
> @@ +67,5 @@
> > +#endif
> > +
> > +#ifndef min
> > +#define min(a,b)            (((a) < (b)) ? (a) : (b))
> > +#endif
> 
> I think we can just use std::max/min here from 
> #include <algorithm>

One note on this - we define NOMINMAX in various parts of the build. min/max here are needed by gdiplugtypes.h which normally pull these from WinDef.h.

Comment 66

[Jim Mathies [:jimm]]

Attachment: scrollbars v.1

Comment 67

[Jim Mathies [:jimm]]

Attachment: base rollup

This is everything that sits below scrollbars.

Comment 68

[Jim Mathies [:jimm]]

Attachment: base rollup

this time without the build error.

Comment 69

[Brian R. Bondy [:bbondy]]

Comment on attachment 598851 [details] [diff] [review]
scrollbars v.1

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

Cool! Looks good :) Will be a quick second pass.

::: widget/windows/nsNativeThemeWin.cpp
@@ +526,5 @@
> +/*
> + * GetThemedTransitionDuration - returns the duration of a fading
> + * widget transition in milliseconds.
> + *
> + * aTheme        An open theme handle used in rendering.

nit for javadoc: 
* @param aTheme An open theme handle used in rendering.
Please fix here and elsewhere.

@@ +532,5 @@
> + *               animation.
> + * aStateIdTo    Theme state that will act as the ending point of an
> + *               animation.
> + *
> + * returns 0 if there is no fade sequence for aPartId between

nit:
@return 0 if there is no fade sequence for aPartId between

@@ +1806,5 @@
> +       gripSize.cx + thumbMgns.cxLeftWidth + thumbMgns.cxRightWidth <=
> +         widgetRect.right - widgetRect.left &&
> +       gripSize.cy + thumbMgns.cyTopHeight + thumbMgns.cyBottomHeight <=
> +         widgetRect.bottom - widgetRect.top) ?
> +       true : false;

nit: "? true: false" is not needed since the expression will already evaluate to true or false.

@@ +1808,5 @@
> +       gripSize.cy + thumbMgns.cyTopHeight + thumbMgns.cyBottomHeight <=
> +         widgetRect.bottom - widgetRect.top) ?
> +       true : false;
> +
> +    nsIContent* content = aFrame->GetContent();

Check for NULL content

@@ +1815,5 @@
> +                                                 TS_NORMAL,
> +                                                 TS_HOVER);
> +    // Rendering the scrollbar
> +    if ((WinUtils::GetWindowsVersion() != WinUtils::WIN7_VERSION &&
> +         WinUtils::GetWindowsVersion() != WinUtils::VISTA_VERSION) ||

Should this check also exclude future windows versions like Win8 Desktop?

@@ +1851,5 @@
> +           aWidgetType == NS_THEME_SCROLLBAR_BUTTON_DOWN ||
> +           aWidgetType == NS_THEME_SCROLLBAR_BUTTON_LEFT ||
> +           aWidgetType == NS_THEME_SCROLLBAR_BUTTON_RIGHT) {
> +    int basicState = GetScrollbarButtonBasicState(state);
> +    nsIContent* content = aFrame->GetContent();

Check for NULL content

@@ +1859,5 @@
> +                                                 SBP_ARROWBTN,
> +                                                 ABS_UPNORMAL,
> +                                                 ABS_UPHOT);
> +    if ((WinUtils::GetWindowsVersion() != WinUtils::WIN7_VERSION &&
> +         WinUtils::GetWindowsVersion() != WinUtils::VISTA_VERSION) ||

Ditto should we also exclude higher versions than Win7 here?

::: widget/windows/nsUXThemeData.cpp
@@ +105,5 @@
>    NS_ASSERTION(!sThemeDLL, "nsUXThemeData being initialized twice!");
>  
> +  if (GetThemeDLL()) {
> +    // Vista and up entry points:
> +    getThemeTransitionDuration = (GetThemeTransitionDurationPtr)GetProcAddress(sThemeDLL, "GetThemeTransitionDuration");

nit: line length
By the way I recently learnt about this trick if you don't already have it:
max char indicator (vertical bar) in Visual Studio's editor http://goo.gl/a8jzG

Comment 70

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #69)
> Comment on attachment 598851 [details] [diff] [review]
> scrollbars v.1
> 
> Review of attachment 598851 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +1815,5 @@
> > +                                                 TS_NORMAL,
> > +                                                 TS_HOVER);
> > +    // Rendering the scrollbar
> > +    if ((WinUtils::GetWindowsVersion() != WinUtils::WIN7_VERSION &&
> > +         WinUtils::GetWindowsVersion() != WinUtils::VISTA_VERSION) ||
> 
> Should this check also exclude future windows versions like Win8 Desktop?

Don't think so since the Win8 desktop has the same fade transitions as Win7.

> nit: line length
> By the way I recently learnt about this trick if you don't already have it:
> max char indicator (vertical bar) in Visual Studio's editor
> http://goo.gl/a8jzG

Thanks, I'll check this out.

Comment 71

[Brian R. Bondy [:bbondy]]

> > Should this check also exclude future windows versions like Win8 Desktop?
> Don't think so since the Win8 desktop has the same fade transitions as Win7.

Which is why I think Win8 handling (and later) should be handled in the same way as Win7.

Comment 72

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #71)
> > > Should this check also exclude future windows versions like Win8 Desktop?
> > Don't think so since the Win8 desktop has the same fade transitions as Win7.
> 
> Which is why I think Win8 handling (and later) should be handled in the same
> way as Win7.

Ah, yes, sorry, I wasn't reading those checks right. I'll update them.

Comment 73

[Jim Mathies [:jimm]]

> +       gripSize.cy + thumbMgns.cyTopHeight + thumbMgns.cyBottomHeight <=
> +         widgetRect.bottom - widgetRect.top) ?
> +       true : false;
> +
> +    nsIContent* content = aFrame->GetContent();

> Check for NULL content

Actually GetFadeState checks for null and returns an appropriate constant, and FinishFadeIn ignores the request if content is null, so these checks aren't needed. However I am missing an aFrame null check here, which I'll add.

Comment 74

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #73)
> > +       gripSize.cy + thumbMgns.cyTopHeight + thumbMgns.cyBottomHeight <=
> > +         widgetRect.bottom - widgetRect.top) ?
> > +       true : false;
> > +
> > +    nsIContent* content = aFrame->GetContent();
> 
> > Check for NULL content
> 
> Actually GetFadeState checks for null and returns an appropriate constant,
> and FinishFadeIn ignores the request if content is null, so these checks
> aren't needed. However I am missing an aFrame null check here, which I'll
> add.

Hmm, QueueAnimation asserts on null content. So I guess I need a check on both.

Comment 75

[Jim Mathies [:jimm]]

Attachment: scrollbars v.2

Comment 76

[Jim Mathies [:jimm]]

Attachment: buttons v.1

Comment 77

[Jim Mathies [:jimm]]

Attachment: check radio v.1

Comment 78

[Jim Mathies [:jimm]]

Comment on attachment 599116 [details] [diff] [review]
scrollbars v.2

crap forgot the version check updates in some of these.

Comment 79

[Jim Mathies [:jimm]]

Attachment: scrollbars v.2

Comment 80

[Jim Mathies [:jimm]]

Attachment: buttons v.1

Comment 81

[Jim Mathies [:jimm]]

Attachment: check radio v.1

Comment 82

[Jim Mathies [:jimm]]

Attachment: toolbar buttons v.1

Comment 83

[Brian R. Bondy [:bbondy]]

Comment on attachment 599120 [details] [diff] [review]
scrollbars v.2

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

Awesome!

Comment 84

[Jim Mathies [:jimm]]

Attachment: testfixes v.1

Comment 85

[Mounir Lamouri (:mounir)]

Comment on attachment 599984 [details] [diff] [review]
testfixes v.1

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

::: content/events/test/test_bug426082.html
@@ +70,5 @@
>        }
> +      // On Vista+ we now support state transitions on themed
> +      // content, so give the button time to normalize.
> +      if (navigator.appVersion.indexOf("Win") != -1)
> +        setTimeout(execNext, 500);

I'm not sure I understand why that is needed. Could you explain?

Comment 86

[Jim Mathies [:jimm]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #85)
> Comment on attachment 599984 [details] [diff] [review]
> testfixes v.1
> 
> Review of attachment 599984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/events/test/test_bug426082.html
> @@ +70,5 @@
> >        }
> > +      // On Vista+ we now support state transitions on themed
> > +      // content, so give the button time to normalize.
> > +      if (navigator.appVersion.indexOf("Win") != -1)
> > +        setTimeout(execNext, 500);
> 
> I'm not sure I understand why that is needed. Could you explain?

The test fires various mouse events on buttons and compares image snapshots to check for correct state. The patches here introduce state transitions that animate over time. So before a comparison can be made, we have to wait for the state transition to complete, otherwise the test will fail.

Comment 87

[Jim Mathies [:jimm]]

Here's a typical failure:

failed | Removing the label should have unpressed the button. - 

expected - 

data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEEAAAAcCAYAAAA+59JsAAACe0lEQVRYhe2Yy4riQBSG610M5FG0XdjzHl7Ipswr2NnEdNbOvIPY4t6WWYoiQRQ34kJixEvURQz8vXA6JJkkfRxaHILCB3VOVZ069YFZFMPjB3bvBv6H30MCIiToug5ZliFJUuqQZRm6ridLeFEUaJqG5XIJ13VTx3K5hKZpeFGUeAmcc5imCcdxsN/vU4fjODBNE5zzeAmSJMF13bs3e0tc14UkSckSzuczdrtdajmfzzQJ2+02tZAlbDYbAr/wgzGwMKKM36T994EkwXEcrNdrAj/xzERUe8F8ryqCiVX0rtofXesWOI5Dk2BZFoHGpfH3UP69CpE9o3HV/phaN4AsYbVaEWigwETwbjDf5SJYoRGz5jPugosRfyVvbWhe5OiGz+UFb17kXWLPK5qE0+mE2WxGQEc+6psgVNAJrBFQ6UTFceMZ9DwDy+veWcH4z7mfcacCgeWhk3qe4XQ6fbcE/wV9DXsi/kVCRN3ARZNqfqOE6XRK4BVPTEC5Hcq3yxC8fHiNP44Zt8sQ2BNeY89Kqvk1ZAmTyYSAdjn8LZR/u1xCi1zjjynja2t+DUnC8XjEeDwmUEeOCSi1gvl6joEJJbS8NQy5+mWuVRLAvD3+/cFa9RwDy9WDNb04fG50H3Ecj0eaBMMwCKjIRn0YM0U0/evUrDeXKRaRZRkUmwYMo4li5pLPqv5xcO7vmqqvRlScDEnC4XDAaDRKLYfDgSZhOBymFpIE27YxGAxSi23byRI455jP57AsC/1+P3VYloX5fJ78qKIoClRVxWKxgG3bqWOxWEBVVShJz2sAUKvVwDm/+6PoLeCco1arha/8eHIHHhIAPCQAAD4AjWe3StwvxHUAAAAASUVORK5CYII=

but got -

data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEEAAAAcCAYAAAA+59JsAAACtUlEQVRYhe2YzUsbQRiH50+pJbL7bwR70rZWCAS8WdoeY1qIW7PiRUrFHI2sB9uFpiBU71bWSkuKSO2xlKKWYhNTG+Pmg03cr/DrIXXZXXc3Y1EsSwIPzPvOO+/MPJA9DEHvB3LdB/gffj0J8JDAZ5cxOr2I2GQ2dIxOL4LPLgdLGM+I4IVVFMoyjHY7dBTKMnhhFeMZ0V9CnBdQqtSgGSZOFDV0aIaJUqWGOC/4S4hNZmG026goamgx2m3EJrPBEnTDxLGihhbdMLtL0AwTvxut0KLRSjiqtyh4jTuEgLhheXygWn89UEs4rDUpyOE2YfFk25nffMqCsGlsXmi9d6+rgEqCapgoyAoFnYM/3nLlt9LoJyMQLrTep9cVoNJKOJAVCnIYIiySW868xLEgwzmfmrN4B0nG469k1brmmTQk977ciDXfz+1QnplSQkPV8flnlQIRt7y+Cf0c3jhqGDza8Ir9xlXMDhGQIdHayxn/3fcs3uBwk9zDLNWZq2io+mVLsF/QdmBLxL9I8OjruGhQz8uScKrj08EJBS8xQBg8lFx5aQJ9Vt5dY499xtIE+sgwnvvuFdSzO41TCgn1Ux3bPyoUvMAAYfBg3ZVf71zimWeNPaYZX7Rnd+o0EmotDR+/lylYQpQwuP/WmZ8ZJCCRFHJWDUF0vjOXSzAg1hr7emevmUECMrjk7GnF7n29z+FHraXRScjvlynoXPDchzGSwit73fxda+5GIoUoYTC2VkZ+P4+xSCcfnbePnXPney7ZenjFwVBJqLY0vN89Ci1VGglyU8O7b79Ci9yklCB9PQwtXSXEpwTsFo9RkBWsfymGjoKsYLd4jPhUwKNKMiOCW1jBXqmCk6YaOvZKFXALK0gGPa8BQGJORJwXrv1R9CqI8wISc6L7yr0nd6AnAUBPAgDgD6Ybzn8F250qAAAAAElFTkSuQmCC

Comment 88

[Mounir Lamouri (:mounir)]

Comment on attachment 599984 [details] [diff] [review]
testfixes v.1

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

So, I understand we are taking snapshots when the animation is still happening. But I think it's quite dangerous to come up with arbitrary setTimeout() to prevent that: it's a very good way to ends up with random oranges.

I think compareSnapshot() should have a specialized method for windows that will call itself back after going to the event loop if the check fails. After X fails (so X times hitting the event loop), we can assume the test really failed. I think that would make the test much less fragile but you will have to make it async and that might be a bit annoying but still doable in less than half an hour I believe.

How does that sound?

Comment 89

[Jim Mathies [:jimm]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #88)
> Comment on attachment 599984 [details] [diff] [review]
> testfixes v.1
> 
> Review of attachment 599984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, I understand we are taking snapshots when the animation is still
> happening. But I think it's quite dangerous to come up with arbitrary
> setTimeout() to prevent that: it's a very good way to ends up with random
> oranges.
> 
> I think compareSnapshot() should have a specialized method for windows that
> will call itself back after going to the event loop if the check fails.
> After X fails (so X times hitting the event loop), we can assume the test
> really failed. I think that would make the test much less fragile but you
> will have to make it async and that might be a bit annoying but still doable
> in less than half an hour I believe.
> 
> How does that sound?

I don't see what we gain from this. Knowing that a few events were processed might add a little stability to the overall test, but since we don't know how much time one iteration of the event loop represents (right? on slow systems, it represents a lot of time, on fast systems, very little) we'll still need the test to keep checking for a set amount of time. Processing 5/10/20 events doesn't necessarily answer the question "have we waited long enough?". Unless I'm missing something. :)

For reference - the max interval any of these transitions takes is 350msec.

Comment 90

[Mounir Lamouri (:mounir)]

We could also do X pass trough the event loop minimum + Y ms minimum (which means, we could have situations like 10 go trough the event loop and 500 ms or 5 go trough the event loop and 1000 ms if X = 5 and Y = 500).

Does that sound a good compromise?

Comment 91

[Jim Mathies [:jimm]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #90)
> We could also do X pass trough the event loop minimum + Y ms minimum (which
> means, we could have situations like 10 go trough the event loop and 500 ms
> or 5 go trough the event loop and 1000 ms if X = 5 and Y = 500).
> 
> Does that sound a good compromise?

I can try and work something like this up. I'm bogged down in Win8 work though so how about a compromise - let me land these as they are, I'll file a follow up to add support to simple test for this type of situation, and if we get any random orange on this test I'll either back out or make getting the test bug fixed a priority? :)

Comment 92

[Mounir Lamouri (:mounir)]

Sounds good. r=me for that plan.

Comment 93

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d43b8507915
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b80fa3e587d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4d338502df0
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c72a1f066ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/1104607364f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f9284edace4
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3739a2c19c1

Comment 94

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out on inbound because of leaks in Windows M4 and mochitest-other:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20fc7b89954c

Comment 95

[Stephen A Pohl [:spohl] (OOO until 2/27)]

We have switched to non-native theming for form controls and scrollbars on all platforms in bug 1535761. Any discrepancy in fade/animation behavior reported here is no longer applicable. Any issues pertaining to the new, non-native scrollbars will be handled by new bugs.