Summary: | REGRESSION (r160085): check-webkit-style: utf8' codec can't decode byte 0x89 in position 0: invalid start byte | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, changseok, commit-queue, galpeter, gbalogh.u-szeged, glenn, llango.u-szeged, mmaxfield, ossy, tgergely.u-szeged, zherczeg | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Daniel Bates
2014-01-09 08:29:36 PST
Also bug 125689 comment 28. This regressed in <http://trac.webkit.org/changeset/160085>. Created attachment 221013 [details]
Patch
Comment on attachment 221013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221013&action=review > Tools/Scripts/webkitpy/common/system/filesystem.py:203 > + def open_file_for_read(self, path): > + return codecs.open(path, 'r', 'utf8', 'replace') > + We shouldn't be adding this one off function with such an ambitious name here. r-. I could see open_text_file_for_reading having an optional argument to do this instead. Can a regression test be added for this fix? Created attachment 221142 [details]
Patch
(In reply to comment #4) > We shouldn't be adding this one off function with such an ambitious name here. You are right, i think it looks better now. (In reply to comment #5) > Can a regression test be added for this fix? Yes, good idea. I added a new test case for that. Comment on attachment 221142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221142&action=review Seems fine to land to stop bot failures as an emergency fix. Thanks for looking into this! Longer term, I think that the approach could be further improved. We could skip known non-text files, and we could also handle decoding failures more gracefully. In fact, we should probably report an explicit error when there is malformed utf-8 in actual text files. I do not know what our compilers do in this case, especially when it's in comments. > Tools/ChangeLog:8 > + Resolve regression and remove a FIXME comment that is already fixed. The final patch doesn't remove any FIXMEs. > Tools/Scripts/webkitpy/common/system/filesystem.py:201 > + def open_text_file_for_reading(self, path, errors='strict'): It's not good that we are calling a function named "open_text_file_for_reading" on all files, not only text ones. This should be fixed. Comment on attachment 221142 [details] Patch Clearing flags on attachment: 221142 Committed r161986: <http://trac.webkit.org/changeset/161986> All reviewed patches have been landed. Closing bug. (In reply to comment #8) > > Tools/ChangeLog:8 > > + Resolve regression and remove a FIXME comment that is already fixed. > > The final patch doesn't remove any FIXMEs. > :( I missed it from the second version of the patch. That FIXME was fixed in r160085. We Should remove that comment. Sorry for that. |