Bug 111176

Summary: [CSS Regions] position: fixed is computed relative to the first region, not the viewport
Product: WebKit Reporter: Mihai Balan <mibalan>
Component: CSSAssignee: Mihnea Ovidenie <mihnea>
Status: RESOLVED FIXED    
Severity: Normal CC: achicu, benjamin, buildbot, commit-queue, esprehn+autocc, glenn, hyatt, kangil.han, kondapallykalyan, mihnea, rniwa, simon.fraser, WebkitBugTracker
Priority: P2 Keywords: AdobeTracked
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
Ref-test highlighting the problem
none
WIP 1
none
Patch
achicu: review-, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Patch 2
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Patch 3
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
WIP1 with alternate approach
none
WIP2 with alternate approach. Rebased and with tests.
none
Patch
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Patch for landing none

Description Mihai Balan 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.
Comment 1 Mihai Balan 2013-03-01 06:47:29 PST
Created attachment 190957 [details]
Ref-test highlighting the problem
Comment 2 Mihnea Ovidenie 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.
Comment 3 Mihnea Ovidenie 2013-07-04 08:48:04 PDT
Created attachment 206094 [details]
Patch
Comment 4 Build Bot 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
Comment 5 Build Bot 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
Comment 6 Mihnea Ovidenie 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.
Comment 7 Alexandru Chiculita 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.
Comment 8 Mihnea Ovidenie 2013-07-14 00:55:46 PDT
Created attachment 206627 [details]
Patch 2
Comment 9 WebKit Commit Bot 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.
Comment 10 Mihnea Ovidenie 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.
Comment 11 Build Bot 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
Comment 12 Build Bot 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
Comment 13 Build Bot 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
Comment 14 Build Bot 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
Comment 15 Mihnea Ovidenie 2013-07-15 01:55:39 PDT
Created attachment 206646 [details]
Patch 3

Correct style and skip another regions compositing test.
Comment 16 Build Bot 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
Comment 17 Build Bot 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
Comment 18 Build Bot 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
Comment 19 Build Bot 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
Comment 20 Dave Hyatt 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.
Comment 21 Dave Hyatt 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...
Comment 22 Mihnea Ovidenie 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.
Comment 23 Mihnea Ovidenie 2013-08-29 09:38:43 PDT
Created attachment 209993 [details]
WIP2 with alternate approach. Rebased and with tests.
Comment 24 Mihnea Ovidenie 2013-08-30 09:50:51 PDT
Created attachment 210123 [details]
Patch
Comment 25 WebKit Commit Bot 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.
Comment 26 Dave Hyatt 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.
Comment 27 Build Bot 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
Comment 28 Build Bot 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
Comment 29 Mihnea Ovidenie 2013-09-03 00:19:08 PDT
Created attachment 210331 [details]
Patch for landing
Comment 30 WebKit Commit Bot 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>
Comment 31 WebKit Commit Bot 2013-09-03 01:20:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 Benjamin Poulain 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?
Comment 33 Mihnea Ovidenie 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.