<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add support for icons in web app manifest"
   href="https://bugs.webkit.org/show_bug.cgi?id=231339#c8">Comment # 8</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add support for icons in web app manifest"
   href="https://bugs.webkit.org/show_bug.cgi?id=231339">bug 231339</a>
              from <span class="vcard"><a class="email" href="mailto:rginsberg@apple.com" title="rginsberg@apple.com">rginsberg@apple.com</a>
</span></b>
        <pre>(In reply to David Quesada from <a href="show_bug.cgi?id=231339#c5">comment #5</a>)
<span class="quote">> Comment on <span class="bz_obsolete"><a href="attachment.cgi?id=441423&action=diff" name="attach_441423" title="Patch for review">attachment 441423</a> <a href="attachment.cgi?id=441423&action=edit" title="Patch for review">[details]</a></span>
> Patch for review

> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=441423&action=review">https://bugs.webkit.org/attachment.cgi?id=441423&action=review</a>

> > 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" ?</span >

Just changed "Purpose" to "IconPurpose".
<span class="quote">> 
> > 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.</span >
Just changed to "ApplicationManifest::Icon"
<span class="quote">> 
> > 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.
> </span >
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.
<span class="quote">> >> 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</span >
Just changed this.
<span class="quote">> 
> 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.)</span >

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.
<span class="quote">> 
> > 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.</span >
Just changed this.
<span class="quote">> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:216
> > +                    logManifestPropertyInvalidURL("scope"_s);

> Should this be "src" rather than "scope"?</span >
Yes thank you that was a mistake.
<span class="quote">> 
> >> 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` ?</span >
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`.</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>