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 tests for file snapshot state #34143

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Add tests for file snapshot state #34143

wants to merge 3 commits into from

Conversation

saschanaz
Copy link
Member

@saschanaz saschanaz commented May 20, 2022

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

LGTM with some instruction nits

<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<label>Select a file first: <input type="file" id="fileInput"></label><br>
<button id="readyButton">Then modify the content of the selected file and click this</button><br>
Copy link
Member

Choose a reason for hiding this comment

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

Be explicit about the required modification (i.e. the size should be smaller)

Copy link
Member Author

@saschanaz saschanaz May 20, 2022

Choose a reason for hiding this comment

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

No, the size can be random. Larger size is also okay. (This is assuming clipping behavior of WebKit. The question is, should it clip?)

FileAPI/snapshot-state/touch-manual.html Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented May 23, 2022

When I emptied a file, FileReader#readAsDataURL() return clipped result by File#size ended up failing in Safari TP with assert_less_than_equal: buffer should be smaller than the initial file size expected a number less than or equal to 5 but got 3221.

One way this test could be improved I suppose is with explicit instructions around what the file contents should be before and after as that would allow you to assert those as well.

@saschanaz
Copy link
Member Author

When I emptied a file, FileReader#readAsDataURL() return clipped result by File#size ended up failing in Safari TP with assert_less_than_equal: buffer should be smaller than the initial file size expected a number less than or equal to 5 but got 3221.

Huh, I can reproduce this, and it's exactly 3221. Interesting.

@annevk
Copy link
Member

annevk commented May 23, 2022

@saschanaz
Copy link
Member Author

saschanaz commented May 23, 2022

https://bugs.webkit.org/show_bug.cgi?id=240801

cc @cdumez

Tried adding a console.log and found it gives the current html page source instead. 👀

Anyway, I'm pretty confident that we all want the timestamp based approach to detect file change, what I'm not confident is whether to clip or not, since it's WebKit only thing. Any idea? (And if yes, should it be padded? I'd say no, I'm not sure that would be 100% web compatible and I'm okay enough with current no-padding behavior. And it's already very edge case enough.)

@annevk
Copy link
Member

annevk commented May 23, 2022

What is the benefit of clipping? Being able to reuse an existing byte buffer for the file? I think I'd rather not clip. Even better would be treating a size change the same as a timestamp change. This would reduce the "leak" even more.

@saschanaz
Copy link
Member Author

Oh yeah, why not do both of checks and just forget about edge cases of clipping? That sounds safer and much easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants