Enable page selection for WebView printing

Enable page selection for WebView printing

Pass pages param from Android to native code.
Add ScriptedPrint IPC message to AwPrintManager.

BUG=705099
Review-Url: https://codereview.chromium.org/2770713003
Cr-Commit-Position: refs/heads/master@{#459567}
Committed: https://chromium.googlesource.com/chromium/src/+/d20a4bb404d43cdafce20ad592b3e3448860da51

[Shimi Zhang, 2017-03-22 20:15:22]

ctzsm@chromium.org changed reviewers:
+ thestig@chromium.org, vitalybuka@chromium.org

[Shimi Zhang, 2017-03-22 20:15:22]

PTAL

[Shimi Zhang, 2017-03-22 20:15:58]

The CQ bit was checked by ctzsm@chromium.org to run a CQ dry run

[Shimi Zhang, 2017-03-22 20:16:00]

The CQ bit was unchecked by ctzsm@chromium.org

[Shimi Zhang, 2017-03-22 21:40:39]

The CQ bit was checked by ctzsm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 21:41:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Shimi Zhang, 2017-03-22 21:44:27]

Description was changed from

==========
Enable page selection for webview printing

pass pages param from Android to native code.
add ScriptedPrint IPC message to AwPrintManger.

BUG=

TBR=sgurun
==========

to

==========
Enable page selection for webview printing

pass pages param from Android to native code.
add ScriptedPrint IPC message to AwPrintManger.

BUG=
==========

[Shimi Zhang, 2017-03-22 22:04:19]

On 2017/03/22 20:15:22, Shimi Zhang wrote:
> PTAL

sgurun@, please review. FYI: thestig@, vitalybuka@.

[Lei Zhang, 2017-03-22 22:26:44]

https://codereview.chromium.org/2770713003/diff/20001/android_webview/browser...
File android_webview/browser/aw_print_manager.cc (right):

https://codereview.chromium.org/2770713003/diff/20001/android_webview/browser...
android_webview/browser/aw_print_manager.cc:84:
PrintHostMsg_UpdatePrintSettings::WriteReplyParams(reply_msg, params, false);
Shouldn't this be PrintHostMsg_ScriptedPrint::WriteReplyParams() ?

https://codereview.chromium.org/2770713003/diff/20001/android_webview/native/...
File android_webview/native/aw_pdf_exporter.cc (right):

https://codereview.chromium.org/2770713003/diff/20001/android_webview/native/...
android_webview/native/aw_pdf_exporter.cc:36: }  // anonymous namespace
no "anonymous " - bad copy+paste from line 81?

https://codereview.chromium.org/2770713003/diff/20001/android_webview/native/...
android_webview/native/aw_pdf_exporter.cc:99: if (page_ranges.size() > 0)
!page_range.empty()

https://codereview.chromium.org/2770713003/diff/20001/android_webview/native/...
File android_webview/native/aw_pdf_exporter.h (right):

https://codereview.chromium.org/2770713003/diff/20001/android_webview/native/...
android_webview/native/aw_pdf_exporter.h:43: const printing::PageRanges&
page_ranges,
You can forward declare instead.

[commit-bot: I haz the power, 2017-03-22 22:30:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 22:30:03]

Dry run: This issue passed the CQ dry run.

[Shimi Zhang, 2017-03-23 17:59:07]

https://codereview.chromium.org/2770713003/diff/20001/android_webview/native/...
File android_webview/native/aw_pdf_exporter.h (right):

https://codereview.chromium.org/2770713003/diff/20001/android_webview/native/...
android_webview/native/aw_pdf_exporter.h:43: const printing::PageRanges&
page_ranges,
On 2017/03/22 at 22:26:43, Lei Zhang wrote:
> You can forward declare instead.

thestig@, this is a typedef, which I think we can't forward declare, the
declaration is:

typedef std::vector<PageRange> PageRanges; 

in printing/page_range.h. Correct me if I am wrong.

With this assumption if we want to remove #include "printing/page_range.h" here,
I came up with one solution:

Since I converted jintArray to vector<int> and then convert vector<int> to
PageRanges outside InitPdfSettings function, we could move the second part into
InitPdfSettings and replace this param with a vector<int>, we could also remove
GetPageRanges function in aw_pdf_exporter.cc.

How do you think?

[Vitaly Buka (NO REVIEWS), 2017-03-23 18:05:40]

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

[sgurun-gerrit only, 2017-03-23 19:16:59]

let's also think about how to write a test. We have a CTS test (code is at the
Android side) but adding one or two tests Chromium side is desireable.

https://codereview.chromium.org/2770713003/diff/20001/android_webview/rendere...
File android_webview/renderer/aw_print_web_view_helper_delegate.cc (right):

https://codereview.chromium.org/2770713003/diff/20001/android_webview/rendere...
android_webview/renderer/aw_print_web_view_helper_delegate.cc:28: return true;
two comments:

I think this is to enable browser asking print settings from user, or at least
that was the original intent in desktop, which does not apply to WebView. Can
you update the comment about showing UI in
https://cs.chromium.org/chromium/src/components/printing/renderer/print_web_v...

also the base class has  default implementation which  true, so you can now
remove this method altogether.

[Lei Zhang, 2017-03-23 19:35:18]

https://codereview.chromium.org/2770713003/diff/20001/android_webview/native/...
File android_webview/native/aw_pdf_exporter.h (right):

https://codereview.chromium.org/2770713003/diff/20001/android_webview/native/...
android_webview/native/aw_pdf_exporter.h:43: const printing::PageRanges&
page_ranges,
On 2017/03/23 17:59:07, Shimi Zhang wrote:
> On 2017/03/22 at 22:26:43, Lei Zhang wrote:
> > You can forward declare instead.
> 
> thestig@, this is a typedef, which I think we can't forward declare, the
> declaration is:
> 
> typedef std::vector<PageRange> PageRanges; 

OK, I didn't check. Ignore my comment then. No need to over-think it.

https://codereview.chromium.org/2770713003/diff/20001/android_webview/rendere...
File android_webview/renderer/aw_print_web_view_helper_delegate.cc (right):

https://codereview.chromium.org/2770713003/diff/20001/android_webview/rendere...
android_webview/renderer/aw_print_web_view_helper_delegate.cc:28: return true;
On 2017/03/23 19:16:59, sgurun wrote:
> two comments:
> 
> I think this is to enable browser asking print settings from user, or at least
> that was the original intent in desktop, which does not apply to WebView. Can
> you update the comment about showing UI in
>
https://cs.chromium.org/chromium/src/components/printing/renderer/print_web_v...

Yes, that was the original intent, but this is fine. The browser side code can
handle the "ask for settings" task however it likes. Better comments are
welcome.

[sgurun-gerrit only, 2017-03-23 19:38:18]

https://codereview.chromium.org/2770713003/diff/20001/android_webview/rendere...
File android_webview/renderer/aw_print_web_view_helper_delegate.cc (right):

https://codereview.chromium.org/2770713003/diff/20001/android_webview/rendere...
android_webview/renderer/aw_print_web_view_helper_delegate.cc:28: return true;
On 2017/03/23 19:35:18, Lei Zhang wrote:
> On 2017/03/23 19:16:59, sgurun wrote:
> > two comments:
> > 
> > I think this is to enable browser asking print settings from user, or at
least
> > that was the original intent in desktop, which does not apply to WebView.
Can
> > you update the comment about showing UI in
> >
>
https://cs.chromium.org/chromium/src/components/printing/renderer/print_web_v...
> 
> Yes, that was the original intent, but this is fine. The browser side code can
> handle the "ask for settings" task however it likes. Better comments are
> welcome.

alright then, no worries about updating comment.

[sgurun-gerrit only, 2017-03-23 21:07:09]

For testing in chromium side, a problem is that Android frameworks classes do
not allow easily creating test classes. Forexample, LayoutResultCallback is an
abstract class with a hidden constructor. So we implemented the tests in Android
side as CTS tests. 

Let's add a CTS test for this (in a separate Android CL). Let's also file an
Android bug to make the constructor public.

[Shimi Zhang, 2017-03-23 23:21:56]

https://codereview.chromium.org/2770713003/diff/20001/android_webview/browser...
File android_webview/browser/aw_print_manager.cc (right):

https://codereview.chromium.org/2770713003/diff/20001/android_webview/browser...
android_webview/browser/aw_print_manager.cc:84:
PrintHostMsg_UpdatePrintSettings::WriteReplyParams(reply_msg, params, false);
On 2017/03/22 22:26:43, Lei Zhang wrote:
> Shouldn't this be PrintHostMsg_ScriptedPrint::WriteReplyParams() ?

Done.

https://codereview.chromium.org/2770713003/diff/20001/android_webview/native/...
File android_webview/native/aw_pdf_exporter.cc (right):

https://codereview.chromium.org/2770713003/diff/20001/android_webview/native/...
android_webview/native/aw_pdf_exporter.cc:36: }  // anonymous namespace
On 2017/03/22 22:26:43, Lei Zhang wrote:
> no "anonymous " - bad copy+paste from line 81?

Done.

https://codereview.chromium.org/2770713003/diff/20001/android_webview/native/...
android_webview/native/aw_pdf_exporter.cc:99: if (page_ranges.size() > 0)
On 2017/03/22 22:26:43, Lei Zhang wrote:
> !page_range.empty()

Done.

https://codereview.chromium.org/2770713003/diff/20001/android_webview/native/...
File android_webview/native/aw_pdf_exporter.h (right):

https://codereview.chromium.org/2770713003/diff/20001/android_webview/native/...
android_webview/native/aw_pdf_exporter.h:43: const printing::PageRanges&
page_ranges,
On 2017/03/22 22:26:43, Lei Zhang wrote:
> You can forward declare instead.

