Conversation
A byte above 0xFF in the numbers list makes the matching try to continue at each of the next `byte-0xFF` positions. (It searches efficiently for the following byte, then recursively checks the rest of the pattern if finding that byte.) Add magic-byte recognition for `video/webv` and `video/x-matroska`, using this format, and remove the match for `audio/weba` (which is an extension for `audio/webm`, which isn't disitinguishable from `video/webm` without checking whether it contains any non-audio streams.)
Package publishingIf you have publishing permissions, you can use the links below to publish the changes after merging this PR.
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthUnused Dependencies ✔️
For details on how to fix these, see dependency_validator. This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with
Coverage
|
| File | Coverage |
|---|---|
| pkgs/mime/lib/src/magic_number.dart | 💔 94 % ⬇️ 6 % |
| pkgs/mime/lib/src/mime_type.dart | 💚 91 % ⬆️ 1 % |
This check for test coverage is informational (issues shown here will not fail the PR).
This check can be disabled by tagging the PR with skip-coverage-check.
There was a problem hiding this comment.
Code Review
This pull request introduces support for magic byte patterns with variable-width gaps (wildcards) in MagicNumber matching, allowing the library to distinguish between video/webm and video/x-matroska files. Feedback on these changes highlights a contradiction in the wildcard mask validation logic, recommends using isNotEmpty instead of checking length > 0 per Effective Dart guidelines, suggests adding explicit parentheses around bitwise operations to improve readability, and corrects several instances of the typo 'EBMF' to 'EBML'.
| // Make sure masks has same value at that position. | ||
| if (masks != null && mask != number) return false; |
There was a problem hiding this comment.
The check if (masks != null && mask != number) return false; requires that the masks string contains the exact same wildcard character (which is > 0xFF) at the wildcard's position.
However, this directly contradicts the documentation for masks on line 17:
/// Optional string containing code points in the 0..255 range to mask with.
Since the wildcard character is not a byte to be matched (it only specifies a lookahead length), there is no functional need to enforce that the mask matches the wildcard character. Enforcing this requires users to put non-byte values (values > 255) into the masks string, which is counter-intuitive and violates the documented contract.
We should remove this check entirely since the mask at a wildcard position is never used for matching anyway.
|
|
||
| const MagicNumber(this.mimeType, this.numbers, [this.masks]); | ||
| const MagicNumber(this.mimeType, this.numbers, [this.masks]) | ||
| : assert(numbers.length > 0), |
There was a problem hiding this comment.
According to Effective Dart (referenced in the repository style guide), we should prefer using isNotEmpty instead of checking length > 0 for strings and collections.
| : assert(numbers.length > 0), | |
| : assert(numbers.isNotEmpty), |
References
- Prefer using .isEmpty and .isNotEmpty to check for empty collections/strings. (link)
| var number = numbers.codeUnitAt(patternCursor); | ||
| if (masks != null) mask = masks.codeUnitAt(patternCursor); | ||
| if (number <= 0xFF) { | ||
| if ((number ^ header[byteCursor]) & mask != 0) { |
There was a problem hiding this comment.
To improve readability and prevent potential operator precedence confusion (especially for developers coming from languages like C/C++ or Java where equality has higher precedence than bitwise operators), please add explicit parentheses around the bitwise operations.
| if ((number ^ header[byteCursor]) & mask != 0) { | |
| if (((number ^ header[byteCursor]) & mask) != 0) { |
| } | ||
| for (var i = 0; i < lookaheadLength; i++) { | ||
| // Quick scan for first byte to match, before recursing. | ||
| if ((number ^ header[byteCursor + i]) & mask == 0 && |
There was a problem hiding this comment.
To improve readability and prevent potential operator precedence confusion (especially for developers coming from languages like C/C++ or Java where equality has higher precedence than bitwise operators), please add explicit parentheses around the bitwise operations.
| if ((number ^ header[byteCursor + i]) & mask == 0 && | |
| if (((number ^ header[byteCursor + i]) & mask) == 0 && |
| '\x00\x00\x00\x00\x66\x74\x79\x70\x6D\x70\x34\x32', | ||
| '\x00\x00\x00\x00\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF', | ||
| ), | ||
| // Look for EBMF DocType header within the next 64 bytes. |
| The pattern entry limits how far ahead it can look. | ||
|
|
||
| * Recognize `video/webm` and `video/x-matroska` by magic numbers, | ||
| and no longer detects all EBMF files as `audio/weba`. |
| 0x00 | ||
| ]); | ||
| _expectMimeType('file', 'video/webm', headerBytes: [ | ||
| 0x1A, 0x45, 0xDF, 0xA3, // EBMF header |
| 0x42, // Anything | ||
| ]); | ||
| _expectMimeType('file', 'video/x-matroska', headerBytes: [ | ||
| 0x1A, 0x45, 0xDF, 0xA3, // EBMF header |
A byte above 0xFF in the numbers list makes the matching try to continue at each of the next
byte-0xFFpositions. (It searches efficiently for the following byte, then recursively checks the rest of the pattern if finding that byte.)Add magic-byte recognition for
video/webvandvideo/x-matroska, using this format, and remove the match foraudio/weba(which is an extension foraudio/webm, which isn't disitinguishable fromvideo/webmwithout checking whether it contains any non-audio streams.)