Legacy scroll behavior on HTMLBodyElement should only apply to the first body element of a document
Created attachment 250619 [details] Patch
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.
(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.
Comment on attachment 250619 [details] Patch Clearing flags on attachment: 250619 Committed r182677: <http://trac.webkit.org/changeset/182677>
All reviewed patches have been landed. Closing bug.
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!
(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.