708443 - minor cleanup in animationPolling.js

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

animationPolling.js calls a function "imageLoadCallback" here:
>26 function failTest ()
>27 {    imageLoadCallback();
http://mxr.mozilla.org/mozilla-central/source/image/test/mochitest/animationPolling.js?mark=27-27#26

but that function isn't defined anywhere, as shown by this search:
http://mxr.mozilla.org/mozilla-central/search?string=imageLoadCallback

Looks like a typo.  Scott, should it be removed, or is there for a reason and we need to make it actually do something?

Comment 1

[Daniel Holbert [:dholbert]]

er I meant "or is _it_ there for a reason"

Comment 2

[Daniel Holbert [:dholbert]]

Looks like this line was added in:
 http://hg.mozilla.org/mozilla-central/rev/47131206c09f
and that cset also removed the definition of "imageLoadCallback".

Comment 3

[Daniel Holbert [:dholbert]]

Taking - I noticed a few other things in animationPolling that could use a tweak, too - I'll just post 'em in a combined patch here.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch 1 v1

Comment 5

[Daniel Holbert [:dholbert]]

Summary of changes:
 - Remove the invalid "imageLoadCallback()" call. (I verified that, in the case of test failure, we currently throw exceptions due to imageLoadCallback not existing -- but with that call removed, we correctly report the failure as "test still doesn't match ref after call #N etc")
 - Removed space between function-name and () in a few spots
 - Fixed some greater-than-80-characters lines (mostly "compareSnapshots" calls).
 - Changed a setTimeout(..., 10) to executeSoon (which does the same thing, basically, without a magic number)

 - changed a repeated setTimeout(..., 20) to use currentTest.pollFreq
 - fixed comment & variable-name in clause where "reference div" isn't actually a div

Comment 6

[Scott Johnson (:jwir3)]

Comment on attachment 579917 [details] [diff] [review]
patch 1 v1

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

Wow, thanks for catching some of these half-line typos. I can't believe it was passing with these.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v1: turn off debug output (w/ boolean global), and add emacs mode line

Currently animationPolling triggers a lot of debugging screenshots to be printed to mochitest logs.

This patch turns that off (but adds a boolean var so we can toggle it on again if necessary for debugging an issue).

I also added an emacs modeline at the top (copypasted from imgutils.js), so that emacs knows to indent by 2 spaces instead of 4.

Comment 8

[Scott Johnson (:jwir3)]

Comment on attachment 580143 [details] [diff] [review]
patch 2 v1: turn off debug output (w/ boolean global), and add emacs mode line

dholbert and I discussed this on IRC. Thanks for doing this, dholbert! r=me.

Comment 9

[Daniel Holbert [:dholbert]]

Landed patch 1 (I gave it a TryServer run overnight last night):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/68f0d4831c48

I'll give patch 2 a TryServer sanity-check and then land it later today.

Comment 10

[Daniel Holbert [:dholbert]]

Landed patch 2:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/9b50bc5ef548

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9b50bc5ef548

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/68f0d4831c48