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

clarify incomplete params #46

Closed
cvazac opened this issue Dec 20, 2017 · 4 comments
Closed

clarify incomplete params #46

cvazac opened this issue Dec 20, 2017 · 4 comments

Comments

@cvazac
Copy link
Contributor

cvazac commented Dec 20, 2017

The spec currently reads:

If any parameter is specified more than once, only the first instance is to be considered. All subsequent occurrences MUST be ignored without signaling an error or otherwise altering the processing of the server-timing-metric.

I think we need to clarify three scenarios, where a param name is supplied with no valid param value

  1. metric;dur=123.4;dur=567.8 yields a duration of 123.4 in chrome
  2. metric;dur=foo;dur=567.8 yields a duration of 0 in chrome
  3. metric;dur;dur=123.4 yields a duration of 123.4 in chrome
@cvazac
Copy link
Contributor Author

cvazac commented Dec 20, 2017

I suspect that there's no debate on 1).

Examples 2) and 3) could be considered discrepant. If we convert the empty string of the first dur param in 3), we'd get a 0.

Consistency options:
A) 2) & 3) both return a 0 duration
B) 2) returns 567.8 and 3) returns 123.4

@yoavweiss
Copy link
Contributor

Seems to me that A) is the more consistent option, and it's also what would naturally fall out of HTML's parsing rules.

IMO, we should clarify the spec language around that, and change implementations and tests accordingly.

@KershawChang
Copy link

I think A) is better, since it would make the code a bit easier.
I mean we can just take the string after "dur" without checking if it's a valid double value.

@cvazac
Copy link
Contributor Author

cvazac commented Dec 28, 2017

fixed with: #47

@cvazac cvazac closed this as completed Dec 28, 2017
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

No branches or pull requests

3 participants