Bug 219022 - Use SetForScope to temporarily change members of TextureMapperPaintOptions instead of copying all members
Summary: Use SetForScope to temporarily change members of TextureMapperPaintOptions in...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-16 22:24 PST by Fujii Hironori
Modified: 2020-11-17 16:51 PST (History)
9 users (show)

See Also:


Attachments
Patch (9.51 KB, patch)
2020-11-16 22:29 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch for landing (9.51 KB, patch)
2020-11-17 12:38 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2020-11-16 22:24:36 PST
Use SetForScope to temporarily change members of TextureMapperPaintOptions instead of copying all memebers
Comment 1 Fujii Hironori 2020-11-16 22:29:53 PST
Created attachment 414309 [details]
Patch
Comment 2 Carlos Garcia Campos 2020-11-17 00:47:17 PST
Comment on attachment 414309 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        just to change some memebers. Use WTF::SetForScope to temporarily

members
Comment 3 Zan Dobersek 2020-11-17 01:29:28 PST
Comment on attachment 414309 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:473
> +        surface = options.surface;

Is this intended? It deviates from the previous behavior.
Comment 4 Zan Dobersek 2020-11-17 01:37:23 PST
Comment on attachment 414309 [details]
Patch

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

>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:473
>> +        surface = options.surface;
> 
> Is this intended? It deviates from the previous behavior.

Even then, it shouldn't matter, it maybe changes behavior but it doesn't really have a different outcome right now.

But switching to using SetForScope instead of copying over the Options struct where necessary, subsequently removing the const qualifier from the Options reference as it's passed across the paint methods, sets out pitfalls for the future, where someone can modify the Options struct without properly scoping that change. I don't think those pitfalls outweigh the microbenefits you get from avoiding these copies, but I don't have a strong opinion on what's done here.
Comment 5 Fujii Hironori 2020-11-17 12:06:05 PST
Comment on attachment 414309 [details]
Patch

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

Thank you for the review.

>> Source/WebCore/ChangeLog:9
>> +        just to change some memebers. Use WTF::SetForScope to temporarily
> 
> members

Will fix.

>>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:473
>>> +        surface = options.surface;
>> 
>> Is this intended? It deviates from the previous behavior.
> 
> Even then, it shouldn't matter, it maybe changes behavior but it doesn't really have a different outcome right now.
> 
> But switching to using SetForScope instead of copying over the Options struct where necessary, subsequently removing the const qualifier from the Options reference as it's passed across the paint methods, sets out pitfalls for the future, where someone can modify the Options struct without properly scoping that change. I don't think those pitfalls outweigh the microbenefits you get from avoiding these copies, but I don't have a strong opinion on what's done here.

I didn't change the behavior. I admit paintIntoSurface looks
tricky code. I will consider how can I improve the code in a
follow-up bug ticket.

I'm going to add a new member to TextureMapperPaintOptions for
perserve-3d in Bug 218969. But, I don't want to copy all members
just to change a single member.
Comment 6 Fujii Hironori 2020-11-17 12:38:55 PST
Created attachment 414371 [details]
Patch for landing
Comment 7 Fujii Hironori 2020-11-17 16:50:20 PST
Comment on attachment 414371 [details]
Patch for landing

Clearing flags on attachment: 414371

Committed r269936: <https://trac.webkit.org/changeset/269936>
Comment 8 Fujii Hironori 2020-11-17 16:50:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2020-11-17 16:51:19 PST
<rdar://problem/71512935>