Bug 60719 – Split canvas from LayerTilerChromium

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alok Priyadarshi]

LayerTilerChromium paints into a bitmap canvas and uploads bitmap pixels to update tile textures. The task of painting and updating textures should be delegated to a custom object so that other types of canvases and update mechanisms can be used.

[Alok Priyadarshi]

Created attachment 93479 [details]
proposed patch

[WebKit Review Bot]

Comment on attachment 93479 [details]
proposed patch

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

[Alok Priyadarshi]

Created attachment 93508 [details]
proposed patch

fixed warning on cr-linux

[Alok Priyadarshi]

Created attachment 93513 [details]
proposed patch

fixed merge issues. grrr

[Adrienne Walker]

Comment on attachment 93513 [details]
proposed patch

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

Awesome, thanks for all this.  I have a bunch of small nits for this patch and some thoughts about where we're going to have to clean up some of this code that you're touching in the future.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:-114
>      LayerChromium::setLayerRenderer(layerRenderer);
>      createTilerIfNeeded();
> -    m_tiler->setLayerRenderer(layerRenderer);

This will not set the layer renderer properly if it changes, because the tiler will already have been created.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:121
> +PassOwnPtr<LayerTextureUpdater> ContentLayerChromium::createTextureUpdater()

I wish that the base version of createTextureUpdater didn't live on ContentLayerChromium, but that can wait until the ImageLayerChromium / ContentLayerChromium split.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:123
> +    LayerPainterChromium* painter = new ContentLayerPainter(m_owner);

adoptPtr on the same line as new.

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:76
> +    const PlatformImage& m_image;

The const PlatformImage& member variable is a little weird here, but I think it's the right choice for this patch.  In the future, when the compositor is on another thread, ImageLayerChromium will send an ImageLayerTextureUpdater to the compositor thread.  I think ultimately ImageLayerChromium should own ImageLayerTextureUpdater, and ImageLayerTextureUpdater should own PlatformImage.  Can you add a "// FIXME: ImageLayerTextureUpdater should own m_image"?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:98
> -PassRefPtr<LayerRendererChromium> LayerRendererChromium::create(PassRefPtr<GraphicsContext3D> context, PassOwnPtr<TilePaintInterface> contentPaint)
> +PassRefPtr<LayerRendererChromium> LayerRendererChromium::create(PassRefPtr<GraphicsContext3D> context, PassOwnPtr<LayerPainterChromium> contentPaint)

The fact that you had to make this change only makes me feel more strongly that we need to make root layers a LayerChromium and not their own special class in some future patch.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:130
> +    LayerTextureUpdater* textureUpdater = new LayerTextureUpdaterBitmap(m_context.get(), contentPaint, m_contextSupportsMapSub);

adoptPtr on the same line.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:64
> +#if USE(SKIA)
> +class SkCanvas;
> +#endif
> +

Why is this needed?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:-198
> -

Nit: don't make unnecessary whitespace changes, because it's more likely to cause merge conflicts for others.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:-236
> -

Nit: same as above.

