David Zbarsky (:dzbarsky) Assignee
	Description • 11 years ago

      No description provided.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 11 years ago

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?  :(

	David Zbarsky (:dzbarsky) Assignee
	Comment 5 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 11 years ago

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

	David Zbarsky (:dzbarsky) Assignee
	Comment 7 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 11 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 11 years ago

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

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 11 years ago

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

r=me

	David Zbarsky (:dzbarsky) Assignee
	Comment 17 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 11 years ago

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

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 11 years ago

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

	David Zbarsky (:dzbarsky) Assignee
	Comment 24 • 11 years ago

(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.

	David Zbarsky (:dzbarsky) Assignee
	Comment 25 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 11 years ago

Add tests?

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 11 years ago

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

r=me

	David Zbarsky (:dzbarsky) Assignee
	Comment 32 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 11 years ago

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

Hmm.  r=me

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 11 years ago

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

	David Zbarsky (:dzbarsky) Assignee
	Comment 36 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 37 • 11 years ago

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