Convert the rest of Canvas to WebIDL

RESOLVED FIXED in mozilla25

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

(Blocks: 1 bug)

unspecified
mozilla25
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 3 obsolete attachments)

119.11 KB, patch
Details | Diff | Splinter Review
15.74 KB, patch
Details | Diff | Splinter Review
32.59 KB, patch
Details | Diff | Splinter Review
11.72 KB, patch
Details | Diff | Splinter Review
30.90 KB, patch
Details | Diff | Splinter Review
18.24 KB, patch
Details | Diff | Splinter Review
8.78 KB, patch
Details | Diff | Splinter Review
7.90 KB, patch
Details | Diff | Splinter Review
3.96 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 731707 [details] [diff] [review]
Move TextMetrics to its own header
Attachment #731707 - Flags: review?(bzbarsky)
(Assignee)

Comment 1

5 years ago
Created attachment 731708 [details] [diff] [review]
Convert TextMetrics to WebIDL
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+
(Assignee)

Comment 3

5 years ago
Created attachment 732105 [details] [diff] [review]
Convert TextMetrics to WebIDL
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?  :(
(Assignee)

Comment 5

5 years ago
Created attachment 732186 [details] [diff] [review]
Delete owned objects if wrapping fails

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

Comment 7

5 years ago
Right, that nullcheck is for all failed returns, not just 'owned' bindings.
(Assignee)

Comment 8

5 years ago
Created attachment 732192 [details] [diff] [review]
Move CanvasPattern to its own file
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+
(Assignee)

Comment 12

5 years ago
Created attachment 732560 [details] [diff] [review]
Delete owned objects if wrapping fails
Attachment #732560 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #732186 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Created attachment 732600 [details] [diff] [review]
Convert CanvasPattern to WebIDL
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-
(Assignee)

Comment 19

5 years ago
Created attachment 739689 [details] [diff] [review]
Move CanvasGradient to its own file
Attachment #739689 - Flags: review?(bzbarsky)
(Assignee)

Comment 20

5 years ago
Created attachment 739691 [details] [diff] [review]
Convert CanvasGradient to WebIDL
Attachment #739691 - Flags: review?(bzbarsky)

Updated

5 years ago
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+
(Assignee)

Comment 24

5 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.
(Assignee)

Comment 25

5 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
(Assignee)

Updated

5 years ago
Attachment #732560 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
Created attachment 742150 [details] [diff] [review]
WrapperCache CanvasPattern
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+
(Assignee)

Comment 30

5 years ago
Created attachment 742168 [details] [diff] [review]
wrappercache CanvasGradient
Attachment #742168 - Flags: review?(bzbarsky)
Comment on attachment 742168 [details] [diff] [review]
wrappercache CanvasGradient

r=me
Attachment #742168 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
(Assignee)

Comment 33

5 years ago
Created attachment 742460 [details] [diff] [review]
pattern and gradient don't need to inherit nsISupports
Attachment #742460 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
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+
Blocks: 580070

Updated

5 years ago
Depends on: 866575
https://hg.mozilla.org/mozilla-central/rev/9bdc850e67f6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.