[Webkit-unassigned] [Bug 162608] [GTK] Update WOFF2 decoder

[bugzilla-daemon at webkit.org]

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

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #289962|review?                     |review+
              Flags|                            |

--- Comment #7 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 289962
  --> https://bugs.webkit.org/attachment.cgi?id=289962
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160928/dba1f569/attachment.html>