Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2643903002:
Fix viewport unit sizes when printing. (Closed)
DescriptionFix viewport unit sizes when printing.
FrameView::viewportSizeForViewportUnits had a special path for inert top
controls that used FrameView::layoutSize directly. The old path used
LayoutView::viewWidth and LayoutView::viewHeight which normally resolve to
FrameView::layoutSize but has a special case for when we're in printing mode.
This patch uses the LayoutView version and only special cases the adjustment
made for the URL bar.
BUG=678414
Review-Url: https://codereview.chromium.org/2643903002
Cr-Commit-Position: refs/heads/master@{#444803}
Committed: https://chromium.googlesource.com/chromium/src/+/b9d998089be2a4ee8a304730a814386168f84641
Patch Set 1 #Patch Set 2 : Updated rune@'s test instead #
Total comments: 6
Patch Set 3 : Fix comment #
Messages
Total messages: 23 (15 generated)
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix viewport unit sizes when printing. FrameView::viewportSizeForViewportUnits had a special path for inert top controls that used FrameView::layoutSize directly. The old path used LayoutView::viewWidth and LayoutView::viewHeight which normally resolve to FrameView::layoutSize but has a special case for when we're in printing mode. This patch uses the LayoutView version and only special cases the adjustment made for the URL bar. BUG=678414 ========== to ========== Fix viewport unit sizes when printing. FrameView::viewportSizeForViewportUnits had a special path for inert top controls that used FrameView::layoutSize directly. The old path used LayoutView::viewWidth and LayoutView::viewHeight which normally resolve to FrameView::layoutSize but has a special case for when we're in printing mode. This patch uses the LayoutView version and only special cases the adjustment made for the URL bar. BUG=678414 ==========
bokan@chromium.org changed reviewers: + skobes@chromium.org
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/01/18 18:43:33, bokan wrote: > ptal Actually, hold off while I figure out the broken tests.
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bokan@chromium.org changed reviewers: + rune@opera.com - skobes@chromium.org
-skobes@ +rune@ since it seems you've recently dealt with a similar issue. I think the values in your test were wrong because of the issue fixed by this patch. I've updated it and used the same calculations to get the same rounding behavior. Wdyt?
On 2017/01/19 00:29:08, bokan wrote: > -skobes@ +rune@ since it seems you've recently dealt with a similar issue. I > think the values in your test were wrong because of the issue fixed by this > patch. I've updated it and used the same calculations to get the same rounding > behavior. Wdyt? Yes, the printing code temporarily changes the size of the LayoutView without changing the m_layoutSize when generating the print layout tree. That should be looked into at some point to see if we can do that more consistent and robust.
lgtm with nit(s). https://codereview.chromium.org/2643903002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2643903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4339: " body { margin: 0px; }" This one's here to avoid #vw overflowing the page, I guess? https://codereview.chromium.org/2643903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4359: constexpr float minimumShrinkFactor = 1.333f; Would be nice to have a common constant. But then again, if it's only used internally in PrintContext and in unit tests, it might not be natural to expose it through PrintContext.h. I'm fine with either. https://codereview.chromium.org/2643903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4361: // The expcted layout size comes from the calculation done in expcted -> expected
https://codereview.chromium.org/2643903002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2643903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4339: " body { margin: 0px; }" On 2017/01/19 08:59:24, rune wrote: > This one's here to avoid #vw overflowing the page, I guess? It's to make the calculation more straightforwardly related to the pageSize, the margin isn't included in the layout size. https://codereview.chromium.org/2643903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4359: constexpr float minimumShrinkFactor = 1.333f; On 2017/01/19 08:59:24, rune wrote: > Would be nice to have a common constant. But then again, if it's only used > internally in PrintContext and in unit tests, it might not be natural to expose > it through PrintContext.h. I'm fine with either. Acknowledged. https://codereview.chromium.org/2643903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4361: // The expcted layout size comes from the calculation done in On 2017/01/19 08:59:24, rune wrote: > expcted -> expected Done.
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rune@opera.com Link to the patchset: https://codereview.chromium.org/2643903002/#ps40001 (title: "Fix comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484843208544080,
"parent_rev": "5c34fe289869ccd38b3cf39add76b00e2c51cb97", "commit_rev":
"b9d998089be2a4ee8a304730a814386168f84641"}
Message was sent while issue was closed.
Description was changed from ========== Fix viewport unit sizes when printing. FrameView::viewportSizeForViewportUnits had a special path for inert top controls that used FrameView::layoutSize directly. The old path used LayoutView::viewWidth and LayoutView::viewHeight which normally resolve to FrameView::layoutSize but has a special case for when we're in printing mode. This patch uses the LayoutView version and only special cases the adjustment made for the URL bar. BUG=678414 ========== to ========== Fix viewport unit sizes when printing. FrameView::viewportSizeForViewportUnits had a special path for inert top controls that used FrameView::layoutSize directly. The old path used LayoutView::viewWidth and LayoutView::viewHeight which normally resolve to FrameView::layoutSize but has a special case for when we're in printing mode. This patch uses the LayoutView version and only special cases the adjustment made for the URL bar. BUG=678414 Review-Url: https://codereview.chromium.org/2643903002 Cr-Commit-Position: refs/heads/master@{#444803} Committed: https://chromium.googlesource.com/chromium/src/+/b9d998089be2a4ee8a304730a814... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b9d998089be2a4ee8a304730a814... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
