Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1407543003:
Preliminary paint property walk implementation for SPv2 (Closed)
|
Created:
5 years, 2 months ago by trchen Modified:
5 years, 2 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj, dshwang, drott+blinkwatch_chromium.org, krit, eae+blinkwatch, f(malita), jchaffraix+rendering, Justin Novosad, leviw+renderwatch, pdr+graphicswatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, rwlbuis, Stephen Chennney, slimming-paint-reviews_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPreliminary paint property walk implementation for SPv2
BUG=537409
Committed: https://crrev.com/6e5dafd02a193d3f180923a1255b487de127879f
Cr-Commit-Position: refs/heads/master@{#355488}
Patch Set 1 #
Total comments: 34
Patch Set 2 : address some of the comments. #
Total comments: 3
Patch Set 3 : remove ObjectPaintProperties::Owner, revert to more concrete naming for transform nodes, added comm… #
Total comments: 2
Patch Set 4 : add test fixture #
Total comments: 11
Patch Set 5 : minor bug fix (perspective does not clear paint offset). switch test to unit test style. add a few … #
Total comments: 62
Patch Set 6 : address the feedback #
Total comments: 10
Patch Set 7 : break down walk function #Patch Set 8 : revert forAllFrameViews change in FrameView.cpp. I don't know what I was thinking. #Messages
Total messages: 46 (9 generated)
trchen@chromium.org changed reviewers: + jbroman@chromium.org, pdr@chromium.org
Just a starting point... Still lots of missing pieces.
https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/core.gypi (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/core.gypi:1971: 'paint/ObjectPaintProperties.cpp', this file is missing from this CL
some initial thoughts; I haven't reviewed the details of the positioning rules yet (aside: those might be easier to review in smaller separate CLs; they're fairly subtle) https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:34: class Owner { So this exists because we don't want to pay sizeof(void*) per DeprecatedPaintLayer, is that right? Do we expect that cost to be large? https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:38: void setObjectPaintProperties(PassOwnPtr<ObjectPaintProperties>); It looks like this is only used for clearing. If so, perhaps "clearObjectPaintProperties" or "resetObjectPaintProperties" would be clearer? https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:51: void setTransformNodeForSelf(PassRefPtr<TransformPaintPropertyNode> node) { m_transformNodeForSelf = node; } Any reason to make this more abstract? For DeprecatedPaintLayer, we know exactly what kinds of transform are possible, a priori. At a minimum, please add comments explaining the difference between "transform node for self" and "transform node for descendants", and how they relate to the transform effects that exist. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:134: matrix.translate(-origin.x(), -origin.y()); TransformationMatrix::applyTransformOrigin https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:15: class PaintPropertyTreeBuilder { Will this object have state in the future? If not, this could just be a function and the rest of these function declarations wouldn't need to be exposed in the header. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:22: struct WalkContextSVG; fwd declaring nonexistent struct https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:24: TransformPaintPropertyNode(const TransformationMatrix& matrix, const FloatPoint3D& origin, TransformPaintPropertyNode* parent = nullptr) Why this change? PassRefPtr<T> is implicitly constructible from T* (so this doesn't make callers work that otherwise wouldn't), but this change precludes passing existing ownership if desired, and makes it less clear that this constructor owns a reference.
chrishtr@chromium.org changed reviewers: + chrishtr@chromium.org
https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:183: class CORE_EXPORT LayoutObject : public ImageResourceClient, public ObjectPaintProperties::Owner { We went to great lengths to avoid inheriting from DisplayItemClient, to avoid making layout objects bigger and have more virtual function calls. Does that reasoning apply in this case or not?
pdr@chromium.org changed reviewers: - chrishtr@chromium.org
I like the direction this is going, but we really need to add tests :) https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:34: class Owner { I like your approach of putting these functions on LayoutObject directly, since that's our final goal. I think it would be best to not use inheritance and just put these functions directly on LayoutObject with appropriate documentation, and then have them forward to similar functions on PaintLayer. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:42: typedef HashMap<const ObjectPaintProperties::Owner*, OwnPtr<ObjectPaintProperties>> NodeMapping; For now, I think we can increase the size of PaintLayer by one pointer and avoid this complexity since there are only O(thousands) of PLs on typical pages. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:51: void setTransformNodeForSelf(PassRefPtr<TransformPaintPropertyNode> node) { m_transformNodeForSelf = node; } On 2015/10/14 at 15:11:58, jbroman wrote: > Any reason to make this more abstract? For DeprecatedPaintLayer, we know exactly what kinds of transform are possible, a priori. At a minimum, please add comments explaining the difference between "transform node for self" and "transform node for descendants", and how they relate to the transform effects that exist. I think the goal was to avoid leaking paint information outside of layout & paint, but I agree with Jeremy that we already do this during chunking so I don't think it's required here. The chunking step will convert css perspective / css transform to just vanilla "transform" in PaintChunk. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayer.h (left): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayer.h:609: // Paint properties encode the hierarchical transform, clip, scroll, and Per the discussion above, I think we should keep these and have LayoutObject's functions call them. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:17: struct PaintPropertyTreeBuilder::WalkContext { CurrentPropertyTreeContext? https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:22: struct PaintPropertyTreeBuilder::WalkContextHTML : public WalkContext { Does this need to be so abstract? What other subclasses will there be? Also, isn't this the LayoutTree and not HTML/DOM tree?
https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:115: bool hasTransform = object.isBox() && style.hasTransform(); I think it's important to unblock some work that depends on property trees working. What do you think about just implementing transform, the most basic support, and then adding fixed/sticky/perspective/etc in followup patches?
https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/core.gypi (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/core.gypi:1971: 'paint/ObjectPaintProperties.cpp', On 2015/10/14 14:41:47, jbroman wrote: > this file is missing from this CL Oops, git added. Mostly boilerplate though. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:183: class CORE_EXPORT LayoutObject : public ImageResourceClient, public ObjectPaintProperties::Owner { On 2015/10/14 17:30:28, chrishtr wrote: > We went to great lengths to avoid inheriting from DisplayItemClient, to avoid > making layout objects bigger and have more virtual function calls. Does that > reasoning apply in this case or not? This guy has no virtual functions, it is more like a mix-in class than an interface. I checked with C++ spec again, an empty class takes 1 byte when used as a leaf class, but may take 0 byte when inherited. (LayoutObject size check should catch this if not 0.) https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:34: class Owner { On 2015/10/14 15:11:58, jbroman wrote: > So this exists because we don't want to pay sizeof(void*) per > DeprecatedPaintLayer, is that right? Do we expect that cost to be large? This is because LayoutSVGModelObject can't have layer, but they still create transforms. Also sometimes we create layer when it is not necessary. In other word, DPL tree topology isn't really compatible with the property trees we want. Also I'm thinking to reduce DPL dependency as much as possible. The result will be something very similar to DPL but much lighter. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:38: void setObjectPaintProperties(PassOwnPtr<ObjectPaintProperties>); On 2015/10/14 15:11:58, jbroman wrote: > It looks like this is only used for clearing. If so, perhaps > "clearObjectPaintProperties" or "resetObjectPaintProperties" would be clearer? Makes sense. Done. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:42: typedef HashMap<const ObjectPaintProperties::Owner*, OwnPtr<ObjectPaintProperties>> NodeMapping; On 2015/10/14 17:52:06, pdr wrote: > For now, I think we can increase the size of PaintLayer by one pointer and avoid > this complexity since there are only O(thousands) of PLs on typical pages. See explanation above. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:51: void setTransformNodeForSelf(PassRefPtr<TransformPaintPropertyNode> node) { m_transformNodeForSelf = node; } On 2015/10/14 17:52:06, pdr wrote: > On 2015/10/14 at 15:11:58, jbroman wrote: > > Any reason to make this more abstract? For DeprecatedPaintLayer, we know > exactly what kinds of transform are possible, a priori. At a minimum, please add > comments explaining the difference between "transform node for self" and > "transform node for descendants", and how they relate to the transform effects > that exist. > > I think the goal was to avoid leaking paint information outside of layout & > paint, but I agree with Jeremy that we already do this during chunking so I > don't think it's required here. The chunking step will convert css perspective / > css transform to just vanilla "transform" in PaintChunk. The real question is who will be using this data? During the paint walk, it will be the object painters who access this, and sure they have the knowledge to interpret the data. I'm thinking the merge algorithm will need this data to reconnect property nodes when it encounters a sub-sequence cache, and the merge algorithm won't have HTML/CSS specific knowledge. IMO we can do either way for now, and make necessary changes when we implement other components. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:17: struct PaintPropertyTreeBuilder::WalkContext { On 2015/10/14 17:52:07, pdr wrote: > CurrentPropertyTreeContext? I'm thinking just Context... Being a nested class told us enough information. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:22: struct PaintPropertyTreeBuilder::WalkContextHTML : public WalkContext { On 2015/10/14 17:52:07, pdr wrote: > Does this need to be so abstract? What other subclasses will there be? Also, > isn't this the LayoutTree and not HTML/DOM tree? I'm thinking of SVG. They are different because they don't carry paint offset around, and they don't have the concept of out-of-flow position container. Anyway I think you are right, at this moment one class should suffice. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:115: bool hasTransform = object.isBox() && style.hasTransform(); On 2015/10/14 18:44:15, pdr wrote: > I think it's important to unblock some work that depends on property trees > working. What do you think about just implementing transform, the most basic > support, and then adding fixed/sticky/perspective/etc in followup patches? We need to keep track of paint offset in order to calculate combined transform from the last transform node. Anyway the work has been done. I do agree that the function is a bit long and needs to be splitted. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:134: matrix.translate(-origin.x(), -origin.y()); On 2015/10/14 15:11:58, jbroman wrote: > TransformationMatrix::applyTransformOrigin Done. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:15: class PaintPropertyTreeBuilder { On 2015/10/14 15:11:58, jbroman wrote: > Will this object have state in the future? If not, this could just be a function > and the rest of these function declarations wouldn't need to be exposed in the > header. I think it is likely we will add some state. Let's say if later we decide to go back to vector to allocate property nodes. Also it may be handy to save some global state, like Setting*, root local frame, or resource accounting stuff. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:22: struct WalkContextSVG; On 2015/10/14 15:11:58, jbroman wrote: > fwd declaring nonexistent struct As discussed below. Let's have just one struct. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:24: TransformPaintPropertyNode(const TransformationMatrix& matrix, const FloatPoint3D& origin, TransformPaintPropertyNode* parent = nullptr) On 2015/10/14 15:11:59, jbroman wrote: > Why this change? PassRefPtr<T> is implicitly constructible from T* (so this > doesn't make callers work that otherwise wouldn't), but this change precludes > passing existing ownership if desired, and makes it less clear that this > constructor owns a reference. You're right. I don't know what I was thinking. Reverted.
https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:51: void setTransformNodeForSelf(PassRefPtr<TransformPaintPropertyNode> node) { m_transformNodeForSelf = node; } On 2015/10/14 at 22:51:13, trchen wrote: > On 2015/10/14 17:52:06, pdr wrote: > > On 2015/10/14 at 15:11:58, jbroman wrote: > > > Any reason to make this more abstract? For DeprecatedPaintLayer, we know > > exactly what kinds of transform are possible, a priori. At a minimum, please add > > comments explaining the difference between "transform node for self" and > > "transform node for descendants", and how they relate to the transform effects > > that exist. > > > > I think the goal was to avoid leaking paint information outside of layout & > > paint, but I agree with Jeremy that we already do this during chunking so I > > don't think it's required here. The chunking step will convert css perspective / > > css transform to just vanilla "transform" in PaintChunk. > > The real question is who will be using this data? During the paint walk, it will be the object painters who access this, and sure they have the knowledge to interpret the data. > > I'm thinking the merge algorithm will need this data to reconnect property nodes when it encounters a sub-sequence cache, and the merge algorithm won't have HTML/CSS specific knowledge. > > IMO we can do either way for now, and make necessary changes when we implement other components. We're thinking in the same way wrt the subsequence problem :) To keep this patch small, lets leave these for now as perspective/transform and do this rename in the patch that solves the subsequence problem because we may do something slightly different. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:115: bool hasTransform = object.isBox() && style.hasTransform(); On 2015/10/14 at 22:51:13, trchen wrote: > On 2015/10/14 18:44:15, pdr wrote: > > I think it's important to unblock some work that depends on property trees > > working. What do you think about just implementing transform, the most basic > > support, and then adding fixed/sticky/perspective/etc in followup patches? > > We need to keep track of paint offset in order to calculate combined transform from the last transform node. Anyway the work has been done. > > I do agree that the function is a bit long and needs to be splitted. In order to land this, we will need a ridiculous number of unittests because we shouldn't land code that isn't well-tested. It'll be much easier to land just the most basic transform code with unittests and fixmes for all the rest of this complexity, then slowly build up support, adding unittests as you go. https://codereview.chromium.org/1407543003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1407543003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:30: // This is intended to be inherited by FrameView and LayoutObject to hold ownership to Would it be possible to use a simpler approach to handle FrameView where the LayoutView includes the FrameView's transform? It's very convenient to have a mapping from LayoutObject to PaintProperties. Just as a small example, if you have a Owner pointer in a debugger, there's not much you can do with that.
https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:51: void setTransformNodeForSelf(PassRefPtr<TransformPaintPropertyNode> node) { m_transformNodeForSelf = node; } This class is in core/; the merge algorithm, which lives in platform/, cannot see it.
https://codereview.chromium.org/1407543003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1407543003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:30: // This is intended to be inherited by FrameView and LayoutObject to hold ownership to On 2015/10/15 at 04:53:52, pdr wrote: > Would it be possible to use a simpler approach to handle FrameView where the LayoutView includes the FrameView's transform? > > It's very convenient to have a mapping from LayoutObject to PaintProperties. Just as a small example, if you have a Owner pointer in a debugger, there's not much you can do with that. Talked with Tien-Ren about this and wanted to brain dump our findings. There will only ever be one property node for the combination of frameview and layoutview. In root-layer-scrolls mode (the future), layoutview holds the scroll node. In non-root-layer-scrolls mode (going away), the frameview would hold the scroll node. Tien-Ren is going to pursue this patch assuming root-layer-scrolls so only LayoutObjects have paint properties, and he's going to put a TODO about handling non-root-layer-scrolling.
https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:51: void setTransformNodeForSelf(PassRefPtr<TransformPaintPropertyNode> node) { m_transformNodeForSelf = node; } On 2015/10/15 04:53:51, pdr wrote: > On 2015/10/14 at 22:51:13, trchen wrote: > > On 2015/10/14 17:52:06, pdr wrote: > > > On 2015/10/14 at 15:11:58, jbroman wrote: > > > > Any reason to make this more abstract? For DeprecatedPaintLayer, we know > > > exactly what kinds of transform are possible, a priori. At a minimum, please > add > > > comments explaining the difference between "transform node for self" and > > > "transform node for descendants", and how they relate to the transform > effects > > > that exist. > > > > > > I think the goal was to avoid leaking paint information outside of layout & > > > paint, but I agree with Jeremy that we already do this during chunking so I > > > don't think it's required here. The chunking step will convert css > perspective / > > > css transform to just vanilla "transform" in PaintChunk. > > > > The real question is who will be using this data? During the paint walk, it > will be the object painters who access this, and sure they have the knowledge to > interpret the data. > > > > I'm thinking the merge algorithm will need this data to reconnect property > nodes when it encounters a sub-sequence cache, and the merge algorithm won't > have HTML/CSS specific knowledge. > > > > IMO we can do either way for now, and make necessary changes when we implement > other components. > > We're thinking in the same way wrt the subsequence problem :) To keep this patch > small, lets leave these for now as perspective/transform and do this rename in > the patch that solves the subsequence problem because we may do something > slightly different. Done. https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1407543003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:115: bool hasTransform = object.isBox() && style.hasTransform(); On 2015/10/15 04:53:52, pdr wrote: > On 2015/10/14 at 22:51:13, trchen wrote: > > On 2015/10/14 18:44:15, pdr wrote: > > > I think it's important to unblock some work that depends on property trees > > > working. What do you think about just implementing transform, the most basic > > > support, and then adding fixed/sticky/perspective/etc in followup patches? > > > > We need to keep track of paint offset in order to calculate combined transform > from the last transform node. Anyway the work has been done. > > > > I do agree that the function is a bit long and needs to be splitted. > > In order to land this, we will need a ridiculous number of unittests because we > shouldn't land code that isn't well-tested. It'll be much easier to land just > the most basic transform code with unittests and fixmes for all the rest of this > complexity, then slowly build up support, adding unittests as you go. IMO without paint offset handling the code will be pretty much useless. We won't get the correct result with even a simple <div style="margin-top:100px; transform:translateZ(0)">. Let's just add tests instead. https://codereview.chromium.org/1407543003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1407543003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:30: // This is intended to be inherited by FrameView and LayoutObject to hold ownership to On 2015/10/15 22:05:59, pdr wrote: > On 2015/10/15 at 04:53:52, pdr wrote: > > Would it be possible to use a simpler approach to handle FrameView where the > LayoutView includes the FrameView's transform? > > > > It's very convenient to have a mapping from LayoutObject to PaintProperties. > Just as a small example, if you have a Owner pointer in a debugger, there's not > much you can do with that. > > Talked with Tien-Ren about this and wanted to brain dump our findings. There > will only ever be one property node for the combination of frameview and > layoutview. In root-layer-scrolls mode (the future), layoutview holds the scroll > node. In non-root-layer-scrolls mode (going away), the frameview would hold the > scroll node. Tien-Ren is going to pursue this patch assuming root-layer-scrolls > so only LayoutObjects have paint properties, and he's going to put a TODO about > handling non-root-layer-scrolling. Nah it's pretty simple to support. Also we always need to create a transform node to establish the viewport anyway. No big deal to add another node for frame scrolling.
https://codereview.chromium.org/1407543003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1407543003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:152: typedef HashMap<const LayoutObject*, OwnPtr<ObjectPaintProperties>> ObjectPaintPropertiesMap; Please doc what this is for. https://codereview.chromium.org/1407543003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1407543003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:59: RefPtr<TransformPaintPropertyNode> m_scrollTranslation; I really recommend we leave all overflow:scroll, sticky, and fixedpos issues for followups. Without scroll nodes these will be hard to really test, and they add unnecessary complexity at this stage.
Hey I added a test fixture that is similar to layout test. Will add more tests if the concept looks good to you.
https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp (right): https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp:62: m_pageHolder = nullptr; Why is this necessary? Doesn't this already happen during ~RenderingTest? https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/paint_property_tree_builder_tests/fixed-position.html (right): https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/paint_property_tree_builder_tests/fixed-position.html:1: <style> If you do this (I'll defer to pdr), please add these to webkit_unit_tests.isolate so that they work on Android (and swarming, if webkit_unit_tests runs in swarming).
https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/paint_property_tree_builder_tests/fixed-position.html-expected.txt (right): https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/paint_property_tree_builder_tests/fixed-position.html-expected.txt:1: === Layout Tree === I think this approach will be good long-term for existing layouttests but WDYT about switching this (and all new tests) to working more like PaintControllerPaintTestForSlimmingPaintV2 where we explicitly assert the properties of the tree?
https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:21: class DumpContext { nit: This should be in an anonymous namespace, to make it more obvious that this isn't intended to be exported, and to prevent it from colliding with another class named DumpContext elsewhere. https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:81: for (auto child: transformTreeIndexToChildrenList[index]) nit: style dictates a space on both sides of the period (here and elsewhere). Also: it's very non-obvious to me what the type of "child" is, and it took me quite awhile to look through the two type definitions to find out. I'd prefer it if this were "const TransformPaintPropertyNode*", or at a minimum "const auto*" so that I know it's a const pointer.
On 2015/10/16 at 17:31:55, jbroman wrote: > https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): > > https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:21: class DumpContext { > nit: This should be in an anonymous namespace, to make it more obvious that this isn't intended to be exported, and to prevent it from colliding with another class named DumpContext elsewhere. > > https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:81: for (auto child: transformTreeIndexToChildrenList[index]) > nit: style dictates a space on both sides of the period (here and elsewhere). > > Also: it's very non-obvious to me what the type of "child" is, and it took me quite awhile to look through the two type definitions to find out. I'd prefer it if this were "const TransformPaintPropertyNode*", or at a minimum "const auto*" so that I know it's a const pointer.
https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:81: for (auto child: transformTreeIndexToChildrenList[index]) On 2015/10/16 at 17:31:55, jbroman wrote: > nit: style dictates a space on both sides of the period (here and elsewhere). By period I mean colon.
https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp (right): https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp:62: m_pageHolder = nullptr; On 2015/10/16 14:46:51, jbroman wrote: > Why is this necessary? Doesn't this already happen during ~RenderingTest? Ordering issue. We toggle RuntimeEnabledFeatures back during ::TearDown, which happens before the destructor. Then LayoutObject destructor freaks out because it sees itself owning paint properties without SPv2 enabled. https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:21: class DumpContext { On 2015/10/16 17:31:55, jbroman wrote: > nit: This should be in an anonymous namespace, to make it more obvious that this > isn't intended to be exported, and to prevent it from colliding with another > class named DumpContext elsewhere. WontFix (obsolete) https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:81: for (auto child: transformTreeIndexToChildrenList[index]) On 2015/10/19 15:12:45, jbroman wrote: > On 2015/10/16 at 17:31:55, jbroman wrote: > > nit: style dictates a space on both sides of the period (here and elsewhere). > > By period I mean colon. WontFix (obsolete) https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/paint_property_tree_builder_tests/fixed-position.html (right): https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/paint_property_tree_builder_tests/fixed-position.html:1: <style> On 2015/10/16 14:46:51, jbroman wrote: > If you do this (I'll defer to pdr), please add these to > webkit_unit_tests.isolate so that they work on Android (and swarming, if > webkit_unit_tests runs in swarming). WontFix (obsolete) https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/paint_property_tree_builder_tests/fixed-position.html-expected.txt (right): https://codereview.chromium.org/1407543003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/paint_property_tree_builder_tests/fixed-position.html-expected.txt:1: === Layout Tree === On 2015/10/16 16:38:46, pdr wrote: > I think this approach will be good long-term for existing layouttests but WDYT > about switching this (and all new tests) to working more like > PaintControllerPaintTestForSlimmingPaintV2 where we explicitly assert the > properties of the tree? Done.
https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:154: To avoid having to create this map in ensureObjectPaintProperties, lets add a static here: // The storage for paint properties is temporarily implemented as a static // map to avoid a memory regression if this were on LayoutObject. This is // only used in slimming paint v2. typedef HashMap<const LayoutObject*, OwnPtr<ObjectPaintProperties>> ObjectPaintPropertiesMap; static ObjectPaintPropertiesMap& objectPaintPropertiesMap() { DEFINE_STATIC_LOCAL(ObjectPaintPropertiesMap, staticObjectPaintPropertiesMap, ()); return staticObjectPaintPropertiesMap; } https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:2520: if (objectPaintPropertiesMap) { We should check for the property here instead of the map: if (objectPaintProperties()) { ... } May need to update objectPaintProperties like so: ObjectPaintProperties* LayoutObject::objectPaintProperties() const { if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) return objectPaintPropertiesMap().get(this); return nullptr; } https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.h:357: // For SPv2 only. These are implemented as a global hash map temporarily, This is useful but more of an implementation detail. Can you move this comment to LayoutObject.cpp above the ObjectPaintPropertiesMap typedef? For readers that hit this, lets use something like the following comment here: // Paint properties encode the hierarchical transform, clip, scroll, and // effect information used for painting. Properties should only be changed // during UpdatePaintProperties, and should only be used during Paint. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp:46: void RenderingTest::TearDown() Is this necessary? It should happen automatically as part of the regular test runner lifecycle (either in SetUp or in the dtor). https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:9: #include "wtf/HashMap.h" These three extra includes aren't necessary. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:56: RefPtr<TransformPaintPropertyNode> m_preTranslation; (bikeshed) m_paintOffsetTranslation may be easier to understand. (similarly in FrameView.h) https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:17: struct PaintPropertyTreeBuilder::Context { Lets doc this with a small comment describing why it's necessary. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:38: RefPtr<TransformPaintPropertyNode> newTransformNodeForPreTranslation; In this function only, lets move these decls down to where they are used. i.e., RefPtr<TranformPaintPropertyNode> newTransformNodeForPreTranslation = TransformPaintPropertyNode::create(frameTranslate, FloatPoint3D(), context.currentTransform); https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:80: return FloatPoint3D(floatValueForLength(style.transformOriginX(), borderBoxSize.width()), floatValueForLength(style.transformOriginY(), borderBoxSize.height()), style.transformOriginZ()); Can you break this line so it's a little easier to read? return FloatPoint3D( floatValueForLength(style.transformOriginX(), borderBoxSize.width()), floatValueForLength(style.transformOriginY(), borderBoxSize.height()), style.transformOriginZ()); (similarly for perspectiveOrigin below). https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:93: ASSERT(object.isBox() != object.isLayoutInline()); // Either or. There are many checks for isBox() in this function. In what cases would we need to support layout inlines at all? Can you add a test for inlines? https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:136: // Step 3: Create a transform node for CSS transform (if presents). Nit: "if presents" -> "if present" (here, and below). https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:191: if (object.isSVGRoot()) { Nit: // TODO(trchen): Implement the SVG transform walk. if (object.isSVGRoot()) return; https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:196: for (LayoutObject* child = object.slowFirstChild(); child; child = child->nextSibling()) { Lets move the recur comment here. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:12: class LayoutObject; "class LayoutObject" and "struct PaintPropertyTreeNode" aren't needed. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:17: void buildPropertyTrees(FrameView& rootFrame); WDYT about making all of these functions const since they only modify their in param? https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:22: void walk(FrameView&, const Context&); Can you doc which tree traversal order is used in this walk? (layout object tree order, not paint order). https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:28: std::string fullPath(Platform::current()->unitTestSupport()->webKitRootDir().utf8().data()); This can be simplified a bit to avoid so many conversions (here, and elsewhere in this file). String fullPath(Platform::current()->unitTestSupport()->webKitRootDir()); fullPath.append("/Source/core/paint/test_data/"); fullPath.append(fileName); WebData inputBuffer = Platform::current()->unitTestSupport()->readFromFile(fullPath); setBodyInnerHTML(String(inputBuffer.data(), inputBuffer.size())); https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:60: FrameView* frameView = document().view(); Lets move this down to where it's used. i.e., EXPECT_EQ(document().view()->preTranslation(), target1Properties->preTranslation()->parent()); https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:128: loadTestData("long-page.html"); This one is so simple it may be better to just inline it: setBodyInnerHTML("<style> body { height: 10000px; } </style>"); https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:181: FrameView* frameView = document().view(); Lets move this down to where it's used. i.e., EXPECT_EQ(document().view()->scrollTranslation(), perspectiveProperties->perspective()->parent()); https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:209: FrameView* frameView = document().view(); Lets move this down to where it's used. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:226: Nit: extra space. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:24: static PassRefPtr<TransformPaintPropertyNode> create(const TransformationMatrix& matrix, const FloatPoint3D& origin, PassRefPtr<TransformPaintPropertyNode> parent = nullptr) { return adoptRef(new TransformPaintPropertyNode(matrix, origin, parent)); } You'll need to update PaintChunkerTest and PaintArtifactToSkCanvasTest to use ::create too. It's a trivial change since they already call adoptRef you can just update them to use TransformPaintPropertyNode::create(...).
A few comments, but assuming the tests pass and pdr has had a closer look at the core logic than I have (it's almost 8pm here), looks good. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:2520: if (objectPaintPropertiesMap) { We could just do it unconditionally (well, perhaps with a REF check just to avoid the assert). No need to search the hashmap twice (once to see if it's there, and again to remove it). It should be safe to remove a non-existent element. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp:46: void RenderingTest::TearDown() On 2015/10/20 at 22:02:30, pdr wrote: > Is this necessary? It should happen automatically as part of the regular test runner lifecycle (either in SetUp or in the dtor). trchen answered this question when I asked it earlier, but if that ordering issue still exists, it merits a comment here, please, if only so we stop asking. :) https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:17: struct PaintPropertyTreeBuilder::Context { On 2015/10/20 at 22:02:30, pdr wrote: > Lets doc this with a small comment describing why it's necessary. I'd also like a brief explanation of how it works. It seems like this is propagated down the layout tree, but I'd like it to be explicit. A brief explanation of what "transformForOutOfFlowPositioned" means (I think it's the transform node that applies to absolute- and relative-positioned kids based on the description, but I'd have to find the uses to check that). https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:86: super-nit: remove this newline? https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:140: style.applyTransform(matrix, toLayoutBox(object).size(), ComputedStyle::ExcludeTransformOrigin, ComputedStyle::IncludeMotionPath, ComputedStyle::IncludeIndependentTransformProperties); nit: Mind wrapping some of these long lines? I know blink has no line limit, but 192 cols is pushing it. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:147: // Step 3: Create a transform node for CSS perspective (if presents). nit: There are two step 3s. I would probably remove the numbers as otherwise we'll have to renumber every time we add, remove or reorder steps. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:170: if (style.position() != StaticPosition || hasTransform) { Does the logic for what creates a containing block for absolute and fixed stuff exist elsewhere in a reusable way, or not? I'm afraid of this diverging from what other parts of the code assume. (If not, I guess we could punt that cleanup until later.) https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:15: class PaintPropertyTreeBuilder { Class comment please. At least a description that this class walks the layout tree in XYZ order and builds paint properties, etc. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:17: void buildPropertyTrees(FrameView& rootFrame); On 2015/10/20 at 22:02:31, pdr wrote: > WDYT about making all of these functions const since they only modify their in param? IMHO either these are (potentially) stateful objects (in which case it's appropriate for the methods to be non-const), or they aren't going to be stateful in the future (in which case it should be a function). https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:69: EXPECT_EQ(matrix, target1Properties->preTranslation()->matrix()); I think this could be written as: EXPECT_EQ(TransformationMatrix().translate(200, 150), target1Properties->preTranslation()->matrix()); Maybe a static method on TransformationMatrix would be easier to understand, but this should work because the temporary lives until the end of the full-expression. It just makes me a little sad that an equality check consumes 5 LOC.
https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:154: On 2015/10/20 22:02:30, pdr wrote: > To avoid having to create this map in ensureObjectPaintProperties, lets add a > static here: > > // The storage for paint properties is temporarily implemented as a static > // map to avoid a memory regression if this were on LayoutObject. This is > // only used in slimming paint v2. > typedef HashMap<const LayoutObject*, OwnPtr<ObjectPaintProperties>> > ObjectPaintPropertiesMap; > static ObjectPaintPropertiesMap& objectPaintPropertiesMap() > { > DEFINE_STATIC_LOCAL(ObjectPaintPropertiesMap, > staticObjectPaintPropertiesMap, ()); > return staticObjectPaintPropertiesMap; > } Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:2520: if (objectPaintPropertiesMap) { On 2015/10/20 23:50:13, jbroman wrote: > We could just do it unconditionally (well, perhaps with a REF check just to > avoid the assert). No need to search the hashmap twice (once to see if it's > there, and again to remove it). It should be safe to remove a non-existent > element. Done as jbroman's proposal. The check was to make sure there is no ObjectPaintProperties ever created in non-SPv2 mode, but I agree the check was extraneous. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.h:357: // For SPv2 only. These are implemented as a global hash map temporarily, On 2015/10/20 22:02:30, pdr wrote: > This is useful but more of an implementation detail. Can you move this comment > to LayoutObject.cpp above the ObjectPaintPropertiesMap typedef? > > For readers that hit this, lets use something like the following comment here: > // Paint properties encode the hierarchical transform, clip, scroll, and > // effect information used for painting. Properties should only be changed > // during UpdatePaintProperties, and should only be used during Paint. Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp:46: void RenderingTest::TearDown() On 2015/10/20 23:50:13, jbroman wrote: > On 2015/10/20 at 22:02:30, pdr wrote: > > Is this necessary? It should happen automatically as part of the regular test > runner lifecycle (either in SetUp or in the dtor). > > trchen answered this question when I asked it earlier, but if that ordering > issue still exists, it merits a comment here, please, if only so we stop asking. > :) Still needed otherwise either an assertion goes off or we left dangling mapping. Both sound terrible so let me add a comment. :) https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:9: #include "wtf/HashMap.h" On 2015/10/20 22:02:30, pdr wrote: > These three extra includes aren't necessary. Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:56: RefPtr<TransformPaintPropertyNode> m_preTranslation; On 2015/10/20 22:02:30, pdr wrote: > (bikeshed) m_paintOffsetTranslation may be easier to understand. > (similarly in FrameView.h) Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:17: struct PaintPropertyTreeBuilder::Context { On 2015/10/20 23:50:13, jbroman wrote: > On 2015/10/20 at 22:02:30, pdr wrote: > > Lets doc this with a small comment describing why it's necessary. > > I'd also like a brief explanation of how it works. It seems like this is > propagated down the layout tree, but I'd like it to be explicit. A brief > explanation of what "transformForOutOfFlowPositioned" means (I think it's the > transform node that applies to absolute- and relative-positioned kids based on > the description, but I'd have to find the uses to check that). Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:38: RefPtr<TransformPaintPropertyNode> newTransformNodeForPreTranslation; On 2015/10/20 22:02:30, pdr wrote: > In this function only, lets move these decls down to where they are used. > > i.e., > RefPtr<TranformPaintPropertyNode> newTransformNodeForPreTranslation = > TransformPaintPropertyNode::create(frameTranslate, FloatPoint3D(), > context.currentTransform); Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:80: return FloatPoint3D(floatValueForLength(style.transformOriginX(), borderBoxSize.width()), floatValueForLength(style.transformOriginY(), borderBoxSize.height()), style.transformOriginZ()); On 2015/10/20 22:02:30, pdr wrote: > Can you break this line so it's a little easier to read? > return FloatPoint3D( > floatValueForLength(style.transformOriginX(), borderBoxSize.width()), > floatValueForLength(style.transformOriginY(), borderBoxSize.height()), > style.transformOriginZ()); > > (similarly for perspectiveOrigin below). Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:86: On 2015/10/20 23:50:13, jbroman wrote: > super-nit: remove this newline? Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:93: ASSERT(object.isBox() != object.isLayoutInline()); // Either or. On 2015/10/20 22:02:30, pdr wrote: > There are many checks for isBox() in this function. In what cases would we need > to support layout inlines at all? Can you add a test for inlines? Yea... Many CSS properties don't apply on inlines. I was surprised to see transform is forced to none on inlines, and position:absolute forces inlines to become blocks. I think the only thing in effect is position:relative. Will add a test for that. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:136: // Step 3: Create a transform node for CSS transform (if presents). On 2015/10/20 22:02:30, pdr wrote: > Nit: "if presents" -> "if present" (here, and below). Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:140: style.applyTransform(matrix, toLayoutBox(object).size(), ComputedStyle::ExcludeTransformOrigin, ComputedStyle::IncludeMotionPath, ComputedStyle::IncludeIndependentTransformProperties); On 2015/10/20 23:50:13, jbroman wrote: > nit: Mind wrapping some of these long lines? I know blink has no line limit, but > 192 cols is pushing it. Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:147: // Step 3: Create a transform node for CSS perspective (if presents). On 2015/10/20 23:50:13, jbroman wrote: > nit: There are two step 3s. I would probably remove the numbers as otherwise > we'll have to renumber every time we add, remove or reorder steps. Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:170: if (style.position() != StaticPosition || hasTransform) { On 2015/10/20 23:50:13, jbroman wrote: > Does the logic for what creates a containing block for absolute and fixed stuff > exist elsewhere in a reusable way, or not? I'm afraid of this diverging from > what other parts of the code assume. > > (If not, I guess we could punt that cleanup until later.) Nope, any divergence is unintentional. shouldCreatePreTranslationNode() above invokes PaintLayer counterpart to ensure we stay in consistent with PaintLayer. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:191: if (object.isSVGRoot()) { On 2015/10/20 22:02:30, pdr wrote: > Nit: > // TODO(trchen): Implement the SVG transform walk. > if (object.isSVGRoot()) > return; Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:196: for (LayoutObject* child = object.slowFirstChild(); child; child = child->nextSibling()) { On 2015/10/20 22:02:30, pdr wrote: > Lets move the recur comment here. Nah, for SVG walk it is going to recur too. Just that we can't assume the children to be LayoutBoxModelObject, and need to do some adjustment to the context. Other things need to be added here as well, for example LayoutFrame needs to walk the sub-frame. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:12: class LayoutObject; On 2015/10/20 22:02:31, pdr wrote: > "class LayoutObject" and "struct PaintPropertyTreeNode" aren't needed. Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:15: class PaintPropertyTreeBuilder { On 2015/10/20 23:50:13, jbroman wrote: > Class comment please. At least a description that this class walks the layout > tree in XYZ order and builds paint properties, etc. I consider the walk ordering being implementation details. Let's add comments in .cpp. But indeed, it worth mention that it walks the whole layout tree, from the root frame, across frame boundaries (not implemented yet, but will be). https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:17: void buildPropertyTrees(FrameView& rootFrame); On 2015/10/20 23:50:13, jbroman wrote: > On 2015/10/20 at 22:02:31, pdr wrote: > > WDYT about making all of these functions const since they only modify their in > param? > > IMHO either these are (potentially) stateful objects (in which case it's > appropriate for the methods to be non-const), or they aren't going to be > stateful in the future (in which case it should be a function). Yep, I feel that it is very likely to become stateful. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:22: void walk(FrameView&, const Context&); On 2015/10/20 22:02:31, pdr wrote: > Can you doc which tree traversal order is used in this walk? (layout object tree > order, not paint order). Ditto. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:28: std::string fullPath(Platform::current()->unitTestSupport()->webKitRootDir().utf8().data()); On 2015/10/20 22:02:31, pdr wrote: > This can be simplified a bit to avoid so many conversions (here, and elsewhere > in this file). > > String fullPath(Platform::current()->unitTestSupport()->webKitRootDir()); > fullPath.append("/Source/core/paint/test_data/"); > fullPath.append(fileName); > WebData inputBuffer = > Platform::current()->unitTestSupport()->readFromFile(fullPath); > setBodyInnerHTML(String(inputBuffer.data(), inputBuffer.size())); Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:60: FrameView* frameView = document().view(); On 2015/10/20 22:02:31, pdr wrote: > Lets move this down to where it's used. > > i.e., > EXPECT_EQ(document().view()->preTranslation(), > target1Properties->preTranslation()->parent()); Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:69: EXPECT_EQ(matrix, target1Properties->preTranslation()->matrix()); On 2015/10/20 23:50:13, jbroman wrote: > I think this could be written as: > > EXPECT_EQ(TransformationMatrix().translate(200, 150), > target1Properties->preTranslation()->matrix()); > > Maybe a static method on TransformationMatrix would be easier to understand, but > this should work because the temporary lives until the end of the > full-expression. > > It just makes me a little sad that an equality check consumes 5 LOC. Cool! Didn't know that matrix operations can be chained. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:128: loadTestData("long-page.html"); On 2015/10/20 22:02:31, pdr wrote: > This one is so simple it may be better to just inline it: > setBodyInnerHTML("<style> body { height: 10000px; } </style>"); Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:181: FrameView* frameView = document().view(); On 2015/10/20 22:02:31, pdr wrote: > Lets move this down to where it's used. > > i.e., > EXPECT_EQ(document().view()->scrollTranslation(), > perspectiveProperties->perspective()->parent()); Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:209: FrameView* frameView = document().view(); On 2015/10/20 22:02:31, pdr wrote: > Lets move this down to where it's used. Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:226: On 2015/10/20 22:02:31, pdr wrote: > Nit: extra space. Done. https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h (right): https://codereview.chromium.org/1407543003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:24: static PassRefPtr<TransformPaintPropertyNode> create(const TransformationMatrix& matrix, const FloatPoint3D& origin, PassRefPtr<TransformPaintPropertyNode> parent = nullptr) { return adoptRef(new TransformPaintPropertyNode(matrix, origin, parent)); } On 2015/10/20 22:02:31, pdr wrote: > You'll need to update PaintChunkerTest and PaintArtifactToSkCanvasTest to use > ::create too. It's a trivial change since they already call adoptRef you can > just update them to use TransformPaintPropertyNode::create(...). Done.
Please also fix the test failures (we probably should be running *_chromium_rel rather than *_blink_rel nowadays, but I expect the failures are the same). I'm pretty happy with the shape of this patch, though. https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:67: EXPECT_EQ(matrix, target1Properties->paintOffsetTranslation()->matrix()); nit: missed one of the TransformationMatrix things https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/test_data/fixed-position.html (right): https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/test_data/fixed-position.html:1: <style> The test data files still need to be added to webkit_unit_tests.isolate so that they work with Android and swarming.
https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/test_data/fixed-position.html (right): https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/test_data/fixed-position.html:1: <style> On 2015/10/21 at 15:33:35, jbroman wrote: > The test data files still need to be added to webkit_unit_tests.isolate so that they work with Android and swarming. And/or WebKitUnitTests.isolate. I have no idea why there are two.
chrishtr@chromium.org changed reviewers: + chrishtr@chromium.org
https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:105: void PaintPropertyTreeBuilder::walk(LayoutBoxModelObject& object, const Context& context) This function is too long.
https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:105: void PaintPropertyTreeBuilder::walk(LayoutBoxModelObject& object, const Context& context) On 2015/10/21 at 17:15:19, chrishtr wrote: > This function is too long. @trchen, what do you think about breaking each step into a function?
With a fix for Chris's comment and two small nits, I think we can land this :) https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:158: staticObjectPaintPropertiesMap, ()); Oops, I think this fell off the line before it.
https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:158: staticObjectPaintPropertiesMap, ()); On 2015/10/21 21:02:45, pdr wrote: > Oops, I think this fell off the line before it. Done. https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:105: void PaintPropertyTreeBuilder::walk(LayoutBoxModelObject& object, const Context& context) On 2015/10/21 17:18:02, pdr wrote: > On 2015/10/21 at 17:15:19, chrishtr wrote: > > This function is too long. > > @trchen, what do you think about breaking each step into a function? Sure! I was thinking about it so we can use the function name to describe each of the step instead of using comments. https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:67: EXPECT_EQ(matrix, target1Properties->paintOffsetTranslation()->matrix()); On 2015/10/21 15:33:35, jbroman wrote: > nit: missed one of the TransformationMatrix things Done. https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/test_data/fixed-position.html (right): https://codereview.chromium.org/1407543003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/test_data/fixed-position.html:1: <style> On 2015/10/21 16:18:20, jbroman wrote: > On 2015/10/21 at 15:33:35, jbroman wrote: > > The test data files still need to be added to webkit_unit_tests.isolate so > that they work with Android and swarming. > > And/or WebKitUnitTests.isolate. I have no idea why there are two. Done. My guess is that because on Android the package is named WebKitUnitTest.apk?
LGTM Please wait for one additional LGTM from jbroman or chrishtr.
No need to block on me. I see my comment was addressed, thanks!
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407543003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407543003/120001
lgtm
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407543003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407543003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by trchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org, jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/1407543003/#ps140001 (title: "revert forAllFrameViews change in FrameView.cpp. I don't know what I was thinking.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407543003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407543003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6e5dafd02a193d3f180923a1255b487de127879f Cr-Commit-Position: refs/heads/master@{#355488} |
