Bug 143651 – Legacy scroll behavior on HTMLBodyElement should only apply to the first body element of a document

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

Legacy scroll behavior on HTMLBodyElement should only apply to the first body element of a document

[Benjamin Poulain]

Created attachment 250619 [details]
Patch

[Sam Weinig]

Comment on attachment 250619 [details]
Patch

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

> Source/WebCore/html/HTMLBodyElement.cpp:243
> +        document().updateLayoutIgnorePendingStylesheets();
> +        Frame* frame = document().frame();
> +        if (!frame)
> +            return 0;
> +        FrameView* view = frame->view();
> +        if (!view)
> +            return 0;
> +        return adjustForZoom(view->contentsScrollPosition().x(), *frame);

Will a layout ever change a contentsScrollPosition().x() which is equal to 0 to something that isn't equal to 0? If it never will, we should consider doing the optimization we have in DOMWindow::scrollX()/DOMWindow::scrollY() here.

[Benjamin Poulain]

(In reply to comment #2)
> Comment on attachment 250619 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250619&action=review
> 
> > Source/WebCore/html/HTMLBodyElement.cpp:243
> > +        document().updateLayoutIgnorePendingStylesheets();
> > +        Frame* frame = document().frame();
> > +        if (!frame)
> > +            return 0;
> > +        FrameView* view = frame->view();
> > +        if (!view)
> > +            return 0;
> > +        return adjustForZoom(view->contentsScrollPosition().x(), *frame);
> 
> Will a layout ever change a contentsScrollPosition().x() which is equal to 0
> to something that isn't equal to 0? If it never will, we should consider
> doing the optimization we have in DOMWindow::scrollX()/DOMWindow::scrollY()
> here.

You are probably right but given that this is legacy code, I don't think we should bother with speculative optimizations.

[Benjamin Poulain]

Comment on attachment 250619 [details]
Patch

Clearing flags on attachment: 250619

Committed r182677: <http://trac.webkit.org/changeset/182677>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 250619 [details]
Patch

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

> Source/WebCore/html/HTMLBodyElement.cpp:242
> +        Frame* frame = document().frame();
> +        if (!frame)
> +            return 0;
> +        FrameView* view = frame->view();
> +        if (!view)
> +            return 0;

Document::view exists to make code sequences like this shorter:

    FrameView* view = document().view();
    if (!view)
        return 0;

Will be nice to have three fewer lines of code in every single one of these functions!

[Benjamin Poulain]

(In reply to comment #6)
> Document::view exists to make code sequences like this shorter:
> 
>     FrameView* view = document().view();
>     if (!view)
>         return 0;
> 
> Will be nice to have three fewer lines of code in every single one of these
> functions!

The code below needs both the Frame and the FrameView.