Bug 71872 - [Chromium] Improve tile invalidation
Summary: [Chromium] Improve tile invalidation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Reveman
URL:
Keywords:
Depends on:
Blocks: 72223 73525
  Show dependency treegraph
 
Reported: 2011-11-08 18:25 PST by David Reveman
Modified: 2011-12-06 09:14 PST (History)
11 users (show)

See Also:


Attachments
Patch (4.84 KB, patch)
2011-11-08 18:26 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (13.65 KB, patch)
2011-11-09 15:37 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (13.66 KB, patch)
2011-11-09 16:14 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (20.65 KB, patch)
2011-11-09 17:02 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (22.04 KB, patch)
2011-11-11 19:20 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (22.62 KB, patch)
2011-11-18 16:01 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (18.38 KB, patch)
2011-11-20 16:45 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (18.39 KB, patch)
2011-11-21 12:21 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (19.22 KB, patch)
2011-11-22 12:39 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (23.09 KB, patch)
2011-11-28 22:08 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (22.91 KB, patch)
2011-11-29 10:10 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (22.92 KB, patch)
2011-11-29 11:32 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (23.72 KB, patch)
2011-11-29 15:13 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (23.74 KB, patch)
2011-11-30 21:19 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (26.49 KB, patch)
2011-11-30 21:45 PST, David Reveman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Reveman 2011-11-08 18:25:11 PST
LayerChromium::setNeedsDisplay currently does a union of all dirty rectangles which is later used by ContentLayerChromium::paintContentsIfDirty to invalidate tiles. TiledLayerChromium should be able to invalidate tiles using the initial dirty rectangles instead of the union to avoid unnecessary tile updates.
Comment 1 David Reveman 2011-11-08 18:26:54 PST
Created attachment 114189 [details]
Patch
Comment 2 David Reveman 2011-11-08 18:30:55 PST
Not sure this is the best way to solve this but something like this should significantly improve the efficiency of the SkPicture based texture uploader.
Comment 3 Adrienne Walker 2011-11-09 12:05:19 PST
Comment on attachment 114189 [details]
Patch

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

Too much unioning.  This is great.  :)

My only (very minor) worry is that LayerChromium::dirtyRect() no longer returns something useful for ContentLayerChromium.  I'm not sure if that's getting used anywhere, but it's a little weird.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:-277
> -    m_tiler->growLayerToContain(contentRect);
> -

Is this going away because if the layer doesn't contain an invalidation rect, then those tiles don't exist, therefore don't need to be invalidated?
Comment 4 David Reveman 2011-11-09 13:26:02 PST
(In reply to comment #3)
> (From update of attachment 114189 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114189&action=review
> 
> Too much unioning.  This is great.  :)
> 
> My only (very minor) worry is that LayerChromium::dirtyRect() no longer returns something useful for ContentLayerChromium.  I'm not sure if that's getting used anywhere, but it's a little weird.

Yea, I agree. That's a little weird. m_dirtyRect is used by other layer types. We could call LayerChromium::setNeedsDisplay from ContentLayerChromium::setNeedsDisplay and keep having LayerChromium::dirtyRect() return something useful but it doesn't really make a difference in the ContentLayerChromium case.

> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:-277
> > -    m_tiler->growLayerToContain(contentRect);
> > -
> 
> Is this going away because if the layer doesn't contain an invalidation rect, then those tiles don't exist, therefore don't need to be invalidated?

We're only interested in invalidating tiles that already exist. Tiles that don't exist but will be needed for drawing will be invalidated when created. The rectangle passed to invalidateRect is no longer intersected by contentBounds so growing the layer here might make it larger than necessary. growLayerToContain is called later in TiledLayerChromium::prepareToUpdate with the visible layer area.
Comment 5 Adrienne Walker 2011-11-09 13:37:23 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 114189 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=114189&action=review
> > 
> > Too much unioning.  This is great.  :)
> > 
> > My only (very minor) worry is that LayerChromium::dirtyRect() no longer returns something useful for ContentLayerChromium.  I'm not sure if that's getting used anywhere, but it's a little weird.
> 
> Yea, I agree. That's a little weird. m_dirtyRect is used by other layer types. We could call LayerChromium::setNeedsDisplay from ContentLayerChromium::setNeedsDisplay and keep having LayerChromium::dirtyRect() return something useful but it doesn't really make a difference in the ContentLayerChromium case.

