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

Add web3dsurvey data gathering to specs and extensions. #3535

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kenrussell
Copy link
Member

@kenrussell kenrussell commented Apr 18, 2023

Thanks to @greggman for suggesting the web3dsurvey refactoring to put
the data collector into a separate iframe.

Thanks to @kainino0x for helping me understand why the iframe was
breaking in the XSL-generated files, and for the workaround.

@kenrussell
Copy link
Member Author

@bhouston did I do this correctly?

Note - if the script's inserted higher up in the extensions' HTML then it breaks their rendering. I'm concerned about this and wonder whether something is going wrong more deeply.

@greggman
Copy link
Contributor

I want to suggest a few things

  1. This script should not be a script, it should be a 3rd party iframe as in

    <iframe src="https://web3dsurvey.com/collector.html" style="width: 1px; height: 1px;"></iframe>
    

    or something along those lines. As a raw <script> it gives a 3rd party site permission to read cookies, local storage, etc of khronos.org or registry.khronos.org which might be storing login credentials.

  2. if the goal of this script is to get data of the general population then is adding it to a site who's only audience is 3D devs adding or subtracting from that goal?

@bhouston
Copy link

This script should not be a script, it should be a 3rd party iframe as in

@greggman I will make the change you suggested - this is a good point.

if the goal of this script is to get data of the general population then is adding it to a site who's only audience is 3D devs adding or subtracting from that goal?

Interestingly, I can add correction for a biased samples. I will look into reporting on this as we go forward. Right now I am collecting about 100K samples per day. It is actually decent and while somewhat biased, it is pretty wide with lots of variation.

@bhouston
Copy link

@kenrussell just put the script in the body probably at the end - it shouldn't be in the section -- I will clarify that on the website. Then it should be fine. I will make a further update in a week or two that does what @greggman suggests and we could do a further update with that change - but I wouldn't wait of it.

@bhouston
Copy link

@greggman I was thinking about this and I may make it into two separate scripts. I will keep the collector.js script but it will be very small and just create an iframe that loads a second script. I will add a Subresource Integrity attribute to the collector.js so that when people embed it, they know exactly what they are getting. Although the seconds script that loads into the iframe will not have a content validation hash so I can continue to version it, but it will load into that sandboxed environment. Does that make sense @greggman ?

@greggman
Copy link
Contributor

greggman commented Apr 18, 2023

The advantage to the iframe is I don't have to verify your script. With Subresource Integrity, I need to go find the source of your script, read through it to make sure it's not doing anything fishy, verify that it generates the same hash. Then, if at some point in the future you need to update the script, I have to through that process again.

With the iframe, I don't have to do any work, just put the iframe in the page. The fact that it's a 3rd party domain is enough for me not to have to care what the script does

@bhouston
Copy link

bhouston commented Apr 24, 2023

I've updated the recommended embedding procedure on Web3DSurvey.com to suggest the iframe embed and I give example code on how to do that based on @greggman's feedback:

<iframe src="https://web3dsurvey.com/collector-iframe.html" style="width: 1px; height: 1px;"></iframe>

It is actually better for collection because on that collector-iframe.html page I can include an Origin Trial token to enable WebGPU on Chrome devices.

With this new approach @greggman would you be interested in embedding the script to help with data collection?

Thanks to @greggman for suggesting the web3dsurvey refactoring to put
the data collector into a separate iframe.

Thanks to @kainino0x for helping me understand why the iframe was
breaking in the XSL-generated files, and for the workaround.
@kenrussell
Copy link
Member Author

Apologies for forgetting to revisit this!

Thanks @greggman for your suggestion to use an iframe, and @bhouston for implementing that a while back. @bhouston , perhaps you would like to update your suggested code to use frameborder="0", which was necessary to keep the WebGL specs rendering cleanly.

Thanks @kainino0x for helping me understand why the XSL-generated HTML was breaking due to a self-closing iframe tag, which is illegal, and for the suggestion to use a non-breaking space inside the iframe to work around that.

I've verified in Chrome's Devtools that this works on all pages (the specs, extension registry, and individual extensions' pages), and that the collector runs a couple of seconds after the main page loads.

@kenrussell
Copy link
Member Author

Thanks @kainino0x for reviewing.

I'd like to give everyone time to review this one especially given that @greggman found the earlier issue which motivated the change to an iframe.

Copy link
Contributor

@greggman greggman left a comment

Choose a reason for hiding this comment

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

lgtm, just a suggestion

@@ -76,6 +76,9 @@
<p><strong>DO NOT IMPLEMENT!!!</strong></p>
</xsl:if>

<xsl:comment>Help the community by recording statistics on availability of 3D APIs. Trick to prevent XSL processor from creating an illegal self-closing iframe tag.</xsl:comment>
<iframe src="https://web3dsurvey.com/collector-iframe.html" style="width: 1px; height: 1px;" frameborder="0"><xsl:text>&#160;</xsl:text></iframe>
Copy link
Contributor

Choose a reason for hiding this comment

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

how about

<iframe src="https://web3dsurvey.com/collector-iframe.html" width="1" height="1" style="border: none;">

IIUC, the code as is is asking for a 300x150 iframe to be scaled to 1x1 where as the suggested code is asking for a 1x1 iframe. Also frameborder="0" is deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT the width/height attributes do the same thing as the CSS, on iframes. So could just as well be all CSS.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right! Thanks!

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

Successfully merging this pull request may close these issues.

4 participants