[Webkit-unassigned] [Bug 142694] Update empty image canvas tests and fix a related bug

[bugzilla-daemon at webkit.org]

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

--- Comment #16 from Yoav Weiss <yoav at yoav.ws> ---
Comment on attachment 248686
  --> https://bugs.webkit.org/attachment.cgi?id=248686
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248686&action=review

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

Explained

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

Added that

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

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1754
>> +    if (image->unavailable())
> 
> I don't see any mention of the unavailable state at https://html.spec.whatwg.org/#check-the-usability-of-the-image-argument
> Could you clarify why this is the right thing to do here?

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

>> 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 (https://html.spec.whatwg.org/#img-none). Aren't we moving out of unavailable state too late here?

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

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

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.

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

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

>> LayoutTests/ChangeLog:25
>> +        This test currently fails and is fixed in https://bugs.webkit.org/show_bug.cgi?id=142677
> 
> "and will be fixed in..."

changed

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

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

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

I've changed it to be the correct result and change expectations accordingly.

-- 
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/20150323/00c63ab4/attachment-0002.html>