Closed Bug 856472 Opened 7 years ago Closed 7 years ago

Convert the rest of Canvas to WebIDL

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 3 obsolete files)

119.11 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
15.74 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
32.59 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
11.72 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
30.90 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
18.24 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
8.78 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.90 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.96 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #731707 - Flags: review?(bzbarsky)
Attached patch Convert TextMetrics to WebIDL (obsolete) — Splinter Review
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
Attachment #731707 - Flags: review?(bzbarsky) → review+
Attachment #731708 - Attachment is obsolete: true
Attachment #732105 - Flags: review?(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?  :(
I'll run this through try to make sure we're actually returning null everywhere we throw.
Attachment #732186 - Flags: review?(bzbarsky)
Note that right now nothing is actually using the 'owned' bindings....
Right, that nullcheck is for all failed returns, not just 'owned' bindings.
Attachment #732192 - Flags: review?(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.
Attachment #732105 - Flags: review?(bzbarsky) → review+
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?
Attachment #732186 - Flags: review?(bzbarsky) → review-
Comment on attachment 732192 [details] [diff] [review]
Move CanvasPattern to its own file

r=me
Attachment #732192 - Flags: review?(bzbarsky) → review+
Attachment #732560 - Flags: review?(bzbarsky)
Attachment #732186 - Attachment is obsolete: true
Attachment #732600 - Flags: review?(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.
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 on attachment 732600 [details] [diff] [review]
Convert CanvasPattern to WebIDL

r=me
Attachment #732600 - Flags: review?(bzbarsky) → review+
Attachment #732560 - Flags: review?(bzbarsky) → review-
Attachment #739689 - Flags: review?(bzbarsky)
Attachment #739691 - Flags: review?(bzbarsky)
Duplicate of this bug: 800553
Comment on attachment 739689 [details] [diff] [review]
Move CanvasGradient to its own file

r=me
Attachment #739689 - Flags: review?(bzbarsky) → review+
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
Attachment #739691 - Flags: review?(bzbarsky) → review+
(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.
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
Attachment #732560 - Attachment is obsolete: true
Attachment #742150 - Flags: review?(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
Attachment #742150 - Flags: review?(bzbarsky) → review+
Attachment #742168 - Flags: review?(bzbarsky)
Comment on attachment 742168 [details] [diff] [review]
wrappercache CanvasGradient

r=me
Attachment #742168 - Flags: review?(bzbarsky) → review+
Whiteboard: [leave open]
Whiteboard: [leave open]
Comment on attachment 742460 [details] [diff] [review]
pattern and gradient don't need to inherit nsISupports

Hmm.  r=me
Attachment #742460 - Flags: review?(bzbarsky) → review+
Depends on: 866575
https://hg.mozilla.org/mozilla-central/rev/9bdc850e67f6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.