[Webkit-unassigned] [Bug 231339] Add support for icons in web app manifest

[bugzilla-daemon at webkit.org]

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

--- Comment #15 from rginsberg at apple.com ---
Comment on attachment 443585
  --> https://bugs.webkit.org/attachment.cgi?id=443585
Patch

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

>> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:46
>> +        enum class Purpose {
> 
> NIT: `enum class Purpose : uint8_t`

Added this

>> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:49
>> +            Any,
> 
> It seems like below this is the default value, so maybe this should be first in the list?

Any is the default, moved to the top of the list

>> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:55
>> +        Vector<Purpose> purposes;
> 
> Can we make `Purpose` into a set of flags (e.g. `1 << 0`, `1 << 1`, etc.) so that we can use `OptionSet` instead?

Added flags and changed `Vector<Purpose>` to `OptionSet<Purpose>`

>> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:246
>> +                        continue;
> 
> having the `continue` here means that the following line wont run, which I don't think you want/intend
> 
> in fact, you don't really `need` the `continue` at all since this is the end of the `if` chain and there's no other logic below this anyways

Made all the changes recommended in the function.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211109/8ccaded1/attachment.htm>