| Summary: | Use SetForScope to temporarily change members of TextureMapperPaintOptions instead of copying all members | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||
| Component: | Platform | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cgarcia, cmarcelo, don.olmstead, ews-watchlist, kondapallykalyan, luiz, magomez, webkit-bug-importer, zan | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Fujii Hironori
2020-11-16 22:24:36 PST
Created attachment 414309 [details]
Patch
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 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 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 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. Created attachment 414371 [details]
Patch for landing
Comment on attachment 414371 [details] Patch for landing Clearing flags on attachment: 414371 Committed r269936: <https://trac.webkit.org/changeset/269936> All reviewed patches have been landed. Closing bug. |