Fix various issues from r343263.

Fix various issues from r343263.

BUG=490809

[Lei Zhang, 2015-08-14 05:44:34]

thestig@chromium.org changed reviewers:
+ arjunpatel@hp.com, nyquist@chromium.org, vitalybuka@chromium.org

[Lei Zhang, 2015-08-14 05:44:36]

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_dialog_controller.h (left):

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_dialog_controller.h:82: void
AddProxyDialogForWebContents(content::WebContents* source,
It's not clear what |source| and |target| refers to.

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_dialog_controller.h:130:
PrintPreviewDialogMap proxied_dialog_map_;
Please do not reuse PrintPreviewDialogMap. Even though this map has the same
std::map template as PrintPreviewDialogMap, the key and value in this map means
something entirely different.

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (left):

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:38: explicit
WebContentsDelegateImpl(WebContents* web_contents,
nit: not explicit

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:199:
base::Closure on_failed_callback_;
const since it never changes

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:206:
content::SessionStorageNamespace* session_storage_namespace =
Since this is derived from |source_web_content|, you can probably retrieve it
later instead of retrieving it here and passing it around.

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:211:
DCHECK(web_contents_);
DistillAndView() will DCHECK its arguments, so this is redundant.

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:243:
PrintPreviewDistiller::~PrintPreviewDistiller() {
Please put the dtor right below the ctor, in the same order as the header file.

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:254:
WebContents* PrintPreviewDistiller::CreateWebContents(
This is a standalone function, so it might as well be just that.

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right):

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:150:
"didNavigateOnInitialContentLoad()");
I haven't tested this to make sure it works.

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_distiller.h (left):

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_distiller.h:12: #include
"content/public/browser/web_contents_observer.h"
nit: not used

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_distiller.h:15: class
URLRequestContextGetter;
nit: not used

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_distiller.h:23:
base::Closure on_failed_callback,
pass callbacks by const ref

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (left):

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1118: if
(print_preview_ui())
This has nothing to do with your CL, I just don't think this will ever fail, so
I changed it.

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_handler.h (left):

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_handler.h:340: void
HandleIsPageDistillableResult(bool distillable);
nit: name it consistently with everything else.

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_handler.h:398: // A print
preview that is responsible for rendering the page after
A PrintPreviewDistiller != "a print preview"

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_ui.h (left):

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:170:
base::WeakPtr<PrintPreviewUI> GetWeakPtr();
You should not create a WeakPtrFactory and publicly hand out WeakPtrs. As seen
in the WeakPtrFactory comments. "This is helpful if you only need weak pointers
within the implementation of a class."

Instead, this class should have subclassed from 
SupportsWeakPtr, if you really needed WeakPtrs for PrintPreviewUI, but it turns
out you don't.

https://codereview.chromium.org/1294663003/diff/1/components/dom_distiller/co...
File components/dom_distiller/core/javascript/dom_distiller_viewer.js (left):

https://codereview.chromium.org/1294663003/diff/1/components/dom_distiller/co...
components/dom_distiller/core/javascript/dom_distiller_viewer.js:14: if (typeof
navigate_on_initial_content_load !== 'undefined' &&
Why isn't |navigate_on_initial_content_load| properly defined as a variable? I
see in https://codereview.chromium.org/1125343004, patchset 11 had it written
the way I did here, but then it changed. What happened? Was the change reviewed?

[PhistucK, 2015-08-14 06:34:03]

phistuck@gmail.com changed reviewers:
+ phistuck@gmail.com

[PhistucK, 2015-08-14 06:34:03]

Just a quick drive by. :)

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right):

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:137: //
prerendering before any navigations occurs. Note that this is
I think you introduced a grammar error here - "navigations occur", not "occurs".

[csaavedra, 2015-08-14 08:21:38]

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_distiller.cc (right):

https://codereview.chromium.org/1294663003/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_distiller.cc:150:
"didNavigateOnInitialContentLoad()");
On 2015/08/14 05:44:35, Lei Zhang wrote:
> I haven't tested this to make sure it works.

This won't work, as this JS might run before the distiller's JS and the method
will be undefined.

https://codereview.chromium.org/1294663003/diff/1/components/dom_distiller/co...
File components/dom_distiller/core/javascript/dom_distiller_viewer.js (left):

https://codereview.chromium.org/1294663003/diff/1/components/dom_distiller/co...
components/dom_distiller/core/javascript/dom_distiller_viewer.js:14: if (typeof
navigate_on_initial_content_load !== 'undefined' &&
On 2015/08/14 05:44:36, Lei Zhang wrote:
> Why isn't |navigate_on_initial_content_load| properly defined as a variable? I
> see in https://codereview.chromium.org/1125343004, patchset 11 had it written
> the way I did here, but then it changed. What happened? Was the change
reviewed?

As explained earlier, the code injected from
PrintPreviewDistiller::DidFinishLoad() can run before the distiller's JS, hence
the method can possibly be undefined. That's why it got changed.

See https://codereview.chromium.org/1125343004/#msg70

[arjunpatel, 2015-10-15 17:01:11]

@Lei, some fixes may not be relevant anymore but others may still be useful. 
Do you want to close this CL and create a new one?

[Vitaly Buka (NO REVIEWS), 2015-11-04 00:11:39]

Description was changed from

==========
Fix various issues from r343263.

BUG=490809
==========

to

==========
Fix various issues from r343263.

BUG=490809
==========

[Vitaly Buka (NO REVIEWS), 2015-11-04 00:11:39]

vitalybuka@chromium.org changed reviewers:
- vitalybuka@chromium.org