CLOSED FIXED 36312
Support viewport meta tag
https://bugs.webkit.org/show_bug.cgi?id=36312
Summary Support viewport meta tag
Yong Li
Reported 2010-03-18 11:21:46 PDT
More and more mobile web developers are using "viewport" meta tag now. It's initially for iPhone only, but some other mobile platforms/browsers have also supported it (like Android), and some others may want this feature, too. To keep "write once, run everywhere" experience, this feature should be added to WebKit upstream in order to discourage fragmentation in WebKit.
Attachments
the patch (12.01 KB, patch)
2010-03-18 11:27 PDT, Yong Li
no flags
fix conflicts and style errors (11.84 KB, patch)
2010-03-18 11:51 PDT, Yong Li
no flags
fix one more style error (11.85 KB, patch)
2010-03-18 12:53 PDT, Yong Li
koivisto: review-
Patch 1 of 2 (12.44 KB, patch)
2010-03-22 12:31 PDT, Daniel Bates
no flags
Patch 2 of 2: Mac and Qt DRT changes and test case (22.11 KB, patch)
2010-03-22 12:47 PDT, Daniel Bates
no flags
Patch 2 of 2: Mac and Qt DRT changes and test case (22.11 KB, patch)
2010-03-22 14:37 PDT, Daniel Bates
no flags
Patch 1 of 2 (12.44 KB, patch)
2010-03-25 11:50 PDT, Daniel Bates
no flags
Patch 2 of 2: Mac and Qt DRT changes and test case (22.10 KB, patch)
2010-03-25 11:52 PDT, Daniel Bates
no flags
Patch (22.20 KB, patch)
2010-03-26 09:38 PDT, Daniel Bates
no flags
Patch (22.17 KB, patch)
2010-03-26 09:48 PDT, Daniel Bates
no flags
Patch (22.20 KB, patch)
2010-03-26 10:48 PDT, Daniel Bates
no flags
Patch (22.20 KB, patch)
2010-03-26 10:57 PDT, Daniel Bates
manyoso: review+
Yong Li
Comment 1 2010-03-18 11:27:23 PDT
Created attachment 51061 [details] the patch
Yong Li
Comment 3 2010-03-18 11:51:19 PDT
Created attachment 51066 [details] fix conflicts and style errors
WebKit Review Bot
Comment 4 2010-03-18 11:52:07 PDT
Attachment 51066 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/dom/Document.cpp:138: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Yong Li
Comment 5 2010-03-18 12:53:46 PDT
Created attachment 51075 [details] fix one more style error
Antti Koivisto
Comment 6 2010-03-19 09:17:27 PDT
Comment on attachment 51075 [details] fix one more style error This will need tests. DRT can be modified to act as a client.
Daniel Bates
Comment 7 2010-03-22 12:31:53 PDT
Created attachment 51332 [details] Patch 1 of 2 Renamed method receiveViewportArguments to dispatchDidReceiveViewportArguments to be more descriptive and to conform to the naming conventions for similar methods in FrameLoaderClient. Added #ifdef/#endif META_VIEWPORT guard around #include ViewportArguments.h and forward declaration in files WebCore/dom/Document.cpp and WebCore/loader/FrameLoaderClient.h, respectively.Fixed some style issues that the style bot missed.
Daniel Bates
Comment 8 2010-03-22 12:47:47 PDT
Created attachment 51336 [details] Patch 2 of 2: Mac and Qt DRT changes and test case Implements DRT changes to support testing META_VIEWPORT for the Mac, and Qt ports. I am unclear how to generate a uuid for a Windows IDL so as to add a similar WebViewportArguments structure for the Windows port and connect up to DRT. Does anybody know how to generate this? From my understanding a similar structure is needed to integrate with GTK DRT. I took a look at some of the GTK Api files (such as webkitwebresource.h) and found the structure convoluted. I take it I would have to define my structure and modify webkitdefines.h? If any GTK folks have any suggestions/insight that would be appreciated.
WebKit Review Bot
Comment 9 2010-03-22 12:53:27 PDT
Attachment 51336 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/mac/WebView/WebViewportArguments.h:34: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Daniel Bates
Comment 10 2010-03-22 14:37:15 PDT
Created attachment 51356 [details] Patch 2 of 2: Mac and Qt DRT changes and test case Fix style error.
George Staikos
Comment 11 2010-03-24 19:24:38 PDT
Comment on attachment 51332 [details] Patch 1 of 2 To be honest, I don't like passing ViewportArguments as a reference because it's not clear what it does. Passing by pointer would be nicer. Beyond that I'm okay but given that others showed review interest as well I'll leave it for a little while for more review before r+.
Kenneth Rohde Christiansen
Comment 12 2010-03-24 21:27:55 PDT
Simon, Antti, Laszlo, shouldn't we enable this for Maemo and Symbian?
Kenneth Rohde Christiansen
Comment 13 2010-03-24 21:44:07 PDT
Comment on attachment 51356 [details] Patch 2 of 2: Mac and Qt DRT changes and test case > +#if ENABLE(META_VIEWPORT) > +void FrameLoaderClientQt::dispatchDidReceiveViewportArguments(const WebCore::ViewportArguments&) > +{ > + if (dumpFrameLoaderCallbacks) > + printf("%s - didReceiveViewportArgumentsForFrame\n", qPrintable(drtDescriptionSuitableForTestResult(m_frame))); > + > + notImplemented(); > +} > +#endif Is Qt supposed to have the notImplemented() here?
Daniel Bates
Comment 14 2010-03-25 10:20:50 PDT
(In reply to comment #11) > (From update of attachment 51332 [details]) > To be honest, I don't like passing ViewportArguments as a reference because > it's not clear what it does. Passing by pointer would be nicer. Beyond that > I'm okay but given that others showed review interest as well I'll leave it for > a little while for more review before r+. Will change.
Daniel Bates
Comment 15 2010-03-25 10:31:52 PDT
(In reply to comment #13) > (From update of attachment 51356 [details]) > > > +#if ENABLE(META_VIEWPORT) > > +void FrameLoaderClientQt::dispatchDidReceiveViewportArguments(const WebCore::ViewportArguments&) > > +{ > > + if (dumpFrameLoaderCallbacks) > > + printf("%s - didReceiveViewportArgumentsForFrame\n", qPrintable(drtDescriptionSuitableForTestResult(m_frame))); > > + > > + notImplemented(); > > +} > > +#endif > > Is Qt supposed to have the notImplemented() here? I can remove it. When implementing this method I followed from such methods as FrameLoaderClientQt::didDisplayInsecureContent and FrameLoader::didRunInsecureContent. No specific implementation is provided in these patches. So, I left the notImplemented() line. Let me know if I should remove it.
Daniel Bates
Comment 16 2010-03-25 11:50:33 PDT
Created attachment 51666 [details] Patch 1 of 2 Changed argument of method FrameLoaderClient::dispatchDidReceiveViewportArguments from reference to pointer based on George's comment.
Daniel Bates
Comment 17 2010-03-25 11:52:35 PDT
Created attachment 51667 [details] Patch 2 of 2: Mac and Qt DRT changes and test case Updated patch to reflect change of argument type in FrameLoaderClient::dispatchDidReceiveViewportArguments from reference to pointer.
Alexey Proskuryakov
Comment 18 2010-03-25 14:04:27 PDT
As discussed on IRC, we should consider aligning this with existing iPhone code.
Daniel Bates
Comment 19 2010-03-26 09:38:09 PDT
Created attachment 51752 [details] Patch Updated patch to conform to the related code used in the WebCore-528.15 source published as part of the iPhone 3.1.3 source code <http://www.opensource.apple.com/source/WebCore/WebCore-528.15/>. As far as I can tell from the publicly available source code, the method Document::processArguments can be made private, so I made this change (David Kilzer will this cause any issues for you?). I also made various style corrections so as to conform to the WebKit Code Style Guidelines. We can further cleanup/refactor this code and add DRT support. I would suggest that we do these in separate bugs. I would appreciate both Maciej Stachowiak's and David Kilzer's feedback regarding the inclusion of this code into the general WebKit project.
WebKit Review Bot
Comment 20 2010-03-26 09:42:14 PDT
Attachment 51752 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/dom/Document.cpp:2347: Declaration has space between type name and * in Frame *frame [whitespace/declaration] [3] WebCore/dom/ViewportArguments.cpp:57: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Daniel Bates
Comment 21 2010-03-26 09:48:05 PDT
Created attachment 51753 [details] Patch Fixed style errors found by style bot. Also, removed extraneous whitespace introduced by changes to files Android.mk and GNUmakefile.am.
Daniel Bates
Comment 22 2010-03-26 09:53:18 PDT
Forgot to mention, I changed references to the function GSMainScreenSize() in method Document::setViewportFeature in the iPhone 3.1.3 source to document->page()->chrome()->windowRect(). This may not be correct. So, we may want to consider adding a method to Chrome/ChromeClient, say screenSize() to return the equivalent of GSMainScreenSize() for the platform.
WebKit Review Bot
Comment 23 2010-03-26 10:39:44 PDT
Daniel Bates
Comment 24 2010-03-26 10:48:25 PDT
Created attachment 51758 [details] Patch Attempt to fix Chromium build issue.
WebKit Review Bot
Comment 25 2010-03-26 10:51:58 PDT
Attachment 51758 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/dom/ViewportArguments.cpp:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Daniel Bates
Comment 26 2010-03-26 10:57:22 PDT
Created attachment 51761 [details] Patch Fix style issue.
David Kilzer (:ddkilzer)
Comment 27 2010-03-28 17:34:22 PDT
(In reply to comment #19) > As far as I can tell from the publicly available source code, the method > Document::processArguments can be made private, so I made this change (David > Kilzer will this cause any issues for you?). I also made various style > corrections so as to conform to the WebKit Code Style Guidelines. That's fine. > We can further cleanup/refactor this code and add DRT support. I would suggest > that we do these in separate bugs. > > I would appreciate both Maciej Stachowiak's and David Kilzer's feedback > regarding the inclusion of this code into the general WebKit project. I defer to Maciej on this.
Eric Seidel (no email)
Comment 28 2010-03-29 11:38:19 PDT
Comment on attachment 51356 [details] Patch 2 of 2: Mac and Qt DRT changes and test case Cleared George Staikos's review+ from obsolete attachment 51356 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Maciej Stachowiak
Comment 29 2010-04-16 16:44:41 PDT
Neither Dave Kilzer nor I have any objections to including this code in WebKit nor are we aware of anyone else objecting. I'm not an expert on this area of code myself, so I am not sure if I would be the best reviewer.
Adam Treat
Comment 30 2010-04-16 17:16:23 PDT
Comment on attachment 51761 [details] Patch As there are no objections from maciej or ddkilzer r=me with the caveat that future patch will add appropriate tests.
Daniel Bates
Comment 31 2010-04-16 21:40:37 PDT
Simon Hausmann
Comment 32 2010-04-20 13:00:58 PDT
Revision r57775 cherry-picked into qtwebkit-2.0 with commit d686f2086018398a39350705a8326b1adf88ad4a
Note You need to log in before you can comment on or make changes to this bug.