Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1125343004:
Add a "Simplify Page" option to the print preview dialog (Closed)
|
Created:
5 years, 7 months ago by arjunpatel Modified:
5 years, 4 months ago Reviewers:
Avi (use Gerrit), jochen (gone - plz use gerrit), nyquist, Lei Zhang, cjhopman, Aleksey Shlyapnikov, gene, PhistucK, Vitaly Buka (NO REVIEWS) CC:
alexg__, arv+watch_chromium.org, chromium-reviews, csaavedra, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description"Simplify Page" print preview option enables
preprocessing of the page using the DOM Distiller.
Rendering happens in a hidden web contents, that lives
in parallel with the originally printed contents.
Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/_-zEoPLFKp0
BUG=490809
R=alekseys@chromium.org, avi@chromium.org, jochen@chromium.org, nyquist@chromium.org, vitalybuka@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/ab26826ebd246afdbcc94d44f00015f10f6db6da
Patch Set 1 #
Total comments: 1
Patch Set 2 : Update AUTHORS file to reflect the correct HP name used in the CLA #
Total comments: 24
Patch Set 3 : Pre-rebase changes #Patch Set 4 : Only show checkbox when page is distillable #Patch Set 5 : Rebase and remove custom URL source #
Total comments: 4
Patch Set 6 : Simplify version of the HiddenPrintPreview and solved issues pointed out by the last review #
Total comments: 37
Patch Set 7 : Refactoring following the proposals in the reviews #
Total comments: 35
Patch Set 8 : More refactors and remove tab_helpers dependency #
Total comments: 18
Patch Set 9 : Removed final and defined closures as const, fixed Windows compilation #Patch Set 10 : Fixup "Add "Print friendly" option to print preview dialog" #
Total comments: 1
Patch Set 11 : Cleanup unused variables #Patch Set 12 : Fixup "Add a hidden renderer for simplified printed pages" #Patch Set 13 : Fix for the failing tests #
Total comments: 1
Messages
Total messages: 83 (18 generated)
arjunpatel@hp.com changed reviewers: + gene@chromium.org, jochen@chromium.org
vitalybuka@chromium.org changed reviewers: + vitalybuka@chromium.org
https://codereview.chromium.org/1125343004/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/1125343004/diff/1/AUTHORS#newcode597 AUTHORS:597: Hewlett-Packard Development Company, L.P. <*@hp.com> I don't see HP on corporate CLA list https://www.chromium.org/developers/contributing-code/external-contributor-ch...
On 2015/05/08 17:14:33, Vitaly Buka wrote: > https://codereview.chromium.org/1125343004/diff/1/AUTHORS > File AUTHORS (right): > > https://codereview.chromium.org/1125343004/diff/1/AUTHORS#newcode597 > AUTHORS:597: Hewlett-Packard Development Company, L.P. <mailto:*@hp.com> > I don't see HP on corporate CLA list > https://www.chromium.org/developers/contributing-code/external-contributor-ch... I updated AUTHORS file to use the company name used in the CLA. Instead of "Hewlett-Packard Development Company, L.P.", it is now "Hewlett-Packard Company". Could you please reverify?
arjunpatel@hp.com changed reviewers: + cjhopman@chromium.org, nyquist@chromium.org
On 2015/05/12 16:22:33, arjunpatel wrote: > On 2015/05/08 17:14:33, Vitaly Buka wrote: > > https://codereview.chromium.org/1125343004/diff/1/AUTHORS > > File AUTHORS (right): > > > > https://codereview.chromium.org/1125343004/diff/1/AUTHORS#newcode597 > > AUTHORS:597: Hewlett-Packard Development Company, L.P. <mailto:*@hp.com> > > I don't see HP on corporate CLA list > > > https://www.chromium.org/developers/contributing-code/external-contributor-ch... > > I updated AUTHORS file to use the company name used in the CLA. Instead of > "Hewlett-Packard Development Company, L.P.", it is now "Hewlett-Packard > Company". > Could you please reverify? No, still see no Hewlett-Packard on the list. Could you please create separate CL for AUTHORS and contact mal@chromium.org regarding this?
On 2015/05/12 16:44:16, Vitaly Buka wrote: > On 2015/05/12 16:22:33, arjunpatel wrote: > > On 2015/05/08 17:14:33, Vitaly Buka wrote: > > > https://codereview.chromium.org/1125343004/diff/1/AUTHORS > > > File AUTHORS (right): > > > > > > https://codereview.chromium.org/1125343004/diff/1/AUTHORS#newcode597 > > > AUTHORS:597: Hewlett-Packard Development Company, L.P. <mailto:*@hp.com> > > > I don't see HP on corporate CLA list > > > > > > https://www.chromium.org/developers/contributing-code/external-contributor-ch... > > > > I updated AUTHORS file to use the company name used in the CLA. Instead of > > "Hewlett-Packard Development Company, L.P.", it is now "Hewlett-Packard > > Company". > > Could you please reverify? > > No, still see no Hewlett-Packard on the list. > Could you please create separate CL for AUTHORS and contact mailto:mal@chromium.org > regarding this? Thanks. Done at https://codereview.chromium.org/1123083003/
some initial comments https://codereview.chromium.org/1125343004/diff/20001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/1125343004/diff/20001/AUTHORS#newcode597 AUTHORS:597: Hewlett-Packard Company <*@hp.com> Remove https://codereview.chromium.org/1125343004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/1125343004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:839: MaybeStartDistillation(initiator); Could this be simplified by using the new DistillAndView from https://codereview.chromium.org/1058193002/ ? Depending on how we deal with the @media print things, it might have to change a little bit though to use the correct URL. https://codereview.chromium.org/1125343004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1263: can_simplify = cmdline->HasSwitch(switches::kEnableDomDistiller) && Should this also check to see if we assume that the distillation will work? See dom_distiller::IsDistillablePage ( https://code.google.com/p/chromium/codesearch#chromium/src/components/dom_dis... ) https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... File components/dom_distiller/content/distiller_page_web_contents.cc (right): https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/content/distiller_page_web_contents.cc:33: content::WebContents* SourcePageHandleWebContents::GetWebContents() const { I think you want to rebase this on top of https://codereview.chromium.org/1058193002/ https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... File components/dom_distiller/core/css/distilledpageprintpreview.css (right): https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/core/css/distilledpageprintpreview.css:1: /* Copyright 2014 The Chromium Authors. All rights reserved. Nit: 2015 https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/core/css/distilledpageprintpreview.css:8: *, Could we re-use the original CSS file, and instead use CSS Media Types to separate the unwanted parts using |@media print| and |@media screen|? https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... File components/dom_distiller/core/distiller_page.h (right): https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/core/distiller_page.h:14: #include "content/public/browser/web_contents.h" //components/dom_distiller/core can not and should not depend on //content. https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/core/distiller_page.h:24: virtual content::WebContents* GetWebContents() const = 0; This can't be part of the SourcePageHandle. That's what SourcePageHandleWebContents is for, which lives in //components/dom_distiller/content. https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... File components/dom_distiller/core/url_utils.h (right): https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/core/url_utils.h:24: // Returns the URL for viewing distilled content for a URL. Mention that this is for printing?
https://codereview.chromium.org/1125343004/diff/20001/build/ios/grit_whitelis... File build/ios/grit_whitelist.txt (right): https://codereview.chromium.org/1125343004/diff/20001/build/ios/grit_whitelis... build/ios/grit_whitelist.txt:23: IDR_DISTILLER_PRINT_PREVEW_CSS why do you need this on IOS?
can you send an intent to implement thread for this to chromium-dev? please file a tracking bug for this feature and reference it from the CL description
https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... File components/dom_distiller/content/distiller_page_web_contents.cc (right): https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/content/distiller_page_web_contents.cc:33: content::WebContents* SourcePageHandleWebContents::GetWebContents() const { On 2015/05/13 07:56:38, nyquist wrote: > I think you want to rebase this on top of > https://codereview.chromium.org/1058193002/ I expect with that the changes to distiller_page_web_contents won't be needed. https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... File components/dom_distiller/content/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/content/dom_distiller_viewer_source.cc:192: class DomDistillerViewerSource::PrintPreviewRequestViewerHandle We probably don't want any printpreview-specific stuff in here. We'll need to figure out a way to inject the things that you want to have different from reader mode. Things that are different in this change: css (what parts?) html (what parts?) ignore theme changes ignore font changes no incremental loading
phistuck@gmail.com changed reviewers: + phistuck@gmail.com
https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... File components/dom_distiller/content/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/content/dom_distiller_viewer_source.cc:192: class DomDistillerViewerSource::PrintPreviewRequestViewerHandle On 2015/05/20 02:00:02, cjhopman wrote: > We probably don't want any printpreview-specific stuff in here. We'll need to > figure out a way to inject the things that you want to have different from > reader mode. > > Things that are different in this change: > > css (what parts?) > html (what parts?) > ignore theme changes > ignore font changes > no incremental loading We've been investigating how to remove the print preview specific code here, but have had some issues. The main problem is that the existing distiller UI is asynchronous. Distilled content is loaded by executing JavaScript and filling out the empty content div. For the case of the print preview, we need to know when to actually generate the print preview PDF. We either need to fill out the content synchronously during initial document load (current approach) or to somehow signal to the HiddenPrintPreview that the document is ready to print. It's unclear what method to use to signal that the document is ready to print. The DOM Distiller code knows when the div is filled out, but that information seems to be hidden within the abstraction. To use the existing UI, we would need to write code to plumb this through to the HiddenPrintPreview. Would that be okay?
https://codereview.chromium.org/1125343004/diff/20001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/1125343004/diff/20001/AUTHORS#newcode597 AUTHORS:597: Hewlett-Packard Company <*@hp.com> On 2015/05/13 07:56:38, nyquist wrote: > Remove Done. https://codereview.chromium.org/1125343004/diff/20001/build/ios/grit_whitelis... File build/ios/grit_whitelist.txt (right): https://codereview.chromium.org/1125343004/diff/20001/build/ios/grit_whitelis... build/ios/grit_whitelist.txt:23: IDR_DISTILLER_PRINT_PREVEW_CSS On 2015/05/15 17:25:44, Vitaly Buka wrote: > why do you need this on IOS? No, this has been removed. https://codereview.chromium.org/1125343004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/1125343004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:839: MaybeStartDistillation(initiator); On 2015/05/13 07:56:38, nyquist wrote: > Could this be simplified by using the new DistillAndView from > https://codereview.chromium.org/1058193002/ ? > > Depending on how we deal with the @media print things, it might have to change a > little bit though to use the correct URL. Yes, depending on whether or not we keep the synchronous DOM Distiller UI, this can use DistillAndView. https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... File components/dom_distiller/content/distiller_page_web_contents.cc (right): https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/content/distiller_page_web_contents.cc:33: content::WebContents* SourcePageHandleWebContents::GetWebContents() const { On 2015/05/20 02:00:02, cjhopman wrote: > On 2015/05/13 07:56:38, nyquist wrote: > > I think you want to rebase this on top of > > https://codereview.chromium.org/1058193002/ > > I expect with that the changes to distiller_page_web_contents won't be needed. Yes, I think this will go away after the rebase. https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/content/distiller_page_web_contents.cc:33: content::WebContents* SourcePageHandleWebContents::GetWebContents() const { On 2015/05/13 07:56:38, nyquist wrote: > I think you want to rebase this on top of > https://codereview.chromium.org/1058193002/ Acknowledged. https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... File components/dom_distiller/core/css/distilledpageprintpreview.css (right): https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/core/css/distilledpageprintpreview.css:1: /* Copyright 2014 The Chromium Authors. All rights reserved. On 2015/05/13 07:56:38, nyquist wrote: > Nit: 2015 Done. https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/core/css/distilledpageprintpreview.css:8: *, On 2015/05/13 07:56:38, nyquist wrote: > Could we re-use the original CSS file, and instead use CSS Media Types to > separate the unwanted parts using |@media print| and |@media screen|? After recent changes, this no longer seems to be necessary, so we have removed the custom CSS. https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... File components/dom_distiller/core/distiller_page.h (right): https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/core/distiller_page.h:14: #include "content/public/browser/web_contents.h" On 2015/05/13 07:56:38, nyquist wrote: > //components/dom_distiller/core can not and should not depend on //content. I believe this will be removed after the rebase. https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/core/distiller_page.h:24: virtual content::WebContents* GetWebContents() const = 0; On 2015/05/13 07:56:39, nyquist wrote: > This can't be part of the SourcePageHandle. That's what > SourcePageHandleWebContents is for, which lives in > //components/dom_distiller/content. Acknowledged. https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... File components/dom_distiller/core/url_utils.h (right): https://codereview.chromium.org/1125343004/diff/20001/components/dom_distille... components/dom_distiller/core/url_utils.h:24: // Returns the URL for viewing distilled content for a URL. On 2015/05/13 07:56:39, nyquist wrote: > Mention that this is for printing? Done.
https://codereview.chromium.org/1125343004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/1125343004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1263: can_simplify = cmdline->HasSwitch(switches::kEnableDomDistiller) && On 2015/05/13 07:56:38, nyquist wrote: > Should this also check to see if we assume that the distillation will work? > See dom_distiller::IsDistillablePage ( > https://code.google.com/p/chromium/codesearch#chromium/src/components/dom_dis... > ) The latest version of the CL does this, showing the checkbox asynchronously as the dialog appears. It seems like many pages are not marked as distillable even using the AdaBoost algorithm, but we've been testing with the alwaystrue heuristic.
We have uploaded rebased version of the patch. It eliminates all the patch changes to the URL Data Source.
vitalybuka@chromium.org changed reviewers: + alekseys@chromium.org
+alekseys for chrome/browser/resources/print_preview/
https://codereview.chromium.org/1125343004/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/ticket_items/print_friendly.js (right): https://codereview.chromium.org/1125343004/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/ticket_items/print_friendly.js:12: * document to print. Add selectionOnly param description. https://codereview.chromium.org/1125343004/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/ticket_items/print_friendly.js:39: return this.isAvailable_ && !this.selectionOnly_.getValue(); So if user unchecks "selection only" this checkbox appears? https://codereview.chromium.org/1125343004/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/preview_generator.js (right): https://codereview.chromium.org/1125343004/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/preview_generator.js:287: this.isPrintFriendlyEnabled_) || Two more spaces indent. https://codereview.chromium.org/1125343004/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/settings/other_options_settings.js (right): https://codereview.chromium.org/1125343004/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/settings/other_options_settings.js:311: this.duplexTicketItem_.isCapabilityAvailable() : this.isAvailable(); Convert it to regular if statement, it's too big to read now.
On 2015/07/01 23:23:51, Aleksey Shlyapnikov wrote: > https://codereview.chromium.org/1125343004/diff/80001/chrome/browser/resource... > File chrome/browser/resources/print_preview/data/ticket_items/print_friendly.js > (right): > > https://codereview.chromium.org/1125343004/diff/80001/chrome/browser/resource... > chrome/browser/resources/print_preview/data/ticket_items/print_friendly.js:12: * > document to print. > Add selectionOnly param description. DONE in the last patch. > > https://codereview.chromium.org/1125343004/diff/80001/chrome/browser/resource... > chrome/browser/resources/print_preview/data/ticket_items/print_friendly.js:39: > return this.isAvailable_ && !this.selectionOnly_.getValue(); > So if user unchecks "selection only" this checkbox appears? Yes, it was a mistake. We now use isCapabilityAvailable to avoid modifying the UI when unchecking the checkbox. > > https://codereview.chromium.org/1125343004/diff/80001/chrome/browser/resource... > File chrome/browser/resources/print_preview/preview_generator.js (right): > > https://codereview.chromium.org/1125343004/diff/80001/chrome/browser/resource... > chrome/browser/resources/print_preview/preview_generator.js:287: > this.isPrintFriendlyEnabled_) || > Two more spaces indent. DONE > > https://codereview.chromium.org/1125343004/diff/80001/chrome/browser/resource... > File chrome/browser/resources/print_preview/settings/other_options_settings.js > (right): > > https://codereview.chromium.org/1125343004/diff/80001/chrome/browser/resource... > chrome/browser/resources/print_preview/settings/other_options_settings.js:311: > this.duplexTicketItem_.isCapabilityAvailable() : this.isAvailable(); > Convert it to regular if statement, it's too big to read now. DONE with early return.
We have uploaded a new patch which simplifies version of the HiddenPrintPreview and solved issues pointed out by the last review.
https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/ticket_items/print_friendly.js (right): https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/print_friendly.js:41: return this.isAvailable_ && !this.selectionOnly_.isCapabilityAvailable(); Wouldn't the behavior be pretty random for the unsuspecting user? Any text selected on the page will cause this option to disappear, even if user wants to print the entire page and unaware of the selection. https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/print_friendly.js:56: this.dispatchChangeEventInternal(); Dispatch this event only when the value was actually changed. https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/print_preview/settings/other_options_settings.js (right): https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... chrome/browser/resources/print_preview/settings/other_options_settings.js:311: this.duplexTicketItem_.isCapabilityAvailable(); if (this.collapseContent) { ... }
On 2015/07/07 01:26:18, Aleksey Shlyapnikov wrote: > https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... > File chrome/browser/resources/print_preview/data/ticket_items/print_friendly.js > (right): > > https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... > chrome/browser/resources/print_preview/data/ticket_items/print_friendly.js:41: > return this.isAvailable_ && !this.selectionOnly_.isCapabilityAvailable(); > Wouldn't the behavior be pretty random for the unsuspecting user? Any text > selected on the page will cause this option to disappear, even if user wants to > print the entire page and unaware of the selection. > Hrm, yep, I see what you are describing. Would it be an option to let the checkbox there even in case of selection? When there is a selection, print friendly would not change what is going to be printed because it is basically text, but in this case the user can understand what is going on. Any other behavior that would fit better? > https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... > chrome/browser/resources/print_preview/data/ticket_items/print_friendly.js:56: > this.dispatchChangeEventInternal(); > Dispatch this event only when the value was actually changed. > Ok, we will add an early return when the values are equal. > https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... > File chrome/browser/resources/print_preview/settings/other_options_settings.js > (right): > > https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... > chrome/browser/resources/print_preview/settings/other_options_settings.js:311: > this.duplexTicketItem_.isCapabilityAvailable(); > if (this.collapseContent) { > ... > } Ok, we will add the braces. Thanks for the review.
Sorry for delay. I waited review from distiller team, and didn't notice how most distiller changes were removed. CL in general looks good, but needs a lot of small fixes. https://codereview.chromium.org/1125343004/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1125343004/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:8678: + <message name="IDS_PRINT_PREVIEW_OPTION_PRINT_FRIENDLY" desc="Checkbox label that provides a choice to print a simplified version of the page."> Please use the same name in the code for this feature. I propose DistillPage. We need something else of UI string too. I'd put "Distill Page" for now here too, and we change to something user-friendly later. https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/print_ticket_store.js (right): https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/print_ticket_store.js:192: this.printFriendly_ = new print_preview.ticket_items.PrintFriendly( "Print friendly" is not verbose name, not clear what is used for. Let use "distillPage" in source code. https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... chrome/browser/resources/print_preview/native_layer.js:70: global.onDetectedSimplifiablePage = DetectDistillablePage https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/tab_... File chrome/browser/ui/tab_helpers.h (right): https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/tab_... chrome/browser/ui/tab_helpers.h:24: class HiddenWebContents; don't need this https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/hidden_print_preview.cc (right): https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. we use "// Copyright 2015" now https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:47: // Examples include rendering a site that redirects to an app URL, I am no sure how distilled web content works is navigation there are possible at all? https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:51: return NULL; nullptr https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:121: : session_storage_namespace_id_(-1), session_storage_namespace_id_ is not used https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:122: profile_(Profile::FromBrowserContext( don't need profile_ member, you can call FromBrowserContext when needed https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:124: target_web_contents_(target_web_contents), target_web_contents -> source_web_contents https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:135: void HiddenPrintPreview::StartRendering( StartRendering starts nothing. Probably should be called CreateDestinationWebContents https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:151: web_contents_.get()->SetDelegate(web_contents_delegate_.get()); maybe better to set/remove delgate in constructor/destructor of WebContentsDelegateImpl https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:167: if (web_contents_.get()) { just "if (web_contents_)" and everywhere else https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:169: content::WebContentsObserver::Observe(NULL); Probably WebContentsDelegateImpl should be a WebContentsObserver and NotificationObserver https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:239: void HiddenPrintPreview::DidStopLoading() { Please remove these three. base class already has them empty. https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:275: dialog_controller->AddProxyDialogForWebContents( why Add here and not when contents created? https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:292: void HiddenPrintPreview::Destroy() { Destroy -> Fail or Abort https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:293: if (rendering_has_been_cancelled_) I don't see who change rendering_has_been_cancelled_ https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.cc:296: print_preview_ui_->OnPrintPreviewFailed(); I don't see other use of print_preview_ui_ here Can you replace with on_failed_callback_ https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/hidden_print_preview.h (right): https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.h:25: class HiddenPrintPreview : public content::NotificationObserver, Please explain purpose of this class here. https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.h:29: HiddenPrintPreview( HiddenPrintPreview -> PrintPreviewDistiller https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.h:30: content::WebContents* target_web_contents, misaligned please run "git cl format" https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.h:40: const gfx::Size& size, why is this virtual? https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.h:85: int64 session_storage_namespace_id_; unused https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.h:88: Profile* profile_; remove please https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.h:93: bool has_finished_loading_; unused https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.h:97: bool rendering_has_been_cancelled_; unused https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.h:102: gfx::Size size_; it's always same, please remove and create constant in cc file https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.h:105: content::WebContents* target_web_contents_; if you can AddProxyDialogForWebContents in constructor, you don't need this too https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/hidden_print_preview.h:111: PrintPreviewUI* print_preview_ui_; replace with callback https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1279: dom_distiller::IsDistillablePage(initiator, false, I don't see guaranty that HandleIsPageDistillableResult will never be called after PrintPreviewHandler is destroyed. So you need weak_factory_.GetWeakPtr() insted of base::Unretained(this)
There are a lot of changes so we are not commenting one by one, we followed all the suggestions and refactored the code with your recommendations in the last two reviews. Thanks for the reviews.
Thanks for updated CL. I have some more comments. This may look like a lot, but they are mostly similar and should be easy to fix. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:44: settings_(settings.release()) { settings.Pass() https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:45: print_preview_distiller_->GetWebContents()->SetDelegate(this); base class has web_contents() getter please use web_contents() instead of print_preview_distiller_->GetWebContents() https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:47: content::WebContentsObserver::Observe( Please use fallowing constructor instead of content::WebContentsObserver::Observe() https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:148: DCHECK(print_preview_distiller_->GetWebContents()); DCHECK(print_preview_distiller_->GetWebContents()); is unnecessary, it will crash next line anyway https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:152: DCHECK(settings_); same here https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:206: PrintPreviewDistiller* print_preview_distiller_; = nullptr https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:214: scoped_ptr<base::DictionaryValue> settings) { Could you bind outside this class and pass callback here new PrintPreviewDistiller(web_contets, base::Bind(&PrintPreviewUI::OnPrintPreviewFailed, print_preview_ui->GetWeakPtr()), settings.Pass()) so this class will have no dependency on PrintPreviewUI please also remove includes if it's possible https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:215: DCHECK(print_preview_ui); DCHECK only if code around is unaffected by unexpected conditions. If it crashes anyway, DCHECK is redundant https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:232: DCHECK(web_contents_.get() == NULL); if possible DCHECK(!web_contents_) if not, then DCHECK_NE(web_contents_.get(), nullptr) https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:246: if (!dialog_controller) { you should drop {} here for consistency https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.h (right): https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:33: virtual ~PrintPreviewDistiller(); Please make "class PrintPreviewDistiller final :" and remove all "virtual" in this file. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:35: void Abort(); Abort also can be removed. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:37: content::WebContents* GetWebContents(); Getter can be removed if WebContentsDelegateImpl uses web_contets() from base class https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:48: class WebContentsDelegateImpl; "class" declaration should go before methods, just after "private:" https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:60: base::Closure on_failed_callback_; This class doesn't need on_failed_callback_, only WebContentsDelegateImpl Could you please move it there. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right): https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.h:213: base::WeakPtrFactory<PrintPreviewUI> weak_ptr_factory_; just base::WeakPtrFactory<PrintPreviewUI> weak_ptr_factory_(this);
Most of the requests are acknowledged and already being worked on except for one issue which we need to confirm. https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/print_ticket_store.js (right): https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/print_ticket_store.js:192: this.printFriendly_ = new print_preview.ticket_items.PrintFriendly( On 2015/07/13 07:00:30, Vitaly Buka wrote: > "Print friendly" is not verbose name, not clear what is used for. > Let use "distillPage" in source code. Acknowledged. https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/1125343004/diff/100001/chrome/browser/resourc... chrome/browser/resources/print_preview/native_layer.js:70: global.onDetectedSimplifiablePage = On 2015/07/13 07:00:30, Vitaly Buka wrote: > DetectDistillablePage Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:44: settings_(settings.release()) { On 2015/07/20 20:45:33, Vitaly Buka wrote: > settings.Pass() Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:45: print_preview_distiller_->GetWebContents()->SetDelegate(this); On 2015/07/20 20:45:32, Vitaly Buka wrote: > base class has web_contents() getter > please use web_contents() > instead of > print_preview_distiller_->GetWebContents() Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:47: content::WebContentsObserver::Observe( On 2015/07/20 20:45:33, Vitaly Buka wrote: > Please use fallowing constructor instead of > content::WebContentsObserver::Observe() > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:148: DCHECK(print_preview_distiller_->GetWebContents()); On 2015/07/20 20:45:33, Vitaly Buka wrote: > DCHECK(print_preview_distiller_->GetWebContents()); > is unnecessary, it will crash next line anyway Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:152: DCHECK(settings_); On 2015/07/20 20:45:33, Vitaly Buka wrote: > same here Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:206: PrintPreviewDistiller* print_preview_distiller_; On 2015/07/20 20:45:33, Vitaly Buka wrote: > = nullptr Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:214: scoped_ptr<base::DictionaryValue> settings) { On 2015/07/20 20:45:33, Vitaly Buka wrote: > Could you bind outside this class and pass callback here > new PrintPreviewDistiller(web_contets, > base::Bind(&PrintPreviewUI::OnPrintPreviewFailed, > print_preview_ui->GetWeakPtr()), settings.Pass()) > > so this class will have no dependency on PrintPreviewUI > please also remove includes if it's possible Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:215: DCHECK(print_preview_ui); On 2015/07/20 20:45:33, Vitaly Buka wrote: > DCHECK only if code around is unaffected by unexpected conditions. > If it crashes anyway, DCHECK is redundant Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:232: DCHECK(web_contents_.get() == NULL); On 2015/07/20 20:45:33, Vitaly Buka wrote: > if possible DCHECK(!web_contents_) > if not, then > DCHECK_NE(web_contents_.get(), nullptr) Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:246: if (!dialog_controller) { On 2015/07/20 20:45:33, Vitaly Buka wrote: > you should drop {} here for consistency Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.h (right): https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:33: virtual ~PrintPreviewDistiller(); On 2015/07/20 20:45:33, Vitaly Buka wrote: > Please make "class PrintPreviewDistiller final :" and remove all "virtual" in > this file. Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:35: void Abort(); On 2015/07/20 20:45:33, Vitaly Buka wrote: > Abort also can be removed. Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:37: content::WebContents* GetWebContents(); On 2015/07/20 20:45:33, Vitaly Buka wrote: > Getter can be removed if WebContentsDelegateImpl uses web_contets() from base > class Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:48: class WebContentsDelegateImpl; On 2015/07/20 20:45:33, Vitaly Buka wrote: > "class" declaration should go before methods, just after "private:" Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:60: base::Closure on_failed_callback_; On 2015/07/20 20:45:33, Vitaly Buka wrote: > This class doesn't need on_failed_callback_, only WebContentsDelegateImpl > Could you please move it there. Acknowledged. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right): https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.h:213: base::WeakPtrFactory<PrintPreviewUI> weak_ptr_factory_; On 2015/07/20 20:45:33, Vitaly Buka wrote: > just > base::WeakPtrFactory<PrintPreviewUI> weak_ptr_factory_(this); Please review and clarify (this is a header file). We already instantiate weak_ptr_factory_ on the constructor passing in this as the parameter.
https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right): https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.h:213: base::WeakPtrFactory<PrintPreviewUI> weak_ptr_factory_; Correct, you can move weak_ptr_factory_(this) from cc to here. Also you can keep it as is. I am not going to insist here. On 2015/07/21 17:25:01, arjunpatel wrote: > On 2015/07/20 20:45:33, Vitaly Buka wrote: > > just > > base::WeakPtrFactory<PrintPreviewUI> weak_ptr_factory_(this); > > Please review and clarify (this is a header file). We already instantiate > weak_ptr_factory_ on the constructor passing in this as the parameter.
vitalybuka@chromium.org changed reviewers: + avi@chromium.org
+avi for chrome/browser/ui/tab_helpers.h
On 2015/07/21 18:43:26, Vitaly Buka wrote: > +avi for chrome/browser/ui/tab_helpers.h I am not convinced. Why do distiller contents need _all_ of the tab helpers? Especially since the distiller contents is hidden, as you called out.
Blindly adding every tab helper isn't the way to go here. https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/tab_... File chrome/browser/ui/tab_helpers.h (right): https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/tab_... chrome/browser/ui/tab_helpers.h:54: // which is supported by tab helpers. But not *all* the tab helpers. Please explicitly add only the ones you need.
https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/tab_... File chrome/browser/ui/tab_helpers.h (right): https://codereview.chromium.org/1125343004/diff/120001/chrome/browser/ui/tab_... chrome/browser/ui/tab_helpers.h:54: // which is supported by tab helpers. On 2015/07/21 20:47:58, Avi wrote: > But not *all* the tab helpers. Please explicitly add only the ones you need. Acknowledged.
Uploaded a new patch with more refactors and remove tab_helpers dependency
https://codereview.chromium.org/1125343004/diff/140001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1125343004/diff/140001/chrome/app/generated_r... chrome/app/generated_resources.grd:8679: + Distill page This sounds very technical; would an average user know what it meant? I personally would go with "Simplify page" but you should run new UI by the UI team. https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/printin... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.cc:190: contents = const_cast<WebContents*>(proxied->second); From what I see, |proxied->second| is already a WebContents*. Why the cast? https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/ticket_items/distill_page.js (right): https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/distill_page.js:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. For new files, remove the (c). One day we'll get around to removing the "(c)" from all the old files. https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:58: const OpenURLParams& params) final { Why final? Please don't use it (here and below); it's not Chromium style. https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.h (right): https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:11: #include "base/observer_list.h" unused; remove https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:14: #include "content/public/browser/notification_registrar.h" both notification lines are unused; remove https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:15: #include "content/public/browser/web_contents_observer.h" unused; remove https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:16: #include "content/public/common/referrer.h" unused; remove https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:17: #include "ui/gfx/geometry/size.h" unused; remove https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:25: class PrintPreviewDistiller final We don't use final in Chromium code. https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:26: : public base::SupportsWeakPtr<PrintPreviewDistiller> { Use WeakPtrFactory as a member instead; it has better semantics during object teardown. https://codereview.chromium.org/1125343004/diff/140001/printing/print_job_con... File printing/print_job_constants.h (right): https://codereview.chromium.org/1125343004/diff/140001/printing/print_job_con... printing/print_job_constants.h:33: PRINTING_EXPORT extern const char kSettingDistillPageEnabled[]; If all the existing items are in alphabetical order, add your item in alphabetical order.
lgtm https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:38: base::Closure on_failed_callback) const base::Closure& on_failed_callback https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:199: base::Closure on_failed_callback, const base::Closure& on_failed_callback https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:213: scoped_ptr<base::DictionaryValue> settings, const base::Closure& on_failed_callback
lgtm Please continue with avi@ and probably nyquist@ for distiller change. https://codereview.chromium.org/1125343004/diff/140001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1125343004/diff/140001/chrome/app/generated_r... chrome/app/generated_resources.grd:8679: + Distill page On 2015/07/23 18:25:52, Avi wrote: > This sounds very technical; would an average user know what it meant? I > personally would go with "Simplify page" but you should run new UI by the UI > team. Feature is under the flag. It will have UI review and we should be able to address this in future CL. I asked to use distill just for consistency with code for now. "Simplify page" is OK too. https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.h (right): https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.h:25: class PrintPreviewDistiller final On 2015/07/23 18:25:52, Avi wrote: > We don't use final in Chromium code. Actually it was my advice. It's allowed according http://chromium-cpp.appspot.com/ But I don't mind to go without final here.
On 2015/07/23 18:45:11, Vitaly Buka wrote: > Actually it was my advice. > It's allowed according http://chromium-cpp.appspot.com/ > But I don't mind to go without final here. Blink uses "final" all over the place. Chromium never uses it. It's one of the things we haven't yet reconciled.
On 2015/07/23 18:25:53, Avi wrote: > https://codereview.chromium.org/1125343004/diff/140001/chrome/app/generated_r... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/1125343004/diff/140001/chrome/app/generated_r... > chrome/app/generated_resources.grd:8679: + Distill page > This sounds very technical; would an average user know what it meant? I > personally would go with "Simplify page" but you should run new UI by the UI > team. > > https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/printin... > File chrome/browser/printing/print_preview_dialog_controller.cc (right): > > https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/printin... > chrome/browser/printing/print_preview_dialog_controller.cc:190: contents = > const_cast<WebContents*>(proxied->second); > From what I see, |proxied->second| is already a WebContents*. Why the cast? > > https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/resourc... > File chrome/browser/resources/print_preview/data/ticket_items/distill_page.js > (right): > > https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/resourc... > chrome/browser/resources/print_preview/data/ticket_items/distill_page.js:1: // > Copyright (c) 2015 The Chromium Authors. All rights reserved. > For new files, remove the (c). One day we'll get around to removing the "(c)" > from all the old files. > > https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): > > https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... > chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:58: const > OpenURLParams& params) final { > Why final? Please don't use it (here and below); it's not Chromium style. > > https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/print_preview/print_preview_distiller.h (right): > > https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... > chrome/browser/ui/webui/print_preview/print_preview_distiller.h:11: #include > "base/observer_list.h" > unused; remove > > https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... > chrome/browser/ui/webui/print_preview/print_preview_distiller.h:14: #include > "content/public/browser/notification_registrar.h" > both notification lines are unused; remove > > https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... > chrome/browser/ui/webui/print_preview/print_preview_distiller.h:15: #include > "content/public/browser/web_contents_observer.h" > unused; remove > > https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... > chrome/browser/ui/webui/print_preview/print_preview_distiller.h:16: #include > "content/public/common/referrer.h" > unused; remove > > https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... > chrome/browser/ui/webui/print_preview/print_preview_distiller.h:17: #include > "ui/gfx/geometry/size.h" > unused; remove > > https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... > chrome/browser/ui/webui/print_preview/print_preview_distiller.h:25: class > PrintPreviewDistiller final > We don't use final in Chromium code. > > https://codereview.chromium.org/1125343004/diff/140001/chrome/browser/ui/webu... > chrome/browser/ui/webui/print_preview/print_preview_distiller.h:26: : public > base::SupportsWeakPtr<PrintPreviewDistiller> { > Use WeakPtrFactory as a member instead; it has better semantics during object > teardown. > > https://codereview.chromium.org/1125343004/diff/140001/printing/print_job_con... > File printing/print_job_constants.h (right): > > https://codereview.chromium.org/1125343004/diff/140001/printing/print_job_con... > printing/print_job_constants.h:33: PRINTING_EXPORT extern const char > kSettingDistillPageEnabled[]; > If all the existing items are in alphabetical order, add your item in > alphabetical order. Done. We are keeping the name suggested by Vitaly (Distill Page)
The code looks good, and the UI string remains terrible. No one will know what "distill page" means. The only reason I know is because I've read the code. I'm begging for you to change that string. https://codereview.chromium.org/1125343004/diff/140001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1125343004/diff/140001/chrome/app/generated_r... chrome/app/generated_resources.grd:8679: + Distill page On 2015/07/23 18:45:11, Vitaly Buka wrote: > On 2015/07/23 18:25:52, Avi wrote: > > This sounds very technical; would an average user know what it meant? I > > personally would go with "Simplify page" but you should run new UI by the UI > > team. > > Feature is under the flag. It will have UI review and we should be able to > address this in future CL. > I asked to use distill just for consistency with code for now. > "Simplify page" is OK too. Yes, it will have UI review later, but there's no reason to not have a good name now.
On 2015/07/28 23:10:57, Avi wrote: > The code looks good, and the UI string remains terrible. No one will know what > "distill page" means. The only reason I know is because I've read the code. > > I'm begging for you to change that string. > > https://codereview.chromium.org/1125343004/diff/140001/chrome/app/generated_r... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/1125343004/diff/140001/chrome/app/generated_r... > chrome/app/generated_resources.grd:8679: + Distill page > On 2015/07/23 18:45:11, Vitaly Buka wrote: > > On 2015/07/23 18:25:52, Avi wrote: > > > This sounds very technical; would an average user know what it meant? I > > > personally would go with "Simplify page" but you should run new UI by the UI > > > team. > > > > Feature is under the flag. It will have UI review and we should be able to > > address this in future CL. > > I asked to use distill just for consistency with code for now. > > "Simplify page" is OK too. > > Yes, it will have UI review later, but there's no reason to not have a good name > now. Ok, let's use that string, I think Vitaly is ok to go back to it
Fixed UI string based on feedback.
On 2015/07/30 17:10:20, arjunpatel wrote: > Fixed UI string based on feedback. LGTM
lgtm
components/dom_distiller lgtm
https://codereview.chromium.org/1125343004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/ticket_items/distill_page.js (right): https://codereview.chromium.org/1125343004/diff/180001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/distill_page.js:28: this.selectionOnly_ = selectionOnly; You're not using selectionOnly_ anywhere, right? Remove it then.
https://codereview.chromium.org/1125343004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/ticket_items/distill_page.js (right): https://codereview.chromium.org/1125343004/diff/180001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/ticket_items/distill_page.js:28: this.selectionOnly_ = selectionOnly; You're not using selectionOnly_ anywhere, right? Remove it then.
lgtm
Added new patch and removed unused variables based on feedback from Aleksey.
lgtm for chrome/browser/resources/print_preview/
CQ?
The CQ bit was checked by arjunpatel@hp.com
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, jochen@chromium.org, nyquist@chromium.org, vitalybuka@chromium.org Link to the patchset: https://codereview.chromium.org/1125343004/#ps200001 (title: "Cleanup unused variables")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1125343004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1125343004/200001
The CQ bit was unchecked by commit-bot@chromium.org
The author arjunpatel@hp.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by vitalybuka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1125343004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1125343004/200001
The CQ bit was unchecked by commit-bot@chromium.org
The author arjunpatel@hp.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by vitalybuka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1125343004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1125343004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: The author arjunpatel@hp.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
Can you please re-base this, there are some conflicts. I can't upload patch into issue you own. Then I'll run try bots and submit manually.
Arjun, I tried to apply the last patch. There are some missing includes which are easy to fix. However I can't find sites where IsDistillablePage returns true. If I enforce true then browser always crash when I enable that option.
Fixup "Add a hidden renderer for simplified printed pages"
Run JavaScript in an isolated world, as suggested in
https://codereview.chromium.org/1271313003/
Also take into consideration that the code we inject will
be executed before the rest of the distiller JavaScript code.
Set our variable directly instead of executing a method.
The CQ bit was checked by vitalybuka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1125343004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1125343004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: The author arjunpatel@hp.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
Failed some times.
On 2015/08/12 18:02:55, Vitaly Buka wrote: > Failed some times. You can't use "Dry run" yet, but probably you can start tests by "git cl try" or but using "Choose trybots" link above.
The CQ bit was checked by arjunpatel@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/1125343004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1125343004/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: The author arjunpatel@hp.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
Message was sent while issue was closed.
Committed patchset #13 (id:240001) manually as ab26826ebd246afdbcc94d44f00015f10f6db6da (presubmit successful).
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
I have a number of issues with this CL. All the nitty things that I can fix, I have fixed in https://codereview.chromium.org/1294663003, but I'm not thrilled about the IPC sending. https://codereview.chromium.org/1125343004/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right): https://codereview.chromium.org/1125343004/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:144: rvh->Send(new PrintMsg_InitiatePrintPreview(rvh->GetRoutingID(), false)); Has this been discussed? Seems a bit crazy to just arbitrarily send printing IPC messages here. If we use the analogy that print preview is a carefully orchestrated dance between the browser and renderer, this is a random stranger cutting in. |
