Last Comment Bug 693319 - Factor out common code from RasterImage checking whether flags match
: Factor out common code from RasterImage checking whether flags match
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
:
Mentors:
Depends on: 683290
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-10 09:12 PDT by Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
Modified: 2012-08-15 13:36 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (5.69 KB, patch)
2011-10-10 13:22 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
bobbyholley: review+
Details | Diff | Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-10-10 09:12:32 PDT
RasterImage has some code that checks whether or not the flags it's handed match the flags it was decoded with and if not, whether another decode can be done.  This code exists in triplicate and should be factored out.
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-10-10 13:22:56 PDT
Created attachment 566011 [details] [diff] [review]
Patch
Comment 2 Bobby Holley (PTO through June 13) 2011-10-10 14:47:15 PDT
Comment on attachment 566011 [details] [diff] [review]
Patch

>+  bool TryToChangeFlags(PRUint32 aNewFlags);

Looks great except for the name. In particular, it doesn't specify the type of flags (decode flags), and we might not even be changing them at all.

ApplyDecodeFlags, maybe? Use your judgement here.

r=bholley with that
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-10-11 07:22:18 PDT
Comment on attachment 566011 [details] [diff] [review]
Patch

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

::: modules/libpr0n/src/RasterImage.cpp
@@ +854,5 @@
>  
> +bool
> +RasterImage::TryToChangeFlags(PRUint32 aNewFlags)
> +{
> +  if (mFrameDecodeFlags == aNewFlags & DECODE_FLAGS_MASK)

That wants to be if (mFrameDecodeFlags == (aNewFlags & DECODE_FLAGS_MASK)), of course ..

/me whistles
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-08-15 13:36:41 PDT
https://hg.mozilla.org/mozilla-central/rev/a33215aa549c

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