Comment on attachment 631112 [details] [diff] [review] v1 Review of attachment 631112 [details] [diff] [review]: ----------------------------------------------------------------- Long patch is long ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp @@ +1341,5 @@ > + > + nsCanvasGradientAzure* gradient; > + nsresult rv = xpc_qsUnwrapArg<nsCanvasGradientAzure>(cx, value, &gradient, > + static_cast<nsISupports**>(getter_AddRefs(holder)), > + &value); Hmm, I thought the new bindings would rid us of this kind of code outside bindings... @@ +1661,4 @@ > } > > +nsCanvasPatternAzure::RepeatMode > +GetRepeatMode(const nsAString& repeat, ErrorResult& error) static @@ +1699,5 @@ > + repeatMode = nsCanvasPatternAzure::NOREPEAT; > + } else { > + error.Throw(NS_ERROR_DOM_SYNTAX_ERR); > + return NULL; > + } Don't you want to use GetRepeatMode here? ::: content/canvas/src/nsCanvasRenderingContext2DAzure.h @@ +19,5 @@ > +#include "mozilla/dom/ImageData.h" > +#include "mozilla/dom/UnionTypes.h" > + > +using namespace mozilla; > +using namespace mozilla::gfx; All these 'using's need to go from the header @@ +183,5 @@ > + > + nsHTMLCanvasElement* GetCanvas() const > + { > + return mCanvasElement; > + } Hmm, don't we have this on nsICanvasRenderingContextInternal too now? @@ +603,5 @@ > + ToHTMLImageOrCanvasOrVideoElement(nsIDOMElement* html, > + HTMLImageOrCanvasOrVideoElement& element) > + { > + nsCOMPtr<nsIContent> content = do_QueryInterface(html); > + if (content) { Early return?

This adds the new APIs we need for the new DOM bindings, and makes the existing XPCOM API call through to them.

Comment on attachment 633622 [details] [diff] [review] v1.1 Asking bz for review in dom/bindings and dom/webidl.

Comment on attachment 633622 [details] [diff] [review] v1.1 Review of attachment 633622 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp @@ +1896,4 @@ > { > + nsString shadowColor; > + GetShadowColor(shadowColor); > + color = shadowColor; See below. @@ +2781,5 @@ > +nsCanvasRenderingContext2DAzure::GetMozTextBaseline(nsAString& tb) > +{ > + nsString textBaseline; > + GetTextBaseline(textBaseline); > + tb = textBaseline; Same as below. (I'm reading backwards.) @@ +3364,5 @@ > +nsCanvasRenderingContext2DAzure::GetMozLineCap(nsAString& capstyle) > +{ > + nsString linecap; > + GetLineCap(linecap); > + capstyle = linecap; There's no need for that if you have GetLineCap take nsAString. @@ +4426,5 @@ > +static already_AddRefed<ImageData> > +CreateImageData(JSContext* cx, nsHTMLCanvasElement* canvas, uint32_t w, > + uint32_t h, ErrorResult& error) > +{ > + using mozilla::CheckedInt; This shouldn't be necessary here. @@ +4444,5 @@ > + // XXX Do we need to enter canvas::GetWrapper()'s compartment first? > + JSObject* darray = JS_NewUint8ClampedArray(cx, len.value()); > + JS::AutoObjectRooter rd(cx, darray); > + if (!darray) { > + return NULL; We need something of an exception, right? ::: content/canvas/src/nsCanvasRenderingContext2DAzure.h @@ +24,5 @@ > + > +using mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElement; > +using mozilla::dom::ImageData; > + > +namespace mgfx = mozilla::gfx; None of these should be present in a header. @@ +36,5 @@ > +class SourceSurface; > +} > +} > + > +using mozilla::dom::Optional; And this not either ::: dom/webidl/CanvasRenderingContext2D.webidl @@ +6,5 @@ > + * The origin of this IDL file is > + * http://www.whatwg.org/specs/web-apps/current-work/ > + * > + * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C > + * liability, trademark and document use rules apply. © Copyright 2004-2011 Apple Computer, Inc., Mozilla Foundation, and Opera Software ASA. You are granted a license to use, reproduce and create derivative works of this document.

Comment on attachment 633622 [details] [diff] [review] v1.1 Review of attachment 633622 [details] [diff] [review]: ----------------------------------------------------------------- I don't know a lot about the new DOM bindings things, but a couple of questions come to mind. In some of these cases I'm probably just missing something obvious. In general I guess I could do with some documentation on how the bindings work! 1) Why are there so many more changes to the Azure version than to the non-azure version? 2) Why are we moving so many things into the header? Are all these things needed here for the bindings to work? It certainly doesn't help the code's legibility! 3) We should try to minimize the float->double conversions. Azure itself is float so the best way to do things for both performance and memory is getting things to float early and keeping them that way.

For #1, I believe Peter only converted the Azure canvas context to new bindings, since performance there is more important and the non-Azure context is slated to go away anyway. For #3, note that the old XPCOM methods are more or less dead code after this patch; they just need to be removed once the new binding becomes non-prefable. But there might still be more conversions from double to float (e.g. in FloatValidate), I guess. One issue here is that in general the API is defined as taking doubles and the difference between double and float is web-detectable if done too early for some of the methods here (e.g. converting to float before doing the r < 0.0 check in Arc() could cause no error to be thrown for very small negative r that underflow to 0, whereas keeping the number as a double until after that check would throw an exception). I'll try to write up some overview documentation on the bindings...

(In reply to Boris Zbarsky (:bz) from comment #7) > For #1, I believe Peter only converted the Azure canvas context to new > bindings, since performance there is more important and the non-Azure > context is slated to go away anyway. > > For #3, note that the old XPCOM methods are more or less dead code after > this patch; they just need to be removed once the new binding becomes > non-prefable. But there might still be more conversions from double to > float (e.g. in FloatValidate), I guess. > > One issue here is that in general the API is defined as taking doubles and > the difference between double and float is web-detectable if done too early > for some of the methods here (e.g. converting to float before doing the r < > 0.0 check in Arc() could cause no error to be thrown for very small negative > r that underflow to 0, whereas keeping the number as a double until after > that check would throw an exception). > > I'll try to write up some overview documentation on the bindings... Thanks a lot for the information, I'll do my best to understand things here! :)

(In reply to Bas Schouten (:bas) from comment #6) > 2) Why are we moving so many things into the header? Are all these things > needed here for the bindings to work? It certainly doesn't help the code's > legibility! I assume you're talking about the classes? I tried to minimize the number of classes moved to the header, but didn't want to rewrite even more so bailed on some. Note that the new bindings can call inline functions, so I tried to make the small ones inline, which leads to more stuff being in the header.

(In reply to Peter Van der Beken [:peterv] from comment #9) > (In reply to Bas Schouten (:bas) from comment #6) > > 2) Why are we moving so many things into the header? Are all these things > > needed here for the bindings to work? It certainly doesn't help the code's > > legibility! > > I assume you're talking about the classes? I tried to minimize the number of > classes moved to the header, but didn't want to rewrite even more so bailed > on some. > Note that the new bindings can call inline functions, so I tried to make the > small ones inline, which leads to more stuff being in the header. Hrm, understood, I'm guessing there's no easy way then to support inlined class member functions?

Not sure what you mean?

Comment on attachment 633622 [details] [diff] [review] v1.1 Review of attachment 633622 [details] [diff] [review]: ----------------------------------------------------------------- In general I'd prefer if inlined member functions have vertical whitespacing between them for readability. I've only looked at the content/canvas portions of this patch! ::: content/canvas/src/nsCanvasRenderingContext2DAzure.h @@ +24,5 @@ > + > +using mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElement; > +using mozilla::dom::ImageData; > + > +namespace mgfx = mozilla::gfx; Indeed. @@ +105,5 @@ > + Point mCenter1; > + Point mCenter2; > + Float mRadius1; > + Float mRadius2; > +}; This can stay in the CPP file. @@ +124,5 @@ > + // Beginning of linear gradient. > + Point mBegin; > + // End of linear gradient. > + Point mEnd; > +}; So can this. @@ +840,5 @@ > + */ > + bool StyleIsColor(Style whichStyle) const > + { > + return !(patternStyles[whichStyle] || > + gradientStyles[whichStyle]); nit: spacing

> But there might still be more conversions from double to float (e.g. in FloatValidate) FloatValidate takes doubles, so we're actually ending up with fewer conversions here.

Comment on attachment 633622 [details] [diff] [review] v1.1 >+++ b/content/canvas/src/nsCanvasRenderingContext2DAzure.cpp >+MatrixToJSObject(JSContext* cx, const Matrix& matrix, ErrorResult& error) Is there a reason we're declaring mozCurrentTransform and mozCurrentTransformInverse as object and not as sequence<double>? Seems like doing the latter would totally obviate the need for this code (though you'd still need to do FloatValidate() on the sequence members in the setter, of couse). Though I guess that doesn't help your situation with the XPCOM version of the method... But it could keep its old impl in terms of canvas utils for now. > nsCanvasRenderingContext2DAzure::GetStrokeStyle_multi(nsAString& aStr, nsISupports **aInterface, PRInt32 *aType) >+ *aInterface = GetStyleAsStringOrInterface(aStr, type, STYLE_STROKE); Need to NS_IF_ADDREF(*aInterface), right? > nsCanvasRenderingContext2DAzure::GetFillStyle_multi(nsAString& aStr, nsISupports **aInterface, PRInt32 *aType) >+ *aInterface = GetStyleAsStringOrInterface(aStr, type, STYLE_FILL); And here. >+nsCanvasRenderingContext2DAzure::CreateLinearGradient(double x0, double y0, double x1, double y1, >+ if (!FloatValidate(x0,y0,x1,y1)) { We should implement the whole "unrestricted double" thing and nix some of these FloatValidate calls on the remaining plain-double methods.... >+GetRepeatMode(const nsAString& repeat, ErrorResult& error) This seems to be unused. Please remove it. >+nsCanvasRenderingContext2DAzure::CreatePattern(const HTMLImageOrCanvasOrVideoElement& element, >+ nsIContent* content; Element* element, please. Then you don't need the AsElement() bit when calling SurfaceFromElement. >+nsCanvasRenderingContext2DAzure::GetMozShadowColor(nsAString& color) >+ nsString shadowColor; You shouldn't need the temporary. Just GetShadowcolor(color) should work. >+nsCanvasRenderingContext2DAzure::GetMozTextBaseline(nsAString& tb) >+ nsString textBaseline; Similar issue. > NS_IMETHODIMP > nsCanvasRenderingContext2DAzure::FillText(const nsAString& text, float x, float y, float maxWidth) >+ ErrorResult rv; >+ FillText(text, x, y, maxWidth); Need to pass rv as last arg. Otherwise this is actually a recursive call, so if anyone calls this they'll crash. > NS_IMETHODIMP > nsCanvasRenderingContext2DAzure::StrokeText(const nsAString& text, float x, float y, float maxWidth) >+ StrokeText(text, x, y, maxWidth); Same issue here. > nsCanvasRenderingContext2DAzure::DrawOrMeasureText(const nsAString& aRawText, > // technically, 0 should be an invalid value as well, but 0 is the default > // arg, and there is no way to tell if the default was used >- if (aMaxWidth < 0) >+ if (aMaxWidth.WasPassed() && aMaxWidth.Value() < 0) > return NS_ERROR_INVALID_ARG; I believe the spec calls for returning 0 when aMaxWidth <= 0. Please file a followup to fix this, now that we can? >+nsCanvasRenderingContext2DAzure::GetMozLineCap(nsAString& capstyle) >+ nsString linecap; No need for the temporary here either. >+nsCanvasRenderingContext2DAzure::SetMozDash(JSContext* cx, We should file another followup to make this follow the spec, if we don't have a bug on it already. >+nsCanvasRenderingContext2DAzure::DrawImage(const HTMLImageOrCanvasOrVideoElement& image, >+ CanvasImageCache::Lookup(domElement, mCanvasElement, &imgSize); And another followup to look into using Element* as the key? >+nsCanvasRenderingContext2DAzure::SetGlobalCompositeOperation(const nsAString& op, >+ ErrorResult& error) This looks infaliible to me, so I'm not sure you need the ErrorResult& here. >+nsCanvasRenderingContext2DAzure::PutImageData(JSContext* cx, ImageData& imageData, double dx, >+ double dy, ErrorResult& error) Fix the indent, please. >+ if (!NS_finite(dx) || !NS_finite(dy)) { You can use FloatValidate() here. >+ JSObject* array = imageData.GetData(cx); >+ >+ MOZ_ASSERT(JS_IsTypedArrayObject(array, cx)); >+ PRUint8* data = reinterpret_cast<PRUint8*>(JS_GetArrayBufferViewData(array, cx)); >+ uint32_t byteLength = JS_GetTypedArrayByteLength(array, cx); How about: dom::Uint8ClampedArray arr(cx, imageData.getData()); and then pass its data and length in to the _explicit_ thing? Do we need to enter the compartment of the imageData.GetData() before doing all this, though? Worth checking with Bobby or sfink. >+nsCanvasRenderingContext2DAzure::PutImageData(JSContext* cx, ImageData& imageData, double dx, >+ double dy, double dirtyX, double dirtyY, double dirtyWidth, >+ double dirtyHeight, ErrorResult& error) Again, fix indent. >+ if (!NS_finite(dx) || !NS_finite(dy) || !NS_finite(dirtyX) || >+ !NS_finite(dirtyY) || !NS_finite(dirtyWidth) || !NS_finite(dirtyHeight)) { FloatValidate() And again, dom::Uint8ClampedArray. And the compartment thing... >+CreateImageData(JSContext* cx, nsHTMLCanvasElement* canvas, uint32_t w, >+ // XXX Do we need to enter canvas::GetWrapper()'s compartment first? Good question! The quickstub doesn't seem to, so I have no problem if we don't, frankly. Or at least file a followup on this. >+ JS::AutoObjectRooter rd(cx, darray); You don't need that, with stack rooting. And with the new handle stuff it won't help, as I understand. >+nsCanvasRenderingContext2DAzure::CreateImageData(JSContext* cx, double sw, >+ if (!NS_finite(sw) || !NS_finite(sh)) { FloatValidate() >+++ b/content/canvas/src/nsCanvasRenderingContext2DAzure.h >+using namespace mozilla; >+using namespace mozilla::gfx; We don't really want to do this in headers.... >+using mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElement; >+using mozilla::dom::ImageData; ... >+using mozilla::dom::Optional; These can all be dealt with via a typedef inside the canvas context class, I think. >+namespace mgfx = mozilla::gfx; There's just one use in this header; just spell it out there? >+namespace mozilla { >+namespace dom { >+template<typename T> class Optional; That's annoying. :( Should we just put Optional in a separate header like Nullable? And maybe even give it a name like BindingTypes.h and put Sequence in there too? I suppose we could even put Nullable in there. >+class nsCanvasGradientAzure : public nsIDOMCanvasGradient We don't really need this moved to the header yet, right? We're still unwrapping CanvasGradient to nsIDOMCanvasGradient, afaict.... I guess it's fine either way. >+class nsCanvasRenderingContext2DAzure : >+ JS::Value GetStrokeStyle(JSContext* cx, ErrorResult& error); >+ void SetStrokeStyle(JSContext* cx, JS::Value& value) >+ JS::Value GetFillStyle(JSContext* cx, ErrorResult& error); >+ void SetFillStyle(JSContext* cx, JS::Value& value) Ideally these would just use a union. Followup bug is OK, I guess. That'll change behavior when random non-string-non-whatever values are passed in, I guess.... >+ bool GetImageSmoothingEnabled() If we don't have one already, would you mind filing a followup bug to unprefix this or at least add the unprefixed alias in the WebIDL? >+++ b/dom/bindings/BindingUtils.h >+UnwrapObject(JSContext* cx, JSObject* obj, U& value, >+ bool unwrapSecurityWrappers = false) Why is this needed? If needed, it seems kinda backwards to me in that unwrapSecurityWrappers = false allows us to unwrap through security wrappers? I'd prefer this change went away, since this patch doesn't seem to make use of it. >diff --git a/dom/bindings/Bindings.conf b/dom/bindings/Bindings.conf >+'CanvasRenderingContext2D': [ >+ 'nativeType': 'nsCanvasRenderingContext2DAzure', >+ 'prefable': True, So this works insofar as there are no WebIDL bindings with CanvasRenderingContext2D arguments, right? Because if such things _did_ exist they would cause havoc when passed a non-Azure canvas... I guess as long as this is prefable we enforce lack of such arguments, but add a comment here about us relying on that? When we remove the prefable bit if we still support non-Azure canvas we might need to add something else to the descriptor that prevents this being used as an argument. >+addExternalIface('HTMLElement') Why do you need this one? >+addExternalIface('Path', headerFile='nsIDOMCanvasRenderingContext2D.h') This seems to be unused so far, at first glance. >+addExternalIface('SVGMatrix') This seems to be unused so far. Why do you not need "'wrapperCache': False" on ImageData? Or is that still coming? >+++ b/dom/webidl/CanvasRenderingContext2D.webidl >+ * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C >+ * liability, trademark and document use rules apply. Please double-check the copyright notice? It's not the same for W3C specs and whatwg specs, or something. >+ * This API cannot currently be used by Web content. It is chrome >+ * only. Can we just mark it [ChromeOnly] in the webidl? If not (e.g. because of risk this late in the game), can we file a followup bug to work on doing that, please? I assume this would apply to both drawWindow and asyncDrawXULElement. >+interface ImageData { >+ readonly attribute object data; >+ //readonly attribute Uint8ClampedArray data; Because we don't have retval codegen for typed arrays yet? Please file a followup bug and reference it in a comment here. get/createImageData should be marked [Creator], right? I didn't actually go through and make sure the IDL is actually sane (i.e. review the spec). If you think I should, please let me know. r=me with the above nits fixed.

(In reply to :Ms2ger from comment #2) > Hmm, I thought the new bindings would rid us of this kind of code outside > bindings... Not until we've switched to new bindings definitively. > static Done. > Don't you want to use GetRepeatMode here? Removed GetRepeatMode. > All these 'using's need to go from the header Done. > Hmm, don't we have this on nsICanvasRenderingContextInternal too now? No. > Early return? No. > See below. Done. > Same as below. (I'm reading backwards.) Done. > There's no need for that if you have GetLineCap take nsAString. Done. > This shouldn't be necessary here. Done (it was copied from CustomQS_Canvas2D.h). > > + JSObject* darray = JS_NewUint8ClampedArray(cx, len.value()); > We need something of an exception, right? Don't think so. > None of these should be present in a header. Done. > And this not either Done. > © Copyright 2004-2011 Apple Computer, Inc., Mozilla Foundation, and Opera > Software ASA. > > You are granted a license to use, reproduce and create derivative works of > this document. Done. > In general I'd prefer if inlined member functions have vertical whitespacing > between them for readability. Done. > This can stay in the CPP file. Done. > So can this. Done. > nit: spacing Rewrapped. > Is there a reason we're declaring mozCurrentTransform and > mozCurrentTransformInverse as object and not as sequence<double>? An attribute's type can't be a sequence. Filed bug 768048 to switch this to arrays. > Need to NS_IF_ADDREF(*aInterface), right? Done. > And here. Done. > We should implement the whole "unrestricted double" thing and nix some of > these FloatValidate calls on the remaining plain-double methods.... Filed bug 767933. > This seems to be unused. Please remove it. Done. > Element* element, please. Then you don't need the AsElement() bit when > calling SurfaceFromElement. Done. > You shouldn't need the temporary. Just GetShadowcolor(color) should work. Done. > Similar issue. Done. > Need to pass rv as last arg. Otherwise this is actually a recursive call, > so if anyone calls this they'll crash. Done. > Same issue here. Done. > I believe the spec calls for returning 0 when aMaxWidth <= 0. Please file a > followup to fix this, now that we can? Filed bug 767934. > No need for the temporary here either. Done. > >+nsCanvasRenderingContext2DAzure::SetMozDash(JSContext* cx, > > We should file another followup to make this follow the spec, if we don't > have a bug on it already. Filed bug 768067. > And another followup to look into using Element* as the key? Filed bug 767936. > This looks infaliible to me, so I'm not sure you need the ErrorResult& here. I kept it because of the comment: "// XXX ERRMSG we need to report an error to developers here!" > Fix the indent, please. Done. > You can use FloatValidate() here. Done. > dom::Uint8ClampedArray arr(cx, imageData.getData()); > > and then pass its data and length in to the _explicit_ thing? Done. > Do we need to enter the compartment of the imageData.GetData() before doing > all this, though? Worth checking with Bobby or sfink. I don't think we do, Bobby though the same. > Again, fix indent. Done. > FloatValidate() > > And again, dom::Uint8ClampedArray. And the compartment thing... Done. > >+ // XXX Do we need to enter canvas::GetWrapper()'s compartment first? > > Good question! The quickstub doesn't seem to, so I have no problem if we > don't, frankly. Or at least file a followup on this. Made this use the infrastructure from bug 768050. > You don't need that, with stack rooting. And with the new handle stuff it > won't help, as I understand. Removed. > FloatValidate() Done. > We don't really want to do this in headers.... Done. > These can all be dealt with via a typedef inside the canvas context class, I > think. Done. > There's just one use in this header; just spell it out there? Done. > >+namespace mozilla { > >+namespace dom { > >+template<typename T> class Optional; > > That's annoying. :( Should we just put Optional in a separate header like > Nullable? And maybe even give it a name like BindingTypes.h and put > Sequence in there too? I suppose we could even put Nullable in there. Will file a followup. > We don't really need this moved to the header yet, right? We're still > unwrapping CanvasGradient to nsIDOMCanvasGradient, afaict.... Yeah, I moved for the future. > Ideally these would just use a union. Followup bug is OK, I guess. Filed bug 768069. > If we don't have one already, would you mind filing a followup bug to > unprefix this or at least add the unprefixed alias in the WebIDL? Filed bug 768072. > I'd prefer this change went away, since this patch doesn't seem to make use > of it. Done. This looks like leftover from something that got removed again. > >diff --git a/dom/bindings/Bindings.conf b/dom/bindings/Bindings.conf > >+'CanvasRenderingContext2D': [ > >+ 'nativeType': 'nsCanvasRenderingContext2DAzure', > >+ 'prefable': True, > > So this works insofar as there are no WebIDL bindings with > CanvasRenderingContext2D arguments, right? Because if such things _did_ > exist they would cause havoc when passed a non-Azure canvas... Right. > I guess as long as this is prefable we enforce lack of such arguments, but > add a comment here about us relying on that? When we remove the prefable > bit if we still support non-Azure canvas we might need to add something else > to the descriptor that prevents this being used as an argument. Added comment. > Why do you need this one? Removed. > This seems to be unused so far, at first glance. Removed. > This seems to be unused so far. Removed. > Why do you not need "'wrapperCache': False" on ImageData? Or is that still > coming? We did. > Please double-check the copyright notice? It's not the same for W3C specs > and whatwg specs, or something. Changed. > If not (e.g. because of risk this late in the game), can we file a followup > bug to work on doing that, please? Filed bug 767931 and added a FIXME comment. > Because we don't have retval codegen for typed arrays yet? Please file a > followup bug and reference it in a comment here. Done (bug 767930). > get/createImageData should be marked [Creator], right? Yeah, done.

https://hg.mozilla.org/integration/mozilla-inbound/rev/438c192e561b ImageData will be done in a followup bug when we don't use the XPConnect bindings for nsCanvasRenderingContext2D and nsCanvasRenderingContext2DAzure anymore, because XPConnect can't wrap ImageData in a new binding.

Sorry, I backed this out on inbound along with bug 762654 because Windows PGO builds were crashing in gfxContext::FillAzure: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a2cc88619ba https://tbpl.mozilla.org/php/getParsedLog.php?id=13051005&tree=Mozilla-Inbound

I tried this on a hunch, and it seems to fix the crashes. I'd moved the GeneralPattern class out of nsCanvasRenderingContext2DAzure to keep it out of the header, but apparently that confuses PGO because of there's another GeneralPattern class in gfxContext.cpp. The crashes were all on a line where we create a GeneralPattern (from gfxContext.cpp) on the stack.

Comment on attachment 636375 [details] [diff] [review] v1.1 Marking bz's review, apparently Bas agreed to delegate to bz.

Comment on attachment 637829 [details] [diff] [review] Rename GeneralPattern to CanvasGeneralPattern v1 r=me

Though it might be good to add a comment explaining why the class name is what it is.

http://hg.mozilla.org/integration/mozilla-inbound/rev/f332c2b64748 http://hg.mozilla.org/integration/mozilla-inbound/rev/7dbb80929252

https://hg.mozilla.org/mozilla-central/rev/7dbb80929252 https://hg.mozilla.org/mozilla-central/rev/f332c2b64748