WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
111176
[CSS Regions] position: fixed is computed relative to the first region, not the viewport
https://bugs.webkit.org/show_bug.cgi?id=111176
Summary
[CSS Regions] position: fixed is computed relative to the first region, not t...
Mihai Balan
Reported
2013-03-01 06:41:35 PST
The CSS Regions spec specifies that nodes that have position: fixed should be positioned relative to the viewport, even when they're extracted in a named flow (see
http://www.w3.org/TR/css3-regions/#flow-into
). Currently, they are positioned relative to the first region in the region chain.
Attachments
Ref-test highlighting the problem
(1.36 KB, application/x-zip-compressed)
2013-03-01 06:47 PST
,
Mihai Balan
no flags
Details
WIP 1
(5.17 KB, application/octet-stream)
2013-06-11 09:57 PDT
,
Mihnea Ovidenie
no flags
Details
Patch
(32.46 KB, patch)
2013-07-04 08:48 PDT
,
Mihnea Ovidenie
achicu
: review-
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
(479.73 KB, application/zip)
2013-07-04 10:38 PDT
,
Build Bot
no flags
Details
Patch 2
(44.89 KB, patch)
2013-07-14 00:55 PDT
,
Mihnea Ovidenie
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
(626.69 KB, application/zip)
2013-07-14 04:12 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
(607.45 KB, application/zip)
2013-07-14 11:50 PDT
,
Build Bot
no flags
Details
Patch 3
(44.99 KB, patch)
2013-07-15 01:55 PDT
,
Mihnea Ovidenie
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
(562.38 KB, application/zip)
2013-07-15 02:42 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
(534.68 KB, application/zip)
2013-07-15 07:46 PDT
,
Build Bot
no flags
Details
WIP1 with alternate approach
(17.03 KB, patch)
2013-08-13 06:13 PDT
,
Mihnea Ovidenie
no flags
Details
Formatted Diff
Diff
WIP2 with alternate approach. Rebased and with tests.
(38.05 KB, patch)
2013-08-29 09:38 PDT
,
Mihnea Ovidenie
no flags
Details
Formatted Diff
Diff
Patch
(50.86 KB, patch)
2013-08-30 09:50 PDT
,
Mihnea Ovidenie
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
(456.96 KB, application/zip)
2013-08-30 13:38 PDT
,
Build Bot
no flags
Details
Patch for landing
(53.57 KB, patch)
2013-09-03 00:19 PDT
,
Mihnea Ovidenie
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Mihai Balan
Comment 1
2013-03-01 06:47:29 PST
Created
attachment 190957
[details]
Ref-test highlighting the problem
Mihnea Ovidenie
Comment 2
2013-06-11 09:57:36 PDT
Created
attachment 204339
[details]
WIP 1 First attempt with fixed elements not positioned by flow thread but by render view. The fixed elements are inserted into the list of flow thread content nodes.
Mihnea Ovidenie
Comment 3
2013-07-04 08:48:04 PDT
Created
attachment 206094
[details]
Patch
Build Bot
Comment 4
2013-07-04 10:38:53 PDT
Comment on
attachment 206094
[details]
Patch
Attachment 206094
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/992154
New failing tests: fast/regions/element-in-named-flow-fixed-from-static.html
Build Bot
Comment 5
2013-07-04 10:38:55 PDT
Created
attachment 206101
[details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Mihnea Ovidenie
Comment 6
2013-07-05 00:11:55 PDT
I have applied the patch on my ML machine and i was unable to reproduce the test failure.
Alexandru Chiculita
Comment 7
2013-07-08 05:05:43 PDT
Comment on
attachment 206094
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=206094&action=review
Looks good, I have a couple of questions below. Is there any other case that we need to patch in order to avoid the CSS Region rules for the content of the fixed elements. For example flowThreadState() == RenderObject::InsideFlowThread will be true even if the element is inside the fixed elements that are now collected by the RenderView. In that case you don't have any flow thread layout state on the stack and it might have undefined behavior.
> Source/WebCore/rendering/RenderBlock.cpp:7758 > + LayoutState* layoutState = view()->layoutState();
nit: this variable seems to be unused.
> Source/WebCore/rendering/RenderBox.cpp:4602 > if ((layoutState && !layoutState->isPaginated()) || (!layoutState && !flowThreadContainingBlock())) > + return false; > + return true;
not: there's no need for the if statement. write just one return statement.
> Source/WebCore/rendering/RenderLayer.cpp:1359 > + layer = layer->parent();
This is based on the fact that the RenderView is the parent of the RenderFlowThread. I think we need some asserts to catch these subtle assumptions. We may need to change that in the future and asserts will help finding such assumptions.
> Source/WebCore/rendering/RenderLayerCompositor.cpp:1787 > + && (layer->renderer()->flowThreadState() == RenderObject::NotInsideFlowThread || layer->renderer()->fixedPositionInNamedFlowWithViewContainingBlock());
What happens for the 3D elements nested in the "positioned: fixed" elements. I think that those layers should have compositing enabled too.
> Source/WebCore/rendering/RenderLayerModelObject.cpp:136 > + layer()->dirtyStackingContainerZOrderLists();
Should we also dirty the receiving stacking container? Where is that done? Can you add a comment to clarify it?
> Source/WebCore/rendering/RenderView.cpp:145 > flowThreadController()->layoutRenderNamedFlowThreads(); > + layoutPositionedObjects(false, true);
This doesn't work with nested flows. For example a fixed region inside a flow will stay without a layout.
Mihnea Ovidenie
Comment 8
2013-07-14 00:55:46 PDT
Created
attachment 206627
[details]
Patch 2
WebKit Commit Bot
Comment 9
2013-07-14 00:58:11 PDT
Attachment 206627
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/TestExpectations', u'LayoutTests/fast/regions/element-in-named-flow-absolute-from-fixed-expected.txt', u'LayoutTests/fast/regions/element-in-named-flow-absolute-from-fixed.html', u'LayoutTests/fast/regions/element-in-named-flow-fixed-from-absolute-expected.txt', u'LayoutTests/fast/regions/element-in-named-flow-fixed-from-absolute.html', u'LayoutTests/fast/regions/element-inflow-fixed-from-outflow-static-expected.txt', u'LayoutTests/fast/regions/element-inflow-fixed-from-outflow-static.html', u'LayoutTests/fast/regions/element-outflow-static-from-inflow-fixed-expected.txt', u'LayoutTests/fast/regions/element-outflow-static-from-inflow-fixed.html', u'LayoutTests/fast/regions/fixed-element-transformed-parent-expected.txt', u'LayoutTests/fast/regions/fixed-element-transformed-parent.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-namedflow-noregions-expected.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-namedflow-noregions.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-region-expected.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-region.html', u'LayoutTests/fast/regions/fixed-pos-region-in-nested-flow-expected.txt', u'LayoutTests/fast/regions/fixed-pos-region-in-nested-flow.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/NodeRenderingContext.cpp', u'Source/WebCore/rendering/FlowThreadController.cpp', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlock.h', u'Source/WebCore/rendering/RenderBox.cpp', u'Source/WebCore/rendering/RenderBox.h', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderLayerModelObject.cpp', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObject.h', u'Source/WebCore/rendering/RenderView.cpp', u'Source/WebCore/rendering/RenderView.h']" exit_code: 1 Source/WebCore/rendering/RenderObject.cpp:658: Missing space before ( in while( [whitespace/parens] [5] Source/WebCore/rendering/RenderBlock.h:511: The parameter name "namedFlow" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Mihnea Ovidenie
Comment 10
2013-07-14 01:09:12 PDT
(In reply to
comment #7
)
> (From update of
attachment 206094
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=206094&action=review
> > Looks good, I have a couple of questions below. > > Is there any other case that we need to patch in order to avoid the CSS Region rules for the content of the fixed elements. For example flowThreadState() == RenderObject::InsideFlowThread will be true even if the element is inside the fixed elements that are now collected by the RenderView. In that case you don't have any flow thread layout state on the stack and it might have undefined behavior. >
I took a look at the code and i was not able to spot such situation. The fixed positioned elements in this situation are always laid out after the flow thread has been laid out and they do not depend on flow thread state on the stack.
> > Source/WebCore/rendering/RenderBlock.cpp:7758 > > + LayoutState* layoutState = view()->layoutState(); > > nit: this variable seems to be unused. >
I moved it closer to the place where it is used.
> > Source/WebCore/rendering/RenderBox.cpp:4602 > > if ((layoutState && !layoutState->isPaginated()) || (!layoutState && !flowThreadContainingBlock())) > > + return false; > > + return true; > > not: there's no need for the if statement. write just one return statement. >
I simplified the function with this occasion.
> > Source/WebCore/rendering/RenderLayer.cpp:1359 > > + layer = layer->parent(); > > This is based on the fact that the RenderView is the parent of the RenderFlowThread. I think we need some asserts to catch these subtle assumptions. We may need to change that in the future and asserts will help finding such assumptions. >
Added an assert to check that the parent layer for the flow thread layer is the view layer.
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1787 > > + && (layer->renderer()->flowThreadState() == RenderObject::NotInsideFlowThread || layer->renderer()->fixedPositionInNamedFlowWithViewContainingBlock()); > > What happens for the 3D elements nested in the "positioned: fixed" elements. I think that those layers should have compositing enabled too.
> I lifted the restriction here for all elements inside a flow thread, except for the flow thread itself. I skipped the test fast/repaint/region-painting-composited-element.html with this occasion.
> > Source/WebCore/rendering/RenderLayerModelObject.cpp:136 > > + layer()->dirtyStackingContainerZOrderLists(); > > Should we also dirty the receiving stacking container? Where is that done? Can you add a comment to clarify it? >
Yes, i have done that and added a test for it.
> > Source/WebCore/rendering/RenderView.cpp:145 > > flowThreadController()->layoutRenderNamedFlowThreads(); > > + layoutPositionedObjects(false, true); > > This doesn't work with nested flows. For example a fixed region inside a flow will stay without a layout.
The fixed elements inside a flow thread that are to be laid out by view are laid out after their flow thread ancestor has been laid out, not in a bulk call after all the flow threads have been laid out.
Build Bot
Comment 11
2013-07-14 04:12:21 PDT
Comment on
attachment 206627
[details]
Patch 2
Attachment 206627
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/1066522
New failing tests: compositing/regions/webkit-flow-renderer-layer-compositing.html
Build Bot
Comment 12
2013-07-14 04:12:25 PDT
Created
attachment 206630
[details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Build Bot
Comment 13
2013-07-14 11:50:55 PDT
Comment on
attachment 206627
[details]
Patch 2
Attachment 206627
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/1063671
New failing tests: fast/multicol/mixed-positioning-stacking-order.html fast/regions/element-in-named-flow-fixed-from-absolute.html fullscreen/full-screen-iframe-with-max-width-height.html fast/regions/element-outflow-static-from-inflow-fixed.html fast/regions/element-inflow-fixed-from-outflow-static.html fast/multicol/mixed-opacity-fixed-test.html compositing/regions/webkit-flow-renderer-layer-compositing.html fast/regions/element-in-named-flow-absolute-from-fixed.html
Build Bot
Comment 14
2013-07-14 11:50:58 PDT
Created
attachment 206634
[details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Mihnea Ovidenie
Comment 15
2013-07-15 01:55:39 PDT
Created
attachment 206646
[details]
Patch 3 Correct style and skip another regions compositing test.
Build Bot
Comment 16
2013-07-15 02:42:33 PDT
Comment on
attachment 206646
[details]
Patch 3
Attachment 206646
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/1068483
New failing tests: fullscreen/full-screen-iframe-with-max-width-height.html
Build Bot
Comment 17
2013-07-15 02:42:36 PDT
Created
attachment 206650
[details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Build Bot
Comment 18
2013-07-15 07:46:00 PDT
Comment on
attachment 206646
[details]
Patch 3
Attachment 206646
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/1077085
New failing tests: fast/multicol/mixed-positioning-stacking-order.html fast/regions/element-in-named-flow-fixed-from-absolute.html fast/regions/element-outflow-static-from-inflow-fixed.html fast/regions/element-inflow-fixed-from-outflow-static.html fast/multicol/mixed-opacity-fixed-test.html http/tests/security/cross-origin-plugin-private-browsing-toggled.html fast/regions/element-in-named-flow-absolute-from-fixed.html
Build Bot
Comment 19
2013-07-15 07:46:02 PDT
Created
attachment 206663
[details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Dave Hyatt
Comment 20
2013-07-15 10:10:30 PDT
Comment on
attachment 206646
[details]
Patch 3 View in context:
https://bugs.webkit.org/attachment.cgi?id=206646&action=review
r- In general, I don't like what's happening here, but it's a spec disagreement. I would rather fixed positioned objects just not be placed inside the named flow thread at all. I don't see any value whatsoever in allowing them to be inside the flow thread. If instead they were just outside the named flow thread completely, then all of this code would get way simpler. Is there any compelling use case for fixed positioned objects being inside the flow threads? Anyway, the rest of the comments are assuming the spec stays the way it is, so here goes: Don't you need to patch RenderLayer::accumulateOffsetTowardsAncestor also?
> Source/WebCore/rendering/FlowThreadController.cpp:105 > flowRenderer->layoutIfNeeded(); > + flowRenderer->view()->layoutNamedFlowFixedPositionedObjects(flowRenderer);
I would make a member function on RenderNamedFlowThread that handles doing both of these operations. layoutNormalFlowAndFixedPositionedObjects.
> Source/WebCore/rendering/RenderBlock.cpp:2878 > + if (namedFlow) { > + ASSERT(this->isRenderView()); > + if (namedFlow != r->flowThreadAncestorForFixedPositionedObjects()) > + continue; > + }
I don't like this code in RenderBlock when it's view-specific. Also, you put it in front of comment that describes code that comes after the code you put in, so you just disconnected the comment from some code it applied to. I would suggest doing something differently here. I think you should refactor layoutPositionedObjects so that the work it does for each object inside the loop is in a helper function, e.g., layoutPositionedObject(). Then you could just have a complete implementation of laying out positioned objects over in layoutNamedFlowPositionedObjects that has your named flow filtering check and that then just calls layoutPositionedObject() to do the rest of the work. That keeps you from having to touch this RenderBlock code.
> Source/WebCore/rendering/RenderBlock.h:511 > - void layoutPositionedObjects(bool relayoutChildren, bool fixedPositionObjectsOnly = false); > + void layoutPositionedObjects(bool relayoutChildren, bool fixedPositionObjectsOnly = false, const RenderNamedFlowThread* = 0);
Shouldn't have to add this, as per my comment above.
> Source/WebCore/rendering/RenderLayerCompositor.cpp:1787 > + && !layer->isOutOfFlowRenderFlowThread();
These aren't equivalent. You just changed behavior for in-flow RenderFlowThreads.
> Source/WebCore/rendering/RenderLayerModelObject.cpp:136 > + if (flowThreadState() == RenderObject::InsideOutOfFlowThread) { > + bool wasFixedPositionedInFlowThread = oldStyle->position() == FixedPosition; > + bool isFixedPositionedInFlowThread = newStyle->position() == FixedPosition; > + if (wasFixedPositionedInFlowThread != isFixedPositionedInFlowThread) > + layer()->dirtyStackingContainerZOrderLists();
Helper function please.
> Source/WebCore/rendering/RenderLayerModelObject.cpp:182 > + if (flowThreadState() == RenderObject::InsideOutOfFlowThread) { > + bool wasFixedPositionedInFlowThread = oldStyle && oldStyle->position() == FixedPosition; > + bool isFixedPositionedInFlowThread = style()->position() == FixedPosition; > + if (wasFixedPositionedInFlowThread != isFixedPositionedInFlowThread) > + layer()->dirtyStackingContainerZOrderLists();
Helper function please, since this is identical to the code above.
> Source/WebCore/rendering/RenderObject.cpp:631 > +bool RenderObject::fixedPositionInNamedFlowWithViewContainingBlock() const > +{ > + return (flowThreadState() == RenderObject::InsideOutOfFlowThread) > + && (style()->position() == FixedPosition) > + && (containingBlock() == view()); > +}
This is only used by RenderLayer. I'd just put it over there rather than adding new code to RenderObject.
> Source/WebCore/rendering/RenderObject.cpp:665 > +RenderFlowThread* RenderObject::flowThreadAncestorForFixedPositionedObjects() const > +{ > + if ((flowThreadState() != InsideOutOfFlowThread) || (style()->position() != FixedPosition)) > + return 0; > + > + RenderObject* curr = const_cast<RenderObject*>(this); > + while (curr) { > + if (curr->isRenderFlowThread()) > + return toRenderFlowThread(curr); > + curr = curr->parent(); > + } > + > + return 0; > +}
I would factor this a little differently. I would have a RenderObject function that does only the while loop. Make flowThreadContainingBlock() also use it for the slow case. Then RenderNamedFlowThread could do the flowThreadState() and fixedPosition condition checks in its own loop, and then all it has to do is call the crawl function if those pass.
> Source/WebCore/rendering/RenderObject.cpp:820 > + || (isOutOfFlowRenderFlowThread() && !toRenderFlowThread(this)->hasRegions());
This check confuses me. I don't quite understand it. Why is hasRegions() supposed to be false here?
> Source/WebCore/rendering/RenderView.cpp:141 > +void RenderView::layoutNamedFlowFixedPositionedObjects(const RenderNamedFlowThread* namedFlow) > +{ > + layoutPositionedObjects(false, true, namedFlow); > +}
I would implement the loop yourself and do the filtering yourself.
Dave Hyatt
Comment 21
2013-07-15 10:26:50 PDT
How about instead of this patch you just hack the layout of the objects to size/place relative to the viewport while still leaving them in the flow threads? This would make the condition easily detected during RenderNamedFlowThread's layoutPositionedObjects...
Mihnea Ovidenie
Comment 22
2013-08-13 06:13:34 PDT
Created
attachment 208623
[details]
WIP1 with alternate approach Alternate approach while leaving the fixed positioned elements inside the flow thread.
Mihnea Ovidenie
Comment 23
2013-08-29 09:38:43 PDT
Created
attachment 209993
[details]
WIP2 with alternate approach. Rebased and with tests.
Mihnea Ovidenie
Comment 24
2013-08-30 09:50:51 PDT
Created
attachment 210123
[details]
Patch
WebKit Commit Bot
Comment 25
2013-08-30 09:53:51 PDT
Attachment 210123
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/regions/element-in-named-flow-absolute-from-fixed-expected.txt', u'LayoutTests/fast/regions/element-in-named-flow-absolute-from-fixed.html', u'LayoutTests/fast/regions/element-in-named-flow-fixed-from-absolute-expected.txt', u'LayoutTests/fast/regions/element-in-named-flow-fixed-from-absolute.html', u'LayoutTests/fast/regions/element-inflow-fixed-from-outflow-static-expected.txt', u'LayoutTests/fast/regions/element-inflow-fixed-from-outflow-static.html', u'LayoutTests/fast/regions/element-outflow-static-from-inflow-fixed-expected.txt', u'LayoutTests/fast/regions/element-outflow-static-from-inflow-fixed.html', u'LayoutTests/fast/regions/fixed-element-transformed-parent-expected.txt', u'LayoutTests/fast/regions/fixed-element-transformed-parent.html', u'LayoutTests/fast/regions/fixed-in-named-flow-scroll-expected.txt', u'LayoutTests/fast/regions/fixed-in-named-flow-scroll.html', u'LayoutTests/fast/regions/fixed-inside-fixed-in-named-flow-expected.html', u'LayoutTests/fast/regions/fixed-inside-fixed-in-named-flow.html', u'LayoutTests/fast/regions/fixed-inside-named-flow-zIndex-expected.html', u'LayoutTests/fast/regions/fixed-inside-named-flow-zIndex.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-namedflow-noregions-expected.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-namedflow-noregions.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-region-expected.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-region.html', u'LayoutTests/fast/regions/fixed-pos-region-in-nested-flow-expected.html', u'LayoutTests/fast/regions/fixed-pos-region-in-nested-flow.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/FlowThreadController.cpp', u'Source/WebCore/rendering/FlowThreadController.h', u'Source/WebCore/rendering/RenderBox.cpp', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObject.h']" exit_code: 1 Source/WebCore/rendering/RenderLayer.h:947: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:947: The parameter name "paintingInfo" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:989: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderLayer.h:990: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderLayer.h:991: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderLayer.h:992: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderLayer.h:993: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderLayer.h:989: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:989: The parameter name "result" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:990: The parameter name "hitTestLocation" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:991: The parameter name "transformState" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:994: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderLayer.cpp:4547: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 13 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Dave Hyatt
Comment 26
2013-08-30 10:08:57 PDT
Comment on
attachment 210123
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=210123&action=review
r=me, but fix all the style queue errors. I also have a few suggestions below.
> Source/WebCore/rendering/RenderBox.cpp:1847 > + // For fixed positioned elements inside out-of-flow named flows, we do not want to > + // map their position further to regions based on their coordinates inside the named flows. > + if (o->isOutOfFlowRenderFlowThread() && fixedPositionedWithNamedFlowContainingBlock()) > + o->mapLocalToContainer(toRenderLayerModelObject(o), transformState, mode, wasFixed); > + else > + o->mapLocalToContainer(repaintContainer, transformState, mode, wasFixed);
I would invert this to give precedence to the non-flow-thread code path. if (!o->isOutOfFlowRenderFlowThread() || !fixedPositionedWithNamedFlowContainingBlock()) // Do the common thing. :)
> Source/WebCore/rendering/RenderBox.cpp:2975 > + if (containingBlock->isRenderNamedFlowThread() && (style()->position() == FixedPosition))
No need for parentheses around (style()->position() == FixedPosition())
> Source/WebCore/rendering/RenderBox.cpp:3033 > + if (containingBlock->isRenderNamedFlowThread() && (style()->position() == FixedPosition))
Ditto.
>> Source/WebCore/rendering/RenderLayer.cpp:4547 >> + RenderLayer* hitLayer = fixedLayer->hitTestLayer(fixedLayer->renderer().flowThreadContainingBlock()->layer(), 0, request, tempResult, >> + hitTestRect, hitTestLocation, false, transformState, zOffsetForDescendants); > > When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Fix the funny indentation here.
Build Bot
Comment 27
2013-08-30 13:38:51 PDT
Comment on
attachment 210123
[details]
Patch
Attachment 210123
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/1582636
New failing tests: fast/regions/fixed-in-named-flow-scroll.html
Build Bot
Comment 28
2013-08-30 13:38:56 PDT
Created
attachment 210156
[details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4
Mihnea Ovidenie
Comment 29
2013-09-03 00:19:08 PDT
Created
attachment 210331
[details]
Patch for landing
WebKit Commit Bot
Comment 30
2013-09-03 01:20:12 PDT
Comment on
attachment 210331
[details]
Patch for landing Clearing flags on attachment: 210331 Committed
r154973
: <
http://trac.webkit.org/changeset/154973
>
WebKit Commit Bot
Comment 31
2013-09-03 01:20:18 PDT
All reviewed patches have been landed. Closing bug.
Benjamin Poulain
Comment 32
2013-09-03 17:02:25 PDT
It looks like the bots MountainLion Debug WebKit2 are red since this patch landed. The following two tests fail: fast/regions/fixed-inside-named-flow-zIndex.html fast/regions/fixed-pos-region-in-nested-flow.html Can you please have a look?
Mihnea Ovidenie
Comment 33
2013-09-03 22:44:45 PDT
(In reply to
comment #32
)
> It looks like the bots MountainLion Debug WebKit2 are red since this patch landed. > > The following two tests fail: > fast/regions/fixed-inside-named-flow-zIndex.html > fast/regions/fixed-pos-region-in-nested-flow.html > > Can you please have a look?
Absolutely, sorry for this, taking a look.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug