Bug 46385 – Keep viewport information in Document

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Luiz Agostini]

For viewport meta tag information to be kept by page cache it needs to be in Document class.

[Luiz Agostini]

Created attachment 68564 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 68564 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68564&action=review

> WebCore/ChangeLog:8
> +        For viewport meta tag information to be kept by page cache it needs to be in Document class.

*in the*.

Add some more info, like "The viewport meta data (layout viewport, scale) needs to be set again when bringing back a page from the page cache. As the viewport data are only retrieved while parsing the document, we need to store it in the Document class." 

As the viewport meta data is only retrieved during parsing,

> WebCore/dom/Document.cpp:3815
> +            frame()->page()->chrome()->client()->didReceiveViewportArguments(frame(), m_viewportArguments);

I think we need to rename didReceiveViewportArguments so better describe that it does. maybe dispatchViewportChangeRequested or similar?

> WebCore/dom/Document.h:280
> +    ViewportArguments viewportArguments() const { return m_viewportArguments; }

I think it is about time to change ViewportArguments to something like ViewportMetaData, but that can be in another patch.

> WebCore/dom/Document.h:1319
> +    ViewportArguments m_viewportArguments;

Maybe the m_valid in ViewportArguments doesn't make sense anymore

[Luiz Agostini]

(In reply to comment #2)
> (From update of attachment 68564 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68564&action=review
> 
> > WebCore/ChangeLog:8
> > +        For viewport meta tag information to be kept by page cache it needs to be in Document class.
> 
> *in the*.
> 
> Add some more info, like "The viewport meta data (layout viewport, scale) needs to be set again when bringing back a page from the page cache. As the viewport data are only retrieved while parsing the document, we need to store it in the Document class." 
> 
> As the viewport meta data is only retrieved during parsing,

ok

> 
> > WebCore/dom/Document.cpp:3815
> > +            frame()->page()->chrome()->client()->didReceiveViewportArguments(frame(), m_viewportArguments);
> 
> I think we need to rename didReceiveViewportArguments so better describe that it does. maybe dispatchViewportChangeRequested or similar?

As it is now I think that didReceiveViewportArguments is a better description of what is is going on then dispatchViewportChangeRequested.
I agree that it could be changed but I would prefer to do it in a future patch.

> 
> > WebCore/dom/Document.h:280
> > +    ViewportArguments viewportArguments() const { return m_viewportArguments; }
> 
> I think it is about time to change ViewportArguments to something like ViewportMetaData, but that can be in another patch.

ok

> 
> > WebCore/dom/Document.h:1319
> > +    ViewportArguments m_viewportArguments;
> 
> Maybe the m_valid in ViewportArguments doesn't make sense anymore

It is only in qt api.

[Luiz Agostini]

Created attachment 68593 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 68593 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68593&action=review

LGTM, r=me

> WebKit/qt/Api/qwebframe.cpp:235
> +    if (!frame || !frame->document())

Don't you always have a frame here?

[Daniel Bates]

Comment on attachment 68593 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68593&action=review

> WebKit/qt/Api/qwebframe_p.h:104
> +    WebCore::ViewportArguments viewportArguments();

There shouldn't be parentheses here.

It looks like you did a find-and-replace for viewportArguments and inadvertently replaced viewportArguments with viewportArguments() here.

[Daniel Bates]

Comment on attachment 68593 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68593&action=review

> WebCore/dom/Document.cpp:3813
> +        if (!frame() || !frame()->page())
> +            frame()->page()->chrome()->client()->didReceiveViewportArguments(frame(), m_viewportArguments);

This doesn't look correct. Why are you calling frame() if it's a null pointer? Similarly, if frame()->page() is null.

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> (From update of attachment 68593 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68593&action=review
> 
> > WebCore/dom/Document.cpp:3813
> > +        if (!frame() || !frame()->page())
> > +            frame()->page()->chrome()->client()->didReceiveViewportArguments(frame(), m_viewportArguments);
> 
> This doesn't look correct. Why are you calling frame() if it's a null pointer? Similarly, if frame()->page() is null.

Auch! I didn't catch this. This should obviously have been frame() && frame()->page()

[Kenneth Rohde Christiansen]

Luiz, can't you do a test where you load another page and then goes back to the original one?

[Luiz Agostini]

(In reply to comment #6)
> (From update of attachment 68593 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68593&action=review
> 
> > WebKit/qt/Api/qwebframe_p.h:104
> > +    WebCore::ViewportArguments viewportArguments();
> 
> There shouldn't be parentheses here.
> 
> It looks like you did a find-and-replace for viewportArguments and inadvertently replaced viewportArguments with viewportArguments() here.

Actually the parentesis are needed. Viewport arguments were stored in QWebFramePrivate but it is not any more. That is exactly what this patch is about. The viewport arguments are now stored in WebCore::Document and a method in QWebFramePrivate is used to retrieve it from document.

[Luiz Agostini]

(In reply to comment #9)
> Luiz, can't you do a test where you load another page and then goes back to the original one?

I did it in QtTestBrowser. But it seems that no page cache is going on in it.

My mistake anyway.

[Daniel Bates]

(In reply to comment #10)
> (In reply to comment #6)
> > (From update of attachment 68593 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=68593&action=review
> > 
> > > WebKit/qt/Api/qwebframe_p.h:104
> > > +    WebCore::ViewportArguments viewportArguments();
> > 
> > There shouldn't be parentheses here.
> > 
> > It looks like you did a find-and-replace for viewportArguments and inadvertently replaced viewportArguments with viewportArguments() here.
> 
> Actually the parentesis are needed. Viewport arguments were stored in QWebFramePrivate but it is not any more. That is exactly what this patch is about. The viewport arguments are now stored in WebCore::Document and a method in QWebFramePrivate is used to retrieve it from document.

Oops. You're right, you added the function QWebFramePrivate::viewportArguments().

[Luiz Agostini]

Created attachment 68627 [details]
patch

Now with layout test.

[Eric Seidel (no email)]

Attachment 68627 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4074095

[Kenneth Rohde Christiansen]

Comment on attachment 68627 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68627&action=review

A comment in the test about what it is testing would be nice.

> LayoutTests/fast/viewport/viewport-128.html:4
> +        <meta name="viewport" content="width=100 ;initial-scale=1">

I guess you should remove the space before ; and add one after.

> LayoutTests/fast/viewport/viewport-128.html:17
> +                    setTimeout('window.location = "data:text/html,<script>history.back()<" + "/script>"', 0);

Why the " + " ?

> LayoutTests/fast/viewport/viewport-128.html:19
> +                    layoutTestController.dumpConfigurationForViewport(320, 352);

How can we make sure this is reusing the viewportArguments and that they havent't been reparsed?

> LayoutTests/fast/viewport/viewport-128.html:24
> +            window.onpageshow = pageshow;

Pretty cool trick :-) just read up on the onpageshow

[Kenneth Rohde Christiansen]

Comment on attachment 68627 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68627&action=review

> WebKit/qt/Api/qwebframe_p.h:104
>      int marginHeight;
>      bool zoomTextOnly;
> -    WebCore::ViewportArguments viewportArguments;
> +    WebCore::ViewportArguments viewportArguments();

You should move the new method up so that it is grouped with the other methods and so that we have fields at the end of the private class.

[Luiz Agostini]

Created attachment 68714 [details]
patch

[Luiz Agostini]

> > LayoutTests/fast/viewport/viewport-128.html:17
> > +                    setTimeout('window.location = "data:text/html,<script>history.back()<" + "/script>"', 0);
> 
> Why the " + " ?

Without it the </script> whould be considered the end of the script

> 
> > LayoutTests/fast/viewport/viewport-128.html:19
> > +                    layoutTestController.dumpConfigurationForViewport(320, 352);
> 
> How can we make sure this is reusing the viewportArguments and that they havent't been reparsed?

Because of evt.persisted. It it is true then the page is coming from page cache.

[Luiz Agostini]

committed r68292:<http://trac.webkit.org/changeset/68292>

[Ademar Reis]

Revision r68292 cherry-picked into qtwebkit-2.1 with commit 90ea290 <http://gitorious.org/webkit/qtwebkit/commit/90ea290>