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

Issue 1575473002: Improve syncing between dom distiller and the print preview request (Closed)

Created:
4 years, 11 months ago by mvendramini_hp
Modified:
4 years, 8 months ago
CC:
chromium-reviews, leandromanica
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Improve syncing between dom distiller and the print preview request The print preview (PDF) generation request was being triggered too early for some cases. This CL introduces safeguards to prevent hasty triggers which could cause race conditions. This fixes printing of distilled content with multiple pages, like http://content.time.com/time/magazine/article/0,9171,2005869,00.html Based on an original patch by: Claudio Saavedra (csaavedra@igalia.com) and Alex Castro (alex@igalia.com) BUG=575699 Committed: https://crrev.com/52191b911b060efc4e4c9173b3b28464d129530a Cr-Commit-Position: refs/heads/master@{#386744}

Patch Set 1 #

Total comments: 6

Patch Set 2 : small code corrections #

Total comments: 10

Patch Set 3 : readability nit. #

Patch Set 4 : Making the new operation (AddViewRequestDelegate) less intrusive on the DomDistillerService's inter… #

Total comments: 12

Patch Set 5 : requested nits, waiting for more feedback before completing final doc for new API #

Patch Set 6 : expanded internal new api doc, some adjustments to the feature's internal mechanism #

Total comments: 6

Patch Set 7 : removed initial_content_loaded_, DidNavigateMainFrame removed, and a call to MaybePrintPreview removed #

Patch Set 8 : distilled_article_ready_ also removed, DidFinishLoad removed, DidNavigateMainFrame reinstated, adde… #

Total comments: 3

Patch Set 9 : renamed maybeprintpreview to doprintpreview #

Patch Set 10 : Removed ViewRequestDelegate inheritance from PrintPreviewDistiller, removed AddViewRequestDelegate … #

Total comments: 2

Patch Set 11 : added TODO item - resources readiness before calling doprintpreview #

Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -27 lines) Patch
M chrome/browser/ui/webui/print_preview/print_preview_distiller.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +20 lines, -19 lines 0 comments Download
M components/dom_distiller/core/javascript/dom_distiller_viewer.js View 1 2 3 4 5 6 7 2 chunks +8 lines, -8 lines 0 comments Download

Messages