I really like that suggestion.  I totally agree that it doesn't make a difference in the ContentLayerChromium case, but it'll keep from causing somebody else unintended behavior later on if this rect starts getting used for something else.  I could easily see somebody adding an "isDirty() { return m_dirtyRect.isEmpty() }" function to LayerChromium for some other use.

> We're only interested in invalidating tiles that already exist. Tiles that don't exist but will be needed for drawing will be invalidated when created. The rectangle passed to invalidateRect is no longer intersected by contentBounds so growing the layer here might make it larger than necessary. growLayerToContain is called later in TiledLayerChromium::prepareToUpdate with the visible layer area.

Sounds good to me.  Just making sure I fully understand what's going on.
Comment 6 Shawn Singh 2011-11-09 14:21:17 PST
Going one small step further, I think its appropriate to simply remove the dirtyRect() accessor.  Like Enne said, I think its not used anywhere.  Conceptually, all layer types should be responsible for dealing with dirty rects in their own way.  (and in practice, it seems like tiled layers are the only case that needs to be addressed).  Does that sound reasonable?

Maybe we can eventually propagate this reduced repainting to the draw side, too (for now, its just one rect called the updateRect that propagates)
Comment 7 David Reveman 2011-11-09 15:37:21 PST
Created attachment 114380 [details]
Patch
Comment 8 WebKit Review Bot 2011-11-09 15:39:59 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 9 Adrienne Walker 2011-11-09 15:44:39 PST
Comment on attachment 114380 [details]
Patch

This looks great.  I think switching to just tracking a dirty flag cleans things up quite a bit.  Thanks!

Also, do the webkit_unit_tests build? I think LayerChromiumTest.cpp uses dirtyRect().
Comment 10 WebKit Review Bot 2011-11-09 15:51:06 PST
Comment on attachment 114380 [details]
Patch

Attachment 114380 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10395136
Comment 11 Darin Fisher (:fishd, Google) 2011-11-09 15:52:20 PST
Comment on attachment 114380 [details]
Patch

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

> Source/WebKit/chromium/public/WebContentLayer.h:64
> +    WEBKIT_EXPORT bool invalid() const;

