[webkit-reviews] review denied: [Bug 67238] Helper methods necessary to find and compute scale factor to zoom to an inner element : [Attachment 105704] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Fady Samuel <fsamuel at chromium.org>'s
request for review:
Bug 67238: Helper methods necessary to find and compute scale factor to zoom to
an inner element
https://bugs.webkit.org/show_bug.cgi?id=67238

Attachment 105704: Patch
https://bugs.webkit.org/attachment.cgi?id=105704&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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?