Bug 231916

Summary:

REGRESSION (r284336): system-preview/badge.html is image failing - clip-path on inlines

Product:

WebKit

Reporter:

ayumi_kojima

Component:

Layout and Rendering

Assignee:

Simon Fraser (smfr) <simon.fraser>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, changseok, esprehn+autocc, ews-watchlist, fred.wang, glenn, heycam, jonlee, koivisto, kondapallykalyan, pdr, simon.fraser, thorton, webkit-bot-watchers-bugzilla, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

iPhone / iPad

OS:

Unspecified

See Also:

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

Attachments:

Description	Flags
Image diff	none
Patch	koivisto: review+

Description ayumi_kojima 2021-10-18 15:59:14 PDT

system-preview/badge.html

Is constantly failing on iOS 15.

History: https://results.webkit.org/?suite=layout-tests&test=system-preview%2Fbadge.html 

Result page: https://build.webkit.org/results/Apple-iOS-15-Simulator-Release-WK2-Tests/r284384%20(215)/results.html#

Comment 1 ayumi_kojima 2021-10-18 15:59:42 PDT

Created attachment 441652 [details]
Image diff

Comment 2 Radar WebKit Bug Importer 2021-10-18 16:00:07 PDT

<rdar://problem/84391702>

Comment 3 ayumi_kojima 2021-10-18 16:01:15 PDT

According to the history, it looks like it started from the changes in https://trac.webkit.org/changeset/284336/webkit

Comment 4 Simon Fraser (smfr) 2021-10-18 20:32:42 PDT

Created attachment 441684 [details]
Patch

Comment 5 Cameron McCormack (:heycam) 2021-10-18 20:50:23 PDT

Comment on attachment 441684 [details]
Patch

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

Non-reviewer r=me with the restore() question answered. I didn't look at the tests closely.

> Source/WebCore/rendering/RenderLayer.cpp:3160
> +        return std::make_pair(clipPath.pathForReferenceRect(snappedReferenceBox), clipPath.windRule());

Nit: I think it might be more common in the codebase to use `return {..., ...};` rather than `return std::make_pair(..., ...);`.

> Source/WebCore/rendering/RenderLayer.cpp:3169
> +        return std::make_pair(clipPath.pathForReferenceRect(shapeRect), WindRule::NonZero);

Nit: And here.

> Source/WebCore/rendering/RenderLayer.cpp:3172
> +    return std::make_pair(Path(), WindRule::NonZero);

Nit: And here.

> Source/WebCore/rendering/RenderLayer.cpp:3209
> +            context.save();

Is there a restore() missing?

Comment 6 Simon Fraser (smfr) 2021-10-18 20:52:43 PDT

(In reply to Cameron McCormack (:heycam) from comment #5)

> Is there a restore() missing?

It's elsewhere (confusingly). Ideally we'd pass a GraphicsContextStateSaver thingy.

Comment 7 Antti Koivisto 2021-10-18 22:11:07 PDT

Comment on attachment 441684 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:3160
>> +        return std::make_pair(clipPath.pathForReferenceRect(snappedReferenceBox), clipPath.windRule());
> 
> Nit: I think it might be more common in the codebase to use `return {..., ...};` rather than `return std::make_pair(..., ...);`.

Even where explicit type is needed std::make_pair is unnecessary as the std::pair constructor can infer the type (since C++17).

Comment 8 Antti Koivisto 2021-10-18 22:13:12 PDT

(In reply to Simon Fraser (smfr) from comment #6)
> (In reply to Cameron McCormack (:heycam) from comment #5)
> 
> > Is there a restore() missing?
> 
> It's elsewhere (confusingly). Ideally we'd pass a GraphicsContextStateSaver
> thingy.

You really should clean this up.

Comment 9 Simon Fraser (smfr) 2021-10-19 13:45:27 PDT

https://trac.webkit.org/changeset/284490/webkit

Will do the GraphicsContxtStateSaver thing separately.

Comment 10 Simon Fraser (smfr) 2021-10-19 14:48:06 PDT

Doing so via bug 231985.

Comment 11 Jon Lee 2022-04-01 15:48:05 PDT

*** Bug 236922 has been marked as a duplicate of this bug. ***