Open Bug 373266 Opened 13 years ago Updated 3 months ago

Add support for Windows widget theme animations

Categories

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

x86
Windows Vista
defect

Tracking

()

People

(Reporter: ddayon, Unassigned)

References

(Blocks 4 open bugs)

Details

(Keywords: polish, Whiteboard: [polish-hard][polish-interactive][polish-p1][tpi:+])

Attachments

(8 files, 25 obsolete files)

19.51 KB, text/html
Details
20.58 KB, patch
joe
: review+
Details | Diff | Splinter Review
19.60 KB, patch
roc
: review+
Details | Diff | Splinter Review
24.59 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
2.85 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
4.09 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
2.48 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
919 bytes, patch
mounir
: review-
Details | Diff | Splinter Review
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.
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.
Did you also test this in Firefox's -safe-mode?
Blocks: 333484
Can't say I'd previously noticed the difference myself, but it is there.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #2)
> Did you also test this in Firefox's -safe-mode?
> 

Safe mode makes no difference.
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.
Depends on: 391541
Depends on: 392644
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.
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...
(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 :)
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.
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. 
At its smallest SIZE not SIDE. Sorry again.
(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.
Version: unspecified → Trunk
this bug is eligible for bug 462080
Keywords: polish
Whiteboard: [polish-hard][polish-interactive]
Whiteboard: [polish-hard][polish-interactive] → [polish-hard][polish-interactive][polish-high-visibility]
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.
For normal use this is completely insane, perhaps this is a limited mobility accessibility feature?
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.
>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.
thanx
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 :)
Whiteboard: [polish-hard][polish-interactive][polish-high-visibility] → [polish-hard][polish-interactive][polish-p1]
Component: Shell Integration → Widget: Win32
Product: Firefox → Core
QA Contact: shell.integration → win32
Blocks: 635355
Assignee: nobody → jmathies
Depends on: 719983
Attached patch wip (obsolete) — Splinter Review
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.
Duplicate of this bug: 542862
generalizing this up.
Summary: Scrollbar doesn't behave like other applications in Vista → Add support for Windows widget theme animations
Attached patch wip (obsolete) — Splinter Review
scrollbars are complete, I have animation working on the gripper, buttons, and the base thumb track. Still have to look at D2D issues.
Attachment #594347 - Attachment is obsolete: true
Attached file test case
Attached patch wip (obsolete) — Splinter Review
- D2D rendering issues fixed
- filed bug 724933 on a scrollbar bug I found while testing
Attachment #594832 - Attachment is obsolete: true
Blocks: 656417
Attached patch base v.1 (obsolete) — Splinter Review
Attachment #595051 - Attachment is obsolete: true
Attached patch scrollbars v.1 (obsolete) — Splinter Review
Attached patch buttons v.1 (obsolete) — Splinter Review
Attached patch base v.1 (obsolete) — Splinter Review
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?
Attachment #595198 - Attachment is obsolete: true
Attachment #595207 - Flags: review?(roc)
(Note, to apply these you'll also need the patch in bug 719983 applied.)
I probably won't get to this until next week.
Blocks: 720703
Attached patch base v.2 (obsolete) — Splinter Review
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.
Attachment #595207 - Attachment is obsolete: true
Attachment #595929 - Flags: review?(roc)
Attachment #595207 - Flags: review?(roc)
Attached patch testfixes v.1 (obsolete) — Splinter Review
Ref test fix which was thrown off by a fading element background.
Attached patch base v.2 (obsolete) — Splinter Review
sorry for the review spam - fixed up some formatting and grammar.
Attachment #595929 - Attachment is obsolete: true
Attachment #595934 - Flags: review?(roc)
Attachment #595929 - Flags: review?(roc)
Attached patch scrollbars v.2 (obsolete) — Splinter Review
Attachment #595199 - Attachment is obsolete: true
Attached patch buttons v.2 (obsolete) — Splinter Review
Attachment #595201 - Attachment is obsolete: true
Attached patch base v.2 (obsolete) — Splinter Review
This addresses a build issue on unix systems, and fixes a problem on linux where nsNativeThemeGTK had already implemented nsIObserver.
Attachment #595934 - Attachment is obsolete: true
Attachment #596254 - Flags: review?(roc)
Attachment #595934 - Flags: review?(roc)
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.
Attachment #595938 - Attachment is obsolete: true
Attachment #595939 - Attachment is obsolete: true
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.
Blocks: 726306
(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 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?
(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.
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.
(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.
Blocks: 726818
Attached patch base v.3 (obsolete) — Splinter Review
Attachment #596254 - Attachment is obsolete: true
Attachment #596254 - Flags: review?(roc)
Comment on attachment 597311 [details] [diff] [review]
base v.3

Updated per comments plus some other cleanup.
Attachment #597311 - Flags: review?(roc)
Attached patch render routines v.1 (obsolete) — Splinter Review
for reference, plus I'll post scrollbars code.
Attached patch scrollbars v.1 (obsolete) — Splinter Review
Attached patch render routines v.1 (obsolete) — Splinter Review
fixing up comment header typos in render routines v.1
Attachment #597312 - Attachment is obsolete: true
Attachment #597313 - Attachment is obsolete: true
Attached patch scrollbars v.1 (obsolete) — Splinter Review
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/jmathies@mozilla.com-efa5576ce9ef
Another try result will be coming up in an hour or two with no major changes and some additional cleanup / comment updates.
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.
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/jmathies@mozilla.com-eee9580d929e
Attached patch base v.4 (obsolete) — Splinter Review
comments addressed.
Attachment #597311 - Attachment is obsolete: true
Attachment #597426 - Flags: review?(roc)
Attachment #597311 - Flags: review?(roc)
Joe, is this something you'd be ok with reviewing?
Attachment #597315 - Attachment is obsolete: true
Attachment #597499 - Flags: review?(joe)
(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 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);
(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.
Attached patch base v.5Splinter Review
comments addressed.
Attachment #597426 - Attachment is obsolete: true
Attachment #597817 - Flags: review?(roc)
Attachment #597426 - Flags: review?(roc)
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.
Attachment #597817 - Flags: review?(roc) → review+
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.
Attachment #597499 - Flags: review?(netzen)
Attachment #597499 - Flags: review?(joe)
Attachment #597499 - Flags: review+
(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 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
Attachment #597499 - Flags: review?(netzen)
Depends on: 699247
(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.
Attached patch scrollbars v.1 (obsolete) — Splinter Review
Attachment #595930 - Attachment is obsolete: true
Attachment #597317 - Attachment is obsolete: true
Attachment #598851 - Flags: review?(netzen)
Attached patch base rollup (obsolete) — Splinter Review
This is everything that sits below scrollbars.
Attached patch base rollup (obsolete) — Splinter Review
this time without the build error.
Attachment #598852 - Attachment is obsolete: true
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
Attachment #598851 - Flags: review?(netzen)
(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.
> > 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.
(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.
> +       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.
(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.
Attached patch scrollbars v.2 (obsolete) — Splinter Review
Attachment #598851 - Attachment is obsolete: true
Attachment #599116 - Flags: review?(netzen)
Attached patch buttons v.1 (obsolete) — Splinter Review
Attachment #599118 - Flags: review?(netzen)
Attached patch check radio v.1 (obsolete) — Splinter Review
Attachment #599119 - Flags: review?(netzen)
Comment on attachment 599116 [details] [diff] [review]
scrollbars v.2

crap forgot the version check updates in some of these.
Attachment #599116 - Flags: review?(netzen)
Attachment #599118 - Flags: review?(netzen)
Attachment #599119 - Flags: review?(netzen)
Attached patch scrollbars v.2Splinter Review
Attachment #599116 - Attachment is obsolete: true
Attachment #599120 - Flags: review?(netzen)
Attached patch buttons v.1Splinter Review
Attachment #599118 - Attachment is obsolete: true
Attachment #599121 - Flags: review?(netzen)
Attached patch check radio v.1Splinter Review
Attachment #599119 - Attachment is obsolete: true
Attachment #599122 - Flags: review?(netzen)
Attachment #599124 - Flags: review?(netzen)
Attachment #599124 - Flags: review?(netzen) → review+
Attachment #599122 - Flags: review?(netzen) → review+
Attachment #599121 - Flags: review?(netzen) → review+
Comment on attachment 599120 [details] [diff] [review]
scrollbars v.2

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

Awesome!
Attachment #599120 - Flags: review?(netzen) → review+
Attached patch testfixes v.1Splinter Review
Attachment #598853 - Attachment is obsolete: true
Attachment #599984 - Flags: review?(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?
(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.
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 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?
Attachment #599984 - Flags: review?(mounir) → review-
(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.
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?
(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? :)
Sounds good. r=me for that plan.
Blocks: 733769
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
No longer blocks: 726306
Assignee: jmathies → nobody
Blocks: 726306
Blocks: 333564
Priority: -- → P4
Whiteboard: [polish-hard][polish-interactive][polish-p1] → [polish-hard][polish-interactive][polish-p1][tpi:]
Whiteboard: [polish-hard][polish-interactive][polish-p1][tpi:] → [polish-hard][polish-interactive][polish-p1][tpi:+]
You need to log in before you can comment on or make changes to this bug.