| Summary: | Overlap map for compositing should ignore empty layers | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adrienne Walker <enne> | ||||||
| Component: | New Bugs | Assignee: | Adrienne Walker <enne> | ||||||
| Status: | RESOLVED WONTFIX | ||||||||
| Severity: | Normal | CC: | enne, jamesr, klobag, mitz, simon.fraser, thorton, wangxianzhu, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | 84630 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Adrienne Walker
2011-06-27 17:51:31 PDT
Created attachment 98833 [details]
Patch
While I'm looking at overlap issues, I might as well clean this tiny little wart too. As far as I can tell, there's no reason to expand the bounds of empty layers to be 1x1 in the overlap map. It just causes layers to be composited when they don't need to be. These layers should be skipped, as anything overlapping them shouldn't cause compositing. Did you do archaeology on this one? I know I added that for a reason. (In reply to comment #3) > Did you do archaeology on this one? I know I added that for a reason. It came from here: https://bugs.webkit.org/show_bug.cgi?id=34065. The test added with that patch continues to pass without any assertions, at least on Chromium Linux and Safari Snow Leopard. I still do not understand that bug fix. If the embed is empty, overlaps shouldn't be composited. If the embed is not empty, then (1,1) is probably the wrong size. (In reply to comment #4) > (In reply to comment #3) > > Did you do archaeology on this one? I know I added that for a reason. > > It came from here: https://bugs.webkit.org/show_bug.cgi?id=34065. The test added with that patch continues to pass without any assertions, at least on Chromium Linux and Safari Snow Leopard. > > I still do not understand that bug fix. If the embed is empty, overlaps shouldn't be composited. If the embed is not empty, then (1,1) is probably the wrong size. Ah, https://bugs.webkit.org/show_bug.cgi?id=34065#c4 tries to explain the issue. I would expect that changing this would cause the assertion to start firing again. (In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > Did you do archaeology on this one? I know I added that for a reason. > > > > It came from here: https://bugs.webkit.org/show_bug.cgi?id=34065. The test added with that patch continues to pass without any assertions, at least on Chromium Linux and Safari Snow Leopard. > > > > I still do not understand that bug fix. If the embed is empty, overlaps shouldn't be composited. If the embed is not empty, then (1,1) is probably the wrong size. > > Ah, https://bugs.webkit.org/show_bug.cgi?id=34065#c4 tries to explain the issue. I would expect that changing this would cause the assertion to start firing again. It doesn't start firing again with this change. The clip rects and roots for all layers in the second test case from bug 34065 are identical with and without this patch. The clip rects and roots for all layers in empty-embed-rects are also identical with and without this patch. smfr: I don't quite follow the explanation in that comment and how that particular fix solves the issue. If you think bug 34065 is still an issue, can you make a test case that fails? The test you submitted with that patch passes with and without the code that went with it right now. Comment on attachment 98833 [details]
Patch
r-, needs more investigation based on my comments.
(In reply to comment #8) > (From update of attachment 98833 [details]) > r-, needs more investigation based on my comments. Are you saying that you're going to investigate this, or that I should? I've seen this overlap entry create a lot of extra unneeded layers on Google Maps. Created attachment 130302 [details]
Rebased
(In reply to comment #10) > Created an attachment (id=130302) [details] > Rebased smfr: I'd like to get this change landed. It continues to cause extra unnecessary layers to be created on a number of sites due to overlap. I feel like I've done a fair amount of investigation already on this bug. Removing these lines of code does not cause the assert to start triggering again in Chromium or in Safari. As I mentioned earlier, the clip rects and clip roots for both tests attached to bug 34065 are identical with and without this patch. If you believe this code is still useful and shouldn't be removed, can you give me any pointers to write a test case that would fail? It's hard to propose an alternate solution when all of the existing test cases continue to pass without this code. Comment on attachment 130302 [details]
Rebased
OK, let's land it and see if anything breaks. Keep an eye out for new assertions related to clip rects etc.
Comment on attachment 130302 [details]
Rebased
Thanks, Simon.
Comment on attachment 130302 [details] Rebased Clearing flags on attachment: 130302 Committed r109981: <http://trac.webkit.org/changeset/109981> All reviewed patches have been landed. Closing bug. *** Bug 78943 has been marked as a duplicate of this bug. *** It looks like this caused https://bugs.webkit.org/show_bug.cgi?id=84558; I'm going to roll it out for now. Keep this regression in mind when fixing the patch. (In reply to comment #17) > It looks like this caused https://bugs.webkit.org/show_bug.cgi?id=84558; I'm going to roll it out for now. Keep this regression in mind when fixing the patch. My patch changed which render layers became composited that just happened to unearth a repaint issue for one port on one platform. Reverting this patch is just papering over the real problem. smfr, timothy_horton: please make a smaller repro case so that this repainting problem can be fixed. My patch was fixing real issues with too many layers being created on certain sites and I'd like to re-land it and not be blocked by Safari Lion repainting bugs. (In reply to comment #18) > (In reply to comment #17) > > It looks like this caused https://bugs.webkit.org/show_bug.cgi?id=84558; I'm going to roll it out for now. Keep this regression in mind when fixing the patch. > > My patch changed which render layers became composited that just happened to unearth a repaint issue for one port on one platform. Reverting this patch is just papering over the real problem. > > smfr, timothy_horton: please make a smaller repro case so that this repainting problem can be fixed. My patch was fixing real issues with too many layers being created on certain sites and I'd like to re-land it and not be blocked by Safari Lion repainting bugs. smfr, thorton: Have you had a chance to investigate this bug and create a smaller repro case? I still believe that this is a repaint problem with Safari's canvas implementation only and would like to re-land this patch. smfr, thorton: If you want this patch, please feel free to land it yourself. I am marking this as WONTFIX based on the previous regression that it was rolled out for. |