Bug 175376 - [CoordGraphics] Track dirty rects that need update in CoordinatedGraphicsLayer
Summary: [CoordGraphics] Track dirty rects that need update in CoordinatedGraphicsLayer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-09 04:18 PDT by Zan Dobersek
Modified: 2018-03-21 07:17 PDT (History)
7 users (show)

See Also:


Attachments
WIP (4.70 KB, patch)
2017-08-09 04:18 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (5.71 KB, patch)
2018-03-20 09:56 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (5.73 KB, patch)
2018-03-21 06:13 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>