|
|
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. |
DescriptionImprove 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 #
Messages
Total messages: 55 (13 generated)
Description was changed from ========== Make the PrintPreviewDistiller a ViewRequestDelegate This way we get notified of the status of the distilling process in the print preview generation. We can wait for all pages of the distilled document to be loaded before firing the print preview process. 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) R=nyquist@chromium.org, thestig@chromium.org, vitalybuka@chromium.org, mdjones@chromium.org, bauerb@chromium.rg BUG=575699 ========== to ========== Make the PrintPreviewDistiller a ViewRequestDelegate This way we get notified of the status of the distilling process in the print preview generation. We can wait for all pages of the distilled document to be loaded before firing the print preview process. 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) R=nyquist@chromium.org, thestig@chromium.org, vitalybuka@chromium.org, mdjones@chromium.org, bauerb@chromium.rg BUG=575699 ==========
mvendramini@hp.com changed reviewers: + bauerb@chromium.org, mdjones@chromium.org, vitalybuka@chromium.org
This CL is suposed to replace this one: https://codereview.chromium.org/1304933005/ They are the same solution, but the aforementioned is incompatible with Android.
https://codereview.chromium.org/1575473002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:158: if (distilled_article_ready_ && initial_content_loaded_) { You could return early if this condition is false. https://codereview.chromium.org/1575473002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_distiller.h (right): https://codereview.chromium.org/1575473002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:17: using dom_distiller::ViewerHandle; Don't use `using` in headers – it imports this into the global namespace for everyone who includes this header! https://codereview.chromium.org/1575473002/diff/1/components/dom_distiller/co... File components/dom_distiller/core/dom_distiller_service.cc (right): https://codereview.chromium.org/1575473002/diff/1/components/dom_distiller/co... components/dom_distiller/core/dom_distiller_service.cc:197: scoped_ptr<ViewerHandle> viewer_handle = task_tracker->AddViewer(delegate); You can just directly return this.
Correcting the requested items. https://codereview.chromium.org/1575473002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:158: if (distilled_article_ready_ && initial_content_loaded_) { On 2016/01/19 17:21:02, Bernhard Bauer wrote: > You could return early if this condition is false. Done. https://codereview.chromium.org/1575473002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_distiller.h (right): https://codereview.chromium.org/1575473002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:17: using dom_distiller::ViewerHandle; On 2016/01/19 17:21:02, Bernhard Bauer wrote: > Don't use `using` in headers – it imports this into the global namespace for > everyone who includes this header! Done - you are right, this was an overlook, good catch. https://codereview.chromium.org/1575473002/diff/1/components/dom_distiller/co... File components/dom_distiller/core/dom_distiller_service.cc (right): https://codereview.chromium.org/1575473002/diff/1/components/dom_distiller/co... components/dom_distiller/core/dom_distiller_service.cc:197: scoped_ptr<ViewerHandle> viewer_handle = task_tracker->AddViewer(delegate); On 2016/01/19 17:21:02, Bernhard Bauer wrote: > You can just directly return this. Done.
lgtm
https://codereview.chromium.org/1575473002/diff/20001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service.cc (right): https://codereview.chromium.org/1575473002/diff/20001/components/dom_distille... components/dom_distiller/core/dom_distiller_service.cc:196: GetOrCreateTaskTrackerForUrl(url, &task_tracker); If this returns true, doesn't that mean that there was no task tracker from before? And if so; can we safely assume that a distillation will soon start? Or should this function just return nullptr in that case? https://codereview.chromium.org/1575473002/diff/20001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service.h (right): https://codereview.chromium.org/1575473002/diff/20001/components/dom_distille... components/dom_distiller/core/dom_distiller_service.h:95: virtual scoped_ptr<ViewerHandle> AddViewRequestDelegate( Please add an explanatory comment as to how this is supposed to be used, and the expected return values. What happens if there was no request to distill the URL from before? Does that do anything? If the distillation has already started, will all callbacks arrive? If distillation has just finished, what will this return, and what types of callbacks can you assume?
https://codereview.chromium.org/1575473002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:152: initial_content_loaded_ = true; If this is a condition for print preview to show content, shouldn't MaybePrintPreview be called here as well? Thought, I believe initial_content_loaded_ will always be true before distilled_article_ready_ is. The first thing distiller does is dump the template and javascript on the page. https://codereview.chromium.org/1575473002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:158: if (!(distilled_article_ready_ && initial_content_loaded_)) Nit: Why not "if (!x || !y) {"?
Description was changed from ========== Make the PrintPreviewDistiller a ViewRequestDelegate This way we get notified of the status of the distilling process in the print preview generation. We can wait for all pages of the distilled document to be loaded before firing the print preview process. 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) R=nyquist@chromium.org, thestig@chromium.org, vitalybuka@chromium.org, mdjones@chromium.org, bauerb@chromium.rg BUG=575699 ========== to ========== Make the PrintPreviewDistiller a ViewRequestDelegate This way we get notified of the status of the distilling process in the print preview generation. We can wait for all pages of the distilled document to be loaded before firing the print preview process. 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) R=nyquist@chromium.org, thestig@chromium.org, vitalybuka@chromium.org, mdjones@chromium.org, bauerb@chromium.org BUG=575699 ==========
Accepted mdjones nit suggestion; plus several comments on functionality. Of particular interest, there's discussion on AddViewRequestDelegate's contract. https://codereview.chromium.org/1575473002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:152: initial_content_loaded_ = true; On 2016/01/20 18:14:16, mdjones wrote: > If this is a condition for print preview to show content, shouldn't > MaybePrintPreview be called here as well? Thought, I believe > initial_content_loaded_ will always be true before distilled_article_ready_ is. > The first thing distiller does is dump the template and javascript on the page. From what we tested, DidFinishLoad is always called first, so yes, initial_content_loaded_ will always be true before. It wouldn't break the logic calling MaybePrintPreview here though, but would likely add unecessary calls. Looking at content/public/browser/web_contents_observer.h DidFinishLoad(...) it really sounds like it has higher IPC precedence, over components/dom_distiller/core/task_tracker.h OnArticleReady(...). I understand cjhopman could help shed some light on the contract/functionality of the TaskTracker? nyquist and mdjones what do you think? https://codereview.chromium.org/1575473002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:158: if (!(distilled_article_ready_ && initial_content_loaded_)) On 2016/01/20 18:14:16, mdjones wrote: > Nit: Why not "if (!x || !y) {"? Sure, Done. https://codereview.chromium.org/1575473002/diff/20001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service.cc (right): https://codereview.chromium.org/1575473002/diff/20001/components/dom_distille... components/dom_distiller/core/dom_distiller_service.cc:196: GetOrCreateTaskTrackerForUrl(url, &task_tracker); On 2016/01/20 16:59:08, nyquist wrote: > If this returns true, doesn't that mean that there was no task tracker from > before? And if so; can we safely assume that a distillation will soon start? Or > should this function just return nullptr in that case? Yes, you are right - in hindsight, that was not a very good idea. nyquist what do you think if I'd just replace that with GetTaskTrackerForUrl() instead? And then return nullptr if it does not exist? This would still work with the current code (since before we call AddViewRequestDelegate() we already called DistillAndView() - which creates the TaskTracker when calling VIewUrl() on MaybeStartDistillation(), and would have the added benefit of discouraging bad usage in the future - please share your thoughts. https://codereview.chromium.org/1575473002/diff/20001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service.h (right): https://codereview.chromium.org/1575473002/diff/20001/components/dom_distille... components/dom_distiller/core/dom_distiller_service.h:95: virtual scoped_ptr<ViewerHandle> AddViewRequestDelegate( On 2016/01/20 16:59:08, nyquist wrote: > Please add an explanatory comment as to how this is supposed to be used, and the > expected return values. > > What happens if there was no request to distill the URL from before? Does that > do anything? If the distillation has already started, will all callbacks arrive? > If distillation has just finished, what will this return, and what types of > callbacks can you assume? Noted. I suggest we keep this pending until we solve the first comment (whether to use GetOrCreateTaskTrackerForUrl or GetTaskTrackerForUrl, for instance) - as soon as we agree on that one, we'll add documentation to this new operation.
https://codereview.chromium.org/1575473002/diff/20001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service.cc (right): https://codereview.chromium.org/1575473002/diff/20001/components/dom_distille... components/dom_distiller/core/dom_distiller_service.cc:196: GetOrCreateTaskTrackerForUrl(url, &task_tracker); On 2016/01/21 16:54:46, mvendramini_hp wrote: > On 2016/01/20 16:59:08, nyquist wrote: > > If this returns true, doesn't that mean that there was no task tracker from > > before? And if so; can we safely assume that a distillation will soon start? > Or > > should this function just return nullptr in that case? > > Yes, you are right - in hindsight, that was not a very good idea. nyquist what > do you think if I'd just replace that with GetTaskTrackerForUrl() instead? And > then return nullptr if it does not exist? This would still work with the current > code (since before we call AddViewRequestDelegate() we already called > DistillAndView() - which creates the TaskTracker when calling VIewUrl() on > MaybeStartDistillation(), and would have the added benefit of discouraging bad > usage in the future - please share your thoughts. Yeah; I think GetTaskTrackerForUrl makes more sense. You have to make it very explicit in the API documentation though, that the code only works if there's already a distillation going on. And like I mentioned in the other comment; there must be a clear documentation as to if and how the callbacks on ViewRequestDelegate is invoked.
Safeguarded the new operation by not modifying DomDistillerService's internal state - and kickstarted the documentation - it might require some further revision. https://codereview.chromium.org/1575473002/diff/20001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service.cc (right): https://codereview.chromium.org/1575473002/diff/20001/components/dom_distille... components/dom_distiller/core/dom_distiller_service.cc:196: GetOrCreateTaskTrackerForUrl(url, &task_tracker); On 2016/01/22 02:23:29, nyquist wrote: > On 2016/01/21 16:54:46, mvendramini_hp wrote: > > On 2016/01/20 16:59:08, nyquist wrote: > > > If this returns true, doesn't that mean that there was no task tracker from > > > before? And if so; can we safely assume that a distillation will soon start? > > Or > > > should this function just return nullptr in that case? > > > > Yes, you are right - in hindsight, that was not a very good idea. nyquist what > > do you think if I'd just replace that with GetTaskTrackerForUrl() instead? And > > then return nullptr if it does not exist? This would still work with the > current > > code (since before we call AddViewRequestDelegate() we already called > > DistillAndView() - which creates the TaskTracker when calling VIewUrl() on > > MaybeStartDistillation(), and would have the added benefit of discouraging bad > > usage in the future - please share your thoughts. > > Yeah; I think GetTaskTrackerForUrl makes more sense. You have to make it very > explicit in the API documentation though, that the code only works if there's > already a distillation going on. > > And like I mentioned in the other comment; there must be a clear documentation > as to if and how the callbacks on ViewRequestDelegate is invoked. Done, adapted the code to not modify DomDistillerService's internal state (tasks_) by using only GetTaskTrackerForUrl(). Also, we added some starting documentation to the header file - please check it out. One thing still missing in the doc: proper explanation on the order of the callbacks - I could really use some help here (cjhopman?). As far as I know, we'd just add the delegate to the tracker, and at that point on we'd get callbacks. I'm not sure what kind of guarantees can be reasonably promised on the API as far as IPC arrival goes - please advise.
On 2016/01/22 12:22:31, mvendramini_hp wrote: > Safeguarded the new operation by not modifying DomDistillerService's internal > state - and kickstarted the documentation - it might require some further > revision. > > https://codereview.chromium.org/1575473002/diff/20001/components/dom_distille... > File components/dom_distiller/core/dom_distiller_service.cc (right): > > https://codereview.chromium.org/1575473002/diff/20001/components/dom_distille... > components/dom_distiller/core/dom_distiller_service.cc:196: > GetOrCreateTaskTrackerForUrl(url, &task_tracker); > On 2016/01/22 02:23:29, nyquist wrote: > > On 2016/01/21 16:54:46, mvendramini_hp wrote: > > > On 2016/01/20 16:59:08, nyquist wrote: > > > > If this returns true, doesn't that mean that there was no task tracker > from > > > > before? And if so; can we safely assume that a distillation will soon > start? > > > Or > > > > should this function just return nullptr in that case? > > > > > > Yes, you are right - in hindsight, that was not a very good idea. nyquist > what > > > do you think if I'd just replace that with GetTaskTrackerForUrl() instead? > And > > > then return nullptr if it does not exist? This would still work with the > > current > > > code (since before we call AddViewRequestDelegate() we already called > > > DistillAndView() - which creates the TaskTracker when calling VIewUrl() on > > > MaybeStartDistillation(), and would have the added benefit of discouraging > bad > > > usage in the future - please share your thoughts. > > > > Yeah; I think GetTaskTrackerForUrl makes more sense. You have to make it very > > explicit in the API documentation though, that the code only works if there's > > already a distillation going on. > > > > And like I mentioned in the other comment; there must be a clear documentation > > as to if and how the callbacks on ViewRequestDelegate is invoked. > > Done, adapted the code to not modify DomDistillerService's internal state > (tasks_) by using only GetTaskTrackerForUrl(). > > Also, we added some starting documentation to the header file - please check it > out. > One thing still missing in the doc: proper explanation on the order of the > callbacks - I could really use some help here (cjhopman?). As far as I know, > we'd just add the delegate to the tracker, and at that point on we'd get > callbacks. I'm not sure what kind of guarantees can be reasonably promised on > the API as far as IPC arrival goes - please advise. Hello, @nyquist @vitalybuka PTAL - is there anything else still pending on this CL? Thanks.
I think this mostly looks good now. Just a few small things. https://codereview.chromium.org/1575473002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:152: initial_content_loaded_ = true; It feels like this should also call MaybePrintPreview(). WDYT? https://codereview.chromium.org/1575473002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:160: RenderViewHost* rvh = web_contents()->GetRenderViewHost(); Nit: I think this would read easier if you added a newline before this line, to separate the exceptional case above. https://codereview.chromium.org/1575473002/diff/60001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service.cc (right): https://codereview.chromium.org/1575473002/diff/60001/components/dom_distille... components/dom_distiller/core/dom_distiller_service.cc:198: } else { Nit: There's a return-statement in the if-block, so you can remove 'else' and then just unindent. Probably with a newline for readability. if-blocks with a single line doesn't need curly braces either in fact, so something like: if (task_tracker) return task_tracker->AddViewer(delegate); return nullptr; https://codereview.chromium.org/1575473002/diff/60001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service.h (right): https://codereview.chromium.org/1575473002/diff/60001/components/dom_distille... components/dom_distiller/core/dom_distiller_service.h:102: // receive callbacks as a |ViewRequestDelegate|. Could you expand this comment with a sentence about whether there is a guaranteed set of callbacks that will arrive? I guess it will have to be best-effort, since we don't store the callbacks to re-delivery.
Completed requested nits, advancing discussion on documentation for the new API https://codereview.chromium.org/1575473002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:152: initial_content_loaded_ = true; On 2016/02/11 15:57:53, nyquist - OOO back 2-16 wrote: > It feels like this should also call MaybePrintPreview(). WDYT? I think it's unnecessary, but it wouldn't hurt to put it there just to be safe. This was a concern raised previously by mdjones - please take a look at his comments @ 2016/01/20 18:14:16, on that day I conducted a test and replied him with my findings and further insights: https://codereview.chromium.org/1575473002/diff/20001/chrome/browser/ui/webui... https://codereview.chromium.org/1575473002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:160: RenderViewHost* rvh = web_contents()->GetRenderViewHost(); On 2016/02/11 15:57:53, nyquist - OOO back 2-16 wrote: > Nit: I think this would read easier if you added a newline before this line, to > separate the exceptional case above. Okay, done! https://codereview.chromium.org/1575473002/diff/60001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service.cc (right): https://codereview.chromium.org/1575473002/diff/60001/components/dom_distille... components/dom_distiller/core/dom_distiller_service.cc:198: } else { On 2016/02/11 15:57:53, nyquist - OOO back 2-16 wrote: > Nit: There's a return-statement in the if-block, so you can remove 'else' and > then just unindent. Probably with a newline for readability. if-blocks with a > single line doesn't need curly braces either in fact, so something like: > > if (task_tracker) > return task_tracker->AddViewer(delegate); > > return nullptr; Done. https://codereview.chromium.org/1575473002/diff/60001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service.h (right): https://codereview.chromium.org/1575473002/diff/60001/components/dom_distille... components/dom_distiller/core/dom_distiller_service.h:102: // receive callbacks as a |ViewRequestDelegate|. On 2016/02/11 15:57:53, nyquist - OOO back 2-16 wrote: > Could you expand this comment with a sentence about whether there is a > guaranteed set of callbacks that will arrive? I guess it will have to be > best-effort, since we don't store the callbacks to re-delivery. I propose we either cross-reference the callbacks documentation at components/dom_distiller/core/task_tracker.h somehow - or re-phrase it here. Alternatively, I think the only guarantee that can be given here is that client code is eligible to get callbacks as long as it holds onto a ViewerHandle. Considering: 1) task_tracker.h:45 "Called when the distilled article contents are available. The DistilledArticleProto is owned by a TaskTracker instance and is invalidated when the corresponding ViewerHandle is destroyed (or when the DomDistillerService is destroyed)." 2) task_tracker.h:51 "Called when an article that is currently under distillation is updated." I'm not 100% confident on what kind of guarantees can be ascertained, given the original task tracker documentation (see above). Is it possible to get help from the original designer of task tracker for documentation purposes? I reckon that would be cjhopman, I highly recommend his input for a definitive answer on API guarantees in this case. @nyquist?
https://codereview.chromium.org/1575473002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/60001/chrome/browser/ui/webui... 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: > On 2016/02/11 15:57:53, nyquist - OOO back 2-16 wrote: > > It feels like this should also call MaybePrintPreview(). WDYT? > > I think it's unnecessary, but it wouldn't hurt to put it there just to be safe. > > This was a concern raised previously by mdjones - please take a look at his > comments @ 2016/01/20 18:14:16, on that day I conducted a test and replied him > with my findings and further insights: > https://codereview.chromium.org/1575473002/diff/20001/chrome/browser/ui/webui... So if you want to do that instead, then I think you should remove one of the members. It looks very unsymmetric to me to have two members, but only trigger on one of them. So either: - remove initial_content_loaded_ - DCHECK(initial_content_loaded_) in MaybePrintPreview() instead of having it as part of the if-statement - Add a call to MaybePrintPreview here. I don't think it adds a lot of overhead to call MaybePrintPreview if you were concerned about performance though. It'll just bail out after the first if-check. To make the code easier to read though, you might want to just use a DCHECK instead, since it's an assumption you make. https://codereview.chromium.org/1575473002/diff/60001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service.h (right): https://codereview.chromium.org/1575473002/diff/60001/components/dom_distille... components/dom_distiller/core/dom_distiller_service.h:102: // receive callbacks as a |ViewRequestDelegate|. On 2016/02/11 19:00:46, mvendramini_hp wrote: > On 2016/02/11 15:57:53, nyquist - OOO back 2-16 wrote: > > Could you expand this comment with a sentence about whether there is a > > guaranteed set of callbacks that will arrive? I guess it will have to be > > best-effort, since we don't store the callbacks to re-delivery. > > I propose we either cross-reference the callbacks documentation at > components/dom_distiller/core/task_tracker.h somehow - or re-phrase it here. > Alternatively, I think the only guarantee that can be given here is that client > code is eligible to get callbacks as long as it holds onto a ViewerHandle. > > Considering: > > 1) task_tracker.h:45 "Called when the distilled article contents are available. > The DistilledArticleProto is owned by a TaskTracker instance and is invalidated > when the corresponding ViewerHandle is destroyed (or when the > DomDistillerService is destroyed)." > > 2) task_tracker.h:51 "Called when an article that is currently under > distillation is updated." > > I'm not 100% confident on what kind of guarantees can be ascertained, given the > original task tracker documentation (see above). > > Is it possible to get help from the original designer of task tracker for > documentation purposes? I reckon that would be cjhopman, I highly recommend his > input for a definitive answer on API guarantees in this case. @nyquist? So, the guarantee is: - If this function returns nullptr, you will get no callbacks. - If this function returns a ViewerHandle, you will at least get the OnArticleFinished() callback. This includes two important cases: a) The article distillation is not finished yet, so the callback will trigger at the same time as when the original distillation request gets it. b) The original distillation request has already gotten its callback, but the ViewerHandle hasn't been deleted yet, so the TaskTracker is still around. When that's the case, the callback will be posted immediately after adding the viewer. - If this function returns a ViewerHandle, and distillation has not finished yet, you will also get all future callbacks for OnArticleUpdated().
Done; 1) adjusted the internal mechanics of the feature (whether to call or not MaybePrintPreview on the first callback (DidFinishLoad) - and downgraded the flag to a DCHECK instead of an earlier if return 2) Expanded the new internal api comment-documentation (AddViewRequestDelegate) https://codereview.chromium.org/1575473002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:152: initial_content_loaded_ = true; On 2016/02/17 23:09:38, nyquist wrote: > On 2016/02/11 19:00:46, mvendramini_hp wrote: > > On 2016/02/11 15:57:53, nyquist - OOO back 2-16 wrote: > > > It feels like this should also call MaybePrintPreview(). WDYT? > > > > I think it's unnecessary, but it wouldn't hurt to put it there just to be > safe. > > > > This was a concern raised previously by mdjones - please take a look at his > > comments @ 2016/01/20 18:14:16, on that day I conducted a test and replied him > > with my findings and further insights: > > > https://codereview.chromium.org/1575473002/diff/20001/chrome/browser/ui/webui... > > So if you want to do that instead, then I think you should remove one of the > members. It looks very unsymmetric to me to have two members, but only trigger > on one of them. > > So either: > - remove initial_content_loaded_ > - DCHECK(initial_content_loaded_) in MaybePrintPreview() instead of having it as > part of the if-statement > - Add a call to MaybePrintPreview here. > > I don't think it adds a lot of overhead to call MaybePrintPreview if you were > concerned about performance though. It'll just bail out after the first > if-check. > > To make the code easier to read though, you might want to just use a DCHECK > instead, since it's an assumption you make. I agree it looks unsymmetric, and I think removing would look better and simpler; though I went with the latter, for defensive coding reasons; https://codereview.chromium.org/1575473002/diff/60001/components/dom_distille... File components/dom_distiller/core/dom_distiller_service.h (right): https://codereview.chromium.org/1575473002/diff/60001/components/dom_distille... components/dom_distiller/core/dom_distiller_service.h:102: // receive callbacks as a |ViewRequestDelegate|. On 2016/02/17 23:09:38, nyquist wrote: > On 2016/02/11 19:00:46, mvendramini_hp wrote: > > On 2016/02/11 15:57:53, nyquist - OOO back 2-16 wrote: > > > Could you expand this comment with a sentence about whether there is a > > > guaranteed set of callbacks that will arrive? I guess it will have to be > > > best-effort, since we don't store the callbacks to re-delivery. > > > > I propose we either cross-reference the callbacks documentation at > > components/dom_distiller/core/task_tracker.h somehow - or re-phrase it here. > > Alternatively, I think the only guarantee that can be given here is that > client > > code is eligible to get callbacks as long as it holds onto a ViewerHandle. > > > > Considering: > > > > 1) task_tracker.h:45 "Called when the distilled article contents are > available. > > The DistilledArticleProto is owned by a TaskTracker instance and is > invalidated > > when the corresponding ViewerHandle is destroyed (or when the > > DomDistillerService is destroyed)." > > > > 2) task_tracker.h:51 "Called when an article that is currently under > > distillation is updated." > > > > I'm not 100% confident on what kind of guarantees can be ascertained, given > the > > original task tracker documentation (see above). > > > > Is it possible to get help from the original designer of task tracker for > > documentation purposes? I reckon that would be cjhopman, I highly recommend > his > > input for a definitive answer on API guarantees in this case. @nyquist? > > So, the guarantee is: > - If this function returns nullptr, you will get no callbacks. > - If this function returns a ViewerHandle, you will at least get the > OnArticleFinished() callback. This includes two important cases: > a) The article distillation is not finished yet, so the callback will trigger > at the same time as when the original distillation request gets it. > b) The original distillation request has already gotten its callback, but the > ViewerHandle hasn't been deleted yet, so the TaskTracker is still around. When > that's the case, the callback will be posted immediately after adding the > viewer. > - If this function returns a ViewerHandle, and distillation has not finished > yet, you will also get all future callbacks for OnArticleUpdated(). Done, expanded the doc-comment, please check it out.
https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:151: "navigate_on_initial_content_load = true;"); Is this still needed now that you get the article update events? https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:173: void DidNavigateMainFrame( Similarly, this method seems unnecessary now that you get the article update events.
Some discussion on whether the first flag should be used. https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:151: "navigate_on_initial_content_load = true;"); On 2016/02/23 19:23:30, mdjones wrote: > Is this still needed now that you get the article update events? Maybe we could remove everything related to initial_content_loaded_ altogether - it does not break the logic, the patch still works, I've tested. mdjones@ and nyquist@ WDYT? https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:173: void DidNavigateMainFrame( On 2016/02/23 19:23:30, mdjones wrote: > Similarly, this method seems unnecessary now that you get the article update > events. Yes, it could be removed, (see my former comment) - i've tested removing this and it's harmless to the patch - at least for the description test page. Please advise.
https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... 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: > On 2016/02/23 19:23:30, mdjones wrote: > > Is this still needed now that you get the article update events? > > Maybe we could remove everything related to initial_content_loaded_ altogether - > it does not break the logic, the patch still works, I've tested. > > mdjones@ and nyquist@ WDYT? Yeah, I think that would be great. It would hopefully simplify this code a bit.
Requests completed: initial_content_loaded_ removed, DidNavigateMainFrame removed, and a call to MaybePrintPreview removed. (apologies for the incorrect patchset title) https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:151: "navigate_on_initial_content_load = true;"); On 2016/02/24 01:53:08, nyquist wrote: > On 2016/02/23 19:45:20, mvendramini_hp wrote: > > On 2016/02/23 19:23:30, mdjones wrote: > > > Is this still needed now that you get the article update events? > > > > Maybe we could remove everything related to initial_content_loaded_ altogether > - > > it does not break the logic, the patch still works, I've tested. > > > > mdjones@ and nyquist@ WDYT? > > Yeah, I think that would be great. It would hopefully simplify this code a bit. Done.
On 2016/02/24 12:30:54, mvendramini_hp wrote: > Requests completed: initial_content_loaded_ removed, DidNavigateMainFrame > removed, and a call to > MaybePrintPreview removed. > > (apologies for the incorrect patchset title) > > https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): > > https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... > chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:151: > "navigate_on_initial_content_load = true;"); > On 2016/02/24 01:53:08, nyquist wrote: > > On 2016/02/23 19:45:20, mvendramini_hp wrote: > > > On 2016/02/23 19:23:30, mdjones wrote: > > > > Is this still needed now that you get the article update events? > > > > > > Maybe we could remove everything related to initial_content_loaded_ > altogether > > - > > > it does not break the logic, the patch still works, I've tested. > > > > > > mdjones@ and nyquist@ WDYT? > > > > Yeah, I think that would be great. It would hopefully simplify this code a > bit. > > Done. Can you also remove the references to "navigate_on_initial_content_load" in dom_distiller_viewer.js since it is now unused?
On 2016/02/24 16:28:09, mdjones wrote: > On 2016/02/24 12:30:54, mvendramini_hp wrote: > > Requests completed: initial_content_loaded_ removed, DidNavigateMainFrame > > removed, and a call to > > MaybePrintPreview removed. > > > > (apologies for the incorrect patchset title) > > > > > https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... > > File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): > > > > > https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... > > chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:151: > > "navigate_on_initial_content_load = true;"); > > On 2016/02/24 01:53:08, nyquist wrote: > > > On 2016/02/23 19:45:20, mvendramini_hp wrote: > > > > On 2016/02/23 19:23:30, mdjones wrote: > > > > > Is this still needed now that you get the article update events? > > > > > > > > Maybe we could remove everything related to initial_content_loaded_ > > altogether > > > - > > > > it does not break the logic, the patch still works, I've tested. > > > > > > > > mdjones@ and nyquist@ WDYT? > > > > > > Yeah, I think that would be great. It would hopefully simplify this code a > > bit. > > > > Done. > > Can you also remove the references to "navigate_on_initial_content_load" in > dom_distiller_viewer.js since it is now unused? nyquist@ and mdjones@ - I detected that patchset 5 and beyond introduced regressions - try simplifying, then unchecking simply page and then re-simplifying the following page: https://en.wikipedia.org/wiki/Ovenbird The absence of the initial_content_loaded_ - more specifically, using a DCHECK on it, and (surprisingly) also calling MaybePrintPreview on DidFinishLoad will break the "reentrancy" of simplifying a page. I recommend we scrape patchset 5, 6 and 7 - except for the extra comment-documentation introduced by patchset 5. If that's a plan, what's the best way to do this? New patchsets to undo patchset 5~7 or a new CL? If not, please let me know the best way to deal with the regression. Thanks.
On 2016/02/24 18:15:44, mvendramini_hp wrote: > On 2016/02/24 16:28:09, mdjones wrote: > > On 2016/02/24 12:30:54, mvendramini_hp wrote: > > > Requests completed: initial_content_loaded_ removed, DidNavigateMainFrame > > > removed, and a call to > > > MaybePrintPreview removed. > > > > > > (apologies for the incorrect patchset title) > > > > > > > > > https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... > > > File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc > (right): > > > > > > > > > https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... > > > chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:151: > > > "navigate_on_initial_content_load = true;"); > > > On 2016/02/24 01:53:08, nyquist wrote: > > > > On 2016/02/23 19:45:20, mvendramini_hp wrote: > > > > > On 2016/02/23 19:23:30, mdjones wrote: > > > > > > Is this still needed now that you get the article update events? > > > > > > > > > > Maybe we could remove everything related to initial_content_loaded_ > > > altogether > > > > - > > > > > it does not break the logic, the patch still works, I've tested. > > > > > > > > > > mdjones@ and nyquist@ WDYT? > > > > > > > > Yeah, I think that would be great. It would hopefully simplify this code a > > > bit. > > > > > > Done. > > > > Can you also remove the references to "navigate_on_initial_content_load" in > > dom_distiller_viewer.js since it is now unused? > > nyquist@ and mdjones@ - I detected that patchset 5 and beyond introduced > regressions - try > simplifying, then unchecking simply page and then re-simplifying the following > page: > https://en.wikipedia.org/wiki/Ovenbird > > The absence of the initial_content_loaded_ - more specifically, using a DCHECK > on it, and (surprisingly) also calling MaybePrintPreview on DidFinishLoad will > break the "reentrancy" of simplifying a page. > > I recommend we scrape patchset 5, 6 and 7 - except for the extra > comment-documentation introduced by patchset 5. > > If that's a plan, what's the best way to do this? New patchsets to undo patchset > 5~7 or a new CL? > If not, please let me know the best way to deal with the regression. > > Thanks. In the past, we have had problems with print preview triggering before the renderer has finished drawing the page (https://codereview.chromium.org/1555043005/). I suspect this is happening in the event the distiller results are already cached; a signal is sent to render the page in PDF format before results are on the page. If this is the case, you could try overriding DidFirstVisuallyNonEmptyPaint in your WebContentsObserver and call maybePrintPreview there.
On 2016/02/24 18:54:21, mdjones wrote: > On 2016/02/24 18:15:44, mvendramini_hp wrote: > > On 2016/02/24 16:28:09, mdjones wrote: > > > On 2016/02/24 12:30:54, mvendramini_hp wrote: > > > > Requests completed: initial_content_loaded_ removed, DidNavigateMainFrame > > > > removed, and a call to > > > > MaybePrintPreview removed. > > > > > > > > (apologies for the incorrect patchset title) > > > > > > > > > > > > > > https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... > > > > File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... > > > > chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:151: > > > > "navigate_on_initial_content_load = true;"); > > > > On 2016/02/24 01:53:08, nyquist wrote: > > > > > On 2016/02/23 19:45:20, mvendramini_hp wrote: > > > > > > On 2016/02/23 19:23:30, mdjones wrote: > > > > > > > Is this still needed now that you get the article update events? > > > > > > > > > > > > Maybe we could remove everything related to initial_content_loaded_ > > > > altogether > > > > > - > > > > > > it does not break the logic, the patch still works, I've tested. > > > > > > > > > > > > mdjones@ and nyquist@ WDYT? > > > > > > > > > > Yeah, I think that would be great. It would hopefully simplify this code > a > > > > bit. > > > > > > > > Done. > > > > > > Can you also remove the references to "navigate_on_initial_content_load" in > > > dom_distiller_viewer.js since it is now unused? > > > > nyquist@ and mdjones@ - I detected that patchset 5 and beyond introduced > > regressions - try > > simplifying, then unchecking simply page and then re-simplifying the following > > page: > > https://en.wikipedia.org/wiki/Ovenbird > > > > The absence of the initial_content_loaded_ - more specifically, using a DCHECK > > on it, and (surprisingly) also calling MaybePrintPreview on DidFinishLoad will > > break the "reentrancy" of simplifying a page. > > > > I recommend we scrape patchset 5, 6 and 7 - except for the extra > > comment-documentation introduced by patchset 5. > > > > If that's a plan, what's the best way to do this? New patchsets to undo > patchset > > 5~7 or a new CL? > > If not, please let me know the best way to deal with the regression. > > > > Thanks. > > In the past, we have had problems with print preview triggering before the > renderer has finished drawing the page > (https://codereview.chromium.org/1555043005/). I suspect this is happening in > the event the distiller results are already cached; a signal is sent to render > the page in PDF format before results are on the page. If this is the case, you > could try overriding DidFirstVisuallyNonEmptyPaint in your WebContentsObserver > and call maybePrintPreview there. mdjones@ I added the call like you suggested, but it didn't work - it will still fail the second time. I suggest initial_contents_loaded to be reinstated - i can make a new patchset to re-include it. Any ideas?
On 2016/02/25 11:01:47, mvendramini_hp wrote: > On 2016/02/24 18:54:21, mdjones wrote: > > On 2016/02/24 18:15:44, mvendramini_hp wrote: > > > On 2016/02/24 16:28:09, mdjones wrote: > > > > On 2016/02/24 12:30:54, mvendramini_hp wrote: > > > > > Requests completed: initial_content_loaded_ removed, > DidNavigateMainFrame > > > > > removed, and a call to > > > > > MaybePrintPreview removed. > > > > > > > > > > (apologies for the incorrect patchset title) > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... > > > > > File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... > > > > > chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:151: > > > > > "navigate_on_initial_content_load = true;"); > > > > > On 2016/02/24 01:53:08, nyquist wrote: > > > > > > On 2016/02/23 19:45:20, mvendramini_hp wrote: > > > > > > > On 2016/02/23 19:23:30, mdjones wrote: > > > > > > > > Is this still needed now that you get the article update events? > > > > > > > > > > > > > > Maybe we could remove everything related to initial_content_loaded_ > > > > > altogether > > > > > > - > > > > > > > it does not break the logic, the patch still works, I've tested. > > > > > > > > > > > > > > mdjones@ and nyquist@ WDYT? > > > > > > > > > > > > Yeah, I think that would be great. It would hopefully simplify this > code > > a > > > > > bit. > > > > > > > > > > Done. > > > > > > > > Can you also remove the references to "navigate_on_initial_content_load" > in > > > > dom_distiller_viewer.js since it is now unused? > > > > > > nyquist@ and mdjones@ - I detected that patchset 5 and beyond introduced > > > regressions - try > > > simplifying, then unchecking simply page and then re-simplifying the > following > > > page: > > > https://en.wikipedia.org/wiki/Ovenbird > > > > > > The absence of the initial_content_loaded_ - more specifically, using a > DCHECK > > > on it, and (surprisingly) also calling MaybePrintPreview on DidFinishLoad > will > > > break the "reentrancy" of simplifying a page. > > > > > > I recommend we scrape patchset 5, 6 and 7 - except for the extra > > > comment-documentation introduced by patchset 5. > > > > > > If that's a plan, what's the best way to do this? New patchsets to undo > > patchset > > > 5~7 or a new CL? > > > If not, please let me know the best way to deal with the regression. > > > > > > Thanks. > > > > In the past, we have had problems with print preview triggering before the > > renderer has finished drawing the page > > (https://codereview.chromium.org/1555043005/). I suspect this is happening in > > the event the distiller results are already cached; a signal is sent to render > > the page in PDF format before results are on the page. If this is the case, > you > > could try overriding DidFirstVisuallyNonEmptyPaint in your WebContentsObserver > > and call maybePrintPreview there. > > mdjones@ I added the call like you suggested, but it didn't work - it will still > fail the second time. > > I suggest initial_contents_loaded to be reinstated - i can make a new patchset > to re-include it. Any ideas? So I took a little bit of time to experiment with this patch. The added DCHECK constantly fails on the second attempt to load the print preview. This is definitely a race condition where print preview tries to generate the pdf before content is rendered. I was able to get the loading spinner in various states on the second time I checked the "simplify" box. After looking closer at the javascript parts, "initial content loaded" means two completely different things where it is used. In c++ it means the template html has loaded on the page and in javascript it means the first page of article content has been added to the dom. The latter is the one you actually need and is almost certainly the only event that needs to call maybePrintPreview. The second page navigation that "DidNavigateMainFrame" was waiting for was a cue that content was ready to be transformed into pdf. Unfortunately this solution introduces a second race condition; any page after the first in a multi-page article may not be rendered by the time print preview is triggered (practically this means the last page). I would expect the solution to this problem to involve waiting for a signal each time a page has been rendered.
On 2016/02/25 17:57:44, mdjones wrote: > On 2016/02/25 11:01:47, mvendramini_hp wrote: > > On 2016/02/24 18:54:21, mdjones wrote: > > > On 2016/02/24 18:15:44, mvendramini_hp wrote: > > > > On 2016/02/24 16:28:09, mdjones wrote: > > > > > On 2016/02/24 12:30:54, mvendramini_hp wrote: > > > > > > Requests completed: initial_content_loaded_ removed, > > DidNavigateMainFrame > > > > > > removed, and a call to > > > > > > MaybePrintPreview removed. > > > > > > > > > > > > (apologies for the incorrect patchset title) > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... > > > > > > File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... > > > > > > chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:151: > > > > > > "navigate_on_initial_content_load = true;"); > > > > > > On 2016/02/24 01:53:08, nyquist wrote: > > > > > > > On 2016/02/23 19:45:20, mvendramini_hp wrote: > > > > > > > > On 2016/02/23 19:23:30, mdjones wrote: > > > > > > > > > Is this still needed now that you get the article update events? > > > > > > > > > > > > > > > > Maybe we could remove everything related to > initial_content_loaded_ > > > > > > altogether > > > > > > > - > > > > > > > > it does not break the logic, the patch still works, I've tested. > > > > > > > > > > > > > > > > mdjones@ and nyquist@ WDYT? > > > > > > > > > > > > > > Yeah, I think that would be great. It would hopefully simplify this > > code > > > a > > > > > > bit. > > > > > > > > > > > > Done. > > > > > > > > > > Can you also remove the references to "navigate_on_initial_content_load" > > in > > > > > dom_distiller_viewer.js since it is now unused? > > > > > > > > nyquist@ and mdjones@ - I detected that patchset 5 and beyond introduced > > > > regressions - try > > > > simplifying, then unchecking simply page and then re-simplifying the > > following > > > > page: > > > > https://en.wikipedia.org/wiki/Ovenbird > > > > > > > > The absence of the initial_content_loaded_ - more specifically, using a > > DCHECK > > > > on it, and (surprisingly) also calling MaybePrintPreview on DidFinishLoad > > will > > > > break the "reentrancy" of simplifying a page. > > > > > > > > I recommend we scrape patchset 5, 6 and 7 - except for the extra > > > > comment-documentation introduced by patchset 5. > > > > > > > > If that's a plan, what's the best way to do this? New patchsets to undo > > > patchset > > > > 5~7 or a new CL? > > > > If not, please let me know the best way to deal with the regression. > > > > > > > > Thanks. > > > > > > In the past, we have had problems with print preview triggering before the > > > renderer has finished drawing the page > > > (https://codereview.chromium.org/1555043005/). I suspect this is happening > in > > > the event the distiller results are already cached; a signal is sent to > render > > > the page in PDF format before results are on the page. If this is the case, > > you > > > could try overriding DidFirstVisuallyNonEmptyPaint in your > WebContentsObserver > > > and call maybePrintPreview there. > > > > mdjones@ I added the call like you suggested, but it didn't work - it will > still > > fail the second time. > > > > I suggest initial_contents_loaded to be reinstated - i can make a new patchset > > to re-include it. Any ideas? > > So I took a little bit of time to experiment with this patch. The added DCHECK > constantly fails on the second attempt to load the print preview. This is > definitely a race condition where print preview tries to generate the pdf before > content is rendered. I was able to get the loading spinner in various states on > the second time I checked the "simplify" box. > > After looking closer at the javascript parts, "initial content loaded" means two > completely different things where it is used. In c++ it means the template html > has loaded on the page and in javascript it means the first page of article > content has been added to the dom. The latter is the one you actually need and > is almost certainly the only event that needs to call maybePrintPreview. The > second page navigation that "DidNavigateMainFrame" was waiting for was a cue > that content was ready to be transformed into pdf. Unfortunately this solution > introduces a second race condition; any page after the first in a multi-page > article may not be rendered by the time print preview is triggered (practically > this means the last page). I would expect the solution to this problem to > involve waiting for a signal each time a page has been rendered. What I mean to say is that you could probably just run the code that is currently in "addToPage" in dom_distiller_viewer.js to a function that runs after everything is on the page, i.e. "showLoadingIndicator" when its parameter is true. With that setup, I don't believe you need another view request delegate and you only need to send a message to trigger print preview in one place, "didNavigateMainFrame".
The CQ bit was checked by mvendramini@hp.com to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: The author mvendramini@hp.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
On 2016/03/02 02:17:01, mdjones wrote: > On 2016/02/25 17:57:44, mdjones wrote: > > On 2016/02/25 11:01:47, mvendramini_hp wrote: > > > On 2016/02/24 18:54:21, mdjones wrote: > > > > On 2016/02/24 18:15:44, mvendramini_hp wrote: > > > > > On 2016/02/24 16:28:09, mdjones wrote: > > > > > > On 2016/02/24 12:30:54, mvendramini_hp wrote: > > > > > > > Requests completed: initial_content_loaded_ removed, > > > DidNavigateMainFrame > > > > > > > removed, and a call to > > > > > > > MaybePrintPreview removed. > > > > > > > > > > > > > > (apologies for the incorrect patchset title) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... > > > > > > > File > chrome/browser/ui/webui/print_preview/print_preview_distiller.cc > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1575473002/diff/100001/chrome/browser/ui/webu... > > > > > > > > chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:151: > > > > > > > "navigate_on_initial_content_load = true;"); > > > > > > > On 2016/02/24 01:53:08, nyquist wrote: > > > > > > > > On 2016/02/23 19:45:20, mvendramini_hp wrote: > > > > > > > > > On 2016/02/23 19:23:30, mdjones wrote: > > > > > > > > > > Is this still needed now that you get the article update > events? > > > > > > > > > > > > > > > > > > Maybe we could remove everything related to > > initial_content_loaded_ > > > > > > > altogether > > > > > > > > - > > > > > > > > > it does not break the logic, the patch still works, I've tested. > > > > > > > > > > > > > > > > > > mdjones@ and nyquist@ WDYT? > > > > > > > > > > > > > > > > Yeah, I think that would be great. It would hopefully simplify > this > > > code > > > > a > > > > > > > bit. > > > > > > > > > > > > > > Done. > > > > > > > > > > > > Can you also remove the references to > "navigate_on_initial_content_load" > > > in > > > > > > dom_distiller_viewer.js since it is now unused? > > > > > > > > > > nyquist@ and mdjones@ - I detected that patchset 5 and beyond introduced > > > > > regressions - try > > > > > simplifying, then unchecking simply page and then re-simplifying the > > > following > > > > > page: > > > > > https://en.wikipedia.org/wiki/Ovenbird > > > > > > > > > > The absence of the initial_content_loaded_ - more specifically, using a > > > DCHECK > > > > > on it, and (surprisingly) also calling MaybePrintPreview on > DidFinishLoad > > > will > > > > > break the "reentrancy" of simplifying a page. > > > > > > > > > > I recommend we scrape patchset 5, 6 and 7 - except for the extra > > > > > comment-documentation introduced by patchset 5. > > > > > > > > > > If that's a plan, what's the best way to do this? New patchsets to undo > > > > patchset > > > > > 5~7 or a new CL? > > > > > If not, please let me know the best way to deal with the regression. > > > > > > > > > > Thanks. > > > > > > > > In the past, we have had problems with print preview triggering before the > > > > renderer has finished drawing the page > > > > (https://codereview.chromium.org/1555043005/). I suspect this is happening > > in > > > > the event the distiller results are already cached; a signal is sent to > > render > > > > the page in PDF format before results are on the page. If this is the > case, > > > you > > > > could try overriding DidFirstVisuallyNonEmptyPaint in your > > WebContentsObserver > > > > and call maybePrintPreview there. > > > > > > mdjones@ I added the call like you suggested, but it didn't work - it will > > still > > > fail the second time. > > > > > > I suggest initial_contents_loaded to be reinstated - i can make a new > patchset > > > to re-include it. Any ideas? > > > > So I took a little bit of time to experiment with this patch. The added DCHECK > > constantly fails on the second attempt to load the print preview. This is > > definitely a race condition where print preview tries to generate the pdf > before > > content is rendered. I was able to get the loading spinner in various states > on > > the second time I checked the "simplify" box. > > > > After looking closer at the javascript parts, "initial content loaded" means > two > > completely different things where it is used. In c++ it means the template > html > > has loaded on the page and in javascript it means the first page of article > > content has been added to the dom. The latter is the one you actually need and > > is almost certainly the only event that needs to call maybePrintPreview. The > > second page navigation that "DidNavigateMainFrame" was waiting for was a cue > > that content was ready to be transformed into pdf. Unfortunately this solution > > introduces a second race condition; any page after the first in a multi-page > > article may not be rendered by the time print preview is triggered > (practically > > this means the last page). I would expect the solution to this problem to > > involve waiting for a signal each time a page has been rendered. > > What I mean to say is that you could probably just run the code that is > currently in "addToPage" in dom_distiller_viewer.js to a function that runs > after everything is on the page, i.e. "showLoadingIndicator" when its parameter > is true. With that setup, I don't believe you need another view request delegate > and you only need to send a message to trigger print preview in one place, > "didNavigateMainFrame". I tested the proposed changes above but the race condition's still there - I get the blue spinner on different states. I will try to debug this a bit more to understand it better - another option is still to go back to patchset 5 plus the documentation introduced on patchset 6. nyquist@ mdjones@ WDYT ?
Applied corrections from mdjones - patch now works and does not introduce regressions.
vitalybuka@chromium.org changed reviewers: - vitalybuka@chromium.org
wychen@chromium.org changed reviewers: + wychen@chromium.org
https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webu... 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 ready here, like images or web fonts. Some signals in the renderer might be useful, like didMeaningfulLayout(WebMeaningfulLayout::FinishedLoading), but it cannot be used as is because our content is dynamically loaded. This could be in another CL.
Starting a discussion on MaybePrintPreview https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:149: rvh->Send(new PrintMsg_InitiatePrintPreview(rvh->GetRoutingID(), false)); On 2016/04/06 06:13:15, wychen wrote: > The resources might still be not ready here, like images or web fonts. > > Some signals in the renderer might be useful, like > didMeaningfulLayout(WebMeaningfulLayout::FinishedLoading), but it cannot be used > as is because our content is dynamically loaded. > > This could be in another CL. Also, at this point, MaybePrintPreview is no longer a descriptive name here - since there is no longer any decision making in this function, there is no longer justification for calling it "Maybe". How about changing it to "DoPrintPreview" or similar? Your concern above should perhaps be implemented in where we decide to call this function (currently from DidNavigateMainFrame) wychen@ mdjones@ WDYT ?
https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webu... 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: > On 2016/04/06 06:13:15, wychen wrote: > > The resources might still be not ready here, like images or web fonts. > > > > Some signals in the renderer might be useful, like > > didMeaningfulLayout(WebMeaningfulLayout::FinishedLoading), but it cannot be > used > > as is because our content is dynamically loaded. > > > > This could be in another CL. > > Also, at this point, MaybePrintPreview is no longer a descriptive name here - > since there is no longer any decision making in this function, there is no > longer justification for calling it "Maybe". > > How about changing it to "DoPrintPreview" or similar? Your concern above should > perhaps be implemented in where we decide to call this function (currently from > DidNavigateMainFrame) > > wychen@ mdjones@ WDYT ? Yeah. The name is confusing now. Renaming it would be appropriate. Do you plan to address the issue about resource readiness in this CL?
On 2016/04/06 19:48:06, wychen wrote: > https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): > > https://codereview.chromium.org/1575473002/diff/140001/chrome/browser/ui/webu... > 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: > > On 2016/04/06 06:13:15, wychen wrote: > > > The resources might still be not ready here, like images or web fonts. > > > > > > Some signals in the renderer might be useful, like > > > didMeaningfulLayout(WebMeaningfulLayout::FinishedLoading), but it cannot be > > used > > > as is because our content is dynamically loaded. > > > > > > This could be in another CL. > > > > Also, at this point, MaybePrintPreview is no longer a descriptive name here - > > since there is no longer any decision making in this function, there is no > > longer justification for calling it "Maybe". > > > > How about changing it to "DoPrintPreview" or similar? Your concern above > should > > perhaps be implemented in where we decide to call this function (currently > from > > DidNavigateMainFrame) > > > > wychen@ mdjones@ WDYT ? > > Yeah. The name is confusing now. Renaming it would be appropriate. > > Do you plan to address the issue about resource readiness in this CL? Renamed. I think it would be better to address the resource readiness in a separate CL. wychen@ please keep an eye on the google groups, I will create a thread there to start a discussion on this resource readiness issue. Thanks.
On 2016/04/07 12:09:23, mvendramini_hp wrote: > Renamed. > > I think it would be better to address the resource readiness in a separate CL. > wychen@ please keep an eye on the google groups, I will create a thread there > to start a discussion on this resource readiness issue. > > Thanks. Now that the basic logic here is correct. Would you mind cleaning up this CL so that we can land it? Thanks!
On 2016/04/11 01:44:51, wychen wrote: > On 2016/04/07 12:09:23, mvendramini_hp wrote: > > Renamed. > > > > I think it would be better to address the resource readiness in a separate CL. > > wychen@ please keep an eye on the google groups, I will create a thread there > > to start a discussion on this resource readiness issue. > > > > Thanks. > > Now that the basic logic here is correct. Would you mind cleaning up this CL > so that we can land it? Thanks! Done! I've removed the ViewRequestDelegate inheritance, and the new API inside dom distiller service to add a ViewRequestDelegate to the tracker. I've also rebased, and there's new content inside PrintPreviewDistiller. This should do it, thanks for reviewing!
The description also needs to be updated. https://codereview.chromium.org/1575473002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:167: DoPrintPreview(); Add a TODO saying resources are not necessarily ready at this point.
Description was changed from ========== Make the PrintPreviewDistiller a ViewRequestDelegate This way we get notified of the status of the distilling process in the print preview generation. We can wait for all pages of the distilled document to be loaded before firing the print preview process. 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) R=nyquist@chromium.org, thestig@chromium.org, vitalybuka@chromium.org, mdjones@chromium.org, bauerb@chromium.org BUG=575699 ========== to ========== Improving 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) R=nyquist@chromium.org, thestig@chromium.org, vitalybuka@chromium.org, mdjones@chromium.org, bauerb@chromium.org BUG=575699 ==========
Added TODO item, updated description as it was way off at this point (thanks to wychen for spotting it). https://codereview.chromium.org/1575473002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1575473002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:167: DoPrintPreview(); On 2016/04/11 21:37:21, wychen wrote: > Add a TODO saying resources are not necessarily ready at this point. Done - added both our names. I would need some help addressing it in the next CL though.
LGTM if wychen@ is happy.
lgtm
Description was changed from ========== Improving 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) R=nyquist@chromium.org, thestig@chromium.org, vitalybuka@chromium.org, mdjones@chromium.org, bauerb@chromium.org BUG=575699 ========== to ========== 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) R=nyquist@chromium.org, thestig@chromium.org, vitalybuka@chromium.org, mdjones@chromium.org, bauerb@chromium.org BUG=575699 ==========
I changed the subject so that it begins with a verb. LGTM
Description was changed from ========== 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) R=nyquist@chromium.org, thestig@chromium.org, vitalybuka@chromium.org, mdjones@chromium.org, bauerb@chromium.org BUG=575699 ========== to ========== 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 ==========
The CQ bit was checked by mvendramini@hp.com
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
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/52191b911b060efc4e4c9173b3b28464d129530a Cr-Commit-Position: refs/heads/master@{#386744} |