Bug 68192 - Add method to scroll current node to specific position in Chromium WebKit API
Summary: Add method to scroll current node to specific position in Chromium WebKit API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-15 14:19 PDT by Varun Jain
Modified: 2011-09-16 06:17 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.12 KB, patch)
2011-09-15 14:21 PDT, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (3.12 KB, patch)
2011-09-15 16:15 PDT, Varun Jain
no flags Details | Formatted Diff | Diff
Patch (3.10 KB, patch)
2011-09-15 16:20 PDT, Varun Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Varun Jain 2011-09-15 14:19:13 PDT
Add method to scroll current node to specific position in Chromium WebKit API
Comment 1 Varun Jain 2011-09-15 14:21:20 PDT
Created attachment 107552 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2011-09-15 14:42:03 PDT
Comment on attachment 107552 [details]
Patch

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

This logic seems to be in the wrong place. If you had it on Element, you could at least test it with window.internals.

> ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

No test?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1834
> +        frame->view()->scrollBy(scrollOffset);

Missing indent.
Comment 3 Varun Jain 2011-09-15 16:15:31 PDT
Created attachment 107560 [details]
Patch
Comment 4 Varun Jain 2011-09-15 16:20:25 PDT
Created attachment 107563 [details]
Patch
Comment 5 Dimitri Glazkov (Google) 2011-09-15 16:24:21 PDT
Comment on attachment 107563 [details]
Patch

please don't forget to file a bug to add a test.
Comment 6 Fady Samuel 2011-09-15 18:24:30 PDT
Comment on attachment 107563 [details]
Patch

Clearing flags on attachment: 107563

Committed r95252: <http://trac.webkit.org/changeset/95252>
Comment 7 Fady Samuel 2011-09-15 18:24:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Fisher (:fishd, Google) 2011-09-15 23:44:20 PDT
Comment on attachment 107563 [details]
Patch

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

> Source/WebKit/chromium/public/WebView.h:179
> +    virtual void scrollFocusedNodeIntoRect(const WebRect& rect) { }

nit: we don't name parameters when the name doesn't add any information.  here the type name
is WebRect, and calling the variable "rect" does not add any information, so the name should be left out.

why did you choose to have the rect be in screen coordinates instead of window coordinates?
that seems like a fairly unusual choice given that most other APIs use window coordinates
(e.g., input events).

> Source/WebKit/chromium/src/WebViewImpl.cpp:1828
> +    LayoutRect bounds = elementNode->boundsInWindowSpace();

it seems like this requires layout() to have been run before calling
this function.  should you perhaps assert that layout is not dirty?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1832
> +    Frame* frame = mainFrameImpl()->frame();

how do you know that the focused node is part of the main frame?
what if the focused node is part of a subframe?
Comment 9 Darin Fisher (:fishd, Google) 2011-09-15 23:46:04 PDT
(In reply to comment #2)
> (From update of attachment 107552 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107552&action=review
> 
> This logic seems to be in the wrong place. If you had it on Element, you could at least test it with window.internals.
> 
> > ChangeLog:6
> > +        Reviewed by NOBODY (OOPS!).
> 
> No test?

I didn't see this question answered.  It seems like you could construct a test using the WebFrameTest framework.
Comment 10 Fady Samuel 2011-09-16 03:47:58 PDT
Comment on attachment 107563 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1832

> 
> how do you know that the focused node is part of the main frame?
> what if the focused node is part of a subframe?

That's a good point. I think the correct, and most general behavior here is, given a path of frames to the mainframe, we should center each frame on the screen along the way. E.g. MainFrame->subFrame->inputbox. inputbox should be centered in subFrame and subFrame should be centered in MainFrame.
Comment 11 Varun Jain 2011-09-16 06:15:42 PDT
Hi Darin,
Dimitri and I agreed to moving this code into FrameView to be able to test it with window.internals in another CL. This is the tracking bug: https://bugs.webkit.org/show_bug.cgi?id=68198


(In reply to comment #8)
> (From update of attachment 107563 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107563&action=review
> 
> > Source/WebKit/chromium/public/WebView.h:179
> > +    virtual void scrollFocusedNodeIntoRect(const WebRect& rect) { }
> 
> nit: we don't name parameters when the name doesn't add any information.  here the type name
> is WebRect, and calling the variable "rect" does not add any information, so the name should be left out.
> 

will fix in followup CL

> why did you choose to have the rect be in screen coordinates instead of window coordinates?
> that seems like a fairly unusual choice given that most other APIs use window coordinates
> (e.g., input events).

No particular reason for this except that the renderer get the rect in screen coordinates. I can change it to window coordinates in the renderer before passing to webview

> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:1828
> > +    LayoutRect bounds = elementNode->boundsInWindowSpace();
> 
> it seems like this requires layout() to have been run before calling
> this function.  should you perhaps assert that layout is not dirty?

Yes.. will check for dirty layout and relayout if required.

> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:1832
> > +    Frame* frame = mainFrameImpl()->frame();
> 
> how do you know that the focused node is part of the main frame?
> what if the focused node is part of a subframe?

I totally missed that! Thanks for pointing out. I do like Fady's suggestion above.
Comment 12 Varun Jain 2011-09-16 06:17:10 PDT
(In reply to comment #9)
> (In reply to comment #2)
> > (From update of attachment 107552 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=107552&action=review
> > 
> > This logic seems to be in the wrong place. If you had it on Element, you could at least test it with window.internals.
> > 
> > > ChangeLog:6
> > > +        Reviewed by NOBODY (OOPS!).
> > 
> > No test?
> 
> I didn't see this question answered.  It seems like you could construct a test using the WebFrameTest framework.

as mentioned earlier, I'll move this code into FrameView and test using window.internals