1259513 - Don't use the gfxContext constructor directly

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

gfxContext needs to have a valid draw target to be constructed.  Don't attempt to make one of those if the target is invalid, and make it obvious to the callers they will not get a gfxContext if the draw target is invalid, by removing direct access to the constructor, and instead having a utility function that can return failure.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: MozReview Request: Bug 1259513: Make gfxContext constructor private, use a utility function that can return nullptr. r?lsalzman

Review commit: https://reviewboard.mozilla.org/r/42243/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42243/

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Would it make more sense to just have the constructor crash and make the users check the drawtarget before constructing the gfxContext?

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

That's what we do today, and it hasn't worked.  I'm hoping that having an explicit method that looks like it can fail, instead of a constructor, which people assume does not, would help this.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8734451 [details]
MozReview Request: Bug 1259513: Make gfxContext constructor private, use a utility function that can return nullptr. r?lsalzman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42243/diff/1-2/

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8734451 [details]
MozReview Request: Bug 1259513: Make gfxContext constructor private, use a utility function that can return nullptr. r?lsalzman

Critical note if the arguments aren't checked, just to make some more noise and a few places where the calling code wasn't consistent.

Comment 6

[Lee Salzman [:lsalzman]]

Comment on attachment 8734451 [details]
MozReview Request: Bug 1259513: Make gfxContext constructor private, use a utility function that can return nullptr. r?lsalzman

https://reviewboard.mozilla.org/r/42243/#review38841

With the caveat that we must now make sure that we check the result of gfxContext::ForDrawTarget when we use it, r+.

Comment 7

[Bas Schouten (:bas.schouten)]

Comment on attachment 8734451 [details]
MozReview Request: Bug 1259513: Make gfxContext constructor private, use a utility function that can return nullptr. r?lsalzman

https://reviewboard.mozilla.org/r/42243/#review39213

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8734451 [details]
MozReview Request: Bug 1259513: Make gfxContext constructor private, use a utility function that can return nullptr. r?lsalzman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42243/diff/2-3/

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #8)
> Comment on attachment 8734451 [details]
> MozReview Request: Bug 1259513: Make gfxContext constructor private, use a
> utility function that can return nullptr. r?lsalzman
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/42243/diff/2-3/

This is just a rebase.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8734451 [details]
MozReview Request: Bug 1259513: Make gfxContext constructor private, use a utility function that can return nullptr. r?lsalzman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42243/diff/3-4/

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8734451 [details]
MozReview Request: Bug 1259513: Make gfxContext constructor private, use a utility function that can return nullptr. r?lsalzman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42243/diff/4-5/

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da45b1e0d42a

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da45b1e0d42a