Last Comment Bug 708443 - minor cleanup in animationPolling.js
: minor cleanup in animationPolling.js
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on:
Blocks: 702093
  Show dependency treegraph
 
Reported: 2011-12-07 15:27 PST by Daniel Holbert [:dholbert]
Modified: 2011-12-09 06:54 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1 v1 (4.76 KB, patch)
2011-12-07 17:46 PST, Daniel Holbert [:dholbert]
jaywir3: review+
Details | Diff | Review
patch 2 v1: turn off debug output (w/ boolean global), and add emacs mode line (1.28 KB, patch)
2011-12-08 12:32 PST, Daniel Holbert [:dholbert]
jaywir3: review+
Details | Diff | Review

Description Daniel Holbert [:dholbert] 2011-12-07 15:27:44 PST
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] 2011-12-07 15:28:13 PST
er I meant "or is _it_ there for a reason"
Comment 2 Daniel Holbert [:dholbert] 2011-12-07 16:48:53 PST
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] 2011-12-07 17:00:54 PST
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] 2011-12-07 17:46:18 PST
Created attachment 579917 [details] [diff] [review]
patch 1 v1
Comment 5 Daniel Holbert [:dholbert] 2011-12-07 17:50:10 PST
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) 2011-12-08 08:41:35 PST
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] 2011-12-08 12:32:13 PST
Created attachment 580143 [details] [diff] [review]
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) 2011-12-08 12:38:07 PST
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] 2011-12-08 13:36:34 PST
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] 2011-12-08 17:42:14 PST
Landed patch 2:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/9b50bc5ef548
Comment 11 Ed Morley [:emorley] 2011-12-09 06:44:40 PST
https://hg.mozilla.org/mozilla-central/rev/9b50bc5ef548
Comment 12 Ed Morley [:emorley] 2011-12-09 06:54:53 PST
https://hg.mozilla.org/mozilla-central/rev/68f0d4831c48

Note You need to log in before you can comment on or make changes to this bug.