[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