Bug 195331 – Update WPT tests for the canvas element

• Component: New Bugs
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

Update WPT tests for the canvas element

[Frédéric Wang (:fredw)]

Created attachment 363646 [details]
Patch

[Frédéric Wang (:fredw)]

> 19:16:51 - youenn: fredw: this is probably one deficiency of svn-apply. Some others have been identified but I cannot find any trace for this one, maybe a new bug should be filed. I cced Jonathan on the bug and r+ed the patch. Maybe you can comment you plan to land this patch manually soon.

As discussed with Youenn on IRC, this patch does not apply, probably because of a bug with svn-apply. It works for me with git apply. I also checked that the modified test still passes on GTK / macOS / iOS sim. If nobody opposes to that, I'll land the patch tomorrow morning (Paris time) and watch the bot.

[Jonathan Bedard]

Comment on attachment 363646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363646&action=review

> LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-canvas-element/size.attributes.parse.whitespace.html:16
> +	100" height="

Bit unclear what has actually changed here. (even looking at the patch in Sublime, I can't see a difference between the removed line and added line)  This line, by the way, is what's broken svn-apply. I wonder is this change were reverted, if this patch would work.

[Frédéric Wang (:fredw)]

(In reply to Jonathan Bedard from comment #3)
> Bit unclear what has actually changed here. (even looking at the patch in
> Sublime, I can't see a difference between the removed line and added line) 
> This line, by the way, is what's broken svn-apply. I wonder is this change
> were reverted, if this patch would work.

This file is auto-generated upstream. I have not check the details, but I suspect this whitespace change was done on purpose, so I'm not sure it's right to revert it.

[Alexey Proskuryakov]

This file needs to be marked as binary, there is no way source control would nicely preserve LF, CR and FF characters in weird order.

Not sure if the change is even intentional, as the lines were and remain broken, just in different ways. Quite possible that upstream messed them up too.

So I don't think that this is a bug in svn-apply. Seems like a deeper investigation is necessary to figure out what the intention was, and the file likely needs to be marked as binary upstream.

[Frédéric Wang (:fredw)]

(In reply to Alexey Proskuryakov from comment #5)
> This file needs to be marked as binary, there is no way source control would
> nicely preserve LF, CR and FF characters in weird order.

OK thanks for the info.

> 
> Not sure if the change is even intentional, as the lines were and remain
> broken, just in different ways. Quite possible that upstream messed them up
> too.

By "broken" you mean "line breaking" not that something is wrong in the content?. Checking the patch in emacs, I see that the only the windows's newline characters had been removed.

> So I don't think that this is a bug in svn-apply. Seems like a deeper
> investigation is necessary to figure out what the intention was, and the
> file likely needs to be marked as binary upstream.

OK, it seems to be https://github.com/web-platform-tests/wpt/pull/1599 ; reviewers suggested to use "
" but that was not done. I'll see if I can fix that upstream tomorrow.

[Alexey Proskuryakov]

Thank you for finding the change! It kind of looks incorrect - deleting a character that needed to be tested just because of source control limitations seems like a bad idea.

[Frédéric Wang (:fredw)]

Committed r242533: <https://trac.webkit.org/changeset/242533>

[Frédéric Wang (:fredw)]

(In reply to Alexey Proskuryakov from comment #7)
> Thank you for finding the change! It kind of looks incorrect - deleting a
> character that needed to be tested just because of source control
> limitations seems like a bad idea.

The reason was not about source control limitation but because of HTML parsing rules ( https://github.com/web-platform-tests/wpt/pull/1599#issuecomment-72433595 ) ; as I said the patch was initially landed without following the suggestion of reviewer to use &xD;. This is now done in the follow-up https://github.com/web-platform-tests/wpt/pull/15685

(In reply to Frédéric Wang (:fredw) from comment #8)
> Committed r242533: <https://trac.webkit.org/changeset/242533>

I landed the latest version. This file still has the special space characters causing issues with SVN apply but I didn't change the file to binary because:

(1) It will prevent us to properly see and control any diff in future import, even when they don't touch the space characters.

(2) Probably it makes sense to test space characters without using escape notation, when they don't need this workaround for proper HTML parsing.

For (2), asking WPT people to change python script because of WebKit/SVN's source limitation is probably a weak argument but I guess we can try if we really want to.