[webkit-reviews] review denied: [Bug 71872] [Chromium] Improve tile invalidation : [Attachment 116893] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied David Reveman
<reveman at chromium.org>'s request for review:
Bug 71872: [Chromium] Improve tile invalidation
https://bugs.webkit.org/show_bug.cgi?id=71872

Attachment 116893: Patch
https://bugs.webkit.org/attachment.cgi?id=116893&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116893&action=review


Thanks for updating this. This looks mostly good but still has a few things to
address before landing, IMO

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:115
> +void ContentLayerChromium::setNeedsDisplay(const FloatRect& dirtyRect)

design question: is it right that this logic lives down on ContentLayerChromium
instead of TiledLayerChromium? What about this logic is specific to content
layers instead of other tiled layer types?

in practice today i don't think it makes much of a difference because the only
other tiled layer subtype we have is ImageLayerChromium, which never does
partial invalidations, but if somebody were to need to bugfix setNeedsDisplay()
(which i will be doing soon) i think it makes more sense for all of this logic
to exist on TiledLayerChromium instead of being pushed to the subclass

> Source/WebKit/chromium/public/WebContentLayer.h:64
> +    // Returns whether the layer is currently invalid.
> +    WEBKIT_EXPORT bool isInvalid() const;

well this comment doesn't add very much

come to think of it, though, i'm not really sure what the API is actually
saying - what information does this communicate to the client of this API that
they wouldn't know?  i can't find any user of invalidRect() in the aura
codebase, nor would i. there are some cases where this API will be downright
weird. example:

* client calls invalidateRect() over some large region
 - isInvalid() would return true
* compositor does an update/paint cycle that only covers some tiles of the
invalidated region
 - paint() call made on the WebContentLayerClient
 - isInvalid() would return false
* the visible rect changes
* compositor does another update/paint cycle
 - paint() call is made on the WebContentLayerClient
 - isInvalid() still returns false

so isInvalid() is not a useful predictor for whether a paint call will be made,
or really anything else except for whether the client has made an invalidate*()
call and there hasn't been an update/paint cycle.

my vote is to remove it. alternately, describe what it does in some way that
would make sense for a client of this API that isn't familiar with all of the
internals of how the compositor works and come up with some way in which the
API is useful

sorry to rant a bit but i want us to get into the habit of being very careful
and judicious with our public APIs