Bug 98514 – [Qt] Fix "ASSERTION FAILED: !document->inPageCache()" when loading a page

• Component: New Bugs
• Status: RESOLVED FIXED

[Jocelyn Turcotte]

[Qt] Fix "ASSERTION FAILED: !document->inPageCache()" when loading a page

[Jocelyn Turcotte]

Created attachment 167321 [details]
Patch

[Build Bot]

Comment on attachment 167321 [details]
Patch

Attachment 167321 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14176534

[Kenneth Rohde Christiansen]

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?

[Jocelyn Turcotte]

(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.

[Kenneth Rohde Christiansen]

(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.

[Jocelyn Turcotte]

(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.

[Jocelyn Turcotte]

(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.

[Jocelyn Turcotte]

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.

[WebKit Review Bot]

Comment on attachment 169596 [details]
Patch

Clearing flags on attachment: 169596

Committed r132054: <http://trac.webkit.org/changeset/132054>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.