[Webkit-unassigned] [Bug 81786] Support fixed position elements in Qt WebKit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=81786





--- Comment #6 from Yael <yael.aharon at nokia.com>  2012-03-21 10:24:14 PST ---
(In reply to comment #5)
Thank you for your review :)

> (From update of attachment 133058 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133058&action=review
> 
> Any good tests? even manual ones?
I will add my test page as a manual test.

> 
> > Source/WebCore/platform/graphics/GraphicsLayer.h:421
> > +    void setFixedPosition(bool fixed) { m_fixedPosition = fixed; }
> > +    bool fixedPosition() const { return m_fixedPosition; }
> 
> setUseFixedPosition? setIsFixedPositioned() ?
> 
setUseFixedPosition() sound good to me :)

> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:433
> > +    PositionInformationMap::iterator end = graphicsLayer->fixedPositionInformation().end();
> > +    for (PositionInformationMap::iterator it = graphicsLayer->fixedPositionInformation().begin(); it != end; ++it)
> > +        m_state.m_fixedPositionInformation.add(it->first, it->second);
> 
> hmmmm, why do you need to copy this?
At the moment, it is not really used. I'll remove that.

> 
> > Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:186
> >  
> > +void WebLayerTreeRenderer::adjustPositionForFixedPositionLayer(WebCore::GraphicsLayer* layer)
> > +{
> > +    ASSERT(layer->fixedPosition());
> 
> This should not need to be called unless we are doing overflow
We need to make sure we are in sync during flick scroll too. 

> 
> > Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:197
> > +            adjustedX = visibleEdgeLeft + left->second.getFloatValue() * m_visibleContentsRect.width() / 100;
> > +        else // it->second.isFixed()
> > +            adjustedX = visibleEdgeLeft + left->second.getFloatValue() * m_contentsScale;
> 
> IF you are zooming in the fixed elements will take over teh whole screen. iOS has a limit for this (say 2.0).
I did not observe a limit with ios 5.1 and my test page. We can add a limit later, if we think it is needed, though :)

> 
> > Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:205
> > +        if (right!= map.end()) {
> > +            if  (right->second.isPercent())
> > +                adjustedX = visibleEdgeRight - right->second.getFloatValue() * m_visibleContentsRect.width() / 100 - layer->size().width();
> > +            else // it->second.isFixed()
> > +                adjustedX = visibleEdgeRight - right->second.getFloatValue() * m_contentsScale - layer->size().width();
> 
> This should be using valueForLength (LengthFunctions.h), I believe
> 
ok

> > Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:213
> > +            adjustedY = visibleEdgeTop + top->second.getFloatValue() * m_visibleContentsRect.height() / 100;
> 
> top->second is pretty unreadable.
> 
I will assign it to a temporary variable with a better name.

> > Source/WebKit2/UIProcess/API/C/WKPreferencesPrivate.h:80
> >  // Defaults to false.
> > +WK_EXPORT void WKPreferencesSetAcceleratedCompositingForFixedPositionEnabled(WKPreferencesRef, bool);
> > +WK_EXPORT bool WKPreferencesGetAcceleratedCompositingForFixedPositionEnabled(WKPreferencesRef);
> 
> Why are we exposing this to C? Are Apple guys interesting in using it? I would leave this out of this patch
> 
ok

> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:124
> > +    webPageProxy->pageGroup()->preferences()->setAcceleratedCompositingForFixedPositionEnabled(true);
> 
> Cant we just tie this to the resizeToContents and avoid the preference?
> 
ok

> > Source/WebKit2/ChangeLog:10
> > +        This is not done for pinching. Only when the pinch gesture is finished, the position is adjusted.
> 
> huh? why?
I tested chrome beta on ICS and ios 5.1. Both do the same. During the pinch, the position is not updates, only after the pinch is done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.