Bug 36312 - Support viewport meta tag
Summary: Support viewport meta tag
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yong Li
URL:
Keywords:
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-03-18 11:21 PDT by Yong Li
Modified: 2010-05-20 13:16 PDT (History)
16 users (show)

See Also:


Attachments
the patch (12.01 KB, patch)
2010-03-18 11:27 PDT, Yong Li
no flags Details | Formatted Diff | Diff
fix conflicts and style errors (11.84 KB, patch)
2010-03-18 11:51 PDT, Yong Li
no flags Details | Formatted Diff | Diff
fix one more style error (11.85 KB, patch)
2010-03-18 12:53 PDT, Yong Li
koivisto: review-
Details | Formatted Diff | Diff
Patch 1 of 2 (12.44 KB, patch)
2010-03-22 12:31 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Patch 1 of 2 (12.44 KB, patch)
2010-03-25 11:50 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Patch (22.20 KB, patch)
2010-03-26 09:38 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (22.17 KB, patch)
2010-03-26 09:48 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (22.20 KB, patch)
2010-03-26 10:48 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (22.20 KB, patch)
2010-03-26 10:57 PDT, Daniel Bates
manyoso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 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.
Comment 1 Yong Li 2010-03-18 11:27:23 PDT
Created attachment 51061 [details]
the patch
Comment 3 Yong Li 2010-03-18 11:51:19 PDT
Created attachment 51066 [details]
fix conflicts and style errors
Comment 4 WebKit Review Bot 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.
Comment 5 Yong Li 2010-03-18 12:53:46 PDT
Created attachment 51075 [details]
fix one more style error
Comment 6 Antti Koivisto 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.
Comment 7 Daniel Bates 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.
Comment 8 Daniel Bates 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.
Comment 9 WebKit Review Bot 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.
Comment 10 Daniel Bates 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.
Comment 11 George Staikos 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+.
Comment 12 Kenneth Rohde Christiansen 2010-03-24 21:27:55 PDT
Simon, Antti, Laszlo, shouldn't we enable this for Maemo and Symbian?
Comment 13 Kenneth Rohde Christiansen 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?
Comment 14 Daniel Bates 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.
Comment 15 Daniel Bates 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.
Comment 16 Daniel Bates 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.
Comment 17 Daniel Bates 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.
Comment 18 Alexey Proskuryakov 2010-03-25 14:04:27 PDT
As discussed on IRC, we should consider aligning this with existing iPhone code.
Comment 19 Daniel Bates 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.
Comment 20 WebKit Review Bot 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.
Comment 21 Daniel Bates 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.
Comment 22 Daniel Bates 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.
Comment 23 WebKit Review Bot 2010-03-26 10:39:44 PDT
Attachment 51753 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1458001
Comment 24 Daniel Bates 2010-03-26 10:48:25 PDT
Created attachment 51758 [details]
Patch

Attempt to fix Chromium build issue.
Comment 25 WebKit Review Bot 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.
Comment 26 Daniel Bates 2010-03-26 10:57:22 PDT
Created attachment 51761 [details]
Patch

Fix style issue.
Comment 27 David Kilzer (:ddkilzer) 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.
Comment 28 Eric Seidel (no email) 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.
Comment 29 Maciej Stachowiak 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.
Comment 30 Adam Treat 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.
Comment 31 Daniel Bates 2010-04-16 21:40:37 PDT
Committed r57775: <http://trac.webkit.org/changeset/57775>
Comment 32 Simon Hausmann 2010-04-20 13:00:58 PDT
Revision r57775 cherry-picked into qtwebkit-2.0 with commit d686f2086018398a39350705a8326b1adf88ad4a