Bug 53707

Created attachment 82739 [details] [PATCH] Patch and Tests for Error messages I don't have the ability to run tests on any of the ports that actually run fast/viewport tests. Also, there might be something I specifically need to do in order to enable CONSOLE output in these tests. But the tests are as follows: 1: <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=2"> no warnings. Works as expected. 2: <meta name="viewport" content="wwidth=100"> ERROR: Viewport argument key "wwidth" not recognized and ignored. 3: <meta name="viewport" content="width=unrecognized-width"> ERROR: Viewport argument value "unrecognized-width" for key "width" not recognized. Content ignored. 4. <meta name="viewport" content="width=123x456"> TIP: Viewport argument value "123x456" for key "width" was truncated to its numeric prefix. 5. <meta name="viewport" content="width=320, height=352"> TIP: Viewport width or height set to physical device width, try using "device-width" constant instead for future compatibility. TIP: Viewport height or height set to physical device height, try using "device-height" constant instead for future compatibility. 6. <meta name="viewport" content="width=device-width; initial-scale=1.0; maximum-scale=1.0; user-scalable=0;"> ERROR: Viewport argument value "device-width;" for key "width" not recognized. Content ignored. TIP: Viewport argument value "1.0;" for key "initial-scale" was truncated to its numeric prefix. TIP: Viewport argument value "1.0;" for key "maximum-scale" was truncated to its numeric prefix. TIP: Viewport argument value "0;" for key "user-scalable" was truncated to its numeric prefix.

Comment on attachment 82739 [details] [PATCH] Patch and Tests for Error messages View in context: https://bugs.webkit.org/attachment.cgi?id=82739&action=review > LayoutTests/ChangeLog:8 > + Added tests specifically to test Console warnings. Great stuff!

To land, this requires changes from a patch on: <http://webkit.org/b/53705> Viewport parsing no longer accepts "1.0;" value as valid. Also, I want to verify that I don't have to do anything special for these tests to output console messages. It would be useless if these tests didn't print the console warnings in the dump render tree results!

Created attachment 83231 [details] [PATCH] One more test for a missing value. Kenneth, is it okay if I add one more test to the mix, for a missing value? This is a patch showing the change to the diff (the extra test).

The test I'm proposing is: 7. <meta name="viewport" content="width="> ERROR: Viewport argument value "" for key "width" not recognized. Content ignored.

Arg, the expected results for all of these tests are going to be blank. That is because CONSOLE MESSAGES only make it to the DumpRenderTree delegate if they are JSMessageSource messages and these messages are HTMLMessageSource, because they come from parsing the HTML. if (source == JSMessageSource) page->chrome()->client()->addMessageToConsole(source, type, level, message, lineNumber, sourceURL); I think its still worth keeping the tests. But it will be weird checking in blank expected results. I can add private delegates, but that seems like a lot of code that isn't used by anyone. I could also make these manual tests. Any preferences?

I just opened: <http://webkit.org/b/54926> All Console Messages should be passed to ChromeClients. I'll block on that, as these tests are useless without warnings in the expected results.

