Refactor webview to use //components/printing

Refactor webview to use //components/printing

Some printing related //android_webview were copied over from
//chrome. Now that the ones in //chrome can move to a component,
webview should also use these files.

Depends on https://codereview.chromium.org/811563008/

BUG=444883, 311308
Committed: https://crrev.com/2fa98a5958ad5ecb86484acbeed3a05326647d4a
Cr-Commit-Position: refs/heads/master@{#314307}

[dgn, 2015-01-08 20:21:22]

dgn@chromium.org changed reviewers:
+ sgurun@chromium.org, vitalybuka@chromium.org

[dgn, 2015-01-08 20:21:22]

For now printing is broken in webview

[Vitaly Buka (NO REVIEWS), 2015-01-08 22:51:02]

On 2015/01/08 20:21:22, dgn wrote:
> For now printing is broken in webview

BUG?

[Vitaly Buka (NO REVIEWS), 2015-01-08 22:54:19]

On 2015/01/08 20:21:22, dgn wrote:
> For now printing is broken in webview

Why it's broken?

[Vitaly Buka (NO REVIEWS), 2015-01-08 22:54:27]

https://codereview.chromium.org/822133003/diff/1/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/822133003/diff/1/components/components.gyp#ne...
components/components.gyp:142: ['OS != "ios" and (enable_basic_printing==1 or
enable_print_preview==1)', {
Isn't (enable_basic_printing==0 and enable_print_preview==0) on IOS?

[sgurun-gerrit only, 2015-01-09 02:42:53]

On 2015/01/08 22:54:27, Vitaly Buka wrote:
> https://codereview.chromium.org/822133003/diff/1/components/components.gyp
> File components/components.gyp (right):
> 
>
https://codereview.chromium.org/822133003/diff/1/components/components.gyp#ne...
> components/components.gyp:142: ['OS != "ios" and (enable_basic_printing==1 or
> enable_print_preview==1)', {
> Isn't (enable_basic_printing==0 and enable_print_preview==0) on IOS?

seconding Vitaly's question: why is printing broken in Webview? If it is broken
due to one of these changes, we have to fix it ASAP.

[dgn, 2015-01-09 10:29:29]

Sorry, I wrote it in a confusing way. My changes are the ones that broke
printing, and I'm currently trying understand why and fix it.

[Vitaly Buka (NO REVIEWS), 2015-01-09 19:09:39]

On 2015/01/09 10:29:29, dgn wrote:
> Sorry, I wrote it in a confusing way. My changes are the ones that broke
> printing, and I'm currently trying understand why and fix it.

this change breaks printing, not the one we are going to land?

[chromium-reviews, 2015-01-09 19:18:47]

Yes, Gmail prints fine with https://codereview.chromium.org/811563008/
The changes in 811563008 don't affect webview since on that patch it still
uses the duplicated files.
On Fri Jan 09 2015 at 7:09:41 PM <vitalybuka@chromium.org> wrote:

> On 2015/01/09 10:29:29, dgn wrote:
> > Sorry, I wrote it in a confusing way. My changes are the ones that broke
> > printing, and I'm currently trying understand why and fix it.
>
> this change breaks printing, not the one we are going to land?
>
> https://codereview.chromium.org/822133003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dgn, 2015-01-09 19:31:07]

Sorry, I replied to the email instead of doing it from the review. Pasting the
message again here.

Yes, Gmail prints fine with https://codereview.chromium.org/811563008/
The changes in 811563008 don't affect webview since on that patch it still uses
the duplicated files.

[dgn, 2015-01-15 19:28:48]

https://codereview.chromium.org/822133003/diff/20001/components/printing/rend...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/822133003/diff/20001/components/printing/rend...
components/printing/renderer/print_web_view_helper.cc:1271:
!GetPrintSettingsFromUser(frame_ref.GetFrame(), node,
This block is disabled with a #if 0 in webview. I did't see a special flag used
in the code to check for webview builds. Would finding/adding something to ifdef
this out be better that such a boolean?

[sgurun-gerrit only, 2015-01-15 19:39:32]

https://codereview.chromium.org/822133003/diff/20001/components/printing/rend...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/822133003/diff/20001/components/printing/rend...
components/printing/renderer/print_web_view_helper.cc:1271:
!GetPrintSettingsFromUser(frame_ref.GetFrame(), node,
On 2015/01/15 19:28:48, dgn wrote:
> This block is disabled with a #if 0 in webview. I did't see a special flag
used
> in the code to check for webview builds. Would finding/adding something to
ifdef
> this out be better that such a boolean?

I think this logic is chrome only. Webview cannot get print settings from user
since it does not have a UI.  we will need to use a delegate for this, which
webview will not implement.

[Torne, 2015-01-15 19:46:16]

There is no #ifdef to check for webview; webview is not intended to forever
have a special build configuration and you can (as of very recently) build
a fully working webview just by running "ninja all" when set up for Android
:)

Any differences between webview and other targets must be either handled by
runtime settings, or by including different source files in the respective
targets (e.g. by having it controlled by a delegate that's implemented in
the chrome/webview layer, outside the component).
On 15 Jan 2015 7:28 pm, <dgn@chromium.org> wrote:

>
> https://codereview.chromium.org/822133003/diff/20001/
> components/printing/renderer/print_web_view_helper.cc
> File components/printing/renderer/print_web_view_helper.cc (right):
>
> https://codereview.chromium.org/822133003/diff/20001/
> components/printing/renderer/print_web_view_helper.cc#newcode1271
> components/printing/renderer/print_web_view_helper.cc:1271:
> !GetPrintSettingsFromUser(frame_ref.GetFrame(), node,
> This block is disabled with a #if 0 in webview. I did't see a special
> flag used in the code to check for webview builds. Would finding/adding
> something to ifdef this out be better that such a boolean?
>
> https://codereview.chromium.org/822133003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dgn, 2015-01-15 22:15:45]

@torne: That's very good to know, thanks! Does it mean that I don't need
"android_webview_telemetry_build=1" to compile webview?

@sgurun: I thought about having a delegate for it, but the code of
GetPrintSettingsFromUser is needed for basic printing and is not dependent on
//chrome. So I think it has its place in the component. I could make a delegate
method to check whether to run it, but that would just be moving the boolean
from the constructor to the delegate. Which might actually be a good thing,
since there are already 3 booleans there, splitting the configuration of print
for the callers between the constructor call and the delegate definition.

So, do you think I should make out_of_process_pdf_enabled,
print_preview_disabled, should_ask_print_settings methods of the delegate
instead of attributes?

[sgurun-gerrit only, 2015-01-16 03:34:15]

On 2015/01/15 22:15:45, dgn wrote:
> @torne: That's very good to know, thanks! Does it mean that I don't need
> "android_webview_telemetry_build=1" to compile webview?
> 
> @sgurun: I thought about having a delegate for it, but the code of
> GetPrintSettingsFromUser is needed for basic printing and is not dependent on
> //chrome. So I think it has its place in the component. I could make a
delegate
> method to check whether to run it, but that would just be moving the boolean
> from the constructor to the delegate. Which might actually be a good thing,
> since there are already 3 booleans there, splitting the configuration of print
> for the callers between the constructor call and the delegate definition.
> 
> So, do you think I should make out_of_process_pdf_enabled,
> print_preview_disabled, should_ask_print_settings methods of the delegate
> instead of attributes?

Yes we can leave the code in components. 
should_as_print_settings does not exist in android_webview duplicate, so I think
it was added later. I am fine either way. If they are attributes, we need to be
able to configure at run time, or if they are delegate we will have a different
implementation. likely leaving them as attributes is easier at this stage. we
can always change them, so no worries.

[dgn, 2015-01-16 10:26:43]

> should_ask_print_settings does not exist in android_webview duplicate, so I
think
> it was added later.

Yes, I just added it in this patch. Could you please review it?

https://codereview.chromium.org/822133003/diff/1/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/822133003/diff/1/components/components.gyp#ne...
components/components.gyp:142: ['OS != "ios" and (enable_basic_printing==1 or
enable_print_preview==1)', {
On 2015/01/08 22:54:27, Vitaly Buka wrote:
> Isn't (enable_basic_printing==0 and enable_print_preview==0) on IOS?

Done.

[sgurun-gerrit only, 2015-01-16 19:17:44]

On 2015/01/16 10:26:43, dgn wrote:
> > should_ask_print_settings does not exist in android_webview duplicate, so I
> think
> > it was added later.
> 
> Yes, I just added it in this patch. Could you please review it?
> 
> https://codereview.chromium.org/822133003/diff/1/components/components.gyp
> File components/components.gyp (right):
> 
>
https://codereview.chromium.org/822133003/diff/1/components/components.gyp#ne...
> components/components.gyp:142: ['OS != "ios" and (enable_basic_printing==1 or
> enable_print_preview==1)', {
> On 2015/01/08 22:54:27, Vitaly Buka wrote:
> > Isn't (enable_basic_printing==0 and enable_print_preview==0) on IOS?
> 
> Done.

Great, I will look at it. Please add a bug number, I believe it is  311308,
which is related to moving printing out of chrome.

Look at this. we are deleting 3256 lines!!!!

[Vitaly Buka (NO REVIEWS), 2015-01-16 20:41:51]

lgtm

Thanks.

[sgurun-gerrit only, 2015-01-16 21:21:51]

On 2015/01/16 20:41:51, Vitaly Buka wrote:
> lgtm
> 
> Thanks.

Thanks for doing this. It looks pretty good. Have you tested printing with
android webview after this CL is applied? 

Just heads up: Once you address the issues, I will probably build a webview with
your changes and do some testing before lgtm'in it. There seems to be some
reasonably large amount of changes (diff shows 737 changes in
print_web_view_helper alone) in the components vs android_webview copies that I
am afraid we can easily miss an issue and continue building up on it. so won't
be rushing to land :)

[sgurun-gerrit only, 2015-01-16 21:22:05]

https://codereview.chromium.org/822133003/diff/40001/android_webview/DEPS
File android_webview/DEPS (right):

https://codereview.chromium.org/822133003/diff/40001/android_webview/DEPS#new...
android_webview/DEPS:11: "+components/printing/common",
is there a reason we cannot move the DEPS change to android_webview/common/DEPS?

https://codereview.chromium.org/822133003/diff/40001/android_webview/common/a...
File android_webview/common/android_webview_message_generator.h (right):

https://codereview.chromium.org/822133003/diff/40001/android_webview/common/a...
android_webview/common/android_webview_message_generator.h:8: #include
"components/printing/common/print_messages.h"
this seems weird. I think just like autofill or pdf, you should create message
generators in components, next to the messages.h

https://code.google.com/p/chromium/codesearch#chromium/src/components/pdf/com...
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/aw_content_renderer_client.cc:151: false /*
out_of_process_pdf_enabled */,
it is nice that you commented each bool parameter!

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/aw_content_renderer_client.cc:155: new
AwPrintWebViewHelperDelegate()));
who should own AwPrintWebViewHelperDelegate() and what is the lifetime?  
If it is going to be PrintWebViewHelperDelegate(), you need to pass it using
.Pass
see the notes in scoped_ptr header:
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
File android_webview/renderer/aw_print_web_view_helper_delegate.cc (right):

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/aw_print_web_view_helper_delegate.cc:9: #include
"third_party/WebKit/public/web/WebLocalFrame.h"
I don't think you need to include this, just a forward class would be
sufficient?

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/aw_print_web_view_helper_delegate.cc:10: 
scope to android_webview namespace

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
File android_webview/renderer/aw_print_web_view_helper_delegate.h (left):

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/aw_print_web_view_helper_delegate.h:9: 
this class is not scoped, is it not needed?

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
File android_webview/renderer/aw_print_web_view_helper_delegate.h (right):

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/aw_print_web_view_helper_delegate.h:9: 
scope to android_webview namespace

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
File android_webview/renderer/print_web_view_helper.cc (left):

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/print_web_view_helper.cc:6: 
Himm, it seems there are 737 differences between this and
components/printing/renderer/print_web_view_helper.cc. I am a little bit scared.

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/print_web_view_helper.cc:757:
SetScriptedPrintBlocked(true);
In webview, we do not allow window.print yet. As far as I can see, it is going
to be enabled after this change. If it is true, we have to do either of:
1. Verify that it works with webview, 
2. Provide a mechanism to disable it for webview, and defer it to a later CL to
enable.
To limit the behavioral changes in this CL, I am opting for 2, but let me know
if you have already tested for it.

https://codereview.chromium.org/822133003/diff/40001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/822133003/diff/40001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:514: new
ChromePrintWebViewHelperDelegate()));
don't you need to use Pass here?

[sgurun-gerrit only, 2015-01-16 21:22:55]

On 2015/01/16 21:22:05, sgurun wrote:
> https://codereview.chromium.org/822133003/diff/40001/android_webview/DEPS
> File android_webview/DEPS (right):
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/DEPS#new...
> android_webview/DEPS:11: "+components/printing/common",
> is there a reason we cannot move the DEPS change to
android_webview/common/DEPS?
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/common/a...
> File android_webview/common/android_webview_message_generator.h (right):
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/common/a...
> android_webview/common/android_webview_message_generator.h:8: #include
> "components/printing/common/print_messages.h"
> this seems weird. I think just like autofill or pdf, you should create message
> generators in components, next to the messages.h
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/pdf/com...
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> File android_webview/renderer/aw_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> android_webview/renderer/aw_content_renderer_client.cc:151: false /*
> out_of_process_pdf_enabled */,
> it is nice that you commented each bool parameter!
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> android_webview/renderer/aw_content_renderer_client.cc:155: new
> AwPrintWebViewHelperDelegate()));
> who should own AwPrintWebViewHelperDelegate() and what is the lifetime?  
> If it is going to be PrintWebViewHelperDelegate(), you need to pass it using
> .Pass
> see the notes in scoped_ptr header:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> File android_webview/renderer/aw_print_web_view_helper_delegate.cc (right):
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> android_webview/renderer/aw_print_web_view_helper_delegate.cc:9: #include
> "third_party/WebKit/public/web/WebLocalFrame.h"
> I don't think you need to include this, just a forward class would be
> sufficient?
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> android_webview/renderer/aw_print_web_view_helper_delegate.cc:10: 
> scope to android_webview namespace
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> File android_webview/renderer/aw_print_web_view_helper_delegate.h (left):
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> android_webview/renderer/aw_print_web_view_helper_delegate.h:9: 
> this class is not scoped, is it not needed?
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> File android_webview/renderer/aw_print_web_view_helper_delegate.h (right):
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> android_webview/renderer/aw_print_web_view_helper_delegate.h:9: 
> scope to android_webview namespace
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> File android_webview/renderer/print_web_view_helper.cc (left):
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> android_webview/renderer/print_web_view_helper.cc:6: 
> Himm, it seems there are 737 differences between this and
> components/printing/renderer/print_web_view_helper.cc. I am a little bit
scared.
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> android_webview/renderer/print_web_view_helper.cc:757:
> SetScriptedPrintBlocked(true);
> In webview, we do not allow window.print yet. As far as I can see, it is going
> to be enabled after this change. If it is true, we have to do either of:
> 1. Verify that it works with webview, 
> 2. Provide a mechanism to disable it for webview, and defer it to a later CL
to
> enable.
> To limit the behavioral changes in this CL, I am opting for 2, but let me know
> if you have already tested for it.
> 
>
https://codereview.chromium.org/822133003/diff/40001/chrome/renderer/chrome_c...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/822133003/diff/40001/chrome/renderer/chrome_c...
> chrome/renderer/chrome_content_renderer_client.cc:514: new
> ChromePrintWebViewHelperDelegate()));
> don't you need to use Pass here?

ah it picked my lgtm_comment. For now I am putting a not Lgtm. it does not
really mean it though.

[Bernhard Bauer, 2015-01-19 10:05:24]

bauerb@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2015-01-19 10:05:25]

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
File android_webview/renderer/aw_print_web_view_helper_delegate.cc (right):

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/aw_print_web_view_helper_delegate.cc:9: #include
"third_party/WebKit/public/web/WebLocalFrame.h"
On 2015/01/16 21:22:04, sgurun wrote:
> I don't think you need to include this, just a forward class would be
> sufficient?

Not even that is necessary; WebLocalFrame is already used in the header, so it
has to be already forward-declared :)

Same for render_view.h above.

https://codereview.chromium.org/822133003/diff/40001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/822133003/diff/40001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:514: new
ChromePrintWebViewHelperDelegate()));
On 2015/01/16 21:22:04, sgurun wrote:
> don't you need to use Pass here?

Not if the scoped_ptr is directly constructed. You only need .Pass() if someone
already has ownership, like an existing scoped_ptr variable.

[dgn, 2015-01-19 17:48:08]

https://codereview.chromium.org/822133003/diff/40001/android_webview/DEPS
File android_webview/DEPS (right):

https://codereview.chromium.org/822133003/diff/40001/android_webview/DEPS#new...
android_webview/DEPS:11: "+components/printing/common",
On 2015/01/16 at 21:22:04, sgurun wrote:
> is there a reason we cannot move the DEPS change to
android_webview/common/DEPS?

Will do. It will also have to be added to browser's deps

https://codereview.chromium.org/822133003/diff/40001/android_webview/common/a...
File android_webview/common/android_webview_message_generator.h (right):

https://codereview.chromium.org/822133003/diff/40001/android_webview/common/a...
android_webview/common/android_webview_message_generator.h:8: #include
"components/printing/common/print_messages.h"
On 2015/01/16 at 21:22:04, sgurun wrote:
> this seems weird. I think just like autofill or pdf, you should create message
generators in components, next to the messages.h
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/pdf/com...
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

Uploaded https://codereview.chromium.org/857053002

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/aw_content_renderer_client.cc:155: new
AwPrintWebViewHelperDelegate()));
On 2015/01/16 at 21:22:04, sgurun wrote:
> who should own AwPrintWebViewHelperDelegate() and what is the lifetime?  
> If it is going to be PrintWebViewHelperDelegate(), you need to pass it using
.Pass
> see the notes in scoped_ptr header:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

The one linked below says it's not needed on temporaries

http://www.chromium.org/developers/smart-pointer-guidelines

[sgurun-gerrit only, 2015-01-19 18:21:35]

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/aw_content_renderer_client.cc:155: new
AwPrintWebViewHelperDelegate()));
On 2015/01/19 17:48:08, dgn wrote:
> On 2015/01/16 at 21:22:04, sgurun wrote:
> > who should own AwPrintWebViewHelperDelegate() and what is the lifetime?  
> > If it is going to be PrintWebViewHelperDelegate(), you need to pass it using
> .Pass
> > see the notes in scoped_ptr header:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...
> 
> The one linked below says it's not needed on temporaries
> 
> http://www.chromium.org/developers/smart-pointer-guidelines

yep, scratch this.

https://codereview.chromium.org/822133003/diff/40001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/822133003/diff/40001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:514: new
ChromePrintWebViewHelperDelegate()));
On 2015/01/19 10:05:25, Bernhard Bauer wrote:
> On 2015/01/16 21:22:04, sgurun wrote:
> > don't you need to use Pass here?
> 
> Not if the scoped_ptr is directly constructed. You only need .Pass() if
someone
> already has ownership, like an existing scoped_ptr variable.

You are right, my bad.

[dgn, 2015-01-19 18:27:09]

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
File android_webview/renderer/print_web_view_helper.cc (left):

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/print_web_view_helper.cc:6: 
On 2015/01/16 21:22:03, sgurun wrote:
> Himm, it seems there are 737 differences between this and
> components/printing/renderer/print_web_view_helper.cc. I am a little bit
scared.

A good part of the changes are style-related or to parts of the code that
webview doesn't use.
It prints in GMail. I'm not sure how to proceed to test more in detail though.

[sgurun-gerrit only, 2015-01-21 00:40:23]

On 2015/01/19 18:27:09, dgn wrote:
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> File android_webview/renderer/print_web_view_helper.cc (left):
> 
>
https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
> android_webview/renderer/print_web_view_helper.cc:6: 
> On 2015/01/16 21:22:03, sgurun wrote:
> > Himm, it seems there are 737 differences between this and
> > components/printing/renderer/print_web_view_helper.cc. I am a little bit
> scared.
> 
> A good part of the changes are style-related or to parts of the code that
> webview doesn't use.
> It prints in GMail. I'm not sure how to proceed to test more in detail though.

I played with it as much as I can today, and it seems to work ok. Thanks,

[dgn, 2015-01-21 18:19:23]

dgn@chromium.org changed reviewers:
+ blundell@chromium.org, thestig@chromium.org

[dgn, 2015-01-21 18:19:24]

sgurun@: does that mean lgtm?

thestig@: Please review changes in
chrome/renderer/chrome_content_renderer_client.cc

blundell@: Please review changes in components/components.gyp

[sgurun-gerrit only, 2015-01-21 18:25:56]

On 2015/01/21 18:19:24, dgn wrote:
> sgurun@: does that mean lgtm?
> 
> thestig@: Please review changes in
> chrome/renderer/chrome_content_renderer_client.cc
> 
> blundell@: Please review changes in components/components.gyp

did you address all the comments except the ones about scoped_ptr?

[chromium-reviews, 2015-01-21 18:31:03]

/facepalm
I forgot to upload the patch.
Also,  https://codereview.chromium.org/857053002 has to land first, my bad.
I'll ping when all is ready.

On Wed Jan 21 2015 at 6:25:58 PM <sgurun@chromium.org> wrote:

> On 2015/01/21 18:19:24, dgn wrote:
> > sgurun@: does that mean lgtm?
>
> > thestig@: Please review changes in
> > chrome/renderer/chrome_content_renderer_client.cc
>
> > blundell@: Please review changes in components/components.gyp
>
> did you address all the comments except the ones about scoped_ptr?
>
> https://codereview.chromium.org/822133003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dgn, 2015-01-23 15:21:09]

https://codereview.chromium.org/822133003/diff/40001/android_webview/DEPS
File android_webview/DEPS (right):

https://codereview.chromium.org/822133003/diff/40001/android_webview/DEPS#new...
android_webview/DEPS:11: "+components/printing/common",
On 2015/01/19 17:48:08, dgn wrote:
> On 2015/01/16 at 21:22:04, sgurun wrote:
> > is there a reason we cannot move the DEPS change to
> android_webview/common/DEPS?
> 
> Will do. It will also have to be added to browser's deps

Done.

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
File android_webview/renderer/aw_print_web_view_helper_delegate.cc (right):

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/aw_print_web_view_helper_delegate.cc:9: #include
"third_party/WebKit/public/web/WebLocalFrame.h"
On 2015/01/19 10:05:25, Bernhard Bauer wrote:
> On 2015/01/16 21:22:04, sgurun wrote:
> > I don't think you need to include this, just a forward class would be
> > sufficient?
> 
> Not even that is necessary; WebLocalFrame is already used in the header, so it
> has to be already forward-declared :)
> 
> Same for render_view.h above.

Done.

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/aw_print_web_view_helper_delegate.cc:10: 
On 2015/01/16 21:22:04, sgurun wrote:
> scope to android_webview namespace

Done.

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
File android_webview/renderer/aw_print_web_view_helper_delegate.h (right):

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/aw_print_web_view_helper_delegate.h:9: 
On 2015/01/16 21:22:04, sgurun wrote:
> scope to android_webview namespace

Done.

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
File android_webview/renderer/print_web_view_helper.cc (left):

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/print_web_view_helper.cc:757:
SetScriptedPrintBlocked(true);
On 2015/01/16 21:22:04, sgurun wrote:
> In webview, we do not allow window.print yet. As far as I can see, it is going
> to be enabled after this change. If it is true, we have to do either of:
> 1. Verify that it works with webview, 
> 2. Provide a mechanism to disable it for webview, and defer it to a later CL
to
> enable.
> To limit the behavioral changes in this CL, I am opting for 2, but let me know
> if you have already tested for it.

Did option 2). See components/printing/renderer/print_web_view_helper.cc in the
new patch.

https://codereview.chromium.org/822133003/diff/40001/android_webview/renderer...
android_webview/renderer/print_web_view_helper.cc:765: return false;
This is where scripted print was blocked in android. Replaced the #ifdef with a
call to the delegate

https://codereview.chromium.org/822133003/diff/60001/components/printing/rend...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/822133003/diff/60001/components/printing/rend...
components/printing/renderer/print_web_view_helper.cc:807: if
(!delegate_->IsScriptedPrintEnabled())
This is where window.print() was previously disabled (but with #ifdef). Now this
will allow to disabled print only for webview in the same way.

[dgn, 2015-01-23 16:49:55]

https://codereview.chromium.org/822133003/diff/60001/android_webview/common/a...
File android_webview/common/android_webview_message_generator.h (right):

https://codereview.chromium.org/822133003/diff/60001/android_webview/common/a...
android_webview/common/android_webview_message_generator.h:8: #include
"components/printing/common/print_messages.h"
Once https://codereview.chromium.org/857053002/
lands, I'll be able to remove this line

[sgurun-gerrit only, 2015-01-23 22:10:38]

On 2015/01/23 16:49:55, dgn wrote:
>
https://codereview.chromium.org/822133003/diff/60001/android_webview/common/a...
> File android_webview/common/android_webview_message_generator.h (right):
> 
>
https://codereview.chromium.org/822133003/diff/60001/android_webview/common/a...
> android_webview/common/android_webview_message_generator.h:8: #include
> "components/printing/common/print_messages.h"
> Once https://codereview.chromium.org/857053002/
> lands, I'll be able to remove this line

lgtm

[dgn, 2015-01-27 14:29:06]

dgn@chromium.org changed reviewers:
+ dcheng@chromium.org
- thestig@chromium.org

[dgn, 2015-01-27 14:29:08]

+dcheng@: Please review android_webview/common/print_messages.h removal. Also
probably relevant, changes in
android_webview/common/android_webview_message_generator.h

+blundell@:Please review changes in components/components.gyp

https://codereview.chromium.org/822133003/diff/60001/android_webview/common/a...
File android_webview/common/android_webview_message_generator.h (right):

https://codereview.chromium.org/822133003/diff/60001/android_webview/common/a...
android_webview/common/android_webview_message_generator.h:8: #include
"components/printing/common/print_messages.h"
On 2015/01/23 16:49:55, dgn wrote:
> Once https://codereview.chromium.org/857053002/
> lands, I'll be able to remove this line

Removed, but CL 857053002 hasn't landed yet.

[dcheng, 2015-01-27 18:22:38]

android_web/view/common/*message* changes LGTM

[dgn, 2015-01-30 16:40:51]

Rebased on master. It can now be merged.

Some changes were made to android webview's print_web_view_helper, but were also
made in the other print_web_view_helper, so the rebase should not change the
behavior:

https://codereview.chromium.org/805903005
https://codereview.chromium.org/867853002
https://codereview.chromium.org/797813006

[Vitaly Buka (NO REVIEWS), 2015-01-30 18:42:30]

lgtm

[dgn, 2015-02-02 17:02:45]

blundell@: can you please review the changes to components/components.gyp

[blundell, 2015-02-02 19:56:01]

rubberstamp LGTM

[dgn, 2015-02-03 09:54:32]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2015-02-03 09:54:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/822133003/100001

[commit-bot: I haz the power, 2015-02-03 10:39:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-03 10:40:07]

Patchset 6 (id:??) landed as
https://crrev.com/2fa98a5958ad5ecb86484acbeed3a05326647d4a
Cr-Commit-Position: refs/heads/master@{#314307}