> Source/WebCore/platform/graphics/chromium/LayerTextureSubImage.cpp:63
> +    if (m_useMapTexSubImage) {

There have been a number of crashes in all of this memcpy code in the past.  Can you split this out into two helper functions like uploadWithMapTexSubImage and uploadWithTexSubImage so that we can see where the problem is in a stack trace if this does crash?

> Source/WebCore/platform/graphics/chromium/LayerTextureSubImage.h:42
> +    LayerTextureSubImage(bool useMapSubForUpload);

This should be explicit.

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:41
> +    LayerTextureUpdater(GraphicsContext3D* context) : m_context(context) { }

This should be explicit.

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:51
> +    virtual void prepareToUpdate(const IntRect& contentRect, const IntSize& tileSize, int borderTexels) = 0;
> +    virtual void updateTextureRect(LayerTexture*, const IntRect& sourceRect, const IntRect& destRect) = 0;

This might be bikeshedding, but for consistency's sake could we call these the same thing as the LayerTilerChromium functions, i.e. update() and upload()?

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:307
> -            IntPoint paintOffset(sourceRect.x() - paintRect.x(), sourceRect.y() - paintRect.y());
> -            if (paintOffset.x() < 0)
> -                CRASH();
> -            if (paintOffset.y() < 0)
> -                CRASH();
> -            if (paintOffset.x() + destRect.width() > paintRect.width())
> -                CRASH();
> -            if (paintOffset.y() + destRect.height() > paintRect.height())
> -                CRASH();
> +            IntPoint paintOffset(sourceRect.x() - m_paintRect.x(), sourceRect.y() - m_paintRect.y());
> +            ASSERT((paintOffset.x() >= 0) && (paintOffset.y() >= 0));
> +            ASSERT(paintOffset.x() + destRect.width() <= m_paintRect.width());
> +            ASSERT(paintOffset.y() + destRect.height() <= m_paintRect.height());

Can you not turn these CRASH calls into ASSERT? These have failed in the past due to unexpected inputs and when that happens it causes memory overruns.  If you want to clean this up, please do so in another patch so that it'd be easily revertible.

[Adrienne Walker]

(In reply to comment #5)
> (From update of attachment 93513 [details])

> > Source/WebCore/platform/graphics/chromium/LayerTextureSubImage.h:42
> > +    LayerTextureSubImage(bool useMapSubForUpload);
> 
> This should be explicit.

Oops.  I didn't mean this.

[James Robinson]

Comment on attachment 93513 [details]
proposed patch

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

>> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:76
>> +    const PlatformImage& m_image;
> 
> The const PlatformImage& member variable is a little weird here, but I think it's the right choice for this patch.  In the future, when the compositor is on another thread, ImageLayerChromium will send an ImageLayerTextureUpdater to the compositor thread.  I think ultimately ImageLayerChromium should own ImageLayerTextureUpdater, and ImageLayerTextureUpdater should own PlatformImage.  Can you add a "// FIXME: ImageLayerTextureUpdater should own m_image"?

how could this work?  The PlatformImage changes on an ImageLayerChromium, potentially every frame (consider an animated gif).  do we make a new uploader every frame?

this seems...dodgy

[James Robinson]

(In reply to comment #7)
> (From update of attachment 93513 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93513&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:76
> >> +    const PlatformImage& m_image;
> > 
> > The const PlatformImage& member variable is a little weird here, but I think it's the right choice for this patch.  In the future, when the compositor is on another thread, ImageLayerChromium will send an ImageLayerTextureUpdater to the compositor thread.  I think ultimately ImageLayerChromium should own ImageLayerTextureUpdater, and ImageLayerTextureUpdater should own PlatformImage.  Can you add a "// FIXME: ImageLayerTextureUpdater should own m_image"?
> 
> how could this work?  The PlatformImage changes on an ImageLayerChromium, potentially every frame (consider an animated gif).  do we make a new uploader every frame?
> 
> this seems...dodgy

Oh, I was mistaken - the object remains the same but the contents change.  This still feels as if something is being misplaced.

[James Robinson]

Comment on attachment 93513 [details]
proposed patch

It seems weird and fragile that for content layers the PlatformCanvas is owned by the updater, but for image layers the PlatformImage is owned by the layer and referenced by the updater.  Can you have the updater have ownership of the PlatformImage directly?

[Alok Priyadarshi]

Comment on attachment 93513 [details]
proposed patch

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

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:-114
>> -    m_tiler->setLayerRenderer(layerRenderer);
> 
> This will not set the layer renderer properly if it changes, because the tiler will already have been created.

Actually it does work. LayerChromium::setLayerRenderer() calls ContentLayerChromium::cleanupResources() which in turn clears the tiler. So now we are re-creating the tiler if layer-renderer changes. We were earlier doing something similar in LayerTilerChromium::reset(). Now that LayerTilerChromium owns LayerTextureUpdater which keeps reference to layer-renderer-context, I think it is cleaner to just create the updater and tiler from scratch whenever layer-renderer changes.

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:121

> 
> I wish that the base version of createTextureUpdater didn't live on ContentLayerChromium, but that can wait until the ImageLayerChromium / ContentLayerChromium split.

Yes this will not be necessary after the ImageLayerChromium / ContentLayerChromium split.

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:123
>> +    LayerPainterChromium* painter = new ContentLayerPainter(m_owner);
> 
> adoptPtr on the same line as new.

done

>>>> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:76
>>>> +    const PlatformImage& m_image;
>>> 
>>> The const PlatformImage& member variable is a little weird here, but I think it's the right choice for this patch.  In the future, when the compositor is on another thread, ImageLayerChromium will send an ImageLayerTextureUpdater to the compositor thread.  I think ultimately ImageLayerChromium should own ImageLayerTextureUpdater, and ImageLayerTextureUpdater should own PlatformImage.  Can you add a "// FIXME: ImageLayerTextureUpdater should own m_image"?
>> 
>> how could this work?  The PlatformImage changes on an ImageLayerChromium, potentially every frame (consider an animated gif).  do we make a new uploader every frame?
>> 
>> this seems...dodgy
> 
> Oh, I was mistaken - the object remains the same but the contents change.  This still feels as if something is being misplaced.

I agree that it will be cleaner if ImageLayerTextureUpdater owned PlatformImage. But ImageLayerChromium still uses PlatformImage size to mark tiler dirty and to updateLayerSize(). I guess you could get image size from somewhere else, but I did not want to change too many things in this patch. I have added a FIXME.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:130
>> +    LayerTextureUpdater* textureUpdater = new LayerTextureUpdaterBitmap(m_context.get(), contentPaint, m_contextSupportsMapSub);
> 
> adoptPtr on the same line.

done

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:64
>> +
> 
> Why is this needed?

Because I removed "#include PlatformCanvas.h" from some other header file. LayerRendererChromium has an SkCanvas member variable.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:-198
>> -
> 
> Nit: don't make unnecessary whitespace changes, because it's more likely to cause merge conflicts for others.

reverted

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:-236
>> -
> 
> Nit: same as above.

reverted

>> Source/WebCore/platform/graphics/chromium/LayerTextureSubImage.cpp:63
>> +    if (m_useMapTexSubImage) {
> 
> There have been a number of crashes in all of this memcpy code in the past.  Can you split this out into two helper functions like uploadWithMapTexSubImage and uploadWithTexSubImage so that we can see where the problem is in a stack trace if this does crash?

done

>> Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:41
>> +    LayerTextureUpdater(GraphicsContext3D* context) : m_context(context) { }
> 
> This should be explicit.

done

>> Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:51
>> +    virtual void updateTextureRect(LayerTexture*, const IntRect& sourceRect, const IntRect& destRect) = 0;
> 
> This might be bikeshedding, but for consistency's sake could we call these the same thing as the LayerTilerChromium functions, i.e. update() and upload()?

For a class named LayerTextureUpdater, update should really update textures. What if we renamed LayerTilerChromium functions?

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:307
>> +            ASSERT(paintOffset.y() + destRect.height() <= m_paintRect.height());
> 
> Can you not turn these CRASH calls into ASSERT? These have failed in the past due to unexpected inputs and when that happens it causes memory overruns.  If you want to clean this up, please do so in another patch so that it'd be easily revertible.

reverted

[Alok Priyadarshi]

Created attachment 93647 [details]
proposed patch

Addressed reviewer comments.

[Adrienne Walker]

(In reply to comment #10)
> (From update of attachment 93513 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93513&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:-114
> >> -    m_tiler->setLayerRenderer(layerRenderer);
> > 
> > This will not set the layer renderer properly if it changes, because the tiler will already have been created.
> 
> Actually it does work. LayerChromium::setLayerRenderer() calls ContentLayerChromium::cleanupResources() which in turn clears the tiler.

Thanks for the explanation.  That makes perfect sense.
 
> >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:64
> >> +
> > 
> > Why is this needed?
> 
> Because I removed "#include PlatformCanvas.h" from some other header file. LayerRendererChromium has an SkCanvas member variable.

Ah, ok.  I just had an assumption that LayerRendererChromium shouldn't depend on any Skia or Cg classes.

I just uploaded a patch to https://bugs.webkit.org/show_bug.cgi?id=60899 to fix this, so if that lands first, then you shouldn't need that #ifdef.

> >> +    virtual void updateTextureRect(LayerTexture*, const IntRect& sourceRect, const IntRect& destRect) = 0;
> > 
> > This might be bikeshedding, but for consistency's sake could we call these the same thing as the LayerTilerChromium functions, i.e. update() and upload()?
> 
> For a class named LayerTextureUpdater, update should really update textures. What if we renamed LayerTilerChromium functions?

Good suggestion.  That sounds fine to me to just call the LayerTilerChromium functions prepareToUpdate and updateTextureRect.  Although, I'd maybe change updateTextureRect to updateTexture because the former sounds like you're updating a rect rather than updating the texture itself.

James, do you have any naming opinions here?

[James Robinson]

Comment on attachment 93647 [details]
proposed patch

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

This is looking pretty good now.  I'm still a bit uneasy about the const PlatformImage& on ImageLayerTextureUpdater, but that doesn't have to be resolved in this patch.  I don't feel strongly about the update() names.  Needs a merge and a few nits but I think we're about there.

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:73
> +        return IntRect(IntPoint(0, 0), m_image.size());

nit: you can just do IntPoint(). this does not seem like a very useful utility function

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:63
> +#if USE(SKIA)
> +class SkCanvas;
> +#endif

If you merge up to ToT this won't be needed any more.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:156
> +    explicit LayerRendererChromium(PassRefPtr<GraphicsContext3D>, PassOwnPtr<LayerPainterChromium> contentPaint);

nit: explicit isn't needed since this c'tor take stwo arguments (you did not break this, but since you're in here anyway might as well clean it up)

> Source/WebCore/platform/graphics/chromium/LayerTextureSubImage.h:42
> +    LayerTextureSubImage(bool useMapSubForUpload);

need explicit here

[Alok Priyadarshi]

Comment on attachment 93647 [details]
proposed patch

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

>> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:73
>> +        return IntRect(IntPoint(0, 0), m_image.size());
> 
> nit: you can just do IntPoint(). this does not seem like a very useful utility function

used IntPoint::zero() instead.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:63
>> +#endif
> 
> If you merge up to ToT this won't be needed any more.

removed

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:156
>> +    explicit LayerRendererChromium(PassRefPtr<GraphicsContext3D>, PassOwnPtr<LayerPainterChromium> contentPaint);
> 
> nit: explicit isn't needed since this c'tor take stwo arguments (you did not break this, but since you're in here anyway might as well clean it up)

done

>> Source/WebCore/platform/graphics/chromium/LayerTextureSubImage.h:42
>> +    LayerTextureSubImage(bool useMapSubForUpload);
> 
> need explicit here

done

[Alok Priyadarshi]

Created attachment 93696 [details]
proposed patch

Main changes:
- Replaced IntPoint(0,0) with IntPoint::zero()
- Removed #ifdef(SKIA) from LayerRendererChromium.h
- explicit constructors
- Renamed LayerTilerChromium member functions: update -> prepareToUpdate and upload -> updateRect

[Adrienne Walker]

Comment on attachment 93696 [details]
proposed patch

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

One small nit:

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:60
> +    void updateRect();

As I mentioned before, I would still like the function names to be consistent.  Can LayerTilerChromium::updateRect => LayerTilerChromium::updateTexture and LayerTextureUpdater::updateTextureRect => LayerTextureUpdater::updateTexture?

[Alok Priyadarshi]

Comment on attachment 93696 [details]
proposed patch

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

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:60
>> +    void updateRect();
> 
> As I mentioned before, I would still like the function names to be consistent.  Can LayerTilerChromium::updateRect => LayerTilerChromium::updateTexture and LayerTextureUpdater::updateTextureRect => LayerTextureUpdater::updateTexture?

updateTexture is fine too. But I do not think it is important for the function names to be consistent. Though they are internally doing the same thing, context is different. LayerTilerChromium is updating many textures for a dirty content-rect while LayerTextureUpdater is updating a rect within a single texture. For the clients of LayerTilerChromium, it should not matter if the tiler even uses textures. They should just be able to invalidate, update, and draw.

But if you feel strongly about this, I can make this change. I am fine either way.

[Adrienne Walker]

(In reply to comment #17)
> (From update of attachment 93696 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93696&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:60
> >> +    void updateRect();
> > 
> > As I mentioned before, I would still like the function names to be consistent.  Can LayerTilerChromium::updateRect => LayerTilerChromium::updateTexture and LayerTextureUpdater::updateTextureRect => LayerTextureUpdater::updateTexture?
> 
> updateTexture is fine too. But I do not think it is important for the function names to be consistent. Though they are internally doing the same thing, context is different. LayerTilerChromium is updating many textures for a dirty content-rect while LayerTextureUpdater is updating a rect within a single texture. For the clients of LayerTilerChromium, it should not matter if the tiler even uses textures. They should just be able to invalidate, update, and draw.

That's reasonable.  Just leave it as-is.

[WebKit Commit Bot]

Comment on attachment 93696 [details]
proposed patch

Clearing flags on attachment: 93696

Committed r86625: <http://trac.webkit.org/changeset/86625>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andrew Wilson]

Rolling out because it's crashing the chromium canaries.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20GPU%20Mesa%20-%20chromium.org&tests=compositing%2Fcolor-matching%2Fimage-color-matching.html

ASSERTION FAILED: imageRect().contains(contentRect)
third_party/WebKit/Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp(60) : virtual void WebCore::ImageLayerTextureUpdater::prepareToUpdate(const WebCore::IntRect&, const WebCore::IntSize&, int)
[3429:3429:2430038712840:ERROR:process_util_posix.cc(108)] Received signal 11

[Andrew Wilson]

Reverted r86625 for reason:

Caused failed assertion on Chromium gpu canary bots

Committed r86640: <http://trac.webkit.org/changeset/86640>

[Adrienne Walker]

Created attachment 93859 [details]
Fix for the image assertion issues.

The above assertion was being triggered because the paint rect was larger than the image itself.  This is because in the update function the paint rect is potentially extended to tile boundaries.

There was some offline discussion between Alok and me about whether we should have the tiler store explicit layer bounds and have different sized textures for outside tiles.  I thought this might be easy, but it ultimately added a lot more complication to the tiler's drawing and recycling mechanisms, where the tiler previously had a single tile size for all textures.

After some thought, I think the best solution is to just clip any request for uploading pixels from outside the image bounds to the image bounds itself.  Other layers have no problem painting outside their bounds, so this is really just an image-specific restriction.  Intersecting with the image bounds mimics the same behavior as before this patch, but the intersection has just moved to the updater from the tiler as a result of this refactoring.

I've attached a patch which implements this suggestion and causes that failing test to pass.

[Alok Priyadarshi]

(In reply to comment #23)
> Created an attachment (id=93859) [details]
> Fix for the image assertion issues.
> 
> The above assertion was being triggered because the paint rect was larger than the image itself.  This is because in the update function the paint rect is potentially extended to tile boundaries.
> 
> There was some offline discussion between Alok and me about whether we should have the tiler store explicit layer bounds and have different sized textures for outside tiles.  I thought this might be easy, but it ultimately added a lot more complication to the tiler's drawing and recycling mechanisms, where the tiler previously had a single tile size for all textures.
> 
> After some thought, I think the best solution is to just clip any request for uploading pixels from outside the image bounds to the image bounds itself.  Other layers have no problem painting outside their bounds, so this is really just an image-specific restriction.  Intersecting with the image bounds mimics the same behavior as before this patch, but the intersection has just moved to the updater from the tiler as a result of this refactoring.
> 

Adrienne: Thanks for the patch. Your solution would certainly work. I think the solution we discussed (keeping layer bounds in tiler) would also work while keeping the sizes for boundary tiles same. Do not clamp the tile size but rather source and destination rect in exactly the same way in your patch. Just move the clamping code to the tiler rather than ImageLayerTextureUpdater. Am I missing something?

[Adrienne Walker]

(In reply to comment #24)

> Adrienne: Thanks for the patch. Your solution would certainly work. I think the solution we discussed (keeping layer bounds in tiler) would also work while keeping the sizes for boundary tiles same. Do not clamp the tile size but rather source and destination rect in exactly the same way in your patch. Just move the clamping code to the tiler rather than ImageLayerTextureUpdater. Am I missing something?

I considered that option as well, but thought this was better for two reasons.  Mostly, clamping in the image layer updater seemed simpler--there's no need to add code to ContentLayerChromium, ImageLayerChromium, LayerRendererChromium, and a number of places in WebViewImpl to keep track of the layer size.  Also, if we've gone ahead and overrendered and the paint rect fills the tile, why not upload all of those pixels in case we can use them later? This seems like a different use case than the images where the image size is fixed.

[Alok Priyadarshi]

(In reply to comment #25)
> (In reply to comment #24)
> 
> > Adrienne: Thanks for the patch. Your solution would certainly work. I think the solution we discussed (keeping layer bounds in tiler) would also work while keeping the sizes for boundary tiles same. Do not clamp the tile size but rather source and destination rect in exactly the same way in your patch. Just move the clamping code to the tiler rather than ImageLayerTextureUpdater. Am I missing something?
> 

I think it is best to land this patch and worry about setting layer bounds later. I am on vacation today. I will merge your patch with mine and try to land it tomorrow.

I have an idea to fix 60983 in a simple way. A couple of questions for my understanding:

> I considered that option as well, but thought this was better for two reasons.  Mostly, clamping in the image layer updater seemed simpler--there's no need to add code to ContentLayerChromium, ImageLayerChromium, LayerRendererChromium, and a number of places in WebViewImpl to keep track of the layer size. 

Clamping in the image layer updater is certainly simpler. But it does seem a little weird that the tiler needs to paint outside the layer bounds. All the places that you mentioned already does setLayerPosition(). May be it can simply be replaced with setLayerRect().

> Also, if we've gone ahead and overrendered and the paint rect fills the tile, why not upload all of those pixels in case we can use them later? This seems like a different use case than the images where the image size is fixed.

I did not understand your last motivation. Are content/root layers able to paint outside the layer bounds? Even if they could in which cases could the pixels outside the bounds be used?

[Adrienne Walker]

(In reply to comment #26)

> > Also, if we've gone ahead and overrendered and the paint rect fills the tile, why not upload all of those pixels in case we can use them later? This seems like a different use case than the images where the image size is fixed.
> 
> I did not understand your last motivation. Are content/root layers able to paint outside the layer bounds? Even if they could in which cases could the pixels outside the bounds be used?

I talked with jamesr and it turns out that I had a misunderstanding about frame resizing.  Apparently the entire root layer needs to be invalidated when the frame size changes.  So, there is no motivation to render outside the layer bounds because it will never be able to be used.

[Adrienne Walker]

Created attachment 93992 [details]
Patch

[James Robinson]

Comment on attachment 93992 [details]
Patch

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

If at first you don't succeed, try try again!

> Source/WebCore/ChangeLog:2
> [phone]  Alok Priyadarshi  <[e-mail]>
> +            Adrienne Walker  <[e-mail]>

nit: same line, should be "Alok Priyadarshi  <[e-mail]> and Adrienne Walker  <[e-mail]>"

do you use your @google.com for webkit patches or @chromium.org?

> Source/WebKit/chromium/ChangeLog:2
> [phone]  Alok Priyadarshi  <[e-mail]>
> +            Adrienne Walker  <[e-mail]>

same nits here

[WebKit Review Bot]

Comment on attachment 93992 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 93992 [details]
Patch

Attachment 93992 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8702915

[Adrienne Walker]

Created attachment 93993 [details]
Patch

[Adrienne Walker]

Ack.  I applied that patch wrong and didn't get any of the new files.  This patch should pass the ews bots.

[Adrienne Walker]

Committed r86805: <http://trac.webkit.org/changeset/86805>