Skip to content

Conversation

@annevk
Copy link
Member

@annevk annevk commented Mar 30, 2016

Fixes #35.

source Outdated
interface <dfn>CanvasPattern</dfn> {
// opaque object
void <span data-x="dom-canvaspattern-setTransform">setTransform</span>(<span>DOMMatrix</span> transform);
void <span data-x="dom-canvaspattern-setTransform">setTransform</span>(<span>DOMMatrixInit</span> transform);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional

@annevk
Copy link
Member Author

annevk commented Mar 30, 2016

Also paging @junov for additional review.

@annevk
Copy link
Member Author

annevk commented Mar 30, 2016

Comments addressed.

source Outdated
@@ -62627,7 +62630,8 @@ try {

<p>The <dfn><code data-x="">setTransform(<var>matrix</var>)</code></dfn> method must
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not new in this commit, but how come the argument is called "matrix" here, and "transform" elsewhere?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And why is data-x=""?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why the argument name is different. Happy to make them consistent. What would you prefer?

I think data-x="" is used since the method is defined twice. It would probably be better to define it once and branch on the number of arguments or some such.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general type names should be indicative of the form of the data, and variable names should be indicative of the meaning of the data. By that reasoning, "transform" is the better name.

@annevk
Copy link
Member Author

annevk commented Mar 31, 2016

Done, apart from defining setTransform() as a single method. There's not really a good way to do that with overloading.

@junov
Copy link
Member

junov commented Mar 31, 2016

Looks good to me.

@zcorpan
Copy link
Member

zcorpan commented Mar 31, 2016

LGTM2

@annevk annevk merged commit 52ef2b1 into master Apr 1, 2016
@annevk annevk deleted the dommatrixinit branch April 1, 2016 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants