Summary: | Legacy scroll behavior on HTMLBodyElement should only apply to the first body element of a document | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||
Component: | New Bugs | Assignee: | Benjamin Poulain <benjamin> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | commit-queue, darin, diego.perini, esprehn+autocc, gyuyoung.kim | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Benjamin Poulain
2015-04-12 14:38:13 PDT
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. |