Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve comments #894

Closed
wants to merge 8 commits into from
Closed

Improve comments #894

wants to merge 8 commits into from

Conversation

MattiasBuelens
Copy link
Contributor

@MattiasBuelens MattiasBuelens commented Aug 4, 2020

This improves the comment fetching to better find the correct parent and member. Instead of extracting the member name from the ID of the <dfn> element, we now parse it from the text content of the element.

We now also emit comments for constructors (when available).

Depends on #920.

@@ -3,11 +3,52 @@
"canvas-todataurl": "Returns a data: URL for the image in the canvas.\n\nThe first argument, if provided, controls the type of the image to be returned (e.g. PNG or JPEG). The default is \"image/png\"; that type is also used if the given type isn't supported. The second argument applies if the type is an image format that supports variable quality (such as \"image/jpeg\"), and is a number in the range 0.0 to 1.0 inclusive indicating the desired quality level for the resulting image.\n\nWhen trying to use types other than \"image/png\", authors can check if the image was really returned in the requested format by checking to see if the returned string starts with one of the exact strings \"data:image/png,\" or \"data:image/png;\". If it does, the image is PNG, and thus the requested type was not supported. (The one exception to this is if the canvas has either no height or no width, in which case the result might simply be \"data:,\".)",
"canvas-toblob": "Creates a Blob object representing a file containing the image in the canvas, and invokes a callback with a handle to that object.\n\nThe second argument, if provided, controls the type of the image to be returned (e.g. PNG or JPEG). The default is \"image/png\"; that type is also used if the given type isn't supported. The third argument applies if the type is an image format that supports variable quality (such as \"image/jpeg\"), and is a number in the range 0.0 to 1.0 inclusive indicating the desired quality level for the resulting image.",
"canvas-transfercontroltooffscreen": "Returns a newly created OffscreenCanvas object that uses the canvas element as a placeholder. Once the canvas element has become a placeholder for an OffscreenCanvas object, its intrinsic size can no longer be changed, and it cannot have a rendering context. The content of the placeholder canvas is updated by calling the commit() method of the OffscreenCanvas object's rendering context.",
"context-2d": "Returns the canvas element.",
"context-canvas": "Returns the canvas element.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now detect more definitions (yay!), but they're still targeting a non-existent Context interface.

This particular definition has the following HTML:

<p>The <dfn id=dom-context-2d-canvas><code>canvas</code></dfn> attribute must return the
  value it was initialized to when the object was created.</p>

The problem is that the <dfn> element does not have a data-dfn-for attribute. So we try to extract the parent and member from the id attribute, but unfortunately it doesn't match the expect parent-member format.

It guess this data-dfn-for attribute is not required when using Wattsi? So we can't really rely upon it here. 🤷

I was thinking we may want to try yet another approach for retargeting comments. Instead of parsing the <dfn> itself, look for an <a> inside of an IDL block that links to the same <dfn> and try to parse that. That's probably gonna cause some other headaches: there might be multiple links, and we'd need to somehow figure out what IDL member corresponds to this link... 😖 Let's maybe leave that for another time? 😛

@MattiasBuelens
Copy link
Contributor Author

I just realized that the only new comments that actually show up in the output are for constructors. So there's nothing exercising the new logic for attributes or methods yet...

I swear this will look more impressive when combined with #890! 😆

@MattiasBuelens
Copy link
Contributor Author

Closing for now.

The fetch-comment script is currently broken, and we're not even sure if we want to fix it or build something else. 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant