Bug 142694 – Update empty image canvas tests and fix a related bug

• Component: New Bugs
• Status: RESOLVED FIXED

[Yoav Weiss]

Update some canvas tests and fix a related bug with unavailable images

[Yoav Weiss]

Created attachment 248652 [details]
Patch

[Yoav Weiss]

Created attachment 248677 [details]
Patch

[Build Bot]

Comment on attachment 248677 [details]
Patch

Attachment 248677 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5690841523814400

New failing tests:
canvas/philip/tests/2d.pattern.image.incomplete.removedsrc.html
fast/canvas/canvas-empty-image-pattern.html

[Build Bot]

Created attachment 248678 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 248677 [details]
Patch

Attachment 248677 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6633007024701440

New failing tests:
canvas/philip/tests/2d.pattern.image.incomplete.removedsrc.html
fast/canvas/canvas-empty-image-pattern.html

[Build Bot]

Created attachment 248679 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Yoav Weiss]

Created attachment 248681 [details]
Patch

[Build Bot]

Comment on attachment 248681 [details]
Patch

Attachment 248681 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6585561594724352

New failing tests:
fast/canvas/canvas-empty-image-pattern.html

[Build Bot]

Created attachment 248682 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 248681 [details]
Patch

Attachment 248681 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5222925238009856

New failing tests:
fast/canvas/canvas-empty-image-pattern.html

[Build Bot]

Created attachment 248683 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Yoav Weiss]

Created attachment 248686 [details]
Patch

[Chris Dumez]

Comment on attachment 248686 [details]
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.

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

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

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

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

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

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

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

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

[Darin Adler]

Comment on attachment 248686 [details]
Patch

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

> Source/WebCore/loader/ImageLoader.h:107
> -    bool m_hasPendingBeforeLoadEvent : 1;
> -    bool m_hasPendingLoadEvent : 1;
> -    bool m_hasPendingErrorEvent : 1;
> -    bool m_imageComplete : 1;
> -    bool m_loadManually : 1;
> -    bool m_elementIsProtected : 1;
> +    unsigned m_hasPendingBeforeLoadEvent : 1;
> +    unsigned m_hasPendingLoadEvent : 1;
> +    unsigned m_hasPendingErrorEvent : 1;
> +    unsigned m_imageComplete : 1;
> +    unsigned m_imageUnavailable : 1;
> +    unsigned m_loadManually : 1;
> +    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.

[Yoav Weiss]

Created attachment 249233 [details]
Patch

[Yoav Weiss]

Comment on attachment 248686 [details]
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.

[Chris Dumez]

Comment on attachment 249233 [details]
Patch

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

r=me with comments.

> Source/WebCore/ChangeLog:28
> +        (WebCore::ImageLoader::updateFromElement): Fixed casting from CachedResourceHandle<CachedImage> to bool.

Why? CachedResourceHandle has an operator for converting to bool already.

> Source/WebCore/ChangeLog:48
> +        (WebCore::ImageLoader::clearImage):

Unrelated change.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1756
> +    if (!image->complete() || !cachedImage)

nit: I would find it more logical to reverse the checks to first check that the load has started THEN check that the load is not complete. Please also update the comment above accordingly.

> Source/WebCore/loader/ImageLoader.cpp:238
> +        m_hasPendingLoadEvent = !!newImage;

Unrelated change?

[Yoav Weiss]

Created attachment 249273 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 249273 [details]
Patch

Clearing flags on attachment: 249273

Committed r181888: <http://trac.webkit.org/changeset/181888>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.