Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(319)

Issue 2089373002: Resize is being triggered too early in Print code. (Closed)

Created:
4 years, 6 months ago by nainar
Modified:
4 years, 4 months ago
Reviewers:
Lei Zhang, esprehn, dcheng
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.

Description

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}

Patch Set 1 #

Patch Set 2 #

Patch Set 3 : Added Browser test in src/components #

Patch Set 4 : Landing with a Manual Test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -1 line) Patch
M components/printing/renderer/print_web_view_helper.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
A third_party/WebKit/ManualTests/printing-resize-starts-transition.html View 1 2 3 1 chunk +30 lines, -0 lines 0 comments Download

Messages

Total messages: 42 (14 generated)
nainar
Hi Elliott! Could you take a look at this patch - I have moved the ...
4 years, 6 months ago (2016-06-23 00:52:54 UTC) #3
esprehn
On 2016/06/23 at 00:52:54, nainar wrote: > Hi Elliott! > > Could you take a ...
4 years, 5 months ago (2016-07-12 02:37:37 UTC) #4
esprehn
Any luck with this? :) Do you want some help with someone experienced in content ...
4 years, 5 months ago (2016-07-19 00:20:20 UTC) #5
nainar
Hi Elliott! This is how far I have gotten so far. I have added the ...
4 years, 5 months ago (2016-07-19 04:32:21 UTC) #6
esprehn
Hey dcheng@ could you help nainar@ write a test for this bug fix? She's trying ...
4 years, 5 months ago (2016-07-19 06:19:56 UTC) #8
dcheng
+thestig, who know more about the print machinery than I do.
4 years, 5 months ago (2016-07-19 12:44:39 UTC) #10
Lei Zhang
Is the ResizeHappensAfterPrintingBegins test case expected to fail with or without the fix? It also ...
4 years, 4 months ago (2016-08-02 21:17:44 UTC) #15
Lei Zhang
On 2016/08/02 21:17:44, Lei Zhang wrote: > Is the ResizeHappensAfterPrintingBegins test case expected to fail ...
4 years, 4 months ago (2016-08-02 22:56:42 UTC) #16
nainar
On 2016/08/02 at 22:56:42, thestig wrote: > On 2016/08/02 21:17:44, Lei Zhang wrote: > > ...
4 years, 4 months ago (2016-08-02 23:45:59 UTC) #17
Lei Zhang
So have you tried tracing the code to see what's happening? Keep in mind I ...
4 years, 4 months ago (2016-08-03 03:17:46 UTC) #18
nainar
Hi, Please find answers to your questions to the best of my ability below: - ...
4 years, 4 months ago (2016-08-03 05:33:50 UTC) #19
Lei Zhang
On 2016/08/03 05:33:50, nainar wrote: > Hi, > > Please find answers to your questions ...
4 years, 4 months ago (2016-08-03 06:07:12 UTC) #20
Lei Zhang
So my theory is that in the browser test, after calling LoadHTML(), the document isn't ...
4 years, 4 months ago (2016-08-03 07:15:07 UTC) #21
nainar
Will attempt to figure out what's going on and get back to you ASAP, hopefully ...
4 years, 4 months ago (2016-08-03 08:11:30 UTC) #22
nainar
Hi, This is unorthodox at best I realize, however since I am making slow progress ...
4 years, 4 months ago (2016-08-04 23:46:16 UTC) #23
Mike Lawther (Google)
(drive by ... zoooom!) This plan sounds OK to me, as long as we have ...
4 years, 4 months ago (2016-08-05 00:09:37 UTC) #24
Lei Zhang
Probably ok until we figure out what's wrong with the automated test, since you tested ...
4 years, 4 months ago (2016-08-05 00:22:13 UTC) #25
nainar
Causing regressions is definitely something I am concerned about. Which is why I would try ...
4 years, 4 months ago (2016-08-05 01:35:03 UTC) #28
Lei Zhang
I'll try some manual testing of my own with this CL and see how that ...
4 years, 4 months ago (2016-08-05 02:51:53 UTC) #31
Lei Zhang
On 2016/08/05 02:51:53, Lei Zhang wrote: > I'll try some manual testing of my own ...
4 years, 4 months ago (2016-08-05 05:29:17 UTC) #32
Lei Zhang
lgtm We can try landing this and see what bug reports we get in return.
4 years, 4 months ago (2016-08-05 05:50:50 UTC) #33
blink-reviews
Thank you for the lgtm and for the investigation! :) I will try to create ...
4 years, 4 months ago (2016-08-05 07:30:29 UTC) #34
chromium-reviews
Thank you for the lgtm and for the investigation! :) I will try to create ...
4 years, 4 months ago (2016-08-05 07:30:29 UTC) #35
esprehn
lgtm
4 years, 4 months ago (2016-08-05 21:17:16 UTC) #36
Lei Zhang
I'm going to CQ this and get it on Canary sooner than later. Then we ...
4 years, 4 months ago (2016-08-05 21:45:04 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2089373002/60001
4 years, 4 months ago (2016-08-05 21:45:40 UTC) #39
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 4 months ago (2016-08-05 21:49:48 UTC) #40
commit-bot: I haz the power
4 years, 4 months ago (2016-08-05 21:52:36 UTC) #42
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/a7760a764855fb5678f7e5e88e11b834416ffbb7
Cr-Commit-Position: refs/heads/master@{#410175}

Powered by Google App Engine
This is Rietveld 408576698