Bug 71872 – [Chromium] Improve tile invalidation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Reveman]

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.

[David Reveman]

Created attachment 114189 [details]
Patch

[David Reveman]

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.

[Adrienne Walker]

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?

[David Reveman]

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

[Adrienne Walker]

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

[Shawn Singh]

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)

[David Reveman]

Created attachment 114380 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Adrienne Walker]

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().

[WebKit Review Bot]

Comment on attachment 114380 [details]
Patch

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

[Darin Fisher (:fishd, Google)]

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.

[Shawn Singh]

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

[David Reveman]

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

[David Reveman]

Created attachment 114388 [details]
Patch

[WebKit Review Bot]

Comment on attachment 114388 [details]
Patch

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

[Darin Fisher (:fishd, Google)]

(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."

[David Reveman]

Created attachment 114397 [details]
Patch

[David Reveman]

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

[WebKit Review Bot]

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

[James Robinson]

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?

[James Robinson]

Also, why is it that WebContentLayer use invalidate / invalid but the implementation uses setNeedsDisplay(rect)?

[David Reveman]

Created attachment 114814 [details]
Patch

[David Reveman]

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

[Darin Fisher (:fishd, Google)]

(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! :)

[Nat Duca]

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

[Antoine Labour]

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

[David Reveman]

Created attachment 115899 [details]
Patch

[David Reveman]

Created attachment 116009 [details]
Patch

[Shawn Singh]

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.

[David Reveman]

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

[David Reveman]

Created attachment 116115 [details]
Patch

[Scott Violet]

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?

[David Reveman]

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

[David Reveman]

Created attachment 116262 [details]
Patch

[James Robinson]

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?

[David Reveman]

Created attachment 116893 [details]
Patch

[David Reveman]

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

[James Robinson]

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

[David Reveman]

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

[David Reveman]

Created attachment 116991 [details]
Patch

[Antoine Labour]

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

[David Reveman]

WebContentLayer::invalidRect() has been removed and invalidate logic has been moved to TiledLayerChromium class in latest patch.

[Adrienne Walker]

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().

[David Reveman]

Created attachment 117008 [details]
Patch

[David Reveman]

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

[Dana Jansens]

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.

[David Reveman]

Created attachment 117055 [details]
Patch

[David Reveman]

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

[James Robinson]

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

[David Reveman]

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

[David Reveman]

Created attachment 117324 [details]
Patch

[James Robinson]

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

[James Robinson]

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

[David Reveman]

Created attachment 117332 [details]
Patch

[David Reveman]

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

[James Robinson]

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?

[David Reveman]

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

[James Robinson]

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?

[David Reveman]

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

[James Robinson]

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.

[David Reveman]

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

[James Robinson]

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

[WebKit Review Bot]

Comment on attachment 117332 [details]
Patch

Clearing flags on attachment: 117332

Committed r101623: <http://trac.webkit.org/changeset/101623>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adrienne Walker]

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