Sometimes a page may take longer to load than expected. If the page changes the body::-webkit-scrollbar css, those changes won't take affect because we don't call updateScrollbars when we need to. Right now we do it on the very first layout, which won't necessarily wait to happen until the page has finished loading. It also happens when resizing the view. I think we have two options here: 1. in FrameView::handleLoadComplete, call resetScrollbars so that we update the scroll bars when the page is done loading 2. ensure that the first layout doesn't occur until handleLoadComplete has been called, then allow/force the first layout. - not sure about this one because I don't really have a good grasp of all the different situations in which we decide to attempt a layout call The patch I'm loading is for 1)
<rdar://problem/12363851>
Created attachment 208038 [details] patch
Created attachment 208074 [details] Patch
Comment on attachment 208074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208074&action=review > Source/WebCore/page/FrameView.cpp:2279 > + if (!m_firstLayout) { This seems… quite surprising. Also its location relative to the comment is not right.
Comment on attachment 208074 [details] Patch Attachment 208074 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1326729
Comment on attachment 208074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208074&action=review > Source/WebCore/ChangeLog:4 > + When the page finishes loading, schedule another layout in which scrollbars will be updated. > + https://bugs.webkit.org/show_bug.cgi?id=119454. This describes what the patch does, not why it necessary. What is the issue that prompted making this change? Also, why can't it be tested?
Comment on attachment 208074 [details] Patch Causing an extra layout is a big deal, and should be avoided. The real question is why any of this is apparently necessary on Windows, but not on Mac.
(In reply to comment #7) > (From update of attachment 208074 [details]) > Causing an extra layout is a big deal, and should be avoided. The real question is why any of this is apparently necessary on Windows, but not on Mac. This problem happens on mac as well. I have a simple test case that dynamically changes the scroll bar css on a timer. (I originally thought dynamically changing the css was a different issue because I couldn't get the scroll bars to update at all with this test case - turns out I was resizing the window horizontally, which does not cause any sort of update to happen, resizing vertically does though). Anyways, point is, this is a problem on both mac and windows, that changing the CSS dynamically does not result in the scrollbars being updated.
Given this, FrameView::handleLoadCompleted is definitely not the right place for the fix. It should be somewhere related to the scrollbar css being changed.
So, regardless of what happens, the fact remains that we are not updating the scroll bars somewhere where we should be. This is what happens in the test case: recalcStyle gets called when the scrollbar css is changed. This results eventually in a layout timer getting fired. There are two places in FrameView::layout where the scroll bars could possibly be updated. One only happens if it's the first layout or if we're changing scroll modes. The other only happens when we are resizing the view, which bails early if the view size doesn't actually change. I'm not sure if it's better to add the css changing case to one or the other, or in a separate case all on it's own. Perhaps making another case for "CSS being changed after first layout" would be best since the other code paths seem to contain logic for things that we don't need (we need only to call updateScrollBars in the CSS case)
(In reply to comment #10) > So, regardless of what happens, the fact remains that we are not updating the scroll bars somewhere where we should be. Not sure what I meant by "regardless of what happens" -> internal dialogue filler perhaps.
Created attachment 208456 [details] patch Note this is a workaround: In this patch we update the scrollbars after the frameview has finished loading and only if there is a mismatch between the style of the scrollbar and the actual scrollbar (whether or not they're custom) An enhanced workedaround was proposed that I could in the FrameView::layout method just check to see if the style and actual scrollbar matched and then update scrollbars as necessary then. That would not only solve this particularly delayed loading problem but also the problem of scrollbars not updating after dynamically changing the scrollbar css. However, doing this causes a lot of unforeseen issues with the ordering during which layouts are called that seem to result in scrollbar updates being disabled altogether, which ultimately causes the document to adopt the default scrollbar modes (vertical: ALWAYS ON and horizontal: ALWAYS off, though the scrollbars are in the correct style). Given that this patch is simply a temporary work around (and thus will be removed soon anyways), I think we should focus on just fixing the bug at hand and not dealing with the dynamic css changing issue for now.
Comment on attachment 208456 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=208456&action=review r- because you are not initializing m_scrollbarUpdatePending in the FrameView constructor, but I think this is okay otherwise. So this is close! > Source/WebCore/page/FrameView.cpp:1295 > + You should remove this new whitespace.
(In reply to comment #13) > (From update of attachment 208456 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208456&action=review > > r- because you are not initializing m_scrollbarUpdatePending in the FrameView constructor, but I think this is okay otherwise. So this is close! Hmm the constructor calls init() which calls reset() which initializes m_scrollbarUpdatePending. Is that okay? > > > Source/WebCore/page/FrameView.cpp:1295 > > + > > You should remove this new whitespace. Will do
(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 208456 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=208456&action=review > > > > r- because you are not initializing m_scrollbarUpdatePending in the FrameView constructor, but I think this is okay otherwise. So this is close! > > Hmm the constructor calls init() which calls reset() which initializes m_scrollbarUpdatePending. > Is that okay? > That's a good point, but I don't think that there is any need to rely on that. That code could change. We should initialize this bool in the constructor just like we do for all of the other bools.
Created attachment 208464 [details] patch
(In reply to comment #16) > Created an attachment (id=208464) [details] > patch Fixed review comments. Thanks!
Comment on attachment 208464 [details] patch Attachment 208464 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1385421
Comment on attachment 208464 [details] patch Attachment 208464 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1350580
Comment on attachment 208464 [details] patch Attachment 208464 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1405345
Comment on attachment 208464 [details] patch Attachment 208464 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1353612
Comment on attachment 208464 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=208464&action=review > Source/WebCore/page/FrameView.cpp:207 > + , m_scrollbarUpdatePending(true) This should be initialized to false.
Comment on attachment 208464 [details] patch Attachment 208464 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1401356
Comment on attachment 208464 [details] patch Attachment 208464 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1369847
Comment on attachment 208464 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=208464&action=review >>> Source/WebCore/page/FrameView.cpp:207 >>> + , m_scrollbarUpdatePending(true) >> >> This should be initialized to false. > > This should be initialized to false. Oh, also, it looks like the builds are failing because you are initializing this in the wrong spot. You need to initialize it in the same order that it is declared in the header. So it should be right after m_firstLayoutCallbackPending and right before m_firstLayout based on where you put it in the header.
Comment on attachment 208464 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=208464&action=review > Source/WebCore/page/FrameView.cpp:2287 > + Document* doc = m_frame->document(); Please don’t abbreviate to “doc” in new code. Use “document” instead. > Source/WebCore/page/FrameView.cpp:2288 > + Element* body = doc ? doc->body() : 0; Document can’t be 0. > Source/WebCore/page/FrameView.cpp:2292 > + Element* docElement = doc ? doc->documentElement() : 0; Same here, documentElement is better. And document can’t be zero.