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

Define supportedEntryTypes #108

Merged
merged 10 commits into from Nov 27, 2018

Conversation

Projects
None yet
4 participants
@npm1
Copy link
Contributor

npm1 commented Oct 5, 2018

Add supportedEntryTypes and include an example of how it would be used. Solves #77


Preview | Diff

Define supportedEntryTypes
Add supportedEntryTypes and include an example of how it would be used. Solves #77
@npm1

This comment has been minimized.

Copy link
Contributor

npm1 commented Oct 5, 2018

@yoavweiss @rniwa @toddreifsteck can you take a look? I can't edit the "Reviewers" section of PR so using comment to ask for review. @igrigorik for when you're back.

@npm1

This comment has been minimized.

Copy link
Contributor

npm1 commented Oct 5, 2018

Show resolved Hide resolved index.html
Show resolved Hide resolved index.html Outdated
Show resolved Hide resolved index.html Outdated
Show resolved Hide resolved index.html Outdated
Show resolved Hide resolved index.html Outdated
Show resolved Hide resolved index.html Outdated

npm1 added some commits Oct 10, 2018

@npm1

This comment has been minimized.

Copy link
Contributor

npm1 commented Nov 20, 2018

BTW this is ready for review again, please take a look. Thanks!

@igrigorik
Copy link
Member

igrigorik left a comment

Overall, looks good. Left a few small comments and suggestions..

FWIW, I'm still wondering if we really need the "supported" prefix (i.e. use performance.entryTypes instead), but if others are comfortable with the longer name.. that works.

Show resolved Hide resolved index.html Outdated
Show resolved Hide resolved index.html
Show resolved Hide resolved index.html Outdated
Feedback
Nits
@rniwa

This comment has been minimized.

Copy link

rniwa commented Nov 21, 2018

Hm... DOMTokenList has supports method as opposed to a list of tokens supported. Perhaps matching that pattern is better. This will also avoid the ordering issue.

@npm1

This comment has been minimized.

Copy link
Contributor

npm1 commented Nov 21, 2018

Consistency with completely unrelated APIs is probably not something we should worry too much about. Otherwise I could say we should also be consistent with PushManager and we will never be happy :) A better question is what would be more ergonomic to web developers. I think both ways are valid so don't have a strong opinion on which is better.

@rniwa

This comment has been minimized.

Copy link

rniwa commented Nov 21, 2018

You say that but all these inconsistencies throughout various features is the worst thing about the Web platform. Ideally, TAG or some other group familiar with all aspects of the Web API should be noticing these differences and fixing them but that doesn't seem to be happening either.

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Nov 22, 2018

@rniwa hmm, I like the idea — good catch. One gotcha I see here is that it also supports add/remove/toggle, which are a bit odd in our context. That said, if we're returning an array, then perhaps there is no difference?

Show resolved Hide resolved index.html
Show resolved Hide resolved index.html Outdated
Show resolved Hide resolved index.html Outdated
Show resolved Hide resolved index.html Outdated
Show resolved Hide resolved index.html Outdated
@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Nov 22, 2018

Hm... DOMTokenList has supports method as opposed to a list of tokens supported. Perhaps matching that pattern is better. This will also avoid the ordering issue.

supports was added to DOMTokenList as a way to feature detect supported attribute values. This is not quite the same case. We already have a way to feature detect support for the various features. This will just provide a cleaner way to do the same as well as discover observable types.

npm1 added some commits Nov 22, 2018

Show resolved Hide resolved index.html Outdated
index.html Outdated
@@ -428,6 +438,22 @@ <h2><dfn>disconnect()</dfn> method</h2>
"WHATWG-DOM#context-object">context object</a>'s <a>observer
buffer</a>.</p>
</section>
<section>
<h2><dfn>supportedEntryTypes</dfn> attribute</h2>

This comment has been minimized.

@yoavweiss

yoavweiss Nov 26, 2018

Contributor

I don't think you want 2 <dfn> to the same attribute, so this should probably be an <a> (with the dfn being in the paragraph below)

This comment has been minimized.

@npm1

npm1 Nov 26, 2018

Contributor

Changed the dfn to the place where attribute getter is described. However, there's not quite repetition. There's supportedEntryTypes, the attribute, and supported entry types, the list maintained by the user agent which is used as the return value.

@yoavweiss
Copy link
Contributor

yoavweiss left a comment

Added a few nitty comments, but I think we're almost there. One thing that bugs me is tests: we can test the presence of the methods, but not necessarily check their return values. I guess we should add tests for those when adding support for that in the various specs that use PO.

@npm1

This comment has been minimized.

Copy link
Contributor

npm1 commented Nov 26, 2018

I'll followup with modifications to other specs as well as tests.

@yoavweiss
Copy link
Contributor

yoavweiss left a comment

LGTM! :)

Please add WPTs that check the method is there (but ignore results) before landing

@npm1

This comment has been minimized.

Copy link
Contributor

npm1 commented Nov 26, 2018

Yoav, there should not be a need for such a test. That's what idlharness is for. When the spec crawler gets updated, it should start failing for implementations that do not have this attribute implemented. So, are you ok just landing this?

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Nov 27, 2018

Makes sense. Ship it! :)

@yoavweiss yoavweiss merged commit 520de3e into w3c:gh-pages Nov 27, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ipr PR deemed acceptable.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment