Bug 36312 - Support viewport meta tag
: Support viewport meta tag
Status: CLOSED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 35784
  Show dependency treegraph
 
Reported: 2010-03-18 11:21 PST by
Modified: 2010-05-20 13:16 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-03-18 11:21:46 PST
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 From 2010-03-18 11:27:23 PST -------
Created an attachment (id=51061) [details]
the patch
------- Comment #3 From 2010-03-18 11:51:19 PST -------
Created an attachment (id=51066) [details]
fix conflicts and style errors
------- Comment #4 From 2010-03-18 11:52:07 PST -------
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 From 2010-03-18 12:53:46 PST -------
Created an attachment (id=51075) [details]
fix one more style error
------- Comment #6 From 2010-03-19 09:17:27 PST -------
(From update of attachment 51075 [details])
This will need tests. DRT can be modified to act as a client.
------- Comment #7 From 2010-03-22 12:31:53 PST -------
Created an attachment (id=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 From 2010-03-22 12:47:47 PST -------
Created an attachment (id=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 From 2010-03-22 12:53:27 PST -------
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 From 2010-03-22 14:37:15 PST -------
Created an attachment (id=51356) [details]
Patch 2 of 2: Mac and Qt DRT changes and test case

Fix style error.
------- Comment #11 From 2010-03-24 19:24:38 PST -------
(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+.
------- Comment #12 From 2010-03-24 21:27:55 PST -------
Simon, Antti, Laszlo, shouldn't we enable this for Maemo and Symbian?
------- Comment #13 From 2010-03-24 21:44:07 PST -------
(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?
------- Comment #14 From 2010-03-25 10:20:50 PST -------
(In reply to comment #11)
> (From update of attachment 51332 [details] [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 From 2010-03-25 10:31:52 PST -------
(In reply to comment #13)
> (From update of attachment 51356 [details] [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 From 2010-03-25 11:50:33 PST -------
Created an attachment (id=51666) [details]
Patch 1 of 2

Changed argument of method FrameLoaderClient::dispatchDidReceiveViewportArguments from reference to pointer based on George's comment.
------- Comment #17 From 2010-03-25 11:52:35 PST -------
Created an attachment (id=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 From 2010-03-25 14:04:27 PST -------
As discussed on IRC, we should consider aligning this with existing iPhone code.
------- Comment #19 From 2010-03-26 09:38:09 PST -------
Created an attachment (id=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 From 2010-03-26 09:42:14 PST -------
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 From 2010-03-26 09:48:05 PST -------
Created an attachment (id=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 From 2010-03-26 09:53:18 PST -------
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 From 2010-03-26 10:39:44 PST -------
Attachment 51753 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1458001
------- Comment #24 From 2010-03-26 10:48:25 PST -------
Created an attachment (id=51758) [details]
Patch

Attempt to fix Chromium build issue.
------- Comment #25 From 2010-03-26 10:51:58 PST -------
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 From 2010-03-26 10:57:22 PST -------
Created an attachment (id=51761) [details]
Patch

Fix style issue.
------- Comment #27 From 2010-03-28 17:34:22 PST -------
(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 From 2010-03-29 11:38:19 PST -------
(From update of attachment 51356 [details])
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 From 2010-04-16 16:44:41 PST -------
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 From 2010-04-16 17:16:23 PST -------
(From update of attachment 51761 [details])
As there are no objections from maciej or ddkilzer r=me with the caveat that future patch will add appropriate tests.
------- Comment #31 From 2010-04-16 21:40:37 PST -------
Committed r57775: <http://trac.webkit.org/changeset/57775>
------- Comment #32 From 2010-04-20 13:00:58 PST -------
Revision r57775 cherry-picked into qtwebkit-2.0 with commit d686f2086018398a39350705a8326b1adf88ad4a