Bug 67238 – Add helper methods to find and compute scale factor for zoom into an element

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Fady Samuel]

Helper methods necessary to find and compute scale factor to zoom to an inner element

[Fady Samuel]

Created attachment 105704 [details]
Patch

[Adam Barth]

Comment on attachment 105704 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This line will prevent this patch from being landed.  We either need to add tests or explain why we're not adding tests.

> Source/WebCore/page/Page.cpp:268
> +    if (node) {

Prefer early return.

> Source/WebCore/page/Page.cpp:282
> +        // TODO(fsamuel, varunjain): snap scale to thresholds

TODO => FIXME.

Also, WebKit doesn't include user names.

> Source/WebCore/page/Page.cpp:283
> +        scaleUnchanged = fabs(mainFrame()->pageScaleFactor() - scale) < 0.1;

0.1 <-- This magic constant is curious.

> Source/WebCore/page/Page.h:145
> +        IntRect getBlockBounds(const IntPoint& inputPoint, int padding);
> +        float getScaleFactorForRect(const IntRect&);

Are there callers for these functions?

Generally whenever we add something to Page, we should ask ourselves whether that's the right place for the API.  Page is a "god" object that likes to collect infinite complexity.  These functions don't appear to use anything from the Page other than it's mainFrame(), and then, even, mostly just the mainFrame()->view().  Perhaps these methods should be on FrameView?

[Adam Barth]

Comment on attachment 105704 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        Helper methods necessary to find and compute scale factor to zoom to an inner element
> +        https://bugs.webkit.org/show_bug.cgi?id=67238

This ChangeLog also doesn't explain why we'd like to make this change.  There's probably some larger context here that would be helpful to explain.

[Fady Samuel]

Padding and the 0.1 are magic numbers that are platform-dependent and may be determined empirically (whatever *feels* right). These helper methods will enable platforms to implement functionality to zoom to a particular div. Making this feel right will, most likely, require a number of iterations based on empirical testing. I suppose this can be moved to FrameView.

(In reply to comment #3)
> (From update of attachment 105704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105704&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +        Helper methods necessary to find and compute scale factor to zoom to an inner element
> > +        https://bugs.webkit.org/show_bug.cgi?id=67238
> 
> This ChangeLog also doesn't explain why we'd like to make this change.  There's probably some larger context here that would be helpful to explain.

[Adam Barth]

It might make sense to make those named constants and add a comment explaining that.

[Simon Fraser (smfr)]

Comment on attachment 105704 [details]
Patch

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

> Source/WebCore/page/Page.cpp:254
> +IntRect Page::getBlockBounds(const IntPoint& inputPoint, int padding)

The method name is very confusing. It also seems weird to do the hit testing and get the rect, but not return the element that was hit.

Agree that this should not be on Page. What's the expected behavior for iframes?

> Source/WebCore/page/Page.cpp:276
> +float Page::getScaleFactorForRect(const IntRect& rect)

Again, this method name doesn't explain what it's trying to do. Should also not be on page. What's the expected behavior inside iframes?

[Fady Samuel]

Comment on attachment 105704 [details]
Patch

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

>> Source/WebCore/page/Page.cpp:254

> 
> The method name is very confusing. It also seems weird to do the hit testing and get the rect, but not return the element that was hit.
> 
> Agree that this should not be on Page. What's the expected behavior for iframes?

So the purpose of this method is to take an input rect, and find the bounds of the deepest block node. We use a readonly hit test to find out which block encompasses that rect.

We'd like to make this code available in WebCore because it's platform-independent. Where would you suggest we place this?

>> Source/WebCore/page/Page.cpp:276
>> +float Page::getScaleFactorForRect(const IntRect& rect)
> 
> Again, this method name doesn't explain what it's trying to do. Should also not be on page. What's the expected behavior inside iframes?

The first method is an input to this method, typically. Once we have a box bounds, we compute the appropriate scale factor to fill the frameView's content area with the particular box we'd like to zoom to...

[Fady Samuel]

I should add the search for the deepest box stops at the iframe boundary.  

(In reply to comment #7)
> (From update of attachment 105704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105704&action=review
> 
> >> Source/WebCore/page/Page.cpp:254
> 
> > 
> > The method name is very confusing. It also seems weird to do the hit testing and get the rect, but not return the element that was hit.
> > 
> > Agree that this should not be on Page. What's the expected behavior for iframes?
> 
> So the purpose of this method is to take an input rect, and find the bounds of the deepest block node. We use a readonly hit test to find out which block encompasses that rect.
> 
> We'd like to make this code available in WebCore because it's platform-independent. Where would you suggest we place this?
> 
> >> Source/WebCore/page/Page.cpp:276
> >> +float Page::getScaleFactorForRect(const IntRect& rect)
> > 
> > Again, this method name doesn't explain what it's trying to do. Should also not be on page. What's the expected behavior inside iframes?
> 
> The first method is an input to this method, typically. Once we have a box bounds, we compute the appropriate scale factor to fill the frameView's content area with the particular box we'd like to zoom to...