Last Comment Bug 784809 - createPattern should allow null for the repeat argument
: createPattern should allow null for the repeat argument
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 762657
  Show dependency treegraph
 
Reported: 2012-08-22 14:19 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-08-27 21:47 PDT (History)
4 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Allow null for the repeat argument of canvas createPattern. (4.54 KB, patch)
2012-08-22 17:19 PDT, Boris Zbarsky [:bz] (still a bit busy)
peterv: review+
Details | Diff | Splinter Review
With the test fixed on non-Azure canvas (4.71 KB, patch)
2012-08-23 14:32 PDT, Boris Zbarsky [:bz] (still a bit busy)
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2012-08-22 14:19:14 PDT
Apparently it used to, and other UAs do that too.  And the spec changed recently to allow it.

This means backing out part of bug 762657.

We should probably try to get this into aurora.
Comment 1 Alex Keybl [:akeybl] 2012-08-22 15:54:19 PDT
If peterv agrees, I have no issue with backing out bug 762657 prior to the merge.

If not, I'd need to know more context about the user impact. Sounds like we're just not ready to meet the spec though.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-08-22 16:26:40 PDT
We don't want to back out bug 762657.  We just need to back out the xpidl change and part of the test changes.

The user impact is that createPattern calls that used to work and should work per the updated spec throw on m-c and aurora right now.

I'm going to have a patch for this very soon.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-08-22 16:56:54 PDT
I'm going to make this nullable for now in the WebIDL, but the right solution is actually [TreatNullAs=EmptyString].  That would need bug 742195, which is not happening on Aurora.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-08-22 16:59:26 PDT
I filed bug 784869 to do the "throw on undefined" thing.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-08-22 17:19:03 PDT
Created attachment 654444 [details] [diff] [review]
Allow null for the repeat argument of canvas createPattern.
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-08-23 00:50:18 PDT
The spec hasn't changed to allow it, and Opera throws as well. See <https://www.w3.org/Bugs/Public/show_bug.cgi?id=17891>.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-08-23 07:14:01 PDT
> The spec hasn't changed to allow it

Rik's editor's draft has...

I believe we should take this to mitigate risk for shipping the webidl canvas stuff, personally.  And I don't see the benefit of switching behavior in a way that might break web pages, honestly.
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-08-23 07:17:30 PDT
Sigh... Can we at least not use "the spec" to refer to the W3C fork without qualification?
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-08-23 07:18:57 PDT
<shrug>.  I've lost tracks of all the various "specs" involved.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-08-23 12:24:33 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b88ccf7bd3e3
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-08-23 12:25:49 PDT
Comment on attachment 654444 [details] [diff] [review]
Allow null for the repeat argument of canvas createPattern.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 762657
User impact if declined: Some sites might throw exceptions that they don't expect
Testing completed (on m-c, etc.): Passes our tests, as far as I can tell.
Risk to taking this patch (and alternatives if risky): Risk is low.  Just allows
   passing null (and undefined) for the second argument to createPattern,
   basically restoring the Firefox 15 behavior.
String or UUID changes made by this patch: None.
Comment 12 Matt Brubeck (:mbrubeck) 2012-08-23 14:12:05 PDT
Sorry, I backed this out because test_canvas.html was failing on Linux.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2266e0721327
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-08-23 14:17:00 PDT
Gah.  Because that's a non-azure canvas, so the test result is different for the moment.  I'm just going to comment that test for undefined out for now.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2012-08-23 14:32:42 PDT
Created attachment 654796 [details] [diff] [review]
With the test fixed on non-Azure canvas
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2012-08-23 17:50:38 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bed30d952cc
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-08-24 20:05:09 PDT
https://hg.mozilla.org/mozilla-central/rev/1bed30d952cc
Comment 17 Alex Keybl [:akeybl] 2012-08-27 18:10:24 PDT
Comment on attachment 654796 [details] [diff] [review]
With the test fixed on non-Azure canvas

[Triage Comment]
Low risk regression fix, approved for Beta 16.
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2012-08-27 21:47:12 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/57ce663f5d6f

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