Bug 175376

Summary: [CoordGraphics] Track dirty rects that need update in CoordinatedGraphicsLayer
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, cmarcelo, ews-watchlist, kondapallykalyan, luiz, noam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
Patch
none
Patch none

Description Zan Dobersek 2017-08-09 04:18:16 PDT
[CoordGraphics] Track dirty rects that need update in CoordinatedGraphicsLayer
Comment 1 Zan Dobersek 2017-08-09 04:18:31 PDT
Created attachment 317694 [details]
WIP
Comment 2 Zan Dobersek 2018-03-20 09:56:36 PDT
Created attachment 336128 [details]
Patch
Comment 3 EWS Watchlist 2018-03-20 09:59:17 PDT
Attachment 336128 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:561:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Carlos Garcia Campos 2018-03-21 02:04:39 PDT
Comment on attachment 336128 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:220
> +        bool fullRect { false };

I find this name a bit confusing, I've had to read the patch several times to understand it. I guess it means the full rect is dirty, so we do nothing when it's false and dirty rects is empty right? I think that instead of using a boolean for this we could make the dirty rects vector std::optional. What do you think?
Comment 5 Zan Dobersek 2018-03-21 03:22:31 PDT
Comment on attachment 336128 [details]
Patch

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

>> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:220
>> +        bool fullRect { false };
> 
> I find this name a bit confusing, I've had to read the patch several times to understand it. I guess it means the full rect is dirty, so we do nothing when it's false and dirty rects is empty right? I think that instead of using a boolean for this we could make the dirty rects vector std::optional. What do you think?

In addition to doing nothing when this is false and no other dirty rect was registered, in setNeedsDisplay() this is set to true, and any recorded rect is dropped, with the intention of redrawing the whole layer during the flush. After this is set to true, any other call to setNeedsDisplay() or setNeedsDisplayInRect() is ignored, because we don't have to bother recording anything anymore if we know the layer will be redrawn in full.

Corresponding to that logic, during flush the whole backing store is invalidated if fullRect is true, otherwise we do invalidation rect-by-rect.
Comment 6 Zan Dobersek 2018-03-21 03:25:41 PDT
Comment on attachment 336128 [details]
Patch

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

>>> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:220
>>> +        bool fullRect { false };
>> 
>> I find this name a bit confusing, I've had to read the patch several times to understand it. I guess it means the full rect is dirty, so we do nothing when it's false and dirty rects is empty right? I think that instead of using a boolean for this we could make the dirty rects vector std::optional. What do you think?
> 
> In addition to doing nothing when this is false and no other dirty rect was registered, in setNeedsDisplay() this is set to true, and any recorded rect is dropped, with the intention of redrawing the whole layer during the flush. After this is set to true, any other call to setNeedsDisplay() or setNeedsDisplayInRect() is ignored, because we don't have to bother recording anything anymore if we know the layer will be redrawn in full.
> 
> Corresponding to that logic, during flush the whole backing store is invalidated if fullRect is true, otherwise we do invalidation rect-by-rect.

I can find a better name though.
Comment 7 Zan Dobersek 2018-03-21 06:13:00 PDT
Created attachment 336187 [details]
Patch
Comment 8 EWS Watchlist 2018-03-21 06:14:11 PDT
Attachment 336187 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:561:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Zan Dobersek 2018-03-21 07:16:24 PDT
Comment on attachment 336187 [details]
Patch

Clearing flags on attachment: 336187

Committed r229804: <https://trac.webkit.org/changeset/229804>
Comment 10 Zan Dobersek 2018-03-21 07:16:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-03-21 07:17:16 PDT
<rdar://problem/38706952>