Closed Bug 587371 Opened 10 years ago Closed 10 years ago

Image.h PRUint32 getters shouldn't be COM-y

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file, 1 obsolete file)

Now that we're not using idl for this stuff anymore, we should simplify calling conventions (in particular, I'm doing this so I can more easily assert things in my patch stack).
Attached patch patch v1 (obsolete) — Splinter Review
Comment on attachment 466041 [details] [diff] [review]
patch v1

Added a patch.

dholbert, this is going to touch your svg stuff too. Do you think you can rebase your patches on top of it?
Attachment #466041 - Flags: review?(joe)
Comment on attachment 466041 [details] [diff] [review]
patch v1

doh - because of where this was in my patch queue, the decoders weren't actually building when I typed make. So I didn't notice that there area a bunch of these calls in the decoders. Let me fix that.
Attachment #466041 - Flags: review?(joe)
(In reply to comment #2)
> dholbert, this is going to touch your svg stuff too. Do you think you can
> rebase your patches on top of it?

Yup, that's fine.  Thanks for the heads-up!
Attached patch patch v2Splinter Review
Added a patch that handles the decoders as well
Attachment #466041 - Attachment is obsolete: true
Attachment #466060 - Flags: review?(joe)
Comment on attachment 466060 [details] [diff] [review]
patch v2

I love this patch. It has so many removed lines!

I am a little bit concerned by the fact that we're removing some mError checks and just returning a value which might be totally invalid. It's probably not the end of the world, or even a problem, but make sure you run a build with this patch through its paces with invalid images.
Attachment #466060 - Flags: review?(joe) → review+
(In reply to comment #6)
> Comment on attachment 466060 [details] [diff] [review]
> patch v2
> 
> I love this patch. It has so many removed lines!
> 

Then you are going to loooove the 20-ish patches I'm going to flag you for soon! ;-)

> I am a little bit concerned by the fact that we're removing some mError checks
> and just returning a value which might be totally invalid. It's probably not
> the end of the world, or even a problem, but make sure you run a build with
> this patch through its paces with invalid images.

I wouldn't worry about it too much. I added those checks in decode-on-draw:
http://hg.mozilla.org/mozilla-central/diff/9f856f094fea/modules/libpr0n/src/imgContainer.cpp#l1.457

So we've never actually shipped with them.

dholbert, what's your timing like for landing svg? Should I plan on landing this with my stack (hope to land sometime this week), or are you likely to beat me to it?
blocks 2 blockers. Nominating.

(Nom nom nom...)
Blocks: 276431, 513681
blocking2.0: --- → ?
blocking2.0: ? → beta5+
(In reply to comment #7)
> dholbert, what's your timing like for landing svg? Should I plan on landing
> this with my stack (hope to land sometime this week), or are you likely to beat
> me to it?

I'm intending to have patches up for review later this week, but I don't think they'll be landing until next week -- so, I'm guessing you'll beat me to landing.

I've added this to my patch queue and am layering on top of it.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Blocks: 587800
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/3aeb7495b4f1

Resolving fixed.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
The timeouts affected every "Win7 opt" mochitest-other cycle between the push & the backout.

They're all timeouts in browser_whitelist2.js, browser_whitelist5.js, and browser_whitelist6.js.  I have no idea how this bug's patches would affect those tests, though...

There was also one (hidden) Win7-64 opt cycle during that time, but the mochitest-other log for that cycle seems to show a crash (probably due to another issue) before getting to the browser_whitelist tests:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282474920.1282476882.15473.gz
(In reply to comment #12)
> The timeouts affected every "Win7 opt" mochitest-other cycle between the push &
> the backout.
> 
> They're all timeouts in browser_whitelist2.js, browser_whitelist5.js, and
> browser_whitelist6.js.  I have no idea how this bug's patches would affect
> those tests, though...
> 

It probably didn't. the push/backout also included all 17 patches from bug 513681 - much more likely candidates (though still rather inscrutable).
repushed this, and it seems to have stuck (leak taken care of, and timeout went away).

http://hg.mozilla.org/mozilla-central/rev/573c5fa45cc4

Resolving fixed.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.