401788 - Canvas throws exceptions on invalid string values

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P2)

Description

[Philip Taylor]

User-Agent:       Opera/9.24 (X11; Linux i686; U; en)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007103004 Minefield/3.0a9pre 

Invalid strings are handled incorrectly in a few places:

canvas.getContext('unrecognised') should return null, but throws NS_ERROR_ILLEGAL_VALUE.

canvas.toDataURL('unrecognised') should fall back to image/png, but throws NS_ERROR_FAILURE.

ctx.{globalCompositeOperation,lineCap,lineJoin} = 'unrecognised' should be ignored, but throw NS_ERROR_NOT_IMPLEMENTED.

Opera and WebKit follow the spec in these cases.


Reproducible: Always

Comment 1

[José Jeria]

Is this still an issue?

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 3

[Marco Bonardo [:mak]]

this is bitrotted against current m-c, please provide an updated patch and re-ask c-n.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2 (merged to tip)

r=jonas

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

If you are confident that the changes between the old and new patch are trivial enough, can you manually set jonas r+ on the new patch and say "carrying forward r+"...

Then I'll be happy to check it in for you (since you tagged as checkin-needed).

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Adding Vlad in CC since I thought he'd have an opinion on this subject.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 456174 [details] [diff] [review]
Patch v2 (merged to tip)

Looks fine, though one small change:

> nsresult
> nsHTMLCanvasElement::ToDataURLImpl(const nsAString& aMimeType,
>                                    const nsAString& aEncoderOptions,
>                                    nsAString& aDataURL)
> {
>-  nsresult rv;

don't do this -- keep the nsresult declaration here, it's used all throughout the function below.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Just following "Declare local variables as near to their use as possible." <https://developer.mozilla.org/En/Developer_Guide/Coding_Style#General_Practices> here.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Can you push this or do you need someone to do it? (Asking because of the checkin-needed)

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I don't understand what's not obvious. I just followed Marco's instructions:

> this is bitrotted against current m-c, please provide an updated patch and
> re-ask c-n.

I would assume that asking checkin-needed for a reviewed patch, which just needed to be updated to current trunk, is an unambiguous request to check it in.

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #5)
> If you are confident that the changes between the old and new patch are trivial
> enough, can you manually set jonas r+ on the new patch and say "carrying
> forward r+"...

Bugzilla doesn't have support for "carrying forward r+". Manually setting r+ isn't useful, as it lacks the important "jonas" bit.

(In reply to comment #10)
> I don't understand what's not obvious. I just followed Marco's instructions:
> 
> > this is bitrotted against current m-c, please provide an updated patch and
> > re-ask c-n.
> 
> I would assume that asking checkin-needed for a reviewed patch, which just
> needed to be updated to current trunk, is an unambiguous request to check it
> in.

Yep.

Comment 12

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/42e56a8bb77a

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks! I added a note to <https://developer.mozilla.org/en/Firefox_4_for_developers#section_3>. It doesn't look like anything else needs to be updated.

Comment 14

[Jim Jeffery not reading bug-mail 1/2/11]

Could this have caused: https://bugzilla.mozilla.org/show_bug.cgi?id=578215