Bug 44091

Created attachment 64551 [details] Patch

I wounder if the clipping should be applied before adjusting the shadow rect. Doesn't your shadow have some blurring on the sides of the screen, if the shadowed rect is bigger than the current viewport?

(In reply to comment #2) > I wounder if the clipping should be applied before adjusting the shadow rect. In that case, we might miss the case where the clip is adjacent to the shadow rect, and the shadow has a large blur radius. But I see your point. I guess when the clip region can not fully contain the enlarged shadow rect (thus, they are intersecting or reverse contained), we need to expand the resulting rect once again. I'll update the patch to reflect this situation.

Created attachment 64556 [details] Patch

Comment on attachment 64556 [details] Patch Is there any performance difference with this?WebCore/platform/graphics/qt/ContextShadow.cpp:212 + why a new line here?

(In reply to comment #5) > (From update of attachment 64556 [details]) > Is there any performance difference with this? Definitely. Some sites, e.g. http://sueddeutsche.de/ has superlarge div, several thousand pixels height, with drop shadow and blur. Without this patch, we keep using QImage as large as that while a much smaller one will do the job just fine. > WebCore/platform/graphics/qt/ContextShadow.cpp:212 > + > why a new line here? Not necessary.

Can this be slower in some cases as well? or is that neglectable?

Created attachment 64564 [details] Patch

Not sure how this is called. But doesn't Qt store the current drawing of the page? So that the page doesn't need to draw on scrolling? That would mean, you just need to calculate the shadow once, and not over and over again?

(In reply to comment #7) > Can this be slower in some cases as well? or is that neglectable? The shadow blur area is guaranteed to be the same or smaller with this patch, vs without this patch. The only possible slowdown is the clipRegion().boundingRect() calculation. But this is really nothing (=negligible) compared to the blur operations itself (the most expensive part!), thus the extra cost is really worth the benefit.

(In reply to comment #9) > Not sure how this is called. But doesn't Qt store the current drawing of the page? So that the page doesn't need to draw on scrolling? That would mean, you just need to calculate the shadow once, and not over and over again? Do you mean the tiled backing store? It's just an optimization to reduce the painting operations, but the shadow (as with other primitives) still needs be drawn once.

(In reply to comment #11) > Do you mean the tiled backing store? It's just an optimization to reduce the painting operations, but the shadow (as with other primitives) still needs be drawn once. Yes I mean tiled backing store. But what do you mean with "still needs be drawn once.", this wouldn't affect scrolling right? Means you just have a longer calculation of the effect once? (I'm just curious)

> Yes I mean tiled backing store. But what do you mean with "still needs be drawn once.", this wouldn't affect scrolling right? Means you just have a longer calculation of the effect once? (I'm just curious) It's just caching the page onto a set of pixmaps (=tiles) with the coverage slightly larger than the viewport. When you scroll a bit and the tiles still cover the new viewport entirely, only bitmap blit is necessary. When you scroll farther and the newly exposed area in the viewport are not covered by any of the tiles, then new tiles are generated (this is where the page is drawn, to the tiles).

Comment on attachment 64564 [details] Patch Clearing flags on attachment: 64564 Committed r65488: <http://trac.webkit.org/changeset/65488>

All reviewed patches have been landed. Closing bug.