Bug 219022 – Use SetForScope to temporarily change members of TextureMapperPaintOptions instead of copying all members

• Component: Platform
• Status: RESOLVED FIXED

[Fujii Hironori]

Use SetForScope to temporarily change members of TextureMapperPaintOptions instead of copying all memebers

[Fujii Hironori]

Created attachment 414309 [details]
Patch

[Carlos Garcia Campos]

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

[Zan Dobersek]

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.

[Zan Dobersek]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 414371 [details]
Patch for landing

[Fujii Hironori]

Comment on attachment 414371 [details]
Patch for landing

Clearing flags on attachment: 414371

Committed r269936: <https://trac.webkit.org/changeset/269936>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/71512935>