Bug 36312

Created attachment 51061 [details] the patch

Documentation: http://developer.apple.com/safari/library/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/MetaTags.html

Created attachment 51066 [details] fix conflicts and style errors

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.

Created attachment 51075 [details] fix one more style error

Comment on attachment 51075 [details] fix one more style error This will need tests. DRT can be modified to act as a client.

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.

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.

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.

Created attachment 51356 [details] Patch 2 of 2: Mac and Qt DRT changes and test case Fix style error.

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

Simon, Antti, Laszlo, shouldn't we enable this for Maemo and Symbian?

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?

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

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

Created attachment 51666 [details] Patch 1 of 2 Changed argument of method FrameLoaderClient::dispatchDidReceiveViewportArguments from reference to pointer based on George's comment.

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.

As discussed on IRC, we should consider aligning this with existing iPhone code.

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.

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.

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.

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.

Attachment 51753 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1458001

Created attachment 51758 [details] Patch Attempt to fix Chromium build issue.

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.

Created attachment 51761 [details] Patch Fix style issue.

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

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.

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.

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.

Committed r57775: <http://trac.webkit.org/changeset/57775>

Revision r57775 cherry-picked into qtwebkit-2.0 with commit d686f2086018398a39350705a8326b1adf88ad4a