Bug 46385 - Keep viewport information in Document
Summary: Keep viewport information in Document
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Luiz Agostini
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-23 12:12 PDT by Luiz Agostini
Modified: 2010-09-27 12:29 PDT (History)
6 users (show)

See Also:


Attachments
patch (9.44 KB, patch)
2010-09-23 12:27 PDT, Luiz Agostini
kenneth: review-
Details | Formatted Diff | Diff
patch (9.94 KB, patch)
2010-09-23 14:36 PDT, Luiz Agostini
dbates: review-
dbates: commit-queue-
Details | Formatted Diff | Diff
patch (12.89 KB, patch)
2010-09-23 17:14 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch (15.09 KB, patch)
2010-09-24 11:06 PDT, Luiz Agostini
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luiz Agostini 2010-09-23 12:12:51 PDT
For viewport meta tag information to be kept by page cache it needs to be in Document class.
Comment 1 Luiz Agostini 2010-09-23 12:27:17 PDT
Created attachment 68564 [details]
patch
Comment 2 Kenneth Rohde Christiansen 2010-09-23 12:34:48 PDT
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
Comment 3 Luiz Agostini 2010-09-23 14:25:44 PDT
(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.
Comment 4 Luiz Agostini 2010-09-23 14:36:34 PDT
Created attachment 68593 [details]
patch
Comment 5 Kenneth Rohde Christiansen 2010-09-23 14:42:37 PDT
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?
Comment 6 Daniel Bates 2010-09-23 14:57:34 PDT
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.
Comment 7 Daniel Bates 2010-09-23 15:05:00 PDT
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.
Comment 8 Kenneth Rohde Christiansen 2010-09-23 15:09:11 PDT
(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()
Comment 9 Kenneth Rohde Christiansen 2010-09-23 15:11:57 PDT
Luiz, can't you do a test where you load another page and then goes back to the original one?
Comment 10 Luiz Agostini 2010-09-23 15:14:23 PDT
(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.
Comment 11 Luiz Agostini 2010-09-23 15:16:01 PDT
(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.
Comment 12 Daniel Bates 2010-09-23 15:24:05 PDT
(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().
Comment 13 Luiz Agostini 2010-09-23 17:14:29 PDT
Created attachment 68627 [details]
patch

Now with layout test.
Comment 14 Eric Seidel 2010-09-23 18:22:53 PDT
Attachment 68627 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4074095
Comment 15 Kenneth Rohde Christiansen 2010-09-23 18:58:25 PDT
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
Comment 16 Kenneth Rohde Christiansen 2010-09-23 19:22:12 PDT
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.
Comment 17 Luiz Agostini 2010-09-24 11:06:04 PDT
Created attachment 68714 [details]
patch
Comment 18 Luiz Agostini 2010-09-24 11:19:57 PDT
> > 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.
Comment 19 Luiz Agostini 2010-09-27 06:54:51 PDT
committed r68292:<http://trac.webkit.org/changeset/68292>
Comment 20 Ademar Reis 2010-09-27 12:29:06 PDT
Revision r68292 cherry-picked into qtwebkit-2.1 with commit 90ea290 <http://gitorious.org/webkit/qtwebkit/commit/90ea290>