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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 18 12:52:25 PDT 2021


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

--- Comment #8 from rginsberg at apple.com ---
(In reply to David Quesada from comment #5)
> Comment on attachment 441423 [details]
> 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" ?

Just changed "Purpose" to "IconPurpose".
> 
> > 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.
Just changed to "ApplicationManifest::Icon"
> 
> > 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.
> 
I just added the `icons` to ApplicationManifest::encode() and 
ApplicationManifest::Decode.
I also added ApplicationManifest::Icon:encode() and 
ApplicationManifest::Icon::decode() because they were necessary for the 
existing functions.
> >> 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
Just changed this.
> 
> 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.)

Also swapped the phrasing out for "array". I did take "list" from the spec originally,
but since array is used more with JSON terminology this change makes sense.
> 
> > 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.
Just changed this.
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:216
> > +                    logManifestPropertyInvalidURL("scope"_s);
> 
> Should this be "src" rather than "scope"?
Yes thank you that was a mistake.
> 
> >> 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` ?
By casting the result of `iconValue->asObject()` in `parseIcons` to 
`const JSON::Object *` I was able to get rid of the `parseGenericStringInIcon` 
function and just call `parseGenericString`.

-- 
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/20211018/8359abdc/attachment.htm>


More information about the webkit-unassigned mailing list