Done.

https://codereview.chromium.org/2770713003/diff/20001/android_webview/rendere...
File android_webview/renderer/aw_print_web_view_helper_delegate.cc (right):

https://codereview.chromium.org/2770713003/diff/20001/android_webview/rendere...
android_webview/renderer/aw_print_web_view_helper_delegate.cc:28: return true;
On 2017/03/23 19:16:59, sgurun wrote:
> two comments:
> 
> I think this is to enable browser asking print settings from user, or at least
> that was the original intent in desktop, which does not apply to WebView. Can
> you update the comment about showing UI in
>
https://cs.chromium.org/chromium/src/components/printing/renderer/print_web_v...
> 
> also the base class has  default implementation which  true, so you can now
> remove this method altogether.

Done.

[Lei Zhang, 2017-03-24 01:16:32]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 01:16:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2017-03-24 01:17:31]

lgtm

[commit-bot: I haz the power, 2017-03-24 01:39:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 01:39:57]

Dry run: This issue passed the CQ dry run.

[sgurun-gerrit only, 2017-03-24 16:57:31]

On 2017/03/24 01:17:31, Lei Zhang wrote:
> lgtm

lgtm, please add internal bug number to description as "internal bug BUGNO" and
also correctly capitalize the description. 
thanks

[Shimi Zhang, 2017-03-24 19:21:03]

Description was changed from

==========
Enable page selection for webview printing

pass pages param from Android to native code.
add ScriptedPrint IPC message to AwPrintManger.

BUG=
==========

to

==========
Enable page selection for webview printing

Pass pages param from Android to native code.
Add ScriptedPrint IPC message to AwPrintManger.

BUG=internal bug b/11694304
==========

[Shimi Zhang, 2017-03-24 21:18:05]

The CQ bit was checked by ctzsm@chromium.org

[commit-bot: I haz the power, 2017-03-24 21:18:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Shimi Zhang, 2017-03-24 21:23:21]

The CQ bit was unchecked by ctzsm@chromium.org

[Shimi Zhang, 2017-03-24 21:28:58]

Description was changed from

==========
Enable page selection for webview printing

Pass pages param from Android to native code.
Add ScriptedPrint IPC message to AwPrintManger.

BUG=internal bug b/11694304
==========

to

==========
Enable page selection for webview printing

Pass pages param from Android to native code.
Add ScriptedPrint IPC message to AwPrintManger.

BUG=705099
==========

[Shimi Zhang, 2017-03-24 21:31:45]

On 2017/03/24 16:57:31, sgurun wrote:
> On 2017/03/24 01:17:31, Lei Zhang wrote:
> > lgtm
> 
> lgtm, please add internal bug number to description as "internal bug BUGNO"
and
> also correctly capitalize the description. 
> thanks

Created a crbug referring that internal bug and cited crbug here.

[Shimi Zhang, 2017-03-24 21:42:07]

Description was changed from

==========
Enable page selection for webview printing

Pass pages param from Android to native code.
Add ScriptedPrint IPC message to AwPrintManger.

BUG=705099
==========

to

==========
Enable page selection for WebView printing

Pass pages param from Android to native code.
Add ScriptedPrint IPC message to AwPrintManger.

BUG=705099
==========

[Shimi Zhang, 2017-03-24 21:43:12]

Description was changed from

==========
Enable page selection for WebView printing

Pass pages param from Android to native code.
Add ScriptedPrint IPC message to AwPrintManger.

BUG=705099
==========

to

==========
Enable page selection for WebView printing

Pass pages param from Android to native code.
Add ScriptedPrint IPC message to AwPrintManager.

BUG=705099
==========

[Shimi Zhang, 2017-03-24 21:43:38]

The CQ bit was checked by ctzsm@chromium.org

[commit-bot: I haz the power, 2017-03-24 21:44:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 21:52:22]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490391818498310,
"parent_rev": "8a29ed05232f634a4ba33d1930a5c3ef8aaee0f5", "commit_rev":
"d20a4bb404d43cdafce20ad592b3e3448860da51"}

[commit-bot: I haz the power, 2017-03-24 21:53:09]

Description was changed from

==========
Enable page selection for WebView printing

Pass pages param from Android to native code.
Add ScriptedPrint IPC message to AwPrintManager.

BUG=705099
==========

to

==========
Enable page selection for WebView printing

Pass pages param from Android to native code.
Add ScriptedPrint IPC message to AwPrintManager.

BUG=705099

Review-Url: https://codereview.chromium.org/2770713003
Cr-Commit-Position: refs/heads/master@{#459567}
Committed:
https://chromium.googlesource.com/chromium/src/+/d20a4bb404d43cdafce20ad592b3...
==========

[commit-bot: I haz the power, 2017-03-24 21:53:10]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d20a4bb404d43cdafce20ad592b3...