Bug 124999 - [Texmap] TextureMapperLayer unnecessary object copying
Summary: [Texmap] TextureMapperLayer unnecessary object copying
Status: RESOLVED DUPLICATE of bug 137271
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-28 23:05 PST by Przemyslaw Szymanski
Modified: 2022-02-27 23:32 PST (History)
6 users (show)

See Also:

Attachments
object copying (2.72 KB, patch)
2013-11-29 00:58 PST, Przemyslaw Szymanski 		no flags Details | Formatted Diff | Diff
patch (2.41 KB, patch)
2013-11-29 01:50 PST, Przemyslaw Szymanski 		bfulgham: review-
Details | Formatted Diff | Diff
patch (2.53 KB, patch)
2014-02-20 02:37 PST, Przemyslaw Szymanski 		bfulgham: review-
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Przemyslaw Szymanski 2013-11-28 23:05:47 PST 
In C++ it is not a good practice to make copy of objects. Mostly because object copying is not cheap operation for CPU especially if an object is large.
Comment 1 Przemyslaw Szymanski 2013-11-29 00:58:25 PST 
Created attachment 218039 [details]
object copying
Comment 2 Przemyslaw Szymanski 2013-11-29 01:47:40 PST 
Comment on attachment 218039 [details]
object copying

wrong attachment format
Comment 3 Przemyslaw Szymanski 2013-11-29 01:50:05 PST 
Created attachment 218041 [details]
patch
Comment 4 Brent Fulgham 2014-02-19 12:25:18 PST 
Comment on attachment 218041 [details]
patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:348
>      for (size_t i = 0; i < rects.size(); ++i) {

Please make this a C++11 loop.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:369
> +        const IntRect& rect = rects[i];

Ditto.
Comment 5 Przemyslaw Szymanski 2014-02-20 02:37:31 PST 
Created attachment 224737 [details]
patch
Comment 6 Przemyslaw Szymanski 2014-02-20 02:41:13 PST 
(In reply to comment #4)
> (From update of attachment 218041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218041&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:348
> >      for (size_t i = 0; i < rects.size(); ++i) {
> 
> Please make this a C++11 loop.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:369
> > +        const IntRect& rect = rects[i];
> 
> Ditto.

Thank you for review. Patch is updated. I hope you thought about range-for not std::foreach
Comment 7 Brent Fulgham 2014-04-16 14:09:15 PDT 
Comment on attachment 224737 [details]
patch

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

Looks very go! Pleaseo switch to 'auto&', but otherwise this looks good.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:348
> +    for (IntRect& rect : rects) {

Please make this for (auto& rect : rects) {

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:367
> +    for (IntRect& rect : rects) {

Ditto.
Comment 8 Fujii Hironori 2020-11-08 18:02:53 PST 
r174168 fixed this.

*** This bug has been marked as a duplicate of bug 137271 ***