| Summary: | Keep viewport information in Document | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Luiz Agostini <luiz> | ||||||||||
| Component: | WebCore Misc. | Assignee: | Luiz Agostini <luiz> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | ademar, commit-queue, dbates, eric, kenneth, tonikitoo | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Luiz Agostini
2010-09-23 12:12:51 PDT
Created attachment 68564 [details]
patch
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 (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. Created attachment 68593 [details]
patch
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 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 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. (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() Luiz, can't you do a test where you load another page and then goes back to the original one? (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. (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. (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(). Created attachment 68627 [details]
patch
Now with layout test.
Attachment 68627 [details] did not build on mac: Build output: http://queues.webkit.org/results/4074095 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 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. Created attachment 68714 [details]
patch
> > 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. committed r68292:<http://trac.webkit.org/changeset/68292> Revision r68292 cherry-picked into qtwebkit-2.1 with commit 90ea290 <http://gitorious.org/webkit/qtwebkit/commit/90ea290> |