Total messages: 55 (13 generated)
mvendramini_hp
This CL is suposed to replace this one: https://codereview.chromium.org/1304933005/ They are the same solution, but ...
4 years, 11 months ago (2016-01-19 11:49:20 UTC) #3
Bernhard Bauer
https://codereview.chromium.org/1575473002/diff/1/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/1/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc#newcode158 chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:158: if (distilled_article_ready_ && initial_content_loaded_) { You could return early ...
4 years, 11 months ago (2016-01-19 17:21:02 UTC) #4
mvendramini_hp
Correcting the requested items. https://codereview.chromium.org/1575473002/diff/1/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/1/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc#newcode158 chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:158: if (distilled_article_ready_ && initial_content_loaded_) { ...
4 years, 11 months ago (2016-01-20 11:56:57 UTC) #5
Bernhard Bauer
lgtm
4 years, 11 months ago (2016-01-20 16:37:32 UTC) #6
nyquist
https://codereview.chromium.org/1575473002/diff/20001/components/dom_distiller/core/dom_distiller_service.cc File components/dom_distiller/core/dom_distiller_service.cc (right): https://codereview.chromium.org/1575473002/diff/20001/components/dom_distiller/core/dom_distiller_service.cc#newcode196 components/dom_distiller/core/dom_distiller_service.cc:196: GetOrCreateTaskTrackerForUrl(url, &task_tracker); If this returns true, doesn't that mean ...
4 years, 11 months ago (2016-01-20 16:59:08 UTC) #7
mdjones
https://codereview.chromium.org/1575473002/diff/20001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/20001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc#newcode152 chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:152: initial_content_loaded_ = true; If this is a condition for ...
4 years, 11 months ago (2016-01-20 18:14:16 UTC) #8
mvendramini_hp
Accepted mdjones nit suggestion; plus several comments on functionality. Of particular interest, there's discussion on ...
4 years, 11 months ago (2016-01-21 16:54:46 UTC) #10
nyquist
https://codereview.chromium.org/1575473002/diff/20001/components/dom_distiller/core/dom_distiller_service.cc File components/dom_distiller/core/dom_distiller_service.cc (right): https://codereview.chromium.org/1575473002/diff/20001/components/dom_distiller/core/dom_distiller_service.cc#newcode196 components/dom_distiller/core/dom_distiller_service.cc:196: GetOrCreateTaskTrackerForUrl(url, &task_tracker); On 2016/01/21 16:54:46, mvendramini_hp wrote: > On ...
4 years, 11 months ago (2016-01-22 02:23:29 UTC) #11
mvendramini_hp
Safeguarded the new operation by not modifying DomDistillerService's internal state - and kickstarted the documentation ...
4 years, 11 months ago (2016-01-22 12:22:31 UTC) #12
mvendramini_hp
On 2016/01/22 12:22:31, mvendramini_hp wrote: > Safeguarded the new operation by not modifying DomDistillerService's internal ...
4 years, 10 months ago (2016-02-10 10:56:20 UTC) #13
nyquist
I think this mostly looks good now. Just a few small things. https://codereview.chromium.org/1575473002/diff/60001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc ...
4 years, 10 months ago (2016-02-11 15:57:53 UTC) #14
mvendramini_hp
Completed requested nits, advancing discussion on documentation for the new API https://codereview.chromium.org/1575473002/diff/60001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): ...
4 years, 10 months ago (2016-02-11 19:00:46 UTC) #15
nyquist
https://codereview.chromium.org/1575473002/diff/60001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/60001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc#newcode152 chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:152: initial_content_loaded_ = true; On 2016/02/11 19:00:46, mvendramini_hp wrote: > ...
4 years, 10 months ago (2016-02-17 23:09:39 UTC) #16
mvendramini_hp
Done; 1) adjusted the internal mechanics of the feature (whether to call or not MaybePrintPreview ...
4 years, 10 months ago (2016-02-18 12:27:02 UTC) #17
mdjones
https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc#newcode151 chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:151: "navigate_on_initial_content_load = true;"); Is this still needed now that ...
4 years, 10 months ago (2016-02-23 19:23:30 UTC) #18
mvendramini_hp
Some discussion on whether the first flag should be used. https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc#newcode151 ...
4 years, 10 months ago (2016-02-23 19:45:20 UTC) #19
nyquist
https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc#newcode151 chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:151: "navigate_on_initial_content_load = true;"); On 2016/02/23 19:45:20, mvendramini_hp wrote: > ...
4 years, 10 months ago (2016-02-24 01:53:08 UTC) #20
mvendramini_hp
Requests completed: initial_content_loaded_ removed, DidNavigateMainFrame removed, and a call to MaybePrintPreview removed. (apologies for the ...
4 years, 10 months ago (2016-02-24 12:30:54 UTC) #21
mdjones
On 2016/02/24 12:30:54, mvendramini_hp wrote: > Requests completed: initial_content_loaded_ removed, DidNavigateMainFrame > removed, and a ...
4 years, 10 months ago (2016-02-24 16:28:09 UTC) #22
mvendramini_hp
On 2016/02/24 16:28:09, mdjones wrote: > On 2016/02/24 12:30:54, mvendramini_hp wrote: > > Requests completed: ...
4 years, 10 months ago (2016-02-24 18:15:44 UTC) #23
mdjones
On 2016/02/24 18:15:44, mvendramini_hp wrote: > On 2016/02/24 16:28:09, mdjones wrote: > > On 2016/02/24 ...
4 years, 10 months ago (2016-02-24 18:54:21 UTC) #24
mvendramini_hp
On 2016/02/24 18:54:21, mdjones wrote: > On 2016/02/24 18:15:44, mvendramini_hp wrote: > > On 2016/02/24 ...
4 years, 10 months ago (2016-02-25 11:01:47 UTC) #25
mdjones
On 2016/02/25 11:01:47, mvendramini_hp wrote: > On 2016/02/24 18:54:21, mdjones wrote: > > On 2016/02/24 ...
4 years, 10 months ago (2016-02-25 17:57:44 UTC) #26
mdjones
On 2016/02/25 17:57:44, mdjones wrote: > On 2016/02/25 11:01:47, mvendramini_hp wrote: > > On 2016/02/24 ...
4 years, 9 months ago (2016-03-02 02:17:01 UTC) #27
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1575473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575473002/120001
4 years, 9 months ago (2016-03-15 19:16:34 UTC) #29
commit-bot: I haz the power
Dry run: The author mvendramini@hp.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com ...
4 years, 9 months ago (2016-03-15 19:16:38 UTC) #31
mvendramini_hp
On 2016/03/02 02:17:01, mdjones wrote: > On 2016/02/25 17:57:44, mdjones wrote: > > On 2016/02/25 ...
4 years, 9 months ago (2016-03-16 17:43:25 UTC) #32
mvendramini_hp
Applied corrections from mdjones - patch now works and does not introduce regressions.
4 years, 8 months ago (2016-04-05 16:02:15 UTC) #33
wychen
https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc#newcode149 chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:149: rvh->Send(new PrintMsg_InitiatePrintPreview(rvh->GetRoutingID(), false)); The resources might still be not ...
4 years, 8 months ago (2016-04-06 06:13:15 UTC) #36
mvendramini_hp
Starting a discussion on MaybePrintPreview https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc#newcode149 chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:149: rvh->Send(new PrintMsg_InitiatePrintPreview(rvh->GetRoutingID(), false)); On ...
4 years, 8 months ago (2016-04-06 13:39:05 UTC) #37
wychen
https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc#newcode149 chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:149: rvh->Send(new PrintMsg_InitiatePrintPreview(rvh->GetRoutingID(), false)); On 2016/04/06 13:39:04, mvendramini_hp wrote: > ...
4 years, 8 months ago (2016-04-06 19:48:06 UTC) #38
mvendramini_hp
On 2016/04/06 19:48:06, wychen wrote: > https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc > File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): > > https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc#newcode149 > ...
4 years, 8 months ago (2016-04-07 12:09:23 UTC) #39
wychen
On 2016/04/07 12:09:23, mvendramini_hp wrote: > Renamed. > > I think it would be better ...
4 years, 8 months ago (2016-04-11 01:44:51 UTC) #40
mvendramini_hp
On 2016/04/11 01:44:51, wychen wrote: > On 2016/04/07 12:09:23, mvendramini_hp wrote: > > Renamed. > ...
4 years, 8 months ago (2016-04-11 19:19:21 UTC) #41
wychen
The description also needs to be updated. https://codereview.chromium.org/1575473002/diff/180001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/180001/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc#newcode167 chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:167: DoPrintPreview(); Add ...
4 years, 8 months ago (2016-04-11 21:37:21 UTC) #42
mvendramini_hp
Added TODO item, updated description as it was way off at this point (thanks to ...
4 years, 8 months ago (2016-04-12 13:01:27 UTC) #44
Bernhard Bauer
LGTM if wychen@ is happy.
4 years, 8 months ago (2016-04-12 14:53:55 UTC) #45
mdjones
lgtm
4 years, 8 months ago (2016-04-12 15:28:09 UTC) #46
wychen
I changed the subject so that it begins with a verb. LGTM
4 years, 8 months ago (2016-04-12 17:26:41 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1575473002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575473002/200001
4 years, 8 months ago (2016-04-12 17:28:31 UTC) #51
commit-bot: I haz the power
Committed patchset #11 (id:200001)
4 years, 8 months ago (2016-04-12 18:24:18 UTC) #53
commit-bot: I haz the power
4 years, 8 months ago (2016-04-12 18:25:44 UTC) #55
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/52191b911b060efc4e4c9173b3b28464d129530a
Cr-Commit-Position: refs/heads/master@{#386744}

Powered by Google App Engine
This is Rietveld 408576698