856472 - Convert the rest of Canvas to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Move TextMetrics to its own header

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Convert TextMetrics to WebIDL

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 731707 [details] [diff] [review]
Move TextMetrics to its own header

The DOMCI_DATA should stay in the .cpp.

r=me with that

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: Convert TextMetrics to WebIDL

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 732105 [details] [diff] [review]
Convert TextMetrics to WebIDL

So there's a problem with using an 'owned' thing here, actually.  The codegen ends up looking like this:

  mozilla::dom::TextMetrics* result;
  result = self->MeasureText(arg0, rv);
  rv.WouldReportJSException();
  if (rv.Failed()) {
    return ThrowMethodFailedWithDetails<true>(cx, rv, "CanvasRenderingContext2D", "measureText");
  }
  if (!WrapNewBindingNonWrapperCachedObject(cx, obj, result, vp)) {
    MOZ_ASSERT(JS_IsExceptionPending(cx));
    return false;

so if WrapNewBindingNonWrapperCachedObject fails without taking ownership of the object, we leak...

It generally looks like our support for 'owned' is pretty half-baked. :(

Do you want to try to fix it here, or just make this refcounted for now?  :(

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: Delete owned objects if wrapping fails

I'll run this through try to make sure we're actually returning null everywhere we throw.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Note that right now nothing is actually using the 'owned' bindings....

Comment 7

[David Zbarsky (:dzbarsky)]

Right, that nullcheck is for all failed returns, not just 'owned' bindings.

Comment 8

[David Zbarsky (:dzbarsky)]

Attachment: Move CanvasPattern to its own file

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 732105 [details] [diff] [review]
Convert TextMetrics to WebIDL

>+  float Width()

const?

>+  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope)

This doesn't need to be virtual.

>+                   self.descriptor.getDescriptor(self.returnType.name).nativeOwnership == 'owned' or

s/self.returnType.name/self.returnType.unroll().inner.identifier.name/

because otherwise if the return type is "Foo?" you will get some weirdness.  And maybe somewhere in here we should assert that self.returnType.isGeckoInterface()...

>+  /*readonly attribute double actualBoundingBoxLeft;

That's really hard to notice.  How about:

  /*
   * NOT IMPLEMENTED YET:

  readonly attribute double actualBoundingBoxLeft;
  readonly attribute double actualBoundingBoxRight;

etc.

r=me with the above fixed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 732186 [details] [diff] [review]
Delete owned objects if wrapping fails

>+++ b/dom/bindings/BindingUtils.h
>     obj = value->WrapObject(cx, scope);

But that can return null in various cases...  Who's in charge of deleting |value| then?

The only way to make it work is to have CGWrapNonWrapperCacheMethod delete stuff as needed, I think.

>+++ b/dom/bindings/Codegen.py

>+                isOwned = "true" if descriptor.nativeOwnership == "owned" else "false"

  isOwned = toStringBool(descriptor.nativeOwnership == "owned")

>+                self.cgRoot.append(CGGeneric("  MOZ_ASSERT(!result);"))

Newline after the ';' is handled by the cgRoot separator or something?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 732192 [details] [diff] [review]
Move CanvasPattern to its own file

r=me

Comment 12

[David Zbarsky (:dzbarsky)]

Attachment: Delete owned objects if wrapping fails

Comment 13

[David Zbarsky (:dzbarsky)]

Attachment: Convert CanvasPattern to WebIDL

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 732560 [details] [diff] [review]
Delete owned objects if wrapping fails

Why do we not need to change the wrappercached case?  Is it not possible to have an owned wrappercached object?

I've been thinking about this some more, and I'm not sure our setup for doing 'owned' is sane at all.  In particular, we don't enforce anywhere that 'owned' interfaces don't inherit from non-'owned' ones or vice versa...  It's a giant footgun.

So my temptation is that the way we should do this is as follows:

1)  Disallow 'owned' on interfaces that either inherit from something or have
    something inheriting from them.
2)  Once we do that, we know exactly which binding to use, so we don't have to
    call WrapObject on the object itself at all, and can just call the
    binding's Wrap().
3)  The binding's Wrap() can explicitly tell us whether it took ownership.

Or something.

I realize that's a bunch of changes.  I'm just not happy trying to audit the code we have now to make sure we're properly handling deletion in all the places where it should happen but not any other places...

No matter what, we should do step 1, because otherwise reasoning about this becomes insane, imo.

Comment 15

[Boris Zbarsky [:bzbarsky]]

And also, I suggest spinning the whole "make 'owned' sane" bit into a separate bug.  And checking with Peter how he thinks it should work, perhaps.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 732600 [details] [diff] [review]
Convert CanvasPattern to WebIDL

r=me

Comment 17

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4bf662e395
https://hg.mozilla.org/integration/mozilla-inbound/rev/b37637c50e4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e84af960621
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e7970330a3e

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b4bf662e395
https://hg.mozilla.org/mozilla-central/rev/b37637c50e4e
https://hg.mozilla.org/mozilla-central/rev/5e84af960621
https://hg.mozilla.org/mozilla-central/rev/3e7970330a3e

Comment 19

[David Zbarsky (:dzbarsky)]

Attachment: Move CanvasGradient to its own file

Comment 20

[David Zbarsky (:dzbarsky)]

Attachment: Convert CanvasGradient to WebIDL

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 739689 [details] [diff] [review]
Move CanvasGradient to its own file

r=me

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 739691 [details] [diff] [review]
Convert CanvasGradient to WebIDL

Hmm.  So per spec, addColorStop takes a double, not a float.  At least file a followup?

>   if (!FloatValidate(offset) || offset < 0.0 || offset > 1.0) {

No need to FloatValidate, since we're using a restricted float.

Note that if you take a double but want to use floats internally you _will_ need to FloatValidate after conversion to float....

r=me

Comment 24

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #23)
> Comment on attachment 739691 [details] [diff] [review]
> Convert CanvasGradient to WebIDL
> 
> Hmm.  So per spec, addColorStop takes a double, not a float.  At least file
> a followup?
> 

Bug 864196.

Comment 25

[David Zbarsky (:dzbarsky)]

I backed out the patches for CanvasPattern because it should be wrappercached.  The 'any' type of the fillStyle/strokeStyle is actually (String or CanvasGradient or CanvasPattern).  It's sad that our tests didn't catch this =(


https://hg.mozilla.org/integration/mozilla-inbound/rev/90a3427e0e72
https://hg.mozilla.org/integration/mozilla-inbound/rev/15349b160448
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea3ffdbddc53

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90a3427e0e72
https://hg.mozilla.org/mozilla-central/rev/15349b160448
https://hg.mozilla.org/mozilla-central/rev/ea3ffdbddc53

Comment 27

[Boris Zbarsky [:bzbarsky]]

Add tests?

Comment 28

[David Zbarsky (:dzbarsky)]

Attachment: WrapperCache CanvasPattern

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 742150 [details] [diff] [review]
WrapperCache CanvasPattern

>+Test the correct behaviour for isPointInPath in the presence of multiple transforms,

Er, no.  ;)  Please fix or remove.

r=me with that

Comment 30

[David Zbarsky (:dzbarsky)]

Attachment: wrappercache CanvasGradient

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 742168 [details] [diff] [review]
wrappercache CanvasGradient

r=me

Comment 32

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d3fa5d860c
https://hg.mozilla.org/integration/mozilla-inbound/rev/131601778912
https://hg.mozilla.org/integration/mozilla-inbound/rev/71aaf2442435
https://hg.mozilla.org/integration/mozilla-inbound/rev/f127889eda58
https://hg.mozilla.org/integration/mozilla-inbound/rev/a25af78ec2fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/05c7d6b210a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b64a96412dd3

Comment 33

[David Zbarsky (:dzbarsky)]

Attachment: pattern and gradient don't need to inherit nsISupports

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 742460 [details] [diff] [review]
pattern and gradient don't need to inherit nsISupports

Hmm.  r=me

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c5d3fa5d860c
https://hg.mozilla.org/mozilla-central/rev/131601778912
https://hg.mozilla.org/mozilla-central/rev/71aaf2442435
https://hg.mozilla.org/mozilla-central/rev/f127889eda58
https://hg.mozilla.org/mozilla-central/rev/a25af78ec2fb
https://hg.mozilla.org/mozilla-central/rev/05c7d6b210a4
https://hg.mozilla.org/mozilla-central/rev/b64a96412dd3

Comment 36

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bdc850e67f6

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9bdc850e67f6