Bug 98186 – [WK2] PageViewportController.cpp is supposed to be a generic WebKit2 file but only works with Qt port.

• Component: WebKit2
• Status: RESOLVED FIXED

[Hugo Parente Lima]

The file is located at Source/WebKit2/UIProcess, it shouldn't depend on Qt but currently it only compiles based on a implicit conversion from WebCore::FloatSize to QSize (because QSize have operator/(float)).

The fix just add a operator/(float) to WebCore::FloatSize and remove the implicit conversion from WebCore::FloatSize to QSize.

I don't know if WebKit have some rule against operator overloads with operands of different types, so make me know about and if so, I can update the patch to fix PageViewportController.cpp, but IMO adding the operator/ on WebCore::FloatSize seems nicer.

[Hugo Parente Lima]

Created attachment 166730 [details]
Patch

[Noam Rosenthal]

Comment on attachment 166730 [details]
Patch

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

Better to use FloatSize::scale explicitly

> Source/WebCore/platform/graphics/FloatSize.h:188
> +inline FloatSize operator/(const FloatSize& a, const float& b)

const float& -> float

[Hugo Parente Lima]

(In reply to comment #2)
> (From update of attachment 166730 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166730&action=review
> 
> Better to use FloatSize::scale explicitly

Ok, I'll do that.
 
> > Source/WebCore/platform/graphics/FloatSize.h:188
> > +inline FloatSize operator/(const FloatSize& a, const float& b)
> 
> const float& -> float

So, if I'm goin to use FloatSize::scale, operator/ will disappear from this patch.

[Hugo Parente Lima]

Created attachment 166927 [details]
Patch

[Noam Rosenthal]

Comment on attachment 166927 [details]
Patch

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

> Source/WebCore/platform/graphics/FloatSize.h:100
> +    FloatSize scaledTo(float s) const

how about scaledSize

> Source/WebKit2/UIProcess/PageViewportController.cpp:148
> +        if (FloatRect(clampedPos, m_viewportSize.scaledTo(1 / m_effectiveScale)).intersects(coveredRect)) {

You're making the same calculation over and over... how about creating a function PageViewportController::viewportSizeInContentsCoordinates that would return that calculation? That way you also don't need a function...

[Hugo Parente Lima]

(In reply to comment #5)
> (From update of attachment 166927 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166927&action=review
> 
> > Source/WebCore/platform/graphics/FloatSize.h:100
> > +    FloatSize scaledTo(float s) const
> 
> how about scaledSize

I used scaledTo to follow the same convention used in this file, like "expandedTo" instead of "expandedSize"
 
> > Source/WebKit2/UIProcess/PageViewportController.cpp:148
> > +        if (FloatRect(clampedPos, m_viewportSize.scaledTo(1 / m_effectiveScale)).intersects(coveredRect)) {
> 
> You're making the same calculation over and over... how about creating a function PageViewportController::viewportSizeInContentsCoordinates that would return that calculation? That way you also don't need a function...

Could be, it's used 3 times, I'll update the patch.

[Hugo Parente Lima]

Created attachment 166953 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166953 [details]
Patch

Clearing flags on attachment: 166953

Committed r130327: <http://trac.webkit.org/changeset/130327>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.