67238 2011-08-30 14:49:58 -0700 Add helper methods to find and compute scale factor for zoom into an element 2012-03-26 11:01:58 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 68075 1 fsamuel fsamuel abarth ddkilzer fishd fsamuel rjkroege simon.fraser tonikitoo varunjain oldest_to_newest 459232 0 fsamuel 2011-08-30 14:49:58 -0700 Helper methods necessary to find and compute scale factor to zoom to an inner element 459236 1 105704 fsamuel 2011-08-30 14:54:06 -0700 Created attachment 105704 Patch 459245 2 105704 abarth 2011-08-30 15:02:14 -0700 Comment on attachment 105704 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? 459249 3 105704 abarth 2011-08-30 15:02:56 -0700 Comment on attachment 105704 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. 459252 4 fsamuel 2011-08-30 15:10:14 -0700 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. 459255 5 abarth 2011-08-30 15:12:25 -0700 It might make sense to make those named constants and add a comment explaining that. 461559 6 105704 simon.fraser 2011-09-02 15:06:59 -0700 Comment on attachment 105704 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? 461580 7 105704 fsamuel 2011-09-02 15:29:03 -0700 Comment on attachment 105704 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... 461591 8 fsamuel 2011-09-02 15:43:49 -0700 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... 105704 2011-08-30 14:54:06 -0700 2011-09-02 15:29:03 -0700 Patch bug-67238-20110830175404.patch text/plain 3933 fsamuel U3VidmVyc2lvbiBSZXZpc2lvbjogOTM5ODAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA4MDE1MjYyZDgwYWRjN2Rl Y2U5MTUyMzM0M2QyZDYxNDNkZGM4ZTZlLi45ZGU2MDBhNjkyNjg0ODllOTQ2ZGYyMmMwZjA3Mzgz OGNlY2Y3MzU0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjEgQEAKKzIwMTEtMDgtMzAgIEZhZHkg U2FtdWVsICA8ZnNhbXVlbEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgSGVscGVyIG1ldGhvZHMg bmVjZXNzYXJ5IHRvIGZpbmQgYW5kIGNvbXB1dGUgc2NhbGUgZmFjdG9yIHRvIHpvb20gdG8gYW4g aW5uZXIgZWxlbWVudAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9NjcyMzgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBObyBuZXcgdGVzdHMuIChPT1BTISkKKworICAgICAgICAqIHBhZ2UvUGFnZS5jcHA6Cisg ICAgICAgIChXZWJDb3JlOjpQYWdlOjpnZXRCbG9ja0JvdW5kcyk6CisgICAgICAgIEdpdmVuIGEg cG9pbnQsIGZpbmQgdGhlIGlubmVybW9zdCBlbGVtZW50ICh3aXRoaW4gYSB0aHJlc2hvbGQgZGVm aW5lZCBieSBwYWRkaW5nKSwKKyAgICAgICAgd2hvc2UgYm91bmRzIGVuY2xvc2UgdGhlIHBvaW50 LgorICAgICAgICAoV2ViQ29yZTo6UGFnZTo6Z2V0U2NhbGVGYWN0b3JGb3JSZWN0KToKKyAgICAg ICAgR2l2ZW4gYSByZWN0LCBmaW5kIHRoZSBzY2FsZSBmYWN0b3IgbmVjZXNzYXJ5IHRvIGNvdmVy IHRoZSBmdWxsIHZpZXdhYmxlIGFyZWEKKyAgICAgICAgd2l0aCB0aGF0IHJlY3QuCisgICAgICAg ICogcGFnZS9QYWdlLmg6CisKIDIwMTEtMDgtMjMgIENocmlzIE1hcnJpbiAgPGNtYXJyaW5AYXBw bGUuY29tPgogCiAgICAgICAgIFttYWNdIHJlcXVlc3RBbmltYXRpb25GcmFtZSBzdXBwb3J0IGZv ciBtYWMgcG9ydApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGFnZS9QYWdlLmNwcCBiL1Nv dXJjZS9XZWJDb3JlL3BhZ2UvUGFnZS5jcHAKaW5kZXggNmQxZjI5OGNjYTE4ZWU5YzhhYTcyYmM3 NzQ1OTViYWI5OWM2MmYzMi4uOGEwZjAxZGNmOTBkNWZkOGY4ZTUwODYxMjIyY2M3OWMyYTc2YWI3 YSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGFnZS9QYWdlLmNwcAorKysgYi9Tb3VyY2Uv V2ViQ29yZS9wYWdlL1BhZ2UuY3BwCkBAIC0yNTEsNiArMjUxLDQ4IEBAIHZvaWQgUGFnZTo6c2V0 TWFpbkZyYW1lKFBhc3NSZWZQdHI8RnJhbWU+IG1haW5GcmFtZSkKICAgICBtX21haW5GcmFtZSA9 IG1haW5GcmFtZTsKIH0KIAorSW50UmVjdCBQYWdlOjpnZXRCbG9ja0JvdW5kcyhjb25zdCBJbnRQ b2ludCYgaW5wdXRQb2ludCwgaW50IHBhZGRpbmcpCit7CisgICAgLy8gVXNlIHRoZSByZWN0LWJh c2VkIGhpdCB0ZXN0IHRvIGZpbmQgdGhlIG5vZGUuCisgICAgSW50UG9pbnQgcG9pbnQgPSBtYWlu RnJhbWUoKS0+dmlldygpLT53aW5kb3dUb0NvbnRlbnRzKGlucHV0UG9pbnQpOworICAgIEhpdFRl c3RSZXN1bHQgcmVzdWx0ID0gbWFpbkZyYW1lKCktPmV2ZW50SGFuZGxlcigpLT5oaXRUZXN0UmVz dWx0QXRQb2ludChwb2ludCwgZmFsc2UsIGZhbHNlLAorICAgICAgICAgICAgRG9udEhpdFRlc3RT Y3JvbGxiYXJzLCBIaXRUZXN0UmVxdWVzdDo6QWN0aXZlIHwgSGl0VGVzdFJlcXVlc3Q6OlJlYWRP bmx5LAorICAgICAgICAgICAgSW50U2l6ZShwYWRkaW5nLCBwYWRkaW5nKSk7CisgICAgTm9kZSog bm9kZSA9IHJlc3VsdC5pbm5lck5vblNoYXJlZE5vZGUoKTsKKyAgICBpZiAoIW5vZGUpCisgICAg ICAgIHJldHVybiBJbnRSZWN0KCk7CisgICAgLy8gRmluZCB0aGUgYmxvY2sgdHlwZSBub2RlIGJh c2VkIG9uIHRoZSBoaXQgbm9kZS4KKyAgICB3aGlsZSAobm9kZSAmJiAoIW5vZGUtPnJlbmRlcmVy KCkgfHwgbm9kZS0+cmVuZGVyZXIoKS0+aXNJbmxpbmUoKSkpCisgICAgICAgIG5vZGUgPSBub2Rl LT5wYXJlbnROb2RlKCk7CisgICAgLy8gUmV0dXJuIHRoZSBib3VuZGluZyBib3ggaW4gdGhlIHdp bmRvdyBjb29yZGluYXRlIHN5c3RlbS4KKyAgICBpZiAobm9kZSkgeworICAgICAgICBJbnRSZWN0 IHJlY3QgPSBub2RlLT5nZXRSZWN0KCk7CisgICAgICAgIEZyYW1lKiBmcmFtZSA9IG5vZGUtPmRv Y3VtZW50KCktPmZyYW1lKCk7CisgICAgICAgIHJldHVybiBmcmFtZS0+dmlldygpLT5jb250ZW50 c1RvV2luZG93KHJlY3QpOworICAgIH0KKyAgICByZXR1cm4gSW50UmVjdCgpOworfQorCitmbG9h dCBQYWdlOjpnZXRTY2FsZUZhY3RvckZvclJlY3QoY29uc3QgSW50UmVjdCYgcmVjdCkKK3sKKyAg ICBmbG9hdCBzY2FsZSA9IG1haW5GcmFtZSgpLT5wYWdlU2NhbGVGYWN0b3IoKTsKKyAgICBib29s IHNjYWxlVW5jaGFuZ2VkID0gdHJ1ZTsKKyAgICBpZiAoIXJlY3QuaXNFbXB0eSgpKSB7CisgICAg ICAgIHNjYWxlID0gKGZsb2F0KSBtYWluRnJhbWUoKS0+dmlldygpLT5zaXplKCkud2lkdGgoKSAv IHJlY3Qud2lkdGgoKTsKKyAgICAgICAgLy8gVE9ETyhmc2FtdWVsLCB2YXJ1bmphaW4pOiBzbmFw IHNjYWxlIHRvIHRocmVzaG9sZHMKKyAgICAgICAgc2NhbGVVbmNoYW5nZWQgPSBmYWJzKG1haW5G cmFtZSgpLT5wYWdlU2NhbGVGYWN0b3IoKSAtIHNjYWxlKSA8IDAuMTsKKyAgICB9CisKKyAgICBp ZiAocmVjdC5pc0VtcHR5KCkgfHwgc2NhbGVVbmNoYW5nZWQpIHsKKyAgICAgICAgLy8gWm9vbSBv dXQgdG8gb3ZlcnZpZXcgbW9kZS4KKyAgICAgICAgaW50IGRvY1dpZHRoID0gbWFpbkZyYW1lKCkt PnZpZXcoKS0+dmlzaWJsZVdpZHRoKCk7CisgICAgICAgIGZsb2F0IG92ZXJ2aWV3U2NhbGUgPSAo ZmxvYXQpIG1haW5GcmFtZSgpLT52aWV3KCktPmNvbnRlbnRzV2lkdGgoKSAvIGRvY1dpZHRoOwor ICAgICAgICByZXR1cm4gb3ZlcnZpZXdTY2FsZTsKKyAgICB9CisKKyAgICByZXR1cm4gc2NhbGU7 Cit9CisKIGJvb2wgUGFnZTo6b3BlbmVkQnlET00oKSBjb25zdAogewogICAgIHJldHVybiBtX29w ZW5lZEJ5RE9NOwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGFnZS9QYWdlLmggYi9Tb3Vy Y2UvV2ViQ29yZS9wYWdlL1BhZ2UuaAppbmRleCA5YjIwYzgxN2Q3YjI3YTQ1NmQ3YzMyMjEzOTll YmRiMTgyMTZkM2VkLi4zMDUwOTA0N2Y3M2U0NDViNmJkY2NlMmNlYjQ1MjViYzJkM2YzYjZhIDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wYWdlL1BhZ2UuaAorKysgYi9Tb3VyY2UvV2ViQ29y ZS9wYWdlL1BhZ2UuaApAQCAtNjgsNiArNjgsOCBAQCBuYW1lc3BhY2UgV2ViQ29yZSB7CiAgICAg Y2xhc3MgSGlzdG9yeUl0ZW07CiAgICAgY2xhc3MgSW5zcGVjdG9yQ2xpZW50OwogICAgIGNsYXNz IEluc3BlY3RvckNvbnRyb2xsZXI7CisgICAgY2xhc3MgSW50UG9pbnQ7CisgICAgY2xhc3MgSW50 UmVjdDsKICAgICBjbGFzcyBNZWRpYUNhblN0YXJ0TGlzdGVuZXI7CiAgICAgY2xhc3MgTWVkaWFT dHJlYW1DbGllbnQ7CiAgICAgY2xhc3MgTWVkaWFTdHJlYW1Db250cm9sbGVyOwpAQCAtMTM5LDYg KzE0MSw5IEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKICAgICAgICAgdm9pZCBzZXRNYWluRnJhbWUo UGFzc1JlZlB0cjxGcmFtZT4pOwogICAgICAgICBGcmFtZSogbWFpbkZyYW1lKCkgY29uc3QgeyBy ZXR1cm4gbV9tYWluRnJhbWUuZ2V0KCk7IH0KIAorICAgICAgICBJbnRSZWN0IGdldEJsb2NrQm91 bmRzKGNvbnN0IEludFBvaW50JiBpbnB1dFBvaW50LCBpbnQgcGFkZGluZyk7CisgICAgICAgIGZs b2F0IGdldFNjYWxlRmFjdG9yRm9yUmVjdChjb25zdCBJbnRSZWN0Jik7CisKICAgICAgICAgYm9v bCBvcGVuZWRCeURPTSgpIGNvbnN0OwogICAgICAgICB2b2lkIHNldE9wZW5lZEJ5RE9NKCk7CiAK