|
|
Created:
4 years, 6 months ago by nainar Modified:
4 years, 4 months ago CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, chromium-reviews, Eric Willigers, rjwright, shans Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionResize is being triggered too early in Print code.
Currently the ResizeForPrinting() function is called too early in
StartPrinting(). The resize causes transitions to be triggered. This
change delays the call to ResizeForPrinting() till after the call to
frame()->printBegin().
BUG=567317
Committed: https://crrev.com/a7760a764855fb5678f7e5e88e11b834416ffbb7
Cr-Commit-Position: refs/heads/master@{#410175}
Patch Set 1 #Patch Set 2 #Patch Set 3 : Added Browser test in src/components #Patch Set 4 : Landing with a Manual Test #
Messages
Total messages: 42 (14 generated)
Description was changed from ========== Fix + manual test - need feedback to make a layout test BUG= ========== to ========== Resize is being triggered too early in Print code. Currently the ResizeForPrinting() function is called too early in StartPrinting(). The resize causes transitions to be triggered. This change delays the call to ResizeForPrinting() till after the call to frame()->printBegin(). BUG=567317 ==========
nainar@chromium.org changed reviewers: + esprehn@chromium.org
Hi Elliott! Could you take a look at this patch - I have moved the call to Resize to later. Should I instead move the code to inside the call to frame()->printBegin()? Also having trouble writing a Layout Test. Any help you can give would be awesome! :)
On 2016/06/23 at 00:52:54, nainar wrote: > Hi Elliott! > > Could you take a look at this patch - I have moved the call to Resize to later. > Should I instead move the code to inside the call to frame()->printBegin()? > Also having trouble writing a Layout Test. Any help you can give would be awesome! :) I think you need to write a content browser test, I'd look at the other printing tests out in content. Hopefully we can move this code into blink someday and write smaller unit tests.
Any luck with this? :) Do you want some help with someone experienced in content browser tests?
Hi Elliott! This is how far I have gotten so far. I have added the test in src/component browser test land cause the change was made here and the browser test I need to add best matches the preexisting ones in this folder as opposed to the ones in src/content and src/chrome where other print browser tests are. The problem I am having is while the ManualTest that I have left attached here fails in current Chrome and passes with my change I can't seem to replicate that using a Browser test (or even a LayoutTest.) This browser test as it is written passes regardless of my change. My guess would be that I am missing something about the way these tests are run - with what dimensions, or something like that that causes the browser test to be different. Any ideas in this regard?
esprehn@chromium.org changed reviewers: + dcheng@chromium.org
Hey dcheng@ could you help nainar@ write a test for this bug fix? She's trying to make our printing code better. :)
dcheng@chromium.org changed reviewers: + thestig@chromium.org
+thestig, who know more about the print machinery than I do.
The CQ bit was checked by nainar@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.
Is the ResizeHappensAfterPrintingBegins test case expected to fail with or without the fix? It also passes for me either way. Will look and see why that is.
On 2016/08/02 21:17:44, Lei Zhang wrote: > Is the ResizeHappensAfterPrintingBegins test case expected to fail with or > without the fix? It also passes for me either way. Will look and see why that > is. To answer my own question, the manual test generates 2 pages before and 1 after, so the browser test is testing for the "after" result.
On 2016/08/02 at 22:56:42, thestig wrote: > On 2016/08/02 21:17:44, Lei Zhang wrote: > > Is the ResizeHappensAfterPrintingBegins test case expected to fail with or > > without the fix? It also passes for me either way. Will look and see why that > > is. > > To answer my own question, the manual test generates 2 pages before and 1 after, so the browser test is testing for the "after" result. Yup, the output with the fix should be: https://drive.google.com/open?id=0B4jZ37ju5_xGTUdPMGlyUEloRE5DNUQ0SC1fbHU2SmV... and the output without the fix should be: https://drive.google.com/open?id=0B4jZ37ju5_xGdVhaZEF0YlRYRHNmUHppbTd3RWZSTll.... The Manual test does show this distinction, however the browser test produces the preview similar to the Manual test with the fix (that is one page) regardless of my change.
So have you tried tracing the code to see what's happening? Keep in mind I know very little about Blink, so I'm just wondering around. I don't have any magical knowledge of the situation here. All I know on the Chromium side is the MockPrinter and the print settings in the tests are a bit different from what the actual browser sends, but I tried tweaking those without any luck. On the Blink side, the values of interest to me are the dimensions of |overflowRect| in LayoutView::documentRect(), and the page count calculation in PrintContext::computePageRectsWithPageSizeInternal(). Without the fix, the manual test in the browser hits PrepareFrameForPreviewDocument(). I see: - printBegin() gets called, and documentRect() has 739x978 as its value. Then computePageRectsWithPageSizeInternal() gets called with doc height and page height of 978, for a page count of 1. - printEnd() gets called, and now documentRect() has 930x1036 as its value. - Later in StartPrinting(), ResizeForPrinting() generates a documentRect() value of 540x1036 - And after ResizeForPrinting(), the documentRect() value becomes 739x1036. - computePageRectsWithPageSizeInternal gets called again, this time with doc height of 1036, and page height of 978, for a page count of 2. With the browser test: - printBegin() gets called, and documentRect() has 719x958 as its value. Then computePageRectsWithPageSizeInternal() gets called with doc height and page height of 958, for a page count of 1. - printEnd() gets called, and now documentRect() has 58x36 as its value. - Later in StartPrinting(), ResizeForPrinting() generates a documentRect() value of 525x944 -> 540x959 - And after ResizeForPrinting(), the documentRect() value becomes 719x958. - computePageRectsWithPageSizeInternal gets called again, this time with doc height of 958, and page height of 958, for a page count of 1. - How is the value in documentRect() calculated exactly? I have no idea. - How does CSS from the webpage affect the documentRect() calculations. I also have no idea. If you can answer these two questions, I can keep digging and try to pinpoint why the manual test and browser test are different.
Hi, Please find answers to your questions to the best of my ability below: - How is the value in documentRect() calculated exactly? When we go into printing Mode we force the LayoutView to paginate here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra... which is what sets the value of LayoutBox::m_frameRect, this value is then used here (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...) to return a documentRect value. This is the IntRect value associated with LayoutView associated with the document node. - How does CSS from the webpage affect the documentRect() calculations. During the Layout Tree Construction in Blink - given a Style recalc, we update the logicalWidth here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... depending on what LayoutObjects have to be changed due to style recalc.
On 2016/08/03 05:33:50, nainar wrote: > Hi, > > Please find answers to your questions to the best of my ability below: Ok, thanks. Once again, as someone who does not work on Blink, none of this makes any sense. I'm just treating this as a generic problem and doing printf() debugging to trace code until I find something interest. I can *try* but I can't promise any results.
So my theory is that in the browser test, after calling LoadHTML(), the document isn't laid out the same way as with the manual test. i.e. the "padding-top: 1000px;" rule probably isn't applied. (No idea why though) In the manual test, before OnPrintPreview() gets called, documentRect() returns 930x1036. In the browser test, before OnPrintPreview() gets called, documentRect() returns only 59x36. You can verify this by calling GetMainFrame()->contentsSize(); right after LoadHTML(). If you figure out why this is happening and fix it, that would probably make your browser test return 2 pages without the bug fix.
Will attempt to figure out what's going on and get back to you ASAP, hopefully with a working browser test.
Hi, This is unorthodox at best I realize, however since I am making slow progress on figuring out the testing issue, is it ok if I land this fix with a ManualTest and work on the Browser test as a follow on from that patch. I have confirmed that the bug fixes the test cases provided by users in Comment 13 on bug 567317, the original report in 579825 (duped against 567317) and Comment 9 in 604824 (also duped against 567317). I can provide the generated print previews if needed as well.
(drive by ... zoooom!) This plan sounds OK to me, as long as we have a filed bug about adding the browser test, referenced in the manual test, and a pinky-swear that it will be worked on as soon as practicable. There seems to be no controversy about the code fix itself, only about making sure it won't regress. Let's get our users fixed well before the m55 branch point, and then address any wider fixes in our testing code.
Probably ok until we figure out what's wrong with the automated test, since you tested manually to make sure the fix works. However, another unknown is how this affects printing of other webpages. Will this CL fix bug 567317 but trigger others? Sorry we don't have good automated testing coverage for printing. :( I also would like to understand how we arrive at this problem. This Chromium code in question has been essentially the same for years. Has the Chromium code been "wrong" this entire time? Why is it that the bug suddenly creeped in near the end of 2015?
The CQ bit was checked by nainar@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...
Causing regressions is definitely something I am concerned about. Which is why I would try to land the fix on Monday (Australian time) so that I can personally track and remain on top of any regressions that may be caused. The Chromium code has been correct, until I landed the patch here: https://codereview.chromium.org/1375833003 which cancels all transitions when we are currently printing or about to exit printing. Since resize also triggers transitions in the Blink side of the code it now too needs to be delayed until after we start printing so that the Blink side of the code can handle those transitions appropriately as well. This is why the problem seems to have arisen now rather than later. I would go so far as to guess that when the printing code was originally written there wasn't much happening in the form of transitions and animations on the Blink side, and now there has been a significant investment there. I have edited the ManualTest to reflect the bug to track my progress with the Browser test. Let me know if this looks good to land? Thanks so much for your help so far! :D
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'll try some manual testing of my own with this CL and see how that goes.
On 2016/08/05 02:51:53, Lei Zhang wrote: > I'll try some manual testing of my own with this CL and see how that goes. There are definitely differences, especially with tables. I can't decide if I like the rendering changes better with or without this CL. They are just different in many cases. e.g. https://en.wikipedia.org/wiki/Phobos_(moon) printed to PDF in landscape on A4. Page 3 is laid out different.
lgtm We can try landing this and see what bug reports we get in return.
Thank you for the lgtm and for the investigation! :) I will try to create a minimum repro of this and investigate further into the matter in the meanwhile. On Fri, Aug 5, 2016 at 3:50 PM <thestig@chromium.org> wrote: > lgtm > > We can try landing this and see what bug reports we get in return. > > https://codereview.chromium.org/2089373002/ > -- Regards, Naina -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Thank you for the lgtm and for the investigation! :) I will try to create a minimum repro of this and investigate further into the matter in the meanwhile. On Fri, Aug 5, 2016 at 3:50 PM <thestig@chromium.org> wrote: > lgtm > > We can try landing this and see what bug reports we get in return. > > https://codereview.chromium.org/2089373002/ > -- Regards, Naina -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
I'm going to CQ this and get it on Canary sooner than later. Then we keep our eyes open for bug reports...
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Resize is being triggered too early in Print code. Currently the ResizeForPrinting() function is called too early in StartPrinting(). The resize causes transitions to be triggered. This change delays the call to ResizeForPrinting() till after the call to frame()->printBegin(). BUG=567317 ========== to ========== Resize is being triggered too early in Print code. Currently the ResizeForPrinting() function is called too early in StartPrinting(). The resize causes transitions to be triggered. This change delays the call to ResizeForPrinting() till after the call to frame()->printBegin(). BUG=567317 Committed: https://crrev.com/a7760a764855fb5678f7e5e88e11b834416ffbb7 Cr-Commit-Position: refs/heads/master@{#410175} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a7760a764855fb5678f7e5e88e11b834416ffbb7 Cr-Commit-Position: refs/heads/master@{#410175} |