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

Use USVString for all URLs #1282

Merged
merged 5 commits into from
May 20, 2016
Merged

Use USVString for all URLs #1282

merged 5 commits into from
May 20, 2016

Conversation

domenic
Copy link
Member

@domenic domenic commented May 19, 2016

This finds all instances where URLs are part of an IDL interface,
including where they are reflecting content attributes, and changes the
type from DOMString to USVString. This has already been done in some
places in a haphazard way, but this commit finishes the process.

This allows us to clarify the rules for reflecting USVString attributes,
and remove the clause that allows DOMString attributes to reflect URLs.

Fixes #1254. Fixes #1266.

/cc @Ms2ger

This finds all instances where URLs are part of an IDL interface,
including where they are reflecting content attributes, and changes the
type from DOMString to USVString. This has already been done in some
places in a haphazard way, but this commit finishes the process.

This allows us to clarify the rules for reflecting USVString attributes,
and remove the clause that allows DOMString attributes to reflect URLs.

Fixes #1254. Fixes #1266.
@annevk
Copy link
Member

annevk commented May 20, 2016

Changes LGTM. Will merge later today in case anyone else wants to have a look.

@@ -24497,7 +24500,7 @@ was an English &lt;a href="/wiki/Music_hall">music hall&lt;/a> singer, ...</pre>
<dt><span data-x="concept-element-dom">DOM interface</span>:</dt>
<dd>
<pre class="idl">interface <dfn>HTMLSourceElement</dfn> : <span>HTMLElement</span> {
[<span>CEReactions</span>] attribute DOMString <span data-x="dom-source-src">src</span>;
[<span>CEReactions</span>] attribute USVString <span data-x="dom-source-src">src</span>;
[<span>CEReactions</span>] attribute DOMString <span data-x="dom-source-type">type</span>;
[<span>CEReactions</span>] attribute DOMString <span data-x="dom-source-srcset">srcset</span>;
Copy link
Member

Choose a reason for hiding this comment

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

Missed this one?

@zcorpan
Copy link
Member

zcorpan commented May 20, 2016

srcset seems to not be covered by the reflection rules, as it's not defined to contain a URL (it has its own microsyntax that has URLs).

Edit: Sorry, there's already a catch-all for USVString. Ignore the above.

@zcorpan
Copy link
Member

zcorpan commented May 20, 2016

Missing the constructors for Worker and SharedWorker

@annevk
Copy link
Member

annevk commented May 20, 2016

Pushed a fixup.

@zcorpan
Copy link
Member

zcorpan commented May 20, 2016

I also pushed a fixup with a bunch more.

@@ -87017,15 +87017,15 @@ interface <dfn>NavigatorOnLine</dfn> {
<pre class="idl">[Constructor(DOMString type, optional <span>ErrorEventInit</span> eventInitDict), Exposed=(Window,Worker)]
interface <dfn>ErrorEvent</dfn> : <span>Event</span> {
readonly attribute DOMString <span data-x="dom-ErrorEvent-message">message</span>;
readonly attribute DOMString <span data-x="dom-ErrorEvent-filename">filename</span>;
readonly attribute USVString <span data-x="dom-ErrorEvent-filename">filename</span>;
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure about this one?

Copy link
Member

Choose a reason for hiding this comment

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

https://html.spec.whatwg.org/multipage/webappapis.html#runtime-script-errors

Let location be an absolute URL that corresponds to the resource from which script was obtained.

...

Initialise event's filename attribute to location.

@zcorpan
Copy link
Member

zcorpan commented May 20, 2016

OK I think I'm done here.

@annevk annevk assigned domenic and unassigned annevk May 20, 2016
@annevk
Copy link
Member

annevk commented May 20, 2016

Looks good, will let @domenic land.

@domenic
Copy link
Member Author

domenic commented May 20, 2016

Great catches, thanks; can't believe I missed them.

@domenic domenic merged commit 018b983 into master May 20, 2016
@domenic domenic deleted the usvstring-reflect branch May 20, 2016 13:02
SimonSapin added a commit to SimonSapin/csswg-drafts that referenced this pull request Apr 21, 2017
As resolved by CSS WG on 2017-04-19, because they are URLs.

This is consistent with HTML: whatwg/html#1282
zcorpan pushed a commit to w3c/csswg-drafts that referenced this pull request Apr 21, 2017
As resolved by CSS WG on 2017-04-19, because they are URLs.

This is consistent with HTML: whatwg/html#1282
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.

3 participants