Store a WebCore::Region instead of IntRect for the visible region of a layer. The bounding rect is still given to prepareToUpdate() and drawTiles() for now. Justifiaction for this change: This patch doesn't do anything more clever with Regions than is currently done with rects. A separate CL will work on CCLayerTreeHostCommon::calculateVisibleRegion() to exclude regions occluded by opaque layers. Then later we can pass the the Region to prepareToUpdate and it can skip occluded tiles within a layer (pending bug #71869). Draw culling can also use this to cull more tiles.
Created attachment 117248 [details] Patch I'd like to check for buy-in to using WebCore::Region, and the direction in which I'm approaching this. It doesn't change any functionality, but just converts the data type over to a Region.
> A separate CL will work on CCLayerTreeHostCommon::calculateVisibleRegion() to exclude regions occluded by opaque layers. To clarify, this should read CCLayerTreeHostCommon::calculateVisibleLayerRegion().
I think this patch needs a clarifying question answered: where do we intend to do culling? This patch seems to imply that we're going to perform culling at the layer level, prior to paint/upload/draw on both the main thread and the impl thread, so that you can skip culled paints in addition to culled draws. Maybe I'm not on the same page as everyone else, but I had been thinking that we were going to perform culling at the quad level, only on the impl thread when drawing, possibly after layers are no longer involved. With that in mind, using a Region doesn't make any sense because it's adding information in the wrong part of the pipeline.
(In reply to comment #3) > I think this patch needs a clarifying question answered: where do we intend to do culling? > > This patch seems to imply that we're going to perform culling at the layer level, prior to paint/upload/draw on both the main thread and the impl thread, so that you can skip culled paints in addition to culled draws. Yeh exactly. > Maybe I'm not on the same page as everyone else, but I had been thinking that we were going to perform culling at the quad level, only on the impl thread when drawing, possibly after layers are no longer involved. With that in mind, using a Region doesn't make any sense because it's adding information in the wrong part of the pipeline. wjmaclean is working on the quad culling you speak of. I'm planning to pick up culling here, as you say, prior to paint/upload/draw. The work should be complementary with the quad draw culling? The regions are copied over to the impl side, and tracking more detailed visible regions will allow draw culling of occluded tiles in the middle of layers, rather than only on the boundaries. The inspiration to look at culling painting came from @piman, I'm not sure if you're concerned that its happening or just didn't see it coming? What concerns do you have with it?
Comment on attachment 117248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117248&action=review I hope the impl side changes were just you doing a bit more s/setVisibleLayerRect/setVisibleLayerRegion/ than necessary. I think it looks great so far otherwise. I think this will be a very worthwhile optimization combined with my per-tile rendering changes. > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:176 > + void setVisibleLayerRegion(const Region& visibleLayerRegion) { m_visibleLayerRegion = visibleLayerRegion; } What's the reason you're also changing to a Region on the impl side? This should be a main thread only change isn't it?
(In reply to comment #4) > (In reply to comment #3) > wjmaclean is working on the quad culling you speak of. I'm planning to pick up culling here, as you say, prior to paint/upload/draw. Thanks. Mostly I'm just trying to get on the same page here. It's hard to evaluate changes if I don't know where everyone is going. :) > The work should be complementary with the quad draw culling? The regions are copied over to the impl side, and tracking more detailed visible regions will allow draw culling of occluded tiles in the middle of layers, rather than only on the boundaries. This is a minor detail, but regions will probably not get copied over to the impl side, as those layers could be animating independently and will have their own notion of which currently regions are visible. > The inspiration to look at culling painting came from @piman, I'm not sure if you're concerned that its happening or just didn't see it coming? What concerns do you have with it? If I had any concerns, it's that I'm not sure what the benefits of "quad culling" after "region-based layer culling" are on the impl side. On the drawing side, would you have any quads to cull after the first pass? It seems like these would be trying to do the same thing on the drawing side in two different places. Is this intended to be a main-thread only concept? My other concern is how similar the region-based layer culling system would be with the quad-based one. (Maybe I just want to have more information.) Would these share internals? Maybe I'd just prefer to have one culling system instead of two, so that it's easier to optimize it in the future, if we want to make occlusion queries more efficient.
(In reply to comment #6) > (In reply to comment #4) > > (In reply to comment #3) > > > wjmaclean is working on the quad culling you speak of. I'm planning to pick up culling here, as you say, prior to paint/upload/draw. > > Thanks. Mostly I'm just trying to get on the same page here. It's hard to evaluate changes if I don't know where everyone is going. :) Cool :) > > The work should be complementary with the quad draw culling? The regions are copied over to the impl side, and tracking more detailed visible regions will allow draw culling of occluded tiles in the middle of layers, rather than only on the boundaries. > > This is a minor detail, but regions will probably not get copied over to the impl side, as those layers could be animating independently and will have their own notion of which currently regions are visible. I had thought about animations, and @vollick (who is working on the animation stuff) believed it should be possible to have an "isAnimating" flag on a layer, which means we need to treat it as non-opaque for painting purposes. If it moves, we'll need to be able to draw what was under it without repainting. (In reply to comment #5) > What's the reason you're also changing to a Region on the impl side? This should be a main thread only change isn't it? (In reply to comment #6) > If I had any concerns, it's that I'm not sure what the benefits of "quad culling" after "region-based layer culling" are on the impl side. On the drawing side, would you have any quads to cull after the first pass? It seems like these would be trying to do the same thing on the drawing side in two different places. Is this intended to be a main-thread only concept? It seems to me like painting and drawing have similar requirements for knowing what is "visible" (or what could become visible in the painting case). The culling I'm planning is all on main thread, but I thought it would be a win to pass that region over to impl side too. With that region, the impl side could in theory just draw the region, but could also cull more (reduce this region further) based on animating layers and other things that are unknowns in the painting step. > My other concern is how similar the region-based layer culling system would be with the quad-based one. (Maybe I just want to have more information.) Would these share internals? Maybe I'd just prefer to have one culling system instead of two, so that it's easier to optimize it in the future, if we want to make occlusion queries more efficient. The current draw culling mechanisms are very tile centered in their approach, and I guess I imagined this side being more just tracking visibility on the layer, and having prepare to update simply create tiles that intersect the visible region. It could be done instead by creating a set of tile quads and passing it through a filter, but I'm not sure the same paradigm makes sense here. What do you think? Also, tracking and storing visible region in the layer requires recalculating it only when the layer (or some layer above it) changes, rather than on every paint, which is why I naturally gravitated toward this approach. I could just ship the Region::bounds() over to the impl side and leave it as is. I'm not sure what's the right thing to do here. Thoughts?
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #4) > > > (In reply to comment #3) > > > > > wjmaclean is working on the quad culling you speak of. I'm planning to pick up culling here, as you say, prior to paint/upload/draw. > > > > Thanks. Mostly I'm just trying to get on the same page here. It's hard to evaluate changes if I don't know where everyone is going. :) > > Cool :) > > > > The work should be complementary with the quad draw culling? The regions are copied over to the impl side, and tracking more detailed visible regions will allow draw culling of occluded tiles in the middle of layers, rather than only on the boundaries. > > > > This is a minor detail, but regions will probably not get copied over to the impl side, as those layers could be animating independently and will have their own notion of which currently regions are visible. > > I had thought about animations, and @vollick (who is working on the animation stuff) believed it should be possible to have an "isAnimating" flag on a layer, which means we need to treat it as non-opaque for painting purposes. If it moves, we'll need to be able to draw what was under it without repainting. > > (In reply to comment #5) > > What's the reason you're also changing to a Region on the impl side? This should be a main thread only change isn't it? > > (In reply to comment #6) > > If I had any concerns, it's that I'm not sure what the benefits of "quad culling" after "region-based layer culling" are on the impl side. On the drawing side, would you have any quads to cull after the first pass? It seems like these would be trying to do the same thing on the drawing side in two different places. Is this intended to be a main-thread only concept? > > It seems to me like painting and drawing have similar requirements for knowing what is "visible" (or what could become visible in the painting case). The culling I'm planning is all on main thread, but I thought it would be a win to pass that region over to impl side too. OK, I'd prefer if we kept the changes to the main thread in this first patch. We can change what's being passed to the impl side at the time when we can do something useful with it there. Also, it might seem like a simple thing to pass the region over to the impl thread right now but with the out-of-process compositor and serialization work I'm doing this is not necessarily the case. Might be more efficient to recompute the regions on the impl side instead. > > With that region, the impl side could in theory just draw the region, but could also cull more (reduce this region further) based on animating layers and other things that are unknowns in the painting step. > > > My other concern is how similar the region-based layer culling system would be with the quad-based one. (Maybe I just want to have more information.) Would these share internals? Maybe I'd just prefer to have one culling system instead of two, so that it's easier to optimize it in the future, if we want to make occlusion queries more efficient. It would be nice to do some code sharing. However, these two culling mechanism should probably run completely independent for optimal performance. > > The current draw culling mechanisms are very tile centered in their approach, and I guess I imagined this side being more just tracking visibility on the layer, and having prepare to update simply create tiles that intersect the visible region. It could be done instead by creating a set of tile quads and passing it through a filter, but I'm not sure the same paradigm makes sense here. What do you think? Also, tracking and storing visible region in the layer requires recalculating it only when the layer (or some layer above it) changes, rather than on every paint, which is why I naturally gravitated toward this approach. I'd prefer an initial version of this that doesn't rely on adding any layer state. If it turns out that these region computations have a significant performance impact, maybe using something more efficient like a fixed size tile-map instead of a region is a better approach. > > I could just ship the Region::bounds() over to the impl side and leave it as is. I'm not sure what's the right thing to do here. Thoughts? Yes, please do that.
(In reply to comment #8) > > The current draw culling mechanisms are very tile centered in their approach, and I guess I imagined this side being more just tracking visibility on the layer, and having prepare to update simply create tiles that intersect the visible region. It could be done instead by creating a set of tile quads and passing it through a filter, but I'm not sure the same paradigm makes sense here. What do you think? Also, tracking and storing visible region in the layer requires recalculating it only when the layer (or some layer above it) changes, rather than on every paint, which is why I naturally gravitated toward this approach. > > I'd prefer an initial version of this that doesn't rely on adding any layer state. If it turns out that these region computations have a significant performance impact, maybe using something more efficient like a fixed size tile-map instead of a region is a better approach. Just looking for clarification on this and what is adding layer state. Should I be switching Rect to a Region on main thread? One issue is that CCLayerTreeHostCommon::walkLayersAndComputeVisibleLayerRects (Regions) expects both main and impl side to store the same visible data structure. If impl side does not want to compute a Region in this way, then main side can't either, afaict. They seem expected to mirror each other. So I can leave the visible area as a rect that does not consider occlusion. But the reason I liked computing occlusion in walkLayersAndCompute... is that it would require only a single walk through the layer tree and O(1) work to compute the region for each layer. If we do this computation individually for each layer, it's going to need on average O(n) work per layer to do this computation which sucks more. I could also look for another place to walk the layer tree and compute the region for all layers, but that requires saving some new state somewhere ie a second member variable unoccludedRegion. And we start heading into the land of variables with similar names that differ slightly. I'm not sure what the use of the visible rect would really be if there was an unoccludedRegion present. But then I also wonder if the draw culling shouldn't be computing the unoccluded region in this walkLayersAndCompute... business as well rather than walking the layer tree during the culling process. I guess as a first cut I could, similar to the first-cut drawing approach iirc, just compute the unoccludedRegion on a per-layer basis doing the O(n) work for each layer. This doesn't require changing any member variables. A follow up could be to move the computation into walkLayersAndCompute, and have both painting and drawing cullers use the computed visibleRegion there instead of walking their respective layer trees themselves. Does that sound about right? If so, is TiledLayerChromium::prepareToUpdate() the right place to do that? Or should I work on getting data structures right (storing visible regions) first and doing culling afterward? > > > > I could just ship the Region::bounds() over to the impl side and leave it as is. I'm not sure what's the right thing to do here. Thoughts? > > Yes, please do that.
(In reply to comment #8) > OK, I'd prefer if we kept the changes to the main thread in this first patch. We can change what's being passed to the impl side at the time when we can do something useful with it there. > > Also, it might seem like a simple thing to pass the region over to the impl thread right now but with the out-of-process compositor and serialization work I'm doing this is not necessarily the case. Might be more efficient to recompute the regions on the impl side instead. I guess all that was to say that it seems like the impl side does do this computation as well. In fact I think I was wrong to say that the main thread passes the visible region over. There is a templated function that can do this for either side instead. @enne you might want to weigh in here with deeper understanding as I'm still new in this code and it'd be easy to make incorrect assumptions.
(In reply to comment #10) > I guess all that was to say that it seems like the impl side does do this computation as well. In fact I think I was wrong to say that the main thread passes the visible region over. There is a templated function that can do this for either side instead. @enne you might want to weigh in here with deeper understanding as I'm still new in this code and it'd be easy to make incorrect assumptions. No, you're quite right. We don't pass the current visibility rect over; we just recalculate it on the other side using whatever state the other layer tree is in. I think that's why I was confused about the idea of the region being passed over and not just recalculated.
Let me first try to clarify what I was trying to say in my previous post. I think it's fine to change setVisibleRect to setVisibleRegion on the main thread side. That rect/region is already part of the layer state so it wouldn't make things worse. However, we might want to try to pull some of these temporary "update" variables out of the layer at some point so adding more things that are just valid for the next update to the layer would be bad. I guess if you update visibleRegion whenever anything changed that affects it instead of by walking the tree and updating all visibleRegion members prior to painting it would no longer be a temporary variable. However, it seems harder to validate the correctness of such an approach. If performance is a concern, I think there's other things we can do before we consider using some persistent state. (In reply to comment #9) > (In reply to comment #8) > > > The current draw culling mechanisms are very tile centered in their approach, and I guess I imagined this side being more just tracking visibility on the layer, and having prepare to update simply create tiles that intersect the visible region. It could be done instead by creating a set of tile quads and passing it through a filter, but I'm not sure the same paradigm makes sense here. What do you think? Also, tracking and storing visible region in the layer requires recalculating it only when the layer (or some layer above it) changes, rather than on every paint, which is why I naturally gravitated toward this approach. > > > > I'd prefer an initial version of this that doesn't rely on adding any layer state. If it turns out that these region computations have a significant performance impact, maybe using something more efficient like a fixed size tile-map instead of a region is a better approach. > > Just looking for clarification on this and what is adding layer state. Should I be switching Rect to a Region on main thread? Yes, I think that makes sense. > > One issue is that CCLayerTreeHostCommon::walkLayersAndComputeVisibleLayerRects (Regions) expects both main and impl side to store the same visible data structure. If impl side does not want to compute a Region in this way, then main side can't either, afaict. They seem expected to mirror each other. > > So I can leave the visible area as a rect that does not consider occlusion. But the reason I liked computing occlusion in walkLayersAndCompute... is that it would require only a single walk through the layer tree and O(1) work to compute the region for each layer. If we do this computation individually for each layer, it's going to need on average O(n) work per layer to do this computation which sucks more. Whatever pass in CCLayerTreeHostCommon that is currently setting the visibleRect is fine for doing this. You definitely don't want to do all this work for each layer separately. FYI, the O(1) complexity to compute the visible region for each layer that you're referring to above includes a region operation which will run in O(n) worst case. > > I could also look for another place to walk the layer tree and compute the region for all layers, but that requires saving some new state somewhere ie a second member variable unoccludedRegion. And we start heading into the land of variables with similar names that differ slightly. I'm not sure what the use of the visible rect would really be if there was an unoccludedRegion present. > > But then I also wonder if the draw culling shouldn't be computing the unoccluded region in this walkLayersAndCompute... business as well rather than walking the layer tree during the culling process. > > I guess as a first cut I could, similar to the first-cut drawing approach iirc, just compute the unoccludedRegion on a per-layer basis doing the O(n) work for each layer. This doesn't require changing any member variables. A follow up could be to move the computation into walkLayersAndCompute, and have both painting and drawing cullers use the computed visibleRegion there instead of walking their respective layer trees themselves. > > Does that sound about right? If so, is TiledLayerChromium::prepareToUpdate() the right place to do that? Sorry that I got you side-tracked. I think the current patch is the right approach. > > Or should I work on getting data structures right (storing visible regions) first and doing culling afterward? I think you can do this region based occlusion culling on the main thread side without worrying about where the visibleRect/Region is currently stored. We might want to do something about where this Rect/Region is stored later but as long as you don't add more state to the layer I think it's fine.
So wait a tick - this is about paint-side visibility culling, right? Why is that a priority? It's not obvious to me that paint-side visibility culling will be able to share very much with draw-time visibility culling, or that paint-side visibility culling is as beneficial as draw-time. Can you describe a bit what the goals and use cases are here? I'm not sure this is worth the complexity of taking on now - a big part of our threading work has been revolving around getting painting out of the critical path for user interactions so that we don't have to worry quite so much about hyper-optimizing paint itself.
Paint is by very far the long pole for resize workloads.
For aura? I'm not completely sure where we expect this to save us - if we expect that most of this painting is non-visible content, does that mean we're making poor layer->texture mapping choices?
(In reply to comment #15) > For aura? I'm not completely sure where we expect this to save us - if we expect that most of this painting is non-visible content, does that mean we're making poor layer->texture mapping choices? Typically an aura window has a frame layer, covered at 90% by the window inner contents, covered at 90% by the web contents.
And what about that is heavy to paint? Is there actually complex stuff being painted there that is obscured by the web content area?
A lot of the savings come from the skipping of the texture upload. Seriously, we're spending 50+ ms painting (even if it's just a clear) and uploading.
If these are just solid color layers then we should implement a solid color layer that doesn't require any paint or upload. I'm really curious to see exactly what this layer hierarchy looks like - perhaps you could show me offline?
(In reply to comment #19) > If these are just solid color layers then we should implement a solid color layer that doesn't require any paint or upload. I'm really curious to see exactly what this layer hierarchy looks like - perhaps you could show me offline? +1 to sharing a sample tree on the bug so we can all see it. :) Also, don't david's texture uploading optimizations help here? What about if we add an "upload when confirmed to be visible"? That's more in line with our near term tasks...
Also, is the paint issue here that Aura layers are 1:1 with CCLayers? A key part of WebCore is tha multiple RenderLayers/RenderObjects are painted onto a single layer. This is analgous to how in Windows, some HWNDs paint into bitmaps and others paint into their parent.
(In reply to comment #12) > Let me first try to clarify what I was trying to say in my previous post. > > I think it's fine to change setVisibleRect to setVisibleRegion on the main thread side. That rect/region is already part of the layer state so it wouldn't make things worse. However, we might want to try to pull some of these temporary "update" variables out of the layer at some point so adding more things that are just valid for the next update to the layer would be bad. In order to change the rect to region on the main thread side, I have to do one of: - Make a second copy of the walkLayersAndCalculateVisibleLayerRects (Regions) with the alternate data type. - Template the return type (this will not work once the function does more inside than it currently does, as Rects don't have subtract). - Change the impl side to hold a Region also, and just use its bounds (like this patch). - Other idea? Which one is a good plan? > I guess if you update visibleRegion whenever anything changed that affects it instead of by walking the tree and updating all visibleRegion members prior to painting it would no longer be a temporary variable. However, it seems harder to validate the correctness of such an approach. If performance is a concern, I think there's other things we can do before we consider using some persistent state. Yeh I don't think that's what I would want to do anyway, just how I thought the code more or less worked, but I see not. And you're right about the region operation being possibly O(n) though likely less, whereas the tree walk can't be less. (In reply to comment #20) > (In reply to comment #19) > > If these are just solid color layers then we should implement a solid color layer that doesn't require any paint or upload. I'm really curious to see exactly what this layer hierarchy looks like - perhaps you could show me offline? > +1 to sharing a sample tree on the bug so we can all see it. :) +1 as well, would be happy to implement such a layer. > Also, don't david's texture uploading optimizations help here? What about if we add an "upload when confirmed to be visible"? That's more in line with our near term tasks... I have no idea how that would look or interact with the current code. Would the upload happen on the impl side?
(In reply to comment #21) > Also, is the paint issue here that Aura layers are 1:1 with CCLayers? A key part of WebCore is tha multiple RenderLayers/RenderObjects are painted onto a single layer. This is analgous to how in Windows, some HWNDs paint into bitmaps and others paint into their parent. We can't really avoid that for web contents that is painting in a separate process. Not sure if there's other situations where unnecessary CCLayers are created in Aura. Definitely something that should be improved if that's the case. Independent of Aura, we still have a lot of layer types that doesn't allow flattening (WebGL, Video..). And if using translateZ=0 is valid way to improve performance that would be another good example. I think it's also worth computing occlusion from transformed layers.
(In reply to comment #22) > (In reply to comment #12) > > Let me first try to clarify what I was trying to say in my previous post. > > > > I think it's fine to change setVisibleRect to setVisibleRegion on the main thread side. That rect/region is already part of the layer state so it wouldn't make things worse. However, we might want to try to pull some of these temporary "update" variables out of the layer at some point so adding more things that are just valid for the next update to the layer would be bad. > > In order to change the rect to region on the main thread side, I have to do one of: > - Make a second copy of the walkLayersAndCalculateVisibleLayerRects (Regions) with the alternate data type. > - Template the return type (this will not work once the function does more inside than it currently does, as Rects don't have subtract). > - Change the impl side to hold a Region also, and just use its bounds (like this patch). > - Other idea? > > Which one is a good plan? I like the approach of templatizing the return type. That way we make sure it's easy to eventually switch to a better data structure than a region. You'll have to do something more clever than operating on rect/region directly. Maybe use a delegate?
I just created these 4 test pages to allow us to validate and evaluate the performance benefit of this optimization: http://www.corp.google.com/~reveman/paint-culling/test.html http://www.corp.google.com/~reveman/paint-culling/test-3x.html http://www.corp.google.com/~reveman/paint-culling/test-scale.html http://www.corp.google.com/~reveman/paint-culling/test-3x-scale.html Basically the same test used for some of my texture upload patches but with a video occluding most of the contents. test.html is with only an animated background and the video pre-scaled to 720p. test-3x-scale.html is with three transformed contents layers underneath the video and the video is now scaled from it's original size by the compositor. Dana will implement an initial patch that allows us to evaluate the benefits of this optimization. We'll decide if it makes sense to land this based on the result of that. Alternative solutions for making these test pages perform better are of course also welcome.
(In reply to comment #19) > If these are just solid color layers then we should implement a solid color layer that doesn't require any paint or upload. I'm really curious to see exactly what this layer hierarchy looks like - perhaps you could show me offline? They're not fully solid. The area underneath the opaque region is though. I can definitely show you offline. (In reply to comment #21) > Also, is the paint issue here that Aura layers are 1:1 with CCLayers? A key part of WebCore is tha multiple RenderLayers/RenderObjects are painted onto a single layer. This is analgous to how in Windows, some HWNDs paint into bitmaps and others paint into their parent. We do that, to the extent that we can. We need a separate layer for the frame, because it will be translucent (it's not in the current theme, but it will be in the future), whereas we want to keep the rest opaque for optimal draw-time perf. (In reply to comment #20) > +1 to sharing a sample tree on the bug so we can all see it. :) In a nutshell you have (sizes are completely arbitrary, but to give you an idea): background (2560x1600, ContentLayer opaque) | |-window frame (2100x1510, ContentLayer opaque - but will be transparent) | | | |- window contents (2050x1500, ContentLayer opaque) | | | |- web content (2000x1500, ExternalTextureLayer opaque) | |-window frame (2100x1510, ContentLayer opaque - but will be transparent) | | | |- window contents (2050x1500, ContentLayer opaque) . | . |- web content (2000x1500, ExternalTextureLayer opaque) . There's other smaller layers for different parts of the system in different states, but they don't really matter for this. > > Also, don't david's texture uploading optimizations help here? What about if we add an "upload when confirmed to be visible"? That's more in line with our near term tasks... Yes, it should help too.
(In reply to comment #7) > The current draw culling mechanisms are very tile centered in their approach, and I guess I imagined this side being more just tracking visibility on the layer, and having prepare to update simply create tiles that intersect the visible region. It could be done instead by creating a set of tile quads and passing it through a filter, but I'm not sure the same paradigm makes sense here. What do you think? Also, tracking and storing visible region in the layer requires recalculating it only when the layer (or some layer above it) changes, rather than on every paint, which is why I naturally gravitated toward this approach. The tile culling is simpler if it can, for each tile, just use the layer it comes from to determine the tile's visibility (otherwise we need to unite many more smaller regions to determine who's occluded). If each Tile/Quad can say which layer it came from, then processing the tiles is simplified.
This is happening in better informed ways in other bugs so closing this.
Comment on attachment 117248 [details] Patch Cleared review? from attachment 117248 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).