(In reply to comment #4) > Created an attachment (id=83231) [details] > [PATCH] One more test for a missing value. > > Kenneth, is it okay if I add one more test to the mix, for a missing value? > This is a patch showing the change to the diff (the extra test). Sure, please go ahead!

(In reply to comment #7) > I just opened: > <http://webkit.org/b/54926> All Console Messages should be passed to ChromeClients. > > I'll block on that, as these tests are useless without warnings in the expected results. I think this would be the right thing to do

Landed in r80068: http://trac.webkit.org/changeset/80068 Watching the bots for expected results for viewport tests.

Build fix for Qt in: http://trac.webkit.org/changeset/80073

Created attachment 84338 [details] [FIX] Poor Build Fix solution for Qt WebKit2 Not up for review, since it was recommended I rollout.

Rolled out r80068 and r80073 in r80077: http://trac.webkit.org/changeset/80077 The fast/viewport tests will be reporting a lot of incorrect failures until this is fixed. Any recommendations on WebKit2 Qt developers would be appreciated. The [FIX] attachment I have above is a poor approach. Maybe we should drop these tips altogether, the Document is only used for tipping users about the "device-width" and "device-height" constants. Removing these tips would simplify the patch.

CC'ing some Qt developers to see if they have a suggestion.

Balazs, could you help fixing Qt-WK2 build?

(In reply to comment #13) > Rolled out r80068 and r80073 in r80077: > http://trac.webkit.org/changeset/80077 > > The fast/viewport tests will be reporting a lot of incorrect failures until > this is fixed. Any recommendations on WebKit2 Qt developers would > be appreciated. The [FIX] attachment I have above is a poor approach. > Maybe we should drop these tips altogether, the Document is only > used for tipping users about the "device-width" and "device-height" > constants. Removing these tips would simplify the patch. Currently there is no public qt-wk2 bot so I think the patch with the build fix is good as it is if it not breaks tests with wk1. We should find an way to fix this for wk2 or remove the tips as you suggested.

Also see http://trac.webkit.org/changeset/80107 and http://trac.webkit.org/changeset/80114.

(In reply to comment #13) > The fast/viewport tests will be reporting a lot of incorrect failures until > this is fixed. Any recommendations on WebKit2 Qt developers would > be appreciated. The [FIX] attachment I have above is a poor approach. > Maybe we should drop these tips altogether, the Document is only > used for tipping users about the "device-width" and "device-height" > constants. Removing these tips would simplify the patch. Another approach would be to turn off tips by default, but provide a way to enable them for individual tests so that we can still test them!

Before I was just trying to match old behavior, but now that I am forced to look deeper at this I think we can make things a little saner. These tips are unique in that they need to know the deviceWidth and deviceHeight known above WebCore by the WebKit clients. They also need a console to add the tip too inside WebCore, but that is a more minor detail. Currently the deviceWidth and deviceHeight are not known at parsing time, but are known when computeViewportAttributes is called. In Qt by accessing environmental variables like getintenv("QTWEBKIT_DEVICE_WIDTH"), and in GTK by the width/height of the window rect accessed via windowRect(webView->priv->corePage->chrome()->windowRect()). I think the ideal solution is to have the deviceWidth and deviceHeight at the time that we parse the viewport arguments. I suggest adding an accessor on ChromeClient along the lines of: ChromeClient.h: virtual FloatRect viewportDeviceRect() = 0; The advantages of this approach are that: • the tips are created at parse time, like all other viewport warnings/tips/errors so we are safe to report viewport warnings to the console like normal. • we eliminate the possibility that the warning could be printed multiple times because there is nothing that prevents computeViewportAttributes from be called/calculated multiple times. While I'm at it, if we still want to keep this tip, I see there is a new keyword for "desktop-width". While I haven't investigated this yet, we could also add an accessor and a tip for this as well: ChromeClient.h: virtual float viewportDesktopWidth() = 0; New Tip when width constant matches viewportDesktopWidth(): TIP: Viewport width constant matches desktop width, try using the "desktop-width" constant instead for future compatibility. Finally, is the following really something we want to encourage? <meta name="viewport" content="width:device-height"> I was just thinking about matching old behavior, but I think we should only tip about "device-width" for width, and "device-height" for height. Do these suggestions sound good to you? I'll start a patch for the above, but as you've seen I'm not the best with other ports =).

(In reply to comment #19) > Before I was just trying to match old behavior, but now that I am forced to > look deeper at this I think we can make things a little saner. > > These tips are unique in that they need to know the deviceWidth and deviceHeight > known above WebCore by the WebKit clients. They also need a console to add the > tip too inside WebCore, but that is a more minor detail. > > Currently the deviceWidth and deviceHeight are not known at parsing time, > but are known when computeViewportAttributes is called. In Qt by accessing > environmental variables like getintenv("QTWEBKIT_DEVICE_WIDTH"), and in > GTK by the width/height of the window rect accessed via > windowRect(webView->priv->corePage->chrome()->windowRect()). These are hardcoded in our webkit2 port though. > I think the ideal solution is to have the deviceWidth and deviceHeight at the > time that we parse the viewport arguments. I suggest adding an accessor > on ChromeClient along the lines of: > > ChromeClient.h: > virtual FloatRect viewportDeviceRect() = 0; Any reason why not just to use a FloatSize? virtual FloatSize viewportDeviceSize() const = 0; (we want these to be const as well) Also keep in mind that we are doing a DPI adjustment (with my device currently 1.5, but I guess iPhone 4 uses 2.0 due to the retina display). You will want an accessor for that as well virtual float viewportDevicePixelRatio() const = 0. The above is not the best solution, as we currently have one with similar name, which is used to tell the web content what dpi adjustment factor was used (@media all and (-webkit-pixel-ratio)) So it will be better with one like virtual int viewportDeviceDPI() = 0; > > The advantages of this approach are that: > > • the tips are created at parse time, like all other viewport warnings/tips/errors > so we are safe to report viewport warnings to the console like normal. > • we eliminate the possibility that the warning could be printed multiple times > because there is nothing that prevents computeViewportAttributes from be > called/calculated multiple times. > > > While I'm at it, if we still want to keep this tip, I see there is a new keyword for > "desktop-width". While I haven't investigated this yet, we could also add an > accessor and a tip for this as well: The desktop-width is what is used for laying out desktop pages (pages with no viewport). The iPhone uses 980, IE on Windows Phone 7 uses 1024, and Android had 3 values to choose from (I believe the former plus 800 which I believe is default) > > ChromeClient.h: > virtual float viewportDesktopWidth() = 0; > > New Tip when width constant matches viewportDesktopWidth(): > TIP: Viewport width constant matches desktop width, try using the "desktop-width" constant instead for future compatibility. > > > Finally, is the following really something we want to encourage? > > <meta name="viewport" content="width:device-height"> Nope because that is not a valid viewport meta tag :-) I guess you want a = instead of : Anyway, I'm not sure and it is currently supported, so why not? > I was just thinking about matching old behavior, but I think we should only > tip about "device-width" for width, and "device-height" for height. I do not feel strong about this. > Do these suggestions sound good to you? I'll start a patch for the above, but > as you've seen I'm not the best with other ports =). I think it sounds fine.

I still would like it possible to make the layout tests use different device-width and -height for testing the actual algorithm

> Any reason why not just to use a FloatSize? > > virtual FloatSize viewportDeviceSize() const = 0; (we want these to be const as well) Yes, that makes more sense and is what I intended. > > Finally, is the following really something we want to encourage? > > > > <meta name="viewport" content="width:device-height"> > > Nope because that is not a valid viewport meta tag :-) I guess you want a = instead of : Whoops, too much JavaScript lately. You're correct. > Anyway, I'm not sure and it is currently supported, so why not? It is supported, and should continue to be supported, but I'm not convinced that it should be encouraged with a console tip. If someone writes width="480" should we really say, try using "device-height" instead? I don't think so.

again with the incorrect syntax... I meant "width=480"

Created attachment 84525 [details] [PATCH] For Try Bots I didn't actually get much time to work on this today. This doesn't handle WebKit2, it just has the default FloatSize() of 0x0. Lets see how the bots do on this. I noticed everyone seems to use IntSizes despite device-width and height possibly being float values. I don't know why that is, but I stuck with FloatSize for the time being and just casted appropriately.

Attachment 84525 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:77: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:532: device_width is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:533: device_height is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 3 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 84525 [details] did not build on qt: Build output: http://queues.webkit.org/results/8078538

Attachment 84525 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8078579

Comment on attachment 84525 [details] [PATCH] For Try Bots View in context: https://bugs.webkit.org/attachment.cgi?id=84525&action=review > Source/WebCore/dom/ViewportArguments.cpp:226 > + FloatSize deviceSize = page->chrome()->client()->viewportDeviceSize(); In reality with our dpi adjustment, this should be something like FloatSize deviceSize = page->chrome()->client()->viewportDeviceSize() / pixelRatio; For instance on the iPhone 4, if im not wrong, device-width will report 320, there in reality it is 640. ie, 640 / 2.0 = 320. The Android extensions makes it possible to avoid this upscaling of the contents, by setting target-densitydpi = device-dpi, so we need to make sure that is parsed at this time.

(In reply to comment #28) > For instance on the iPhone 4, if im not wrong, device-width will > report 320, there in reality it is 640. ie, 640 / 2.0 = 320. > > The Android extensions makes it possible to avoid this upscaling of the > contents, by setting target-densitydpi = device-dpi, so we need to make > sure that is parsed at this time. That is an interesting point. So, given that, we would need to wait until both the target-densitydpi and width are parsed, so essentially after the entire meta tag parsing? Is that really necessary. <meta name="viewport" content="width=320, target-densitydpi=2.0"> <meta name="viewport" content="target-densitydpi=2.0, width=320"> With target-densitydpi does the "device-width" constant ever change, or does it just change its effective value?

style: > Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:77: Alphabetical sorting problem. > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:532: device_width is incorrectly named. Don't use underscores in your identifier names. > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:533: device_height is incorrectly named. Don't use underscores in your identifier names. Fixed. Updated the older efl style. qt: > Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp: In function ‘QSize WebCore::queryDeviceSizeForScreenContainingWidget(const QWidget*)’: > Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:724: error: incomplete type ‘QApplication’ used in nested name specifier > Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp: In member function ‘virtual WebCore::FloatSize WebCore::ChromeClientQt::viewportDeviceSize() const’: > Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:759: error: ‘d’ was not declared in this scope Fixed. Included qapplication and d is m_webPage->d. efl: > fatal: Unable to create '/mnt/eflews/git/webkit/.git/index.lock': File exists. git issue. gtk: > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:114: error: 'FloatSize' does not name a type > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:654: error: no 'WebCore::FloatSize WebKit::ChromeClient::viewportDeviceSize() const' member function declared in class 'WebKit::ChromeClient' Fixed. Forgot namespace, so WebCore::FloatSize.

Created attachment 84592 [details] [PATCH] Try For Bots Take 2.

(In reply to comment #29) > (In reply to comment #28) > > For instance on the iPhone 4, if im not wrong, device-width will > > report 320, there in reality it is 640. ie, 640 / 2.0 = 320. > > > > The Android extensions makes it possible to avoid this upscaling of the > > contents, by setting target-densitydpi = device-dpi, so we need to make > > sure that is parsed at this time. > > That is an interesting point. So, given that, we would need to wait until > both the target-densitydpi and width are parsed, so essentially after > the entire meta tag parsing? Is that really necessary. > > <meta name="viewport" content="width=320, target-densitydpi=2.0"> > <meta name="viewport" content="target-densitydpi=2.0, width=320"> > > With target-densitydpi does the "device-width" constant ever change, > or does it just change its effective value? In the above case it is not necessary as you are using fixed values. But in the case of using device-width is it. <meta name="viewport" content="width=device-width, target-densitydpi=160"> on a 480 wide device substitutes device-width with 320, due to the higher dpi of 240, where as <meta name="viewport" content="width=device-width, target-densitydpi=240"> substitutes device-width with 480. The target-densitydpi only affects device-width and device-height substitution as well as sets the pixelRatio (target-densitydpi / 160; default is 160). Google calls a pixel at 160 DPI a density independent pixel. The fact is that the original iPhone has a DPI of 160 which resulted in everyone adapting their web pages and web apps to that. On a phone with a higher DPI such as 240 the buttons etc become too small, so they are scaled up. Even the iPhone 4 scaled everything up with 2.0 when there is a viewport meta tag present. With the target-densitydpi features, it is possible to actually take advantage of the increased DPI by for instance disabling the upscaling and using different CSS using media queries such as @media all and (-webkit-pixel-ratio... ) { }

> In the above case it is not necessary as you are using fixed values. > But in the case of using device-width is it. If the user is already using the "device-width" keyword, we don't need to provide a tip that they are using the keyword. The code change I'm adding would never be reached in that case, only when there is a numeric constant. > <meta name="viewport" content="width=device-width, target-densitydpi=160"> > > on a 480 wide device substitutes device-width with 320, due to the higher dpi of 240, where as > > <meta name="viewport" content="width=device-width, target-densitydpi=240"> > > substitutes device-width with 480. Your descriptions sound backwards to the meta tags. Are they? I see what you're saying, but in that case my question above still applies. I'll rephrase it below with your examples (assuming they were backwards) with the description. On a 480px wide device, are you suggesting that both of following should print a tip that the width constant could be device-width? <meta name="viewport" content="width=480, target-densitydpi=160"> <meta name="viewport" content="width=320, target-densitydpi=240">

Attachment 84592 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8085334

Created attachment 84617 [details] [PATCH] For Try Bots Take 3.

(In reply to comment #33) > > In the above case it is not necessary as you are using fixed values. > > But in the case of using device-width is it. > > If the user is already using the "device-width" keyword, we don't need > to provide a tip that they are using the keyword. The code change I'm > adding would never be reached in that case, only when there is a > numeric constant. Good point, and I guess we could do with that, but when I think about it <meta name="viewport" content="width=320, target-densitydpi=160"> should yield a warning to use device-width, but so should <meta name="viewport" content="width=480, target-densitydpi=240"> and <meta name="viewport" content="width=480, target-densitydpi=device-dpi"> for a device with dpi 240 and device-width 480. It all comes down to how useful these warnings are. You could say that if someone knows how to use target-densitydpi, he surely also knows how to use device-width. It could even be that the user actually wanted 480 always and just wanted the scale to change to fit the 480 into the given view. I have seem many such cases. With that in mind, we could even show the warning always when someone uses a real number value instead of the device-* defines. > > <meta name="viewport" content="width=device-width, target-densitydpi=160"> > > > > on a 480 wide device substitutes device-width with 320, due to the higher dpi of 240, where as > > > > <meta name="viewport" content="width=device-width, target-densitydpi=240"> > > > > substitutes device-width with 480. > > Your descriptions sound backwards to the meta tags. Are they? Sorry, what do you mean? > I see what you're saying, but in that case my question above still applies. > I'll rephrase it below with your examples (assuming they were backwards) > with the description. > > On a 480px wide device, are you suggesting that both of following should > print a tip that the width constant could be device-width? > > <meta name="viewport" content="width=480, target-densitydpi=160"> > <meta name="viewport" content="width=320, target-densitydpi=240"> Yes, if 160 and 240 were swapped: :-) device-width with target-densitydpi=240 on a 480px wide device is == 480, hense <meta name="viewport" content="width=480, target-densitydpi=240"> <meta name="viewport" content="width=320, target-densitydpi=160">

> <meta name="viewport" content="width=480, target-densitydpi=240"> > <meta name="viewport" content="width=320, target-densitydpi=160"> To explain myself better. By default we assume we are a 160 dpi device (as the original iPhone). ie. device-width for a 160 dpi device is considered 320, which fits with the original iPhone. But the device has a real width of 480 and a real dpi of 240. Hense using device-dpi or 240 should yield a device-width of 480, ie. it's actual width. The calculo is: device-width = real width / (target-densitydpi / 160) = 480 / (240 / 160) = 480 / 1.5 = 320. I hope that clears it up.

> The calculo is: device-width = real width / (target-densitydpi / 160) = 480 / (240 / 160) = 480 / 1.5 = 320. Strip that :-( I wrote it wrong a) without target-densitydpi argument (assuming target-densitydpi of 160) device-width = real width / (real dpi / target-densitydpi) = 480 / (240 / 160) = 480 / 1.5 = 320. b) with target-densitydpi = device-dpi (which is 240) device-width = real width / (real dpi / target-densitydpi) = 480 / (240 / 240) = 480 / 1.0 = 480.

> a) without target-densitydpi argument (assuming target-densitydpi of 160) > > device-width = real width / (real dpi / target-densitydpi) = 480 / (240 / 160) = 480 / 1.5 = 320. > > b) with target-densitydpi = device-dpi (which is 240) > > device-width = real width / (real dpi / target-densitydpi) = 480 / (240 / 240) = 480 / 1.0 = 480. Okay, that is much clearer, Thanks! > It all comes down to how useful these warnings are. You could say that if > someone knows how to use target-densitydpi, he surely also knows how > to use device-width. It could even be that the user actually wanted 480 > always and just wanted the scale to change to fit the 480 into the given > view. I have seem many such cases. In that case, it sounds to me like, with target-densitydpi in the mix, these tips aren't as useful as they used to be. I'm fine with removing them. It would greatly simplify the patch as well because I wouldn't need to touch all the other ports. However, if we want to improve the tip, what would be a good suggestion? After Document::processViewport processes the ViewportArguments, we could look at the end result and present some tips or warnings. I can't think of any good ones off the top of my head.

Created attachment 84657 [details] [PATCH] Remove Confusing Tips - Improve Warnings + Errors I decided to just remove them. Much simpler patch in the end. After landing this I would need to rebase all viewport tests for gtk/qt + add expected results for the new tests.

Good work Joseph. I have a few viewport related patches that I will try upstreaming soonish. I will cc you to those.

Landed in r80483: http://trac.webkit.org/changeset/80483 I'll be watching the bots to pull expected test results for LayoutTests/fast/viewport/*. Many are expected to change and some (added in this patch) will be new results.

http://trac.webkit.org/changeset/80483 might have broken Qt Linux Release The following tests are not passing: fast/viewport/viewport-100.html fast/viewport/viewport-101.html fast/viewport/viewport-102.html fast/viewport/viewport-103.html fast/viewport/viewport-104.html fast/viewport/viewport-105.html fast/viewport/viewport-106.html fast/viewport/viewport-107.html fast/viewport/viewport-108.html fast/viewport/viewport-109.html fast/viewport/viewport-110.html fast/viewport/viewport-111.html fast/viewport/viewport-112.html fast/viewport/viewport-113.html fast/viewport/viewport-114.html fast/viewport/viewport-115.html fast/viewport/viewport-116.html fast/viewport/viewport-117.html fast/viewport/viewport-118.html fast/viewport/viewport-129.html fast/viewport/viewport-29.html fast/viewport/viewport-30.html fast/viewport/viewport-31.html fast/viewport/viewport-32.html fast/viewport/viewport-35.html fast/viewport/viewport-36.html fast/viewport/viewport-38.html fast/viewport/viewport-39.html fast/viewport/viewport-40.html fast/viewport/viewport-41.html fast/viewport/viewport-42.html fast/viewport/viewport-43.html fast/viewport/viewport-44.html fast/viewport/viewport-47.html fast/viewport/viewport-48.html fast/viewport/viewport-49.html fast/viewport/viewport-61.html fast/viewport/viewport-62.html fast/viewport/viewport-63.html fast/viewport/viewport-64.html fast/viewport/viewport-66.html fast/viewport/viewport-67.html fast/viewport/viewport-68.html fast/viewport/viewport-69.html fast/viewport/viewport-70.html fast/viewport/viewport-71.html fast/viewport/viewport-72.html fast/viewport/viewport-73.html fast/viewport/viewport-74.html fast/viewport/viewport-75.html fast/viewport/viewport-76.html fast/viewport/viewport-77.html fast/viewport/viewport-78.html fast/viewport/viewport-79.html fast/viewport/viewport-80.html fast/viewport/viewport-81.html fast/viewport/viewport-83.html fast/viewport/viewport-85.html fast/viewport/viewport-88.html fast/viewport/viewport-90.html

Expected Results Landed in r80490: http://trac.webkit.org/changeset/80490 I'll mark this as closed now, but keep an eye out for any outlying viewport tests that other ports test but were not tested by the Qt port.