Make the dependency of enable_print_preview and enable_pdf explicit

Make the dependency of enable_print_preview and enable_pdf explicit

Print preview needs pdf module enabled to function. Also, the
utility component depends on pdf module only when print_preview is
enabled implicitly (on Windows only) or explicitly. Make the gn build
file changes to reflect these dependencies.

BUG=609726
Committed: https://crrev.com/5addafe27695c6caf4f6d1b757b4cef54b6b598b
Cr-Commit-Position: refs/heads/master@{#395519}

[Wei Li, 2016-05-18 18:19:32]

weili@chromium.org changed reviewers:
+ thestig@chromium.org

[Wei Li, 2016-05-18 18:19:33]

for review, thanks

[Lei Zhang, 2016-05-18 21:43:37]

https://codereview.chromium.org/1994713002/diff/1/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1994713002/diff/1/build/config/BUILD.gn#newco...
build/config/BUILD.gn:73: assert(!is_win, "set enable_pdf=true on Windows.")
I'd prefer we remove this check, and put all the checks below in the printing
block. Essentially:

- Windows basic printing requires PDF
- Print preview requires PDF

https://codereview.chromium.org/1994713002/diff/1/chrome/utility/BUILD.gn
File chrome/utility/BUILD.gn (right):

https://codereview.chromium.org/1994713002/diff/1/chrome/utility/BUILD.gn#new...
chrome/utility/BUILD.gn:103: if (!enable_print_preview && !is_win) {
Would you mind negating this?

if (enable_print_preview || is_win) {
  deps += [ "//pdf" ]
} else {
  sources -= [
    "printing_handler.cc",
    "printing_handler.h",
  ]
}

https://codereview.chromium.org/1994713002/diff/1/chrome/utility/BUILD.gn#new...
chrome/utility/BUILD.gn:103: if (!enable_print_preview && !is_win) {
Also, "enable_print_preview" used to imply "enable_basic_printing" but later we
changed it so that the two variables are independent of each other. Given that,
this should actually be:

(enable_print_preview || (enable_basic_printing & is_win))

Can we make that change while we are here?
chrome/utility/chrome_content_utility_client.cc and
chrome/utility/printing_handler.h have ifdefs that should be updated as well.

[Wei Li, 2016-05-19 17:12:35]

Patchset #2 (id:20001) has been deleted

[Wei Li, 2016-05-19 17:13:30]

thanks

https://codereview.chromium.org/1994713002/diff/1/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1994713002/diff/1/build/config/BUILD.gn#newco...
build/config/BUILD.gn:73: assert(!is_win, "set enable_pdf=true on Windows.")
On 2016/05/18 21:43:37, Lei Zhang wrote:
> I'd prefer we remove this check, and put all the checks below in the printing
> block. Essentially:
> 
> - Windows basic printing requires PDF
> - Print preview requires PDF

Done.

https://codereview.chromium.org/1994713002/diff/1/chrome/utility/BUILD.gn
File chrome/utility/BUILD.gn (right):

https://codereview.chromium.org/1994713002/diff/1/chrome/utility/BUILD.gn#new...
chrome/utility/BUILD.gn:103: if (!enable_print_preview && !is_win) {
Thanks for the explanation. Done.

https://codereview.chromium.org/1994713002/diff/1/chrome/utility/BUILD.gn#new...
chrome/utility/BUILD.gn:103: if (!enable_print_preview && !is_win) {
On 2016/05/18 21:43:37, Lei Zhang wrote:
> Would you mind negating this?
> 
> if (enable_print_preview || is_win) {
>   deps += [ "//pdf" ]
> } else {
>   sources -= [
>     "printing_handler.cc",
>     "printing_handler.h",
>   ]
> }

Done.

https://codereview.chromium.org/1994713002/diff/1/chrome/utility/BUILD.gn#new...
chrome/utility/BUILD.gn:103: if (!enable_print_preview && !is_win) {
On 2016/05/18 21:43:37, Lei Zhang wrote:
> Also, "enable_print_preview" used to imply "enable_basic_printing" but later
we
> changed it so that the two variables are independent of each other. Given
that,
> this should actually be:
> 
> (enable_print_preview || (enable_basic_printing & is_win))
> 
> Can we make that change while we are here?
> chrome/utility/chrome_content_utility_client.cc and
> chrome/utility/printing_handler.h have ifdefs that should be updated as well.

Thanks for the explanation. Done.

[Lei Zhang, 2016-05-19 17:33:31]

lgtm

[Wei Li, 2016-05-20 01:08:31]

Description was changed from

==========
Make the dependency of enable_print_preview and enable_pdf explicit

Print preview needs pdf module enabled to function. Also, the
utility component depends on pdf module only when print_preview is
enabled implicitly (on Windows only) or explicitly. Make the gn build
file changes to reflect these dependencies.

BUG=609726
==========

to

==========
Make the dependency of enable_print_preview and enable_pdf explicit

Print preview needs pdf module enabled to function. Also, the
utility component depends on pdf module only when print_preview is
enabled implicitly (on Windows only) or explicitly. Make the gn build
file changes to reflect these dependencies.

BUG=609726
==========

[Wei Li, 2016-05-20 01:08:31]

weili@chromium.org changed reviewers:
+ brettw@chromium.org

[Wei Li, 2016-05-20 01:09:20]

Brett, could you pls do a owner review for the gn file change? thanks

[brettw, 2016-05-23 20:28:49]

build/config LGTM

[Wei Li, 2016-05-23 22:26:34]

The CQ bit was checked by weili@chromium.org

[Wei Li, 2016-05-23 22:26:35]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1994713002/#ps60001
(title: "fix cc file")

[commit-bot: I haz the power, 2016-05-23 22:27:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1994713002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1994713002/60001

[commit-bot: I haz the power, 2016-05-24 02:46:33]

Description was changed from

==========
Make the dependency of enable_print_preview and enable_pdf explicit

Print preview needs pdf module enabled to function. Also, the
utility component depends on pdf module only when print_preview is
enabled implicitly (on Windows only) or explicitly. Make the gn build
file changes to reflect these dependencies.

BUG=609726
==========

to

==========
Make the dependency of enable_print_preview and enable_pdf explicit

Print preview needs pdf module enabled to function. Also, the
utility component depends on pdf module only when print_preview is
enabled implicitly (on Windows only) or explicitly. Make the gn build
file changes to reflect these dependencies.

BUG=609726
==========

[commit-bot: I haz the power, 2016-05-24 02:46:33]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-05-24 02:47:57]

Description was changed from

==========
Make the dependency of enable_print_preview and enable_pdf explicit

Print preview needs pdf module enabled to function. Also, the
utility component depends on pdf module only when print_preview is
enabled implicitly (on Windows only) or explicitly. Make the gn build
file changes to reflect these dependencies.

BUG=609726
==========

to

==========
Make the dependency of enable_print_preview and enable_pdf explicit

Print preview needs pdf module enabled to function. Also, the
utility component depends on pdf module only when print_preview is
enabled implicitly (on Windows only) or explicitly. Make the gn build
file changes to reflect these dependencies.

BUG=609726

Committed: https://crrev.com/5addafe27695c6caf4f6d1b757b4cef54b6b598b
Cr-Commit-Position: refs/heads/master@{#395519}
==========

[commit-bot: I haz the power, 2016-05-24 02:47:59]

Patchset 3 (id:??) landed as
https://crrev.com/5addafe27695c6caf4f6d1b757b4cef54b6b598b
Cr-Commit-Position: refs/heads/master@{#395519}