[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

David Quesada <david_quesada at apple.com> changed:

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

--- Comment #5 from David Quesada <david_quesada at apple.com> ---
Comment on attachment 441423
  --> https://bugs.webkit.org/attachment.cgi?id=441423
Patch for review

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

> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:45
> +    enum class Purpose {

The "Purpose" isn't really a property of the manifest itself, so I don't think "ApplicationManifest::Purpose" is the clearest name. What about "ApplicationManifest::IconPurpose" or "ApplicationManifest::Icon::Purpose" ?

> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:51
> +    struct ApplicationManifestIcon {

Since this is embedded *within* ApplicationManifest, I think just calling it "ApplicationManifest::Icon" might be a better name. The full name of "ApplicationManifest::ApplicationManifestIcon" is somewhat redundant.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:67
>      template<class Encoder> void encode(Encoder&) const;

Is there a reason to not also update ApplicationManifest::encode() and ApplicationManifest::decode() in this patch? Those methods will need to learn about `icons` in order to send the icon information from the web process to the UI process.

>> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:194
>> +        logDeveloperWarning(makeString("The value of icons is not a valid list."_s));
> 
> I think you can just pass "The value of icons is not a valid list."_s

I think calling it an "array" rather than "list" might be more accurate with respect to JSON terminology. (I did just check the spec and notice it uses "list" in this part of the algorithm, but maybe "array" is more familiar term to developers who might see this message.)

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198
> +    for (auto& iconValue : *arrayValue) {

Nit: I think `const auto&` might be preferred here since you're not mutating the icons.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:216
> +                    logManifestPropertyInvalidURL("scope"_s);

Should this be "src" rather than "scope"?

>> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:341
>> +String ApplicationManifestParser::parseGenericStringInIcon(RefPtr<WTF::JSONImpl::Object> &manifest, const String& propertyName)
> 
> In C++, space should be after the ampersand

Can `manifest` be made const?

>> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.h:57
>> +    String parseGenericStringInIcon(RefPtr<WTF::JSONImpl::Object> &, const String&);
> 
> No space before the ampersand.

Why does this method take a `WTF::JSONImpl::Object` while other methods use `JSON::Object` ?

-- 
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/20211015/c7a283ea/attachment-0001.htm>