isInvalid might be a better name for this function since it returns a boolean.  it is asking a question, so might as well phrase it like a question.
Comment 12 Shawn Singh 2011-11-09 16:04:31 PST
(In reply to comment #11)
> (From update of attachment 114380 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114380&action=review
> 
> > Source/WebKit/chromium/public/WebContentLayer.h:64
> > +    WEBKIT_EXPORT bool invalid() const;
> 
> isInvalid might be a better name for this function since it returns a boolean.  it is asking a question, so might as well phrase it like a question.

would it be OK if we do the same for the dirty() flag, too?   i.e. --> isDirty()?  But if anyone disagrees, I'll defer.
Comment 13 David Reveman 2011-11-09 16:06:07 PST
(In reply to comment #6)
> Going one small step further, I think its appropriate to simply remove the dirtyRect() accessor.  Like Enne said, I think its not used anywhere.  Conceptually, all layer types should be responsible for dealing with dirty rects in their own way.  (and in practice, it seems like tiled layers are the only case that needs to be addressed).  Does that sound reasonable?

Updated patch replaces m_dirtyRect with a boolean. I think this cleans things up a bit. As you mentioned, dirty region tracking is the responsibility of each layer type on its own using whatever data structure is appropriate. The base layer class can still keep track of whether the layer is dirty or not, which is all non-tiled layer types need.

> 
> Maybe we can eventually propagate this reduced repainting to the draw side, too (for now, its just one rect called the updateRect that propagates)

Yea, I think we should be able to remove updateRect and instead determine what needs to be re-drawn based on the tiles we're updating.
Comment 14 David Reveman 2011-11-09 16:14:46 PST
Created attachment 114388 [details]
Patch
Comment 15 WebKit Review Bot 2011-11-09 16:44:57 PST
Comment on attachment 114388 [details]
Patch

Attachment 114388 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10403117
Comment 16 Darin Fisher (:fishd, Google) 2011-11-09 16:56:53 PST
(In reply to comment #12)
> > isInvalid might be a better name for this function since it returns a boolean.  it is asking a question, so might as well phrase it like a question.
> 
> would it be OK if we do the same for the dirty() flag, too?   i.e. --> isDirty()?  But if anyone disagrees, I'll defer.

Yes.  The idea is to make code read more like spoken language.  You don't say "the layer dirty."  You say "the layer is dirty."
Comment 17 David Reveman 2011-11-09 17:02:35 PST
Created attachment 114397 [details]
Patch
Comment 18 David Reveman 2011-11-09 17:05:43 PST
(In reply to comment #9)
> (From update of attachment 114380 [details])
> This looks great.  I think switching to just tracking a dirty flag cleans things up quite a bit.  Thanks!
> 
> Also, do the webkit_unit_tests build? I think LayerChromiumTest.cpp uses dirtyRect().

Latest patch fixes the unit tests and makes sure empty rectangle invalidation behavior doesn't change.
Comment 19 WebKit Review Bot 2011-11-09 17:46:35 PST
Comment on attachment 114397 [details]
Patch

Attachment 114397 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10394164

New failing tests:
svg/dynamic-updates/SVGFEDisplacementMapElement-dom-in2-attr.html
Comment 20 James Robinson 2011-11-11 14:09:13 PST
Since it's a bool and it is set by calling setNeedsDisplay() and cleared by calling resetNeedsDisplay() shouldn't the getter be called needsDisplay() and the member variable called m_needsDisplay?
Comment 21 James Robinson 2011-11-11 14:10:24 PST
Also, why is it that WebContentLayer use invalidate / invalid but the implementation uses setNeedsDisplay(rect)?
Comment 22 David Reveman 2011-11-11 19:20:26 PST
Created attachment 114814 [details]
Patch
Comment 23 David Reveman 2011-11-11 19:27:53 PST
(In reply to comment #20)
> Since it's a bool and it is set by calling setNeedsDisplay() and cleared by calling resetNeedsDisplay() shouldn't the getter be called needsDisplay() and the member variable called m_needsDisplay?

Fixed in latest patch.

I didn't do anything about invalidate/invalid in WebContentLayer as I don't know the reason for that naming.
Comment 24 Darin Fisher (:fishd, Google) 2011-11-14 11:11:20 PST
(In reply to comment #21)
> Also, why is it that WebContentLayer use invalidate / invalid but the implementation uses setNeedsDisplay(rect)?

using consistent terminology for this stuff seems like a great idea to me! :)
Comment 25 Nat Duca 2011-11-14 11:13:02 PST
(In reply to comment #24)
> (In reply to comment #21)
> > Also, why is it that WebContentLayer use invalidate / invalid but the implementation uses setNeedsDisplay(rect)?
> 
> using consistent terminology for this stuff seems like a great idea to me! :)

Someone should bring this up with @piman...
Comment 26 Antoine Labour 2011-11-14 11:35:31 PST
(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #21)
> > > Also, why is it that WebContentLayer use invalidate / invalid but the implementation uses setNeedsDisplay(rect)?
> > 
> > using consistent terminology for this stuff seems like a great idea to me! :)
> 
> Someone should bring this up with @piman...

See https://bugs.webkit.org/show_bug.cgi?id=69107#c15
You guys figure out what you like and stick with it :)

I agree that consistency would be good. However, we have to realize that there are differences between the internals of WebCore (e.g GraphicsLayer uses setNeedsDisplay) and of chrome (e.g. chrome's RenderWidget uses didInvaildateRect), so anything in the middle will be inconsistent with one or the other.

My opinion is that "setNeedsDisplay" is somewhat misleading, whereas invalidate/invalid is more traditional. But I'm really not attached to anything.
Comment 27 David Reveman 2011-11-18 16:01:59 PST
Created attachment 115899 [details]
Patch
Comment 28 David Reveman 2011-11-20 16:45:10 PST
Created attachment 116009 [details]
Patch
Comment 29 Shawn Singh 2011-11-21 11:30:01 PST
Comment on attachment 116009 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:90
> +    if (m_needsDisplay)

should this be !m_needsDisplay ?

> Source/WebKit/chromium/src/WebContentLayer.cpp:59
> +bool WebContentLayer::isInvalid() const

Question: I'm not very familiar with this part of the code yet, but I wonder if this is accidentally misleading, making it sound like the entire layer is dirty?  I just wanted to bring this up, its not my place to say this should be fixed =)

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:-659
> -    verifyFloatRectsAlmostEqual(FloatRect(0.0f, 0.0f, 5.0f, 10.0f), testLayer->dirtyRect());

FYI we recently changed this - I wouldn't be surprised if this causes a patch conflict.
Comment 30 David Reveman 2011-11-21 12:20:52 PST
(In reply to comment #29)
> (From update of attachment 116009 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116009&action=review
> 
> > Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:90
> > +    if (m_needsDisplay)
> 
> should this be !m_needsDisplay ?

Oh nasty typo. I'm surprised this didn't show up in the layout tests. Thanks for noticing this! Uploading a new patch.

> 
> > Source/WebKit/chromium/src/WebContentLayer.cpp:59
> > +bool WebContentLayer::isInvalid() const
> 
> Question: I'm not very familiar with this part of the code yet, but I wonder if this is accidentally misleading, making it sound like the entire layer is dirty?  I just wanted to bring this up, its not my place to say this should be fixed =)

Sounds like another reason to make this consistent with LayerChromium (setNeedsDisplay/needsDisplay).

> 
> > Source/WebKit/chromium/tests/LayerChromiumTest.cpp:-659
> > -    verifyFloatRectsAlmostEqual(FloatRect(0.0f, 0.0f, 5.0f, 10.0f), testLayer->dirtyRect());
> 
> FYI we recently changed this - I wouldn't be surprised if this causes a patch conflict.
Comment 31 David Reveman 2011-11-21 12:21:18 PST
Created attachment 116115 [details]
Patch
Comment 32 Scott Violet 2011-11-22 09:35:05 PST
Comment on attachment 116115 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:279
>  void LayerChromium::resetNeedsDisplay()

Does this needs to be overriden to mark tiles as clean too?
Comment 33 David Reveman 2011-11-22 12:38:42 PST
(In reply to comment #32)
> (From update of attachment 116115 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116115&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:279
> >  void LayerChromium::resetNeedsDisplay()
> 
> Does this needs to be overriden to mark tiles as clean too?

Good point. That some of the needsDisplay functions are virtual but not others is  weird. Proper support for resetNeedsDisplay by ContentLayerChromium requires that we separate initial tile invalidation from invalidation from setNeedsDisplay. Seems like unnecessary complexity for something that's not really needed. I'll upload a new patch that just removes the resetNeedsDisplay function instead. To be consistent I'm also making needsDisplay() virtual in this patch.
Comment 34 David Reveman 2011-11-22 12:39:08 PST
Created attachment 116262 [details]
Patch
Comment 35 James Robinson 2011-11-28 16:34:39 PST
This patch is marked as blocked on several other bugs but I don't see any actual dependency.  This would be very useful for fixing another tricky bug, could we land it ahead of the other ones?
Comment 36 David Reveman 2011-11-28 22:08:00 PST
Created attachment 116893 [details]
Patch
Comment 37 David Reveman 2011-11-28 22:24:51 PST
(In reply to comment #35)
> This patch is marked as blocked on several other bugs but I don't see any actual dependency.  This would be very useful for fixing another tricky bug, could we land it ahead of the other ones?

Sure, no prob. Latest patch is without any dependencies.

These changes are somewhat covered by LayerChromiumTest but I've been wanting to add some additional tests once https://bugs.webkit.org/show_bug.cgi?id=72192 landed. I'm OK landing this now and adding these additional unit tests once 72192 has landed. Created https://bugs.webkit.org/show_bug.cgi?id=73285 to remind myself.
Comment 38 James Robinson 2011-11-28 22:53:02 PST
Comment on attachment 116893 [details]
Patch

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
Comment 39 David Reveman 2011-11-29 08:44:29 PST
(In reply to comment #38)
> (From update of attachment 116893 [details])
> 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?

None, afaik. I'll happily move it to tiled layer class if no one knows why this is currently in the content layer class.

> 
> 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

Agree.

> 
> > 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

I vote for removing it too.
Comment 40 David Reveman 2011-11-29 10:10:17 PST
Created attachment 116991 [details]
Patch
Comment 41 Antoine Labour 2011-11-29 10:52:22 PST
(In reply to comment #39)
> (In reply to comment #38)
> > 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
> 
> I vote for removing it too.

Yes, remove it, nobody uses it.
Comment 42 David Reveman 2011-11-29 11:00:57 PST
WebContentLayer::invalidRect() has been removed and invalidate logic has been moved to TiledLayerChromium class in latest patch.
Comment 43 Adrienne Walker 2011-11-29 11:19:50 PST
Comment on attachment 116991 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:297
> +    IntRect dirty(IntPoint(), bounds());

I know this isn't what the code was before, but I think this should be contentBounds() instead of bounds().
Comment 44 David Reveman 2011-11-29 11:32:13 PST
Created attachment 117008 [details]
Patch
Comment 45 David Reveman 2011-11-29 11:33:18 PST
(In reply to comment #43)
> (From update of attachment 116991 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116991&action=review
> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:297
> > +    IntRect dirty(IntPoint(), bounds());
> 
> I know this isn't what the code was before, but I think this should be contentBounds() instead of bounds().

Fixed.
Comment 46 Dana Jansens 2011-11-29 13:33:40 PST
Comment on attachment 117008 [details]
Patch

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

I'm not sure if it's what you'd like, but I can imagine not needing two virtual setNeedsDisplay functions. A little less code duplication/room for error later on?

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:109
> +    virtual void setNeedsDisplay();

This could be an inline function that calls the virtual one above with IntRect(IntPoint(), contentBounds()).

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:295
> +void TiledLayerChromium::setNeedsDisplay()

Then this function could go away entirely.
Comment 47 David Reveman 2011-11-29 15:13:52 PST
Created attachment 117055 [details]
Patch
Comment 48 David Reveman 2011-11-29 15:27:42 PST
(In reply to comment #46)
> (From update of attachment 117008 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117008&action=review
> 
> I'm not sure if it's what you'd like, but I can imagine not needing two virtual setNeedsDisplay functions. A little less code duplication/room for error later on?
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:109
> > +    virtual void setNeedsDisplay();
> 
> This could be an inline function that calls the virtual one above with IntRect(IntPoint(), contentBounds()).
> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:295
> > +void TiledLayerChromium::setNeedsDisplay()
> 
> Then this function could go away entirely.

Glad you pointed this out. Made me realize that the patch included a subtle but possibly very important difference in behavior compared to before. Calling setNeedsDisplay() without an arg on a layer with empty bounds caused the needDisplay flag to be set.

New patch fixes this problem and adds a unit test case to check it. I also removed some code duplication but kept setNeedDisplay() virtual. Making this just an inlined LayerChromium function that calls the virtual setNeedsDisplay(FloatRect&) will require us to explicitly call LayerChromium::setNeedsDisplay() in a few places. I'm fine doing that but considering that the needDisplay() getter is currently also a virtual function I'd prefer to keep it the way it is.
Comment 49 James Robinson 2011-11-30 19:05:25 PST
Comment on attachment 117055 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:109
> +    virtual void setNeedsDisplay() { LayerChromium::setNeedsDisplay(FloatRect(FloatPoint(), contentBounds())); }

does this need to be virtual? the only override's implementation matches this one exactly
Comment 50 David Reveman 2011-11-30 21:17:10 PST
(In reply to comment #49)
> (From update of attachment 117055 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117055&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:109
> > +    virtual void setNeedsDisplay() { LayerChromium::setNeedsDisplay(FloatRect(FloatPoint(), contentBounds())); }
> 
> does this need to be virtual? the only override's implementation matches this one exactly

No, it doesn't. I just wanted some buy-in before changing that as we'll have to call LayerChromium::setNeedsDisplay() explicitly in a few places or change the name of one of the setNeedsDisplay functions.
Comment 51 David Reveman 2011-11-30 21:19:32 PST
Created attachment 117324 [details]
Patch
Comment 52 James Robinson 2011-11-30 21:40:30 PST
Comment on attachment 117324 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        No new tests. (OOPS!)

svn presubmit check will barf on this

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:142
> +    LayerChromium::setNeedsDisplay();

why the qualifier here? setNeedsDisplay() isn't virtual or anything
Comment 53 James Robinson 2011-11-30 21:41:50 PST
(In reply to comment #50)
> (In reply to comment #49)
> > (From update of attachment 117055 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=117055&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:109
> > > +    virtual void setNeedsDisplay() { LayerChromium::setNeedsDisplay(FloatRect(FloatPoint(), contentBounds())); }
> > 
> > does this need to be virtual? the only override's implementation matches this one exactly
> 
> No, it doesn't. I just wanted some buy-in before changing that as we'll have to call LayerChromium::setNeedsDisplay() explicitly in a few places or change the name of one of the setNeedsDisplay functions.

I'm not sure I really understand what you are saying here.  It looks like there's only one zero-arg setNeedsDisplay() function in the *LayerChromium tree, unless I'm missing one.
Comment 54 David Reveman 2011-11-30 21:45:14 PST
Created attachment 117332 [details]
Patch
Comment 55 David Reveman 2011-11-30 21:52:13 PST
(In reply to comment #53)
> (In reply to comment #50)
> > (In reply to comment #49)
> > > (From update of attachment 117055 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=117055&action=review
> > > 
> > > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:109
> > > > +    virtual void setNeedsDisplay() { LayerChromium::setNeedsDisplay(FloatRect(FloatPoint(), contentBounds())); }
> > > 
> > > does this need to be virtual? the only override's implementation matches this one exactly
> > 
> > No, it doesn't. I just wanted some buy-in before changing that as we'll have to call LayerChromium::setNeedsDisplay() explicitly in a few places or change the name of one of the setNeedsDisplay functions.
> 
> I'm not sure I really understand what you are saying here.  It looks like there's only one zero-arg setNeedsDisplay() function in the *LayerChromium tree, unless I'm missing one.

The last two patches I uploaded implement two different ways of making LayerChromium::setNeedsDisplay() not virtual. First patch calls LayerChromium::setNeedsDisplay() explicitly in ImageLayerChromium and WebContentLayer. The second instead renames setNeedsDisplay(FloatRect&) to setNeedsDisplayRect(FloatRect&). I'm OK with landing either one of these last three patches. If I had to pick one out of the three I'd go with number 3 and setNeedsDisplayRect.
Comment 56 James Robinson 2011-11-30 21:56:34 PST
I'm still deeply confused :(. In c++ you can overload a function based on # of args so why can't we have

class LayerChromium {
  virtual void setNeedsDisplay(const FloatRect&);
  void setNeedsDisplay() { setNeedsDisplay(FloatRect(FloatPoint(), contentBounds()); }
};

or is that what one of these patches does?
Comment 57 David Reveman 2011-11-30 22:04:07 PST
(In reply to comment #56)
> I'm still deeply confused :(. In c++ you can overload a function based on # of args so why can't we have
> 
> class LayerChromium {
>   virtual void setNeedsDisplay(const FloatRect&);
>   void setNeedsDisplay() { setNeedsDisplay(FloatRect(FloatPoint(), contentBounds()); }
> };
> 
> or is that what one of these patches does?

We can but this code won't build if TiledLayerChromium only implements setNeedsDisplay(const FloatRect&):

void someFunction(TiledLayerChromium *tiledLayer)
{
    tiledLayer->setNeedsDisplay();
}

it would have to be:

void someFunction(TiledLayerChromium *tiledLayer)
{
    static_cast<LayerChromium>(tiledLayer)->setNeedsDisplay();
}

The second to last patch handles this like above while the last patch avoids this by renaming the setNeedsDisplay(const FloatRect&) function instead.
Comment 58 James Robinson 2011-11-30 22:09:03 PST
class A {
public:
    virtual void foo(int) { }
    void foo() { foo(1); }
};

class B : public A {
};

int main() {
    B* b = new B;
    b->foo();
    return 0;
}

seems to work?
Comment 59 David Reveman 2011-11-30 22:16:00 PST
(In reply to comment #58)
> class A {
> public:
>     virtual void foo(int) { }
>     void foo() { foo(1); }
> };
> 
> class B : public A {
> };
> 
> int main() {
>     B* b = new B;
>     b->foo();
>     return 0;
> }
> 
> seems to work?

yes, that works :)

class A {
public:
    virtual void foo(int) { }
    void foo() { foo(1); }
};

class B : public A {
    virtual void foo(int) { }
};

int main() {
    B* b = new B;
    b->foo();
    return 0;
}

but this doesn't.
Comment 60 James Robinson 2011-11-30 22:18:42 PST
OK, thanks for being patient.  That's fucked up.

Let's go with the rect suffix version and file a bug to port everything to Go.
Comment 61 David Reveman 2011-11-30 22:25:09 PST
(In reply to comment #60)
> OK, thanks for being patient.  That's fucked up.
> 
> Let's go with the rect suffix version and file a bug to port everything to Go.

Haha, no prob. I changed the review flag on the rect suffix version of the patch.
Comment 62 James Robinson 2011-11-30 22:25:34 PST
Comment on attachment 117332 [details]
Patch

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

OK hurray! R=me

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:218
> +        setNeedsDisplayRect(FloatRect(0, 0, bounds().width(), bounds().height()));

the old code used bounds() here but should this really be contentBounds()?

feel free to fix in a follow-up if that's the case

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:151
> +        m_videoLayer->setNeedsDisplayRect(IntRect(0, 0, m_videoLayer->bounds().width(), m_videoLayer->bounds().height()));

hm, shouldn't this just call setNeedsDisplay() with no args?

again this can be done in a follow-up, IMO
Comment 63 WebKit Review Bot 2011-11-30 23:40:23 PST
Comment on attachment 117332 [details]
Patch

Clearing flags on attachment: 117332

Committed r101623: <http://trac.webkit.org/changeset/101623>
Comment 64 WebKit Review Bot 2011-11-30 23:40:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 65 Adrienne Walker 2011-12-01 09:38:44 PST
(In reply to comment #64)
> All reviewed patches have been landed.  Closing bug.

:(

class A {
public:
    virtual void foo(int) { }
    void foo() { foo(1); }
};

class B : public A {
    using A::foo; // expose base class function hidden by virtual
    virtual void foo(int) { }
};

int main() {
    B* b = new B;
    b->foo();
    return 0;
}

^ This works.