[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

Devin Rousso <drousso at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |drousso at apple.com

--- Comment #13 from Devin Rousso <drousso 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`

> 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?

> 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?

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:180
> +    auto value = manifest.getValue("icons");

Can we name this something more descriptive?  Maybe `manifestIcons`?  You could then have `manifestIconsArray` down below :)

NIT: `"icons"_s`
NIT: I'd also move this right above `if (!value)`

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:186
> +    auto arrayValue = value->asArray();

NIT: I'd move this one line below so it's right above `if (!arrayValue)

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:195
> +        auto autoObjectValue = iconValue->asObject();

`iconObject`?

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198
> +        const JSON::Object *objectValue = autoObjectValue.get();

I think you should `const auto& iconJSON = *iconObject;` so that you don't have to keep dereferencing below

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:238
> +                

Style: unnecessary newline

> 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

-- 
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/cd9c46f8/attachment.htm>