[Qt] Fix "ASSERTION FAILED: !document->inPageCache()" when loading a page
Created attachment 167321 [details] Patch
Comment on attachment 167321 [details] Patch Attachment 167321 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14176534
Comment on attachment 167321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167321&action=review is this covered by tests? ie, the restore from history? > Source/WebCore/page/Frame.cpp:780 > +void Frame::createView(const IntSize& viewportSize, const Color& backgroundColor, bool transparent, > + const IntSize& fixedLayoutSize, const IntRect& fixedVisibleContentRect , > + bool useFixedLayout, ScrollbarMode horizontalScrollbarMode, bool horizontalLock, > + ScrollbarMode verticalScrollbarMode, bool verticalLock) Shouldnt this not just be one line > Source/WebCore/page/Frame.h:88 > - void createView(const IntSize&, const Color&, bool, const IntSize&, bool, > - ScrollbarMode = ScrollbarAuto, bool horizontalLock = false, > + void createView(const IntSize&, const Color&, bool, > + const IntSize& fixedLayoutSize = IntSize(), const IntRect& fixedVisibleContentRect = IntRect(), > + bool useFixedLayout = false, ScrollbarMode = ScrollbarAuto, bool horizontalLock = false, > ScrollbarMode = ScrollbarAuto, bool verticalLock = false); It is getting a bit confusing with all the defaults. Doesn't some of the fixed* ones imply others? ie. if you have a fixedlayoutsize isn't that the same as useFixedLayout == true? > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:271 > const QSize preferredLayoutSize = page->preferredContentsSize(); > > + > ScrollbarMode hScrollbar = (ScrollbarMode) m_webFrame->scrollBarPolicy(Qt::Horizontal); unrelated change?
(In reply to comment #3) > (From update of attachment 167321 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167321&action=review > > is this covered by tests? ie, the restore from history? This assert is currently triggered by the QML tests, layout tests don't have the page cache enabled. > > > Source/WebCore/page/Frame.cpp:780 > > +void Frame::createView(const IntSize& viewportSize, const Color& backgroundColor, bool transparent, > > + const IntSize& fixedLayoutSize, const IntRect& fixedVisibleContentRect , > > + bool useFixedLayout, ScrollbarMode horizontalScrollbarMode, bool horizontalLock, > > + ScrollbarMode verticalScrollbarMode, bool verticalLock) > > Shouldnt this not just be one line That would be a pretty long line and I think it's more readable like this, I just aligned it to 4-spaces to make the style check happy. > > > Source/WebCore/page/Frame.h:88 > > - void createView(const IntSize&, const Color&, bool, const IntSize&, bool, > > - ScrollbarMode = ScrollbarAuto, bool horizontalLock = false, > > + void createView(const IntSize&, const Color&, bool, > > + const IntSize& fixedLayoutSize = IntSize(), const IntRect& fixedVisibleContentRect = IntRect(), > > + bool useFixedLayout = false, ScrollbarMode = ScrollbarAuto, bool horizontalLock = false, > > ScrollbarMode = ScrollbarAuto, bool verticalLock = false); > > It is getting a bit confusing with all the defaults. Doesn't some of the fixed* ones imply others? ie. if you have a fixedlayoutsize isn't that the same as useFixedLayout == true? Yep !fixedLayoutSize.isEmpty() || !fixedVisibleContentRect.isEmpty() requires that useFixedLayout == true. I have to admit that this is ugly, ideally there would be a way to call those setters directly on the view from the outside in a way that wouldn't trigger a layout (before it gets attached) but this would be a much bigger change. I could remove the useFixedLayout = false and deduce it from the above, tell me what you think. > > > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:271 > > const QSize preferredLayoutSize = page->preferredContentsSize(); > > > > + > > ScrollbarMode hScrollbar = (ScrollbarMode) m_webFrame->scrollBarPolicy(Qt::Horizontal); > > unrelated change? Yep I'll get that out.
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 167321 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=167321&action=review > > > > is this covered by tests? ie, the restore from history? > This assert is currently triggered by the QML tests, layout tests don't have the page cache enabled. > > > > > > Source/WebCore/page/Frame.cpp:780 > > > +void Frame::createView(const IntSize& viewportSize, const Color& backgroundColor, bool transparent, > > > + const IntSize& fixedLayoutSize, const IntRect& fixedVisibleContentRect , > > > + bool useFixedLayout, ScrollbarMode horizontalScrollbarMode, bool horizontalLock, > > > + ScrollbarMode verticalScrollbarMode, bool verticalLock) > > > > Shouldnt this not just be one line > That would be a pretty long line and I think it's more readable like this, I just aligned it to 4-spaces to make the style check happy. I know and your IDE should wrap it for you :-) I think that is the current webkit style; dont wrap these lines. > > It is getting a bit confusing with all the defaults. Doesn't some of the fixed* ones imply others? ie. if you have a fixedlayoutsize isn't that the same as useFixedLayout == true? > Yep !fixedLayoutSize.isEmpty() || !fixedVisibleContentRect.isEmpty() requires that useFixedLayout == true. > I have to admit that this is ugly, ideally there would be a way to call those setters directly on the view from the outside in a way that wouldn't trigger a layout (before it gets attached) but this would be a much bigger change. > I could remove the useFixedLayout = false and deduce it from the above, tell me what you think. Better fix it as you suggested in another patch.
(In reply to comment #5) > I know and your IDE should wrap it for you :-) I think that is the current webkit style; dont wrap these lines. I personally find it confusing to have auto-wrapping enabled in the editor, and it's not specified in the WebKit style doc. I'd prefer to leave it like this. > > Yep !fixedLayoutSize.isEmpty() || !fixedVisibleContentRect.isEmpty() requires that useFixedLayout == true. > > I have to admit that this is ugly, ideally there would be a way to call those setters directly on the view from the outside in a way that wouldn't trigger a layout (before it gets attached) but this would be a much bigger change. > > I could remove the useFixedLayout = false and deduce it from the above, tell me what you think. > > Better fix it as you suggested in another patch. I'll try it out and see if it doesn't mess things up too much, that function reaches out to every single platform.
(In reply to comment #6) > I'll try it out and see if it doesn't mess things up too much, that function reaches out to every single platform. After investigation, the only clean way I came up with to reduce the number of parameters of Frame::createView is to allow passing a self-created FrameView to it. The compromises that I see this would imply: - The platforms code would now be responsible to handle some logic details like creating the FrameView using the viewportSize if it's the main frame rather than letting createView handle it, etc. - This would open a door where a second FrameView would be active on a Frame since we can't call setView(0) before the creation. Overall createView is just a convenience method for stuff that needs to happen on an interdependent Frame and FrameView pair when the load is about to commit. I don't think that having too many parameters passed to it outweighs the above issues, especially the second one. If you have a better idea let me know, otherwise I think that keeping those parameters would work better. One way I see to make this less of a mess would be not to pass the useFixedLayout boolean and then call "frameView->setUseFixedLayout(!fixedLayoutSize.isEmpty());" I also started looking at removing ScrollView::setUseFixedLayout completely and rely only on setFixedLayoutSize, though some chromium tests at least seem to be using setUseFixedLayout alone (and they have it exposed on their WebView). It would also make it less clear that giving an empty/non-empty size to setFixedLayoutSize would disable/trigger this "mode" of the FrameView.
Created attachment 169596 [details] Patch It took some time to look at it and I think adding this parameter is the cleanest solution. Splitting createView would create too much code duplication accross platforms code. The difference with the previous patch are the fixed exports and the unrelated empty line removed.
Comment on attachment 169596 [details] Patch Clearing flags on attachment: 169596 Committed r132054: <http://trac.webkit.org/changeset/132054>
All reviewed patches have been landed. Closing bug.