Bug 46258 – [EFL] Add support for scaling the contents

• Component: WebKit EFL
• Status: RESOLVED INVALID

[Alex Bredariol Grilo]

[EFL] Add support for scaling the contents

[Alex Bredariol Grilo]

Created attachment 68361 [details]
Patch

[Alex Bredariol Grilo]

Created attachment 68367 [details]
Patch

[Kenneth Rohde Christiansen]

Hyatt, there are some ScrollView changes here that it would be nice if you could have a look at.

[Kenneth Rohde Christiansen]

Comment on attachment 68367 [details]
Patch

Why do you guys need all these changes? Can't you scale in the painting system instead? That works pretty well for Qt and we do not require changes like the above.

[Alex Bredariol Grilo]

(In reply to comment #4)

Kenneth, all these changes were made because all scrollbars and innerframes are scaled too. We also apply scaling in the painting system as you said, to get the contents scaled

[Kenneth Rohde Christiansen]

You don't want those to be scaled or?

[Alex Bredariol Grilo]

(In reply to comment #6)
> You don't want those to be scaled or?

We want them to be scaled. The patch includes this feature.

[Kenneth Rohde Christiansen]

Comment on attachment 68367 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68367&action=review

> WebCore/platform/ScrollView.cpp:148
> -        updateScrollbars(scrollOffset());
> +        updateScrollbars(m_scrollOffset);

scrollOffset should be inline anyway, I do not really like this change

> WebCore/platform/ScrollView.cpp:223
> -    return IntRect(IntPoint(m_scrollOffset.width(), m_scrollOffset.height()),
> -                   IntSize(max(0, width() - (verticalScrollbar() && !includeScrollbars ? verticalScrollbar()->width() : 0)), 
> -                           max(0, height() - (horizontalScrollbar() && !includeScrollbars ? horizontalScrollbar()->height() : 0))));
> +
> +    int w = width() - (verticalScrollbar() && !includeScrollbars ? verticalScrollbar()->width() : 0);
> +    int h = height() - (horizontalScrollbar() && !includeScrollbars ? horizontalScrollbar()->height() : 0);
> +
> +    if (useScale) {
> +        float scale = (this == root()) ? scaleFactor() : 1.0f;
> +        return IntRect(static_cast<int>(ceil(m_scrollOffset.width() / scale)),
> +                       static_cast<int>(ceil(m_scrollOffset.height() / scale)),
> +                       max(0, static_cast<int>(ceil(w / scale))),
> +                       max(0, static_cast<int>(ceil(h / scale))));
> +    } else {
> +        return IntRect(m_scrollOffset.width(), m_scrollOffset.height(),
> +                       max(0, w), max(0, h));
> +    }

Hyatt needs to look at this. This probably also wont work for webkit2. I think you need something like the setActualVisibleContentRect that we introduced a little while ago. I will have some patches soon that makes that actually work 100%.

[Andreas Kling]

Comment on attachment 68367 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68367&action=review

Whoa, this one is all over the place.
Please break this up into smaller patches, preferably separating the EFL-specific changes from the WebCore changes.

> WebCore/platform/ScrollView.cpp:870
> -    context->clip(visibleContentRect());
> +    IntRect visibleRect = visibleContentRect();
> +    context->clip(visibleRect);

What's going on here?

> WebCore/rendering/RenderBoxModelObject.cpp:114
> -    bool contextIsScaled = !currentTransform.isIdentityOrTranslationOrFlipped();
> +    bool contextIsScaled = false;

Please explain this change.

[Alex Bredariol Grilo]

(In reply to comment #9)
> (From update of attachment 68367 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68367&action=review
> 
> > WebCore/platform/ScrollView.cpp:870
> > -    context->clip(visibleContentRect());
> > +    IntRect visibleRect = visibleContentRect();
> > +    context->clip(visibleRect);
> 
> What's going on here?

It remains of some previous multiplication that was removed later.
Sorry for that, it will be removed in the updated patch.

> > WebCore/rendering/RenderBoxModelObject.cpp:114
> > -    bool contextIsScaled = !currentTransform.isIdentityOrTranslationOrFlipped();
> > +    bool contextIsScaled = false;
> 
> Please explain this change.

Sorry for that too. This remains of an old implementation. It will
also be removed in the updated patch.

[Alex Bredariol Grilo]

Created attachment 75701 [details]
Patch

[Alex Bredariol Grilo]

This patch corrects the problems pointed by Kling and the first problem pointed by Kenneth. We are still working on the other suggestion made by Kenneth.

Hyatt, do you have any other suggestion about the patch?

[Dave Hyatt]

Isn't this entire patch unnecessary?  Beth already added fixed scaling using transforms.  Is there any reason why that doesn't work for you?

See the page scale stuff.

Frame.h: pageScaleFactor()

etc.

I think that does what you want... perfectly applies a scale without doing any relayout.

[Antonio Gomes]

Comment on attachment 75701 [details]
Patch

it changes much of cross platform code, and comes without a single test. that wont get in like this I think.

[WebKit Review Bot]

Attachment 75701 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/6725106

[Ryuan Choi]

Close bug because WebCore already have this feature as another way.