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

Clean up the Dependencies section - fix #47 #50

Merged
merged 5 commits into from Aug 15, 2017

Conversation

Projects
None yet
3 participants
@siusin
Copy link
Contributor

siusin commented Aug 11, 2017

To fix #47 .

  • API referrer source was renamed as referrer source, following the change in the HTML spec;
  • remove the normative references to FileAPI, XHR and the Typed Array Specification, since those terms are no longer needed by the Beacon spec;
  • a few other minor changes.

I failed to find a good way to fix the warning,

No for operation sendBeacon in Navigator

The suggestion in the warning dialog doesn't work out.


Preview | Diff

@siusin siusin requested a review from igrigorik Aug 11, 2017

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Aug 11, 2017

@siusin awesome work! On a first pass, this looks good.

Re, sendBeacon warning: @marcoscaceres any tips on this one? Preview: https://cdn.rawgit.com/siusin/beacon/97428ec4a08aafce48ff698caf5e6c2ea8e9628d/index.html

index.html Outdated
@@ -41,25 +41,9 @@
license: 'w3c-software-doc',
wgPublicList: "public-web-perf",
noLegacyStyle: true,
github: "https://github.com/w3c/beacon",
testSuiteURI: "https://github.com/w3c/web-platform-tests/tree/master/beacon",

This comment has been minimized.

Copy link
@marcoscaceres

marcoscaceres Aug 14, 2017

Member

Please use:
https://w3c-test.org/beacon/

Otherwise, it's painful for people to run the tests without cloning the tests, etc.

This comment has been minimized.

Copy link
@siusin

siusin Aug 14, 2017

Author Contributor

Ok.

</ul>
</dd>
<dt>HTML5</dt>
<dt>HTML52</dt>

This comment has been minimized.

Copy link
@marcoscaceres

marcoscaceres Aug 14, 2017

Member

Um, it's not gonna be fun having to bounce between WHATWG HTML and W3C HTML... can we just stick to [[!HTML]]?

This comment has been minimized.

Copy link
@siusin

siusin Aug 14, 2017

Author Contributor

@marcoscaceres I've tried but there is no "referrer source" in the WHATWG HTML.

This comment has been minimized.

Copy link
@marcoscaceres

marcoscaceres Aug 14, 2017

Member

Sorry, not sure what you mean by no "referrer source"?

This comment has been minimized.

Copy link
@igrigorik

igrigorik Aug 14, 2017

Member

Reviewing the logic, I believe we can drop that step entirely. That step was added before we called out to Fetch, which already handles this for us: https://w3c.github.io/webappsec-referrer-policy/#ref-for-determine-requests-referrer-3

@siusin we'd need to remove the step, and the extra variable declaration in step 7.

This comment has been minimized.

Copy link
@siusin

siusin Aug 15, 2017

Author Contributor

@marcoscaceres I meant the WHATWG version has dropped the term "referrer source" :)

@igrigorik make sense! Shall we merge this and make that change in a separated pull?

This comment has been minimized.

Copy link
@igrigorik

igrigorik Aug 15, 2017

Member

@siusin sounds good.

index.html Outdated
<section id="sec-sendBeacon-method">
<h2><code>sendBeacon</code> Method</h2>
<section data-dfn-for="sendBeacon" data-dfn-for="sendBeacon">
<h3><dfn><code>sendBeacon</code></dfn> Method</h3>

This comment has been minimized.

Copy link
@marcoscaceres

marcoscaceres Aug 14, 2017

Member

Nit:

<h3><code><dfn>sendBeacon</dfn></code> Method</h3>

(code on the outside)

index.html Outdated
<h2>Beacon</h2>
<section id="sec-sendBeacon-method">
<h2><code>sendBeacon</code> Method</h2>
<section data-dfn-for="sendBeacon" data-dfn-for="sendBeacon">

This comment has been minimized.

Copy link
@marcoscaceres

marcoscaceres Aug 14, 2017

Member

Should be:

data-dfn-for="Navigator" data-dfn-for="Navigator"

That should fix it :)

I'll try to make the ReSpec more clear that you need to put the identifier of the Interface/dictionary/emun, etc. there - not the method.

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Aug 14, 2017

Just noting that I'm aware that having to add data-dfn-for="Navigator" data-dfn-for="Navigator" is very annoying (as a spec Editor, I too feel this pain).

I will teach ReSpec to add those automatically in the future - and then only yell when there is ambiguity.

As always, I appreciate feedback to make the tool easier to use - and, as ReSpec is open source, contributions from the community are very much welcome.

@siusin

This comment has been minimized.

Copy link
Contributor Author

siusin commented Aug 14, 2017

@marcoscaceres thanks for the tips!

We got a few unexpected warning messages after the update --

Found linkless element with text 'sendbeacon' but no matching <dfn>

We are using <a>sendBeacon</a> to link to the method, any thoughts?

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Aug 14, 2017

having a look

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Aug 14, 2017

Ok, generally you wouldn't do this, but as this spec is only about one method:

Change body to:

<body data-link-for="Navigator">

Change the sendBeacon dfn to:

<h3><code><dfn>sendBeacon()</dfn></code> Method</h3>

And then, do a find a replace, and change all:

<a>sendBeacon</a>

to:

<a>sendBeacon()</a>
@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Aug 14, 2017

Noting, "data-link-for" is telling ReSpec, "if you can't find a xref for something, it's probably something on Navigator".

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Aug 14, 2017

(if it gives you any more grief, please feel free to merge and I can take over any last remaining issues... but testing the above locally, it works). You can grab it from here:

https://gist.github.com/marcoscaceres/0e8c12ab51d1ae817f98878fe478b896

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Aug 14, 2017

oh, yeah 🥓 🥓 🥓 🥓!

@siusin

This comment has been minimized.

Copy link
Contributor Author

siusin commented Aug 14, 2017

Yeah, everything works perfectly now ;)

@marcoscaceres ++

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Aug 14, 2017

you might want to give that a:

tidy -config tidyconfig.txt -o index.html index.html
@siusin

This comment has been minimized.

Copy link
Contributor Author

siusin commented Aug 14, 2017

Sure, will fix the bugs reported by HTML Tidy later.

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Aug 14, 2017

@marcoscaceres @siusin \o/ .. thanks for the great work on this one!

Left one nitpick on "referrer source" discussion, but we can also take that one as a separate update. @siusin I'll defer to you.

@igrigorik igrigorik merged commit eb050e2 into w3c:gh-pages Aug 15, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ipr PR deemed acceptable.
Details
@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Aug 15, 2017

@siusin @marcoscaceres thanks (again) to both of you for the great work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.