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

getEntriesByName and friends should explain if they're case sensitive or not #57

Closed
esprehn opened this issue Aug 30, 2016 · 14 comments
Closed
Labels
Milestone

Comments

@esprehn
Copy link

esprehn commented Aug 30, 2016

Blink has it case insensitive, the spec should probably explicitly call out ASCII Case insensitive if that's what's desired, or we should change the implementation.

@plehegar plehegar added the bug label Sep 1, 2016
@plehegar
Copy link
Member

plehegar commented Sep 1, 2016

I wrote a test for this:
https://www.w3.org/2016/09/pt-naming.html

Edge and Firefox are case sensitive on getEntriesByType, unlike Chrome.

getEntriesByName is trickier. Chrome is also case sensitive on resource names.

If we switch to case insensitive for getEntriesByName, it will trigger all kind of questions if the name contains a URL.

It seems that the conservative approach would be to explain that those methods are case sensitive. @toddreifsteck, wdyt?

@plehegar plehegar added this to the Level 2 milestone Sep 1, 2016
@yoavweiss
Copy link
Contributor

FWIW, I agree that it should be case sensitive

@yoavweiss
Copy link
Contributor

Also, looking at the code, entry type in Blink is indeed case-insensitive (in both getEntriesByType and getEntriesByName), but the name comparison is case-sensitive.

@plehegar
Copy link
Member

plehegar commented Sep 7, 2016

btw, getEntries is case insensitive for the property names. getEntries({'entryType':'resource'}) will return the same thing as getEntries({'EntryType':'resource'}). Ditto for 'name' vs 'NAME'. I updated the test mentioned earlier.

@igrigorik
Copy link
Member

igrigorik commented Sep 12, 2016

So, what's the conclusion here? It sounds like...

  • We have consensus that name checks should be case sensitive.
  • Type should be case insensitive.
  • Property names for getEntries (e.g. getEntries({'entryType':'...'}) should be case insensitive.

Yay / nay?

@plehegar
Copy link
Member

Property names should be case insensitive, eg getEntries({'ENTRYTYPE': ...}) or getEntries({'NAME':...} will work. At least, FF/ED/CH implementations seem to agree on that.

Property values should be case sensitive I think, otherwise we'll get in trouble for resource timing entries imho (getEntriesByName('https://w3c-test.org/RESOURCES/TESTHARNESS.JS') vs getEntriesByName('https://w3c-test.org/resources/testharness.js') ).

At least, implementations need to be consistent because, unless I got mixed up, all of the current implementations don't make sense to me. getEntriesByType('RESOURCE') must return the same thing as getEntries({'entryType': 'RESOURCE'}) and none of the implementations do that?!?

I updated
https://www.w3.org/2016/09/pt-naming.html
to reflect what I'd like but note that none of the implementations are passing those. Someone should double check the test.

@igrigorik
Copy link
Member

Enumerating the various cases...

  1. getEntriesByName('foo') != getEntriesByName('FOO')
    • Must be case sensitive. I think we have agreemend on this.
  2. getEntriesByType('resource') vs getEntriesByType('RESOURCE')
    • Could be case sensitive; case sensitivity doesn't add any value.
  3. getEntries({'name': 'foo'}) vs getEntries({'name': 'FOO'})
    • Must be case sensitive.
  4. getEntries({'type': 'resource')} vs getEntries({'TYPE': 'resource'})
    • Same as (2).
  5. getEntries({'type': 'resource'}) vs getEntries({'type': 'RESOURCE'})
    • Same as (2).

In terms of available strategies we could...

  1. Declare all of the of above to be case sensitive. This won't break anything and it keeps things simple from spec perspective. The con is that there are cases where developers may see this behavior as a gotcha.
  2. Define special behavior for DOMString type + DOMString initiator (insensitive) vs DOMString name (case sensitive).
    • PerformanceEntryFilterOptions property values subject to same policy.
    • PerformanceEntryFilterOptions property names are case insensitive.

There are some other combinations possible here.. but I'm not sure they make a lot of sense (happy to hear other opinions :)). Personally, with my "web app developer" hat on, I would prefer (2), but I don't think (1) is entirely unreasonable either.

@toddreifsteck
Copy link
Member

Ilya, Yoav and I took a look and it appears that getEntries({filter}) is not yet implemented in any browser. We also discussed that adding it would be difficult to feature detect. Because of this, I think we can cut it from the Performance Timeline specification. This simplifies the issue significantly down to:

  1. Should getEntriesByType be case sensitive?
    Yes.. but... this one isn't quite as obvious. IE was first to implement and had it this way. Edge/Firefox are this way. Also.. if we add a dictionary filter overload, we would want case sensitivity to match expected JavaScript object behavior. Unless a site ONLY works for Chrome, this should be a safe bug fix.
  2. Should getEntriesByName be case sensitive?
    Yes, it can have URLs.

@igrigorik
Copy link
Member

For reference: 32b0420 - this is where we introduced getEntries filtering.

@toddreifsteck
Copy link
Member

@annevk made a great comment at W3C TPAC that name being able to contain URLs is unfortunate. It would have been ideal for URLs to have been neatly separated so they could be processed correctly for equality. Case sensitivity is a good first step, but it would be ideal if we could break the URLs into a URL field and we could potentially add some type of URL creation to the getEntriesByName processing model to improve the accuracy of matches.

For the short term, we are going to stick to case sensitive matching.

@igrigorik
Copy link
Member

Resolved via #58, closing. We'll track {filter} stuff in #60.

igrigorik added a commit that referenced this issue Sep 28, 2016
sideshowbarker pushed a commit to web-platform-tests/wpt that referenced this issue Sep 29, 2016
@igrigorik
Copy link
Member

To close the loop on compat discussion of making getEntriesByType case sensitive..

Doing a scan over HTTP Archive shows exactly zero instances of uppercase/mixed-case use for popular keywords (e.g. "resource"): https://gist.github.com/igrigorik/9e66f92338821af4976b3cf25ab7f55a. Which is to say, I believe this is a safe change for Blink.

/cc fyi.. @yoavweiss @RByers

@RByers
Copy link

RByers commented Sep 29, 2016

Given there are no problematic results in HTTP Archive and other engines are already case sensitive, I agree it should be low risk for blink to change, and that (in absence of any other reports of compat impact) we should just consider it a bug-fix (not a breaking API change). Is there a chromium bug filed?

@igrigorik
Copy link
Member

MXEBot pushed a commit to mirror/chromium that referenced this issue Oct 4, 2016
The performance timing spec has been clarified[1] to make getEntriesByType
case sensitive. This CL updates Blink to this new requirement.

[1] w3c/performance-timeline#57

BUG=651507

Review-Url: https://codereview.chromium.org/2390733003
Cr-Commit-Position: refs/heads/master@{#422650}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants