Bug 68192 – Add method to scroll current node to specific position in Chromium WebKit API

• Component: New Bugs
• Status: RESOLVED FIXED

[Varun Jain]

Add method to scroll current node to specific position in Chromium WebKit API

[Varun Jain]

Created attachment 107552 [details]
Patch

[Dimitri Glazkov (Google)]

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.

[Varun Jain]

Created attachment 107560 [details]
Patch

[Varun Jain]

Created attachment 107563 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 107563 [details]
Patch

please don't forget to file a bug to add a test.

[Fady Samuel]

Comment on attachment 107563 [details]
Patch

Clearing flags on attachment: 107563

Committed r95252: <http://trac.webkit.org/changeset/95252>

[Fady Samuel]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

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?

[Darin Fisher (:fishd, Google)]

(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.

[Fady Samuel]

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.

[Varun Jain]

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.

[Varun Jain]

(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