Bug 80554

Created attachment 130738 [details] Patch

No new tests (already causes fixed-layout-360x240.html to fail when ENABLE(VIEWPORT) is set). That's because it treats the layout test as if it were a desktop page, and sets the fixed layout accordingly, avoiding the setting of the test. Do we want to avoid setting fixed layout manually in tests and instead test the viewport tag?

(In reply to comment #2) > No new tests (already causes fixed-layout-360x240.html to fail when > ENABLE(VIEWPORT) is set). > > That's because it treats the layout test as if it were a desktop page, and sets the fixed layout accordingly, avoiding the setting of the test. Do we want to avoid setting fixed layout manually in tests and instead test the viewport tag? Why would we not always have access to the screen DPI? This seems like a property independent of layout...

(In reply to comment #3) > (In reply to comment #2) > > No new tests (already causes fixed-layout-360x240.html to fail when > > ENABLE(VIEWPORT) is set). > > > > That's because it treats the layout test as if it were a desktop page, and sets the fixed layout accordingly, avoiding the setting of the test. Do we want to avoid setting fixed layout manually in tests and instead test the viewport tag? Hmm, on a second look, it's more of a coincidence that this test was failing. It was actually failing on the ASSERT(dpi > 0) because the test harness didn't provide a value for it. I think we can leave that test as is (as long as we get rid of the assertion), but I'll see if there's a way to reliably test the actual bug I saw. > > Why would we not always have access to the screen DPI? This seems like a property independent of layout... True, but we currently ask the browser process on Android, mainly to avoid code duplication. Because we also need to go to the browser process for the window size anyway, it doesn't seem like there's any benefit to guaranteeing the dpi value is always available.

> Hmm, on a second look, it's more of a coincidence that this test was failing. It was actually failing on the ASSERT(dpi > 0) because the test harness didn't provide a value for it. I think we can leave that test as is (as long as we get rid of the assertion), but I'll see if there's a way to reliably test the actual bug I saw. > Hmm, I recall seeing it fail when viewport was enabled as well... > > Why would we not always have access to the screen DPI? This seems like a property independent of layout... > > True, but we currently ask the browser process on Android, mainly to avoid code duplication. Because we also need to go to the browser process for the window size anyway, it doesn't seem like there's any benefit to guaranteeing the dpi value is always available. Do we need to go to the browser for window size? Can we not use the WebView's size, instead?

(In reply to comment #5) > > > Why would we not always have access to the screen DPI? This seems like a property independent of layout... > > > > True, but we currently ask the browser process on Android, mainly to avoid code duplication. Because we also need to go to the browser process for the window size anyway, it doesn't seem like there's any benefit to guaranteeing the dpi value is always available. > > Do we need to go to the browser for window size? Can we not use the WebView's size, instead? My (second-hand) understanding is that the browser is the decider of window sizes. For example, in a popup ad in Windows, WebKit will start loading the content from the network at the same time as a request is issued to Windows to create/position a window such that it fits on the screen. So the window size may not be available to the WebView early in loading. Of course, on mobile the situation is typically simpler than that, but we have the same architecture.

(In reply to comment #6) > (In reply to comment #5) > > > > Why would we not always have access to the screen DPI? This seems like a property independent of layout... > > > > > > True, but we currently ask the browser process on Android, mainly to avoid code duplication. Because we also need to go to the browser process for the window size anyway, it doesn't seem like there's any benefit to guaranteeing the dpi value is always available. > > > > Do we need to go to the browser for window size? Can we not use the WebView's size, instead? > > My (second-hand) understanding is that the browser is the decider of window sizes. For example, in a popup ad in Windows, WebKit will start loading the content from the network at the same time as a request is issued to Windows to create/position a window such that it fits on the screen. So the window size may not be available to the WebView early in loading. Of course, on mobile the situation is typically simpler than that, but we have the same architecture. One of my concerns is IPCs are fairly expensive operations and this method is called fairly frequently. I was hoping to send out a patch to get rid of the use of IPCs here, but I didn't study the implications of this change enough.

(In reply to comment #7) > One of my concerns is IPCs are fairly expensive operations and this method is called fairly frequently. I was hoping to send out a patch to get rid of the use of IPCs here, but I didn't study the implications of this change enough. Is it really that frequent? It seems like after this CL, it should be called at most twice on initialization. Anyway, I think it should be possible to invert the IPC relationship in the embedder so that the browser sends the size to the renderer, instead of having a sync IPC the other way. But for the purposes of the code in WebKit I'm changing here, I don't think that changes the fact that the GetWindowRect function may sometimes return no value.

Created attachment 131483 [details] Patch

I added a unit test and also a small fix to the navigation reset logic (found after trying out the change some more). PTAL.

Created attachment 135181 [details] Patch

Uploaded a new patch with a few more tweaks. I left in the dpi assertion after all, as we are now guaranteed to get it on render process initialization on Android, so that resolves that debate.

Created attachment 135190 [details] Patch

After discussing with Fady, removed the initial-scale related fixes from this patch, I'll make a separate bug for them.

Created attachment 135233 [details] Patch

I moved the test into the initial-scale bug http://webk.it/82949 where it's a better fit.

Created attachment 138486 [details] Patch

I added another call site to WebViewImpl::resize() as that's another place where the inputs to the viewport calculation can change. I also switched the guards to use the new viewportEnabled setting.

Comment on attachment 138486 [details] Patch Attachment 138486 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12519271 New failing tests: WebFrameTest.DeviceScaleFactorUsesDefaultWithoutViewportTag

Created attachment 138575 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 138615 [details] Patch

Fady, are you happy with this patch now?

(In reply to comment #22) > Fady, are you happy with this patch now? LGTM! Thanks! :-)

Comment on attachment 138615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138615&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:1304 > + Document* document = mainFrameImpl()->frame()->document(); > + if (document) { The frame should always have a document. > Source/WebKit/chromium/src/WebViewImpl.cpp:1306 > + ViewportArguments viewportArguments = document->viewportArguments(); > + m_page->chrome()->client()->dispatchViewportPropertiesDidChange(viewportArguments); These lines aren't indented correctly. WebKit uses four-space indent. > Source/WebKit/chromium/src/WebViewImpl.cpp:2966 > + if (!document) > + return; This should be impossible.

Created attachment 141060 [details] Patch

All done.

Comment on attachment 141060 [details] Patch Sorry for the delay.

Comment on attachment 141060 [details] Patch Clearing flags on attachment: 141060 Committed r117170: <http://trac.webkit.org/changeset/117170>

All reviewed patches have been landed. Closing bug.

I am a bit afraid that removing this from FrameView will break Qt. I am cc'ing Zalan to see if this is the case. Zalan it might make sense to write a autotest similar to the one apparently asserting for Chromium. That should be possible using my new UI side testing framework.

(In reply to comment #30) > I am a bit afraid that removing this from FrameView will break Qt. I am cc'ing Zalan to see if this is the case. It was initially added for Chromium in bug 80554 (December), hopefully no other ports have come to rely on it.

(In reply to comment #31) > (In reply to comment #30) > > I am a bit afraid that removing this from FrameView will break Qt. I am cc'ing Zalan to see if this is the case. > > It was initially added for Chromium in bug 80554 (December), hopefully no other ports have come to rely on it. Sorry, I meant bug 73495 :)