<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body><span class="vcard"><a class="email" href="mailto:david_quesada@apple.com" title="David Quesada <david_quesada@apple.com>"> <span class="fn">David Quesada</span></a>
</span> changed
          <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>
          <br>
             <table border="1" cellspacing="0" cellpadding="8">
          <tr>
            <th>What</th>
            <th>Removed</th>
            <th>Added</th>
          </tr>

         <tr>
           <td style="text-align:right;">CC</td>
           <td>
                
           </td>
           <td>david_quesada@apple.com
           </td>
         </tr></table>
      <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#c5">Comment # 5</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:david_quesada@apple.com" title="David Quesada <david_quesada@apple.com>"> <span class="fn">David Quesada</span></a>
</span></b>
        <pre>Comment on <span class=""><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>

<span class="quote">> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:45
> +    enum class Purpose {</span >

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 class="quote">> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:51
> +    struct ApplicationManifestIcon {</span >

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 class="quote">> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:67
>      template<class Encoder> void encode(Encoder&) const;</span >

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

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 class="quote">> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198
> +    for (auto& iconValue : *arrayValue) {</span >

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

<span class="quote">> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:216
> +                    logManifestPropertyInvalidURL("scope"_s);</span >

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

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

Can `manifest` be made const?

<span class="quote">>> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.h:57
>> +    String parseGenericStringInIcon(RefPtr<WTF::JSONImpl::Object> &, const String&);

> No space before the ampersand.</span >

Why does this method take a `WTF::JSONImpl::Object` while other methods use `JSON::Object` ?</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>