RESOLVED FIXED 68207
Minor cleanups related to LayerChromium 
https://bugs.webkit.org/show_bug.cgi?id=68207
Summary Minor cleanups related to LayerChromium
Shawn Singh
Reported 2011-09-15 18:53:58 PDT
While studying LayerChromium to make unit tests, I came across several points that should be cleaned up. 1. fix const correctness of preserves3D and replicaLayer accessors 2. remove default NULL argument in the LayerChromium::create function. All existing instances of LayerChromium give an instance of the delegate, and it seems error prone to keep this default arg. 3. re-name border functions to reflect that they are only for debug use 4. remove m_contentsDirty from LayerChromium, which had redundant and error-prone semantics with m_dirtyRect. In particular, layers were using m_contentsDirty or m_dirtyRect, but not both. 5. NOT included in this first patch: is it OK if we re-name isRootLayer to isNonCompositedContent ? The only place the "isRootLayer" flag is being set is in the NonCompositedContentHost. it seems appropriate to call it isNonCompositedContent() because (1) we gradually move away from having 3 different "root" layers (2) the notion of a root layer becomes more homogeneous with other layers Please let me know what you think.
Attachments
Patch (11.63 KB, patch)
2011-09-15 18:57 PDT, Shawn Singh 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Shawn Singh
Comment 1 2011-09-15 18:57:13 PDT
Created attachment 107584 [details] Patch
James Robinson
Comment 2 2011-09-15 18:58:32 PDT
(In reply to comment #0) > While studying LayerChromium to make unit tests, I came across several points that should be cleaned up. > > 1. fix const correctness of preserves3D and replicaLayer accessors Yes, please! > > 2. remove default NULL argument in the LayerChromium::create function. All existing instances of LayerChromium give an instance of the delegate, and it seems error prone to keep this default arg. Yes, please! > > 3. re-name border functions to reflect that they are only for debug use Yes, please! > > 4. remove m_contentsDirty from LayerChromium, which had redundant and error-prone semantics with m_dirtyRect. In particular, layers were using m_contentsDirty or m_dirtyRect, but not both. I'm not sure what this means for layers that can have dirtyness vs layers that can only be entirely clean or entirely dirty. There probably is room for improvement, though. > > 5. NOT included in this first patch: is it OK if we re-name isRootLayer to isNonCompositedContent ? The only place the "isRootLayer" flag is being set is in the NonCompositedContentHost. it seems appropriate to call it isNonCompositedContent() because (1) we gradually move away from having 3 different "root" layers (2) the notion of a root layer becomes more homogeneous with other layers We have a root layer and we have a nonCompositedContent layer and they aren't quite the same thing. I think LayerChromium::isRootLayer really does mean isNonCompositedContent, so that's a good rename to make. In other places, it's not so clear. > > > Please let me know what you think. Most of these cleanups sound great (see above). I would strongly prefer that you do the orthogonal cleanups in separate patches. They will be small, but that means that they will be trivial to review + land with minimal chance of regression. Also, having more patches to your name is a good thing (it'll help you earn committer status sooner, for example).
James Robinson
Comment 3 2011-09-15 19:02:42 PDT
Comment on attachment 107584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107584&action=review I really wish you had done this as separate patches. In fact, It'd be great if you could break it up - most of the changes look completely non-controversial and obvious, but because it's all one big patch I have to sit down and think carefully about all the changes instead of just approving most of them individually :(. I'll look at this later when I get a chance. > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:284 > m_dirtyRect.setLocation(FloatPoint()); > m_dirtyRect.setSize(bounds()); > - m_contentsDirty = true; > setNeedsCommit(); i'm a little wary of this for 0x0 layers
Shawn Singh
Comment 4 2011-09-15 19:18:32 PDT
OK I'll split each one into separate bugs. I'll look into 0x0 layers, too. But my initial reaction is that if its 0x0 it would not have any content to paint in the first place, right? For layers that can only be entirely clean/dirty, for as long as i've seen the code, its marked the entire layer as the dirty rect. So if we're OK with those semantics, that will become the sole way to tell if a layer needs repainting. Thanks!
Note You need to log in before you can comment on or make changes to this bug.