Summary: | [GTK] Update WOFF2 decoder | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Khaled Hosny <dr.khaled.hosny> | ||||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bugs-noreply, cgarcia, commit-queue, fred.wang, mcatanzaro | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | Other | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
See Also: |
https://bugzilla.mozilla.org/show_bug.cgi?id=1305944 https://bugs.webkit.org/show_bug.cgi?id=162678 |
||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 162668 | ||||||||||||||
Attachments: |
|
Description
Khaled Hosny
2016-09-27 06:08:39 PDT
Created attachment 289942 [details]
Patch
Attachment 289942 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/graphics/WOFFFileFormat.cpp:117: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 2 in 19 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 289945 [details]
Patch, v2
Comment on attachment 289945 [details] Patch, v2 View in context: https://bugs.webkit.org/attachment.cgi?id=289945&action=review > Source/WebCore/platform/graphics/WOFFFileFormat.cpp:103 > + return Write(data, m_size, n); I think this should be removed. > Source/WebCore/platform/graphics/WOFFFileFormat.cpp:104 > + if (!m_vector.tryReserveCapacity(m_size + n)) Is it needed? It seems that WTF::Vector::append already calls expandCapacity. > Source/WebCore/platform/graphics/WOFFFileFormat.cpp:113 > + if (!m_vector.tryReserveCapacity(m_size + n)) Same here. > Source/WebCore/platform/graphics/WOFFFileFormat.cpp:116 > + m_vector.insert(offset, static_cast<const char*>(data), n); I wonder if it's possible to copy the data in one step. > Source/WebCore/platform/graphics/WOFFFileFormat.cpp:128 > + size_t m_size; Do you really need this member? It seems that you can just use m_vector::size() Comment on attachment 289945 [details] Patch, v2 View in context: https://bugs.webkit.org/attachment.cgi?id=289945&action=review >> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:103 >> + return Write(data, m_size, n); > > I think this should be removed. Good catch, I think it was a left over from previous trials. >> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:104 >> + if (!m_vector.tryReserveCapacity(m_size + n)) > > Is it needed? It seems that WTF::Vector::append already calls expandCapacity. I’m not sure what is going on, but if I don’t do that decoding will always fail. >> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:116 >> + m_vector.insert(offset, static_cast<const char*>(data), n); > > I wonder if it's possible to copy the data in one step. I’m not sure if Vector has an API to do that, I didn’t seem to find any. >> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:128 >> + size_t m_size; > > Do you really need this member? It seems that you can just use m_vector::size() I removed it. Created attachment 289962 [details]
Patch, v3
This should address the comments on the previous patch.
Comment on attachment 289962 [details] Patch, v3 View in context: https://bugs.webkit.org/attachment.cgi?id=289962&action=review r=me on the condition that Frederic says it's good. > Source/WebCore/platform/graphics/WOFFFileFormat.cpp:100 > + bool Write(const void *data, size_t n) void* data, not void *data Is this a virtual method? If so you need to mark it override. If not, it should be named write with a lowercase W, since WebKit methods are camelCase. > Source/WebCore/platform/graphics/WOFFFileFormat.cpp:108 > + bool Write(const void *data, size_t offset, size_t n) Ditto > Source/WebCore/platform/graphics/WOFFFileFormat.cpp:125 > + Vector<char>& m_vector; Optional: I think it would be safer to keep a plain Vector<char> here instead of a reference, and sink it to WOFF2VectorOut by changing the constructor to take a Vector<char>&& instead of a Vector<char>. Then you will need to use WTFMove to pass the vector to the constructor. The advantage of doing it that way is the WOFF2VectorOut becomes harder to misuse (more robust to future modification); currently it breaks badly if the passed in vector goes out of scope before the WOFF2VectorOut does. But this is optional; the current way is OK too. Comment on attachment 289962 [details] Patch, v3 View in context: https://bugs.webkit.org/attachment.cgi?id=289962&action=review Yes, this looks good to me with Michael's comments addressed. >> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:100 >> + bool Write(const void *data, size_t n) > > void* data, not void *data > > Is this a virtual method? If so you need to mark it override. If not, it should be named write with a lowercase W, since WebKit methods are camelCase. Yes, they are pure virtual and should indeed be marked override. https://github.com/google/woff2/blob/4e698b8c6c5e070d53c340db9ddf160e21070ede/src/woff2_out.h#L59 (In reply to comment #8) > Comment on attachment 289962 [details] > Patch, v3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289962&action=review > > Yes, this looks good to me with Michael's comments addressed. We should also import http://test.csswg.org/harness/test/woff2_dev/single/header-totalsfntsize-001/ as the patch makes this test passes. Comment on attachment 289962 [details] Patch, v3 View in context: https://bugs.webkit.org/attachment.cgi?id=289962&action=review >> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:125 >> + Vector<char>& m_vector; > > Optional: I think it would be safer to keep a plain Vector<char> here instead of a reference, and sink it to WOFF2VectorOut by changing the constructor to take a Vector<char>&& instead of a Vector<char>. Then you will need to use WTFMove to pass the vector to the constructor. The advantage of doing it that way is the WOFF2VectorOut becomes harder to misuse (more robust to future modification); currently it breaks badly if the passed in vector goes out of scope before the WOFF2VectorOut does. But this is optional; the current way is OK too. I tried doing that, but then nothing was written to the `sfnt` vector, not sure if I’m doing something wrong: diff --git a/Source/WebCore/platform/graphics/WOFFFileFormat.cpp b/Source/WebCore/platform/graphics/WOFFFileFormat.cpp index c2323cf..60eb7df 100644 --- a/Source/WebCore/platform/graphics/WOFFFileFormat.cpp +++ b/Source/WebCore/platform/graphics/WOFFFileFormat.cpp @@ -93,8 +93,8 @@ bool isWOFF(SharedBuffer& buffer) #if USE(WOFF2) class WOFF2VectorOut : public woff2::WOFF2Out { public: - WOFF2VectorOut(Vector<char>& vector) - : m_vector(vector) + WOFF2VectorOut(Vector<char>&& vector) + : m_vector(WTFMove(vector)) { } bool Write(const void* data, size_t n) override @@ -122,7 +122,7 @@ public: } private: - Vector<char>& m_vector; + Vector<char> m_vector; }; #endif @@ -148,7 +148,7 @@ bool convertWOFFToSfnt(SharedBuffer& woff, Vector<char>& sfnt) if (!sfnt.tryReserveCapacity(sfntSize)) return false; - WOFF2VectorOut out(sfnt); + WOFF2VectorOut out(WTFMove(sfnt)); return woff2::ConvertWOFF2ToTTF(woffData, woffSize, &out); } #endif Created attachment 290070 [details]
Patch, v4
Updated the patch to address the style comments and add a new test based on the one in WOFF2 test suite.
(In reply to comment #10) > I tried doing that, but then nothing was written to the `sfnt` vector, not > sure if I’m doing something wrong: No, I simply missed that sfnt was a parameter passed by reference and needs to be kept valid, so my suggestion was wrong. Created attachment 290072 [details]
Patch, v5
Comment on attachment 290072 [details] Patch, v5 Rejecting attachment 290072 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 290072, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/2161227 Committed r206511: <http://trac.webkit.org/changeset/206511> |