<html>
    <head>
      <base href="https://bugs.webkit.org/" />
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Update empty image canvas tests and fix a related bug"
   href="https://bugs.webkit.org/show_bug.cgi?id=142694#c16">Comment # 16</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Update empty image canvas tests and fix a related bug"
   href="https://bugs.webkit.org/show_bug.cgi?id=142694">bug 142694</a>
              from <span class="vcard"><a class="email" href="mailto:yoav@yoav.ws" title="Yoav Weiss <yoav@yoav.ws>"> <span class="fn">Yoav Weiss</span></a>
</span></b>
        <pre>Comment on <span class="bz_obsolete"><a href="attachment.cgi?id=248686&action=diff" name="attach_248686" title="Patch">attachment 248686</a> <a href="attachment.cgi?id=248686&action=edit" title="Patch">[details]</a></span>
Patch

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

<span class="quote">>> Source/WebCore/ChangeLog:10
>> +        these tests and fixes a canvas bug the new tests exposed.

> Please explain in detail which bug you fixed and how you changed Web-Exposed behavior. Pointing to a spec and comparing the new behavior to other browsers would also be helpful.</span >

Explained

<span class="quote">>> Source/WebCore/ChangeLog:11
>> +

> Please indicate in the ChangeLog where you got the new tests from. You can also include the W3C bug URL that explains which tests were replaced by which.</span >

Added that

<span class="quote">>> Source/WebCore/html/HTMLImageElement.h:81
>> +    bool unavailable() const;

> Ideally, as per coding style, this would be called isUnavaiable(). I would also rename complete() to isComplete() for consistency.</span >

Since I dropped unavailable(), leaving complete() alone as well. Let me know if you think I should change it (possibly as a separate patch).

<span class="quote">>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1754
>> +    if (image->unavailable())

> I don't see any mention of the unavailable state at <a href="https://html.spec.whatwg.org/#check-the-usability-of-the-image-argument">https://html.spec.whatwg.org/#check-the-usability-of-the-image-argument</a>
> Could you clarify why this is the right thing to do here?</span >

I no longer think it is. Changed the patch accordingly.

<span class="quote">>> Source/WebCore/loader/ImageLoader.cpp:155
>> +        m_imageUnavailable = false;

> The spec says we are supposed to move out of "unavailable" state as soon as we have enough data to determine the image's dimensions (<a href="https://html.spec.whatwg.org/#img-none">https://html.spec.whatwg.org/#img-none</a>). Aren't we moving out of unavailable state too late here?</span >

That's correct. I've dropped the "unavailable" approach in favor of a simpler "is CachedImage created?" one.

<span class="quote">>> Source/WebCore/loader/ImageLoader.cpp:284
>> +    m_imageUnavailable = false;

> We should have moved out of "unavailable" state as soon as we got enough data to know the image dimensions.</span >

True. I've taken a different approach since tracking "unavailable" seems like an overkill for the problem I'm trying to solve (canvas pattern with an empty image).
Therefore, I've dropped "unavailable" tracking in favor of a simple check if CachedImage exists at all.

<span class="quote">>> Source/WebCore/loader/ImageLoader.h:107
>> +    unsigned m_elementIsProtected : 1;

> Please don’t make this change. We fixed the style checking script to no longer complain about this, and there is no downside to the old way.</span >

I've reverted this change (which I only did because the style script told me to).

<span class="quote">>> LayoutTests/ChangeLog:25
>> +        This test currently fails and is fixed in <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Stop image from displaying when src attribute is removed or emptied"
   href="show_bug.cgi?id=142677">https://bugs.webkit.org/show_bug.cgi?id=142677</a>

> "and will be fixed in..."</span >

changed

<span class="quote">>> LayoutTests/TestExpectations:501
>> +webkit.org/b/142677 canvas/philip/tests/2d.pattern.image.incomplete.removedsrc.html [ Pass Failure ]

> Why [ Pass, Failure] ? This means flaky. Either commit the incorrect expectations (I think this would be OK since you are fixing the test in a follow-up patch) or commit the correct expectations and mark the test as [ Failure ] (ideal).</span >

You're right. replaced it with Failure along with the correct expected result.

<span class="quote">>> LayoutTests/canvas/philip/tests/2d.pattern.image.incomplete.removedsrc-expected.txt:8
>> +file:///Users/yweiss/OS/WebKit/LayoutTests/canvas/philip/tests/2d.pattern.image.incomplete.removedsrc.html:24:30

> These absolute paths are going to be an issue. Either commit the "correct" expected result or tweak the test so that it does not print absolute paths.</span >

I've changed it to be the correct result and change expectations accordingly.</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>