Mark printing code as basic printing and/or print preview code.

Mark printing code as basic printing and/or print preview code.

Committed: https://crrev.com/9b14c590e296463faefd60cee87b31469d2e3b38
Cr-Commit-Position: refs/heads/master@{#369502}

[Lei Zhang, 2015-12-29 19:26:26]

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

[Lei Zhang, 2015-12-29 19:26:26]

I know this is a lot of ifdefs, but it's a rough first pass at marking what's
what in PrintWebViewHelper.

[Vitaly Buka (NO REVIEWS), 2016-01-04 21:36:49]

https://codereview.chromium.org/1556463003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/1556463003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1006: #if
defined(ENABLE_BASIC_PRINTING)
As I remember basic printing is system dialog printing.
I made some efforts to enable (enable_basic_printing==0 and
enable_print_preview==1) builds.

Idea was to remove system dialog at all. 
But having that mobile platforms use that code, it's not going to happen.

So are you removing (enable_basic_printing==0 and enable_print_preview==1) case
intentionally?

[Lei Zhang, 2016-01-07 03:01:09]

https://codereview.chromium.org/1556463003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/1556463003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1006: #if
defined(ENABLE_BASIC_PRINTING)
On 2016/01/04 21:36:49, Vitaly Buka wrote:
> As I remember basic printing is system dialog printing.
> I made some efforts to enable (enable_basic_printing==0 and
> enable_print_preview==1) builds.
> 
> Idea was to remove system dialog at all. 
> But having that mobile platforms use that code, it's not going to happen.
> 
> So are you removing (enable_basic_printing==0 and enable_print_preview==1)
case
> intentionally?

We have:
no printing - embedded platforms?
enable_basic_printing only - Android
enable_print_preview only - ChromeOS
full printing - desktop

I'm getting rid of the defined(OS_CHROME) checks in this CL. In particular:

PrintWebViewHelper::PrintPagesNative() has a ChromeOS NOTREACHED(), and if you
follow the call stack for that, we should never get here on ChromeOS either.
ChromeOS should always have |is_cloud_printer| set to true. But rather than
marking this block as !OS_CHROME, I'm using the ENABLE_BASIC_PRINTING feature
flag.

[Vitaly Buka (NO REVIEWS), 2016-01-07 04:19:35]

https://codereview.chromium.org/1556463003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/1556463003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1006: #if
defined(ENABLE_BASIC_PRINTING)
On 2016/01/07 03:01:09, Lei Zhang wrote:
> On 2016/01/04 21:36:49, Vitaly Buka wrote:
> > As I remember basic printing is system dialog printing.
> > I made some efforts to enable (enable_basic_printing==0 and
> > enable_print_preview==1) builds.
> > 
> > Idea was to remove system dialog at all. 
> > But having that mobile platforms use that code, it's not going to happen.
> > 
> > So are you removing (enable_basic_printing==0 and enable_print_preview==1)
> case
> > intentionally?
> 
> We have:
> no printing - embedded platforms?
> enable_basic_printing only - Android
> enable_print_preview only - ChromeOS
> full printing - desktop

It was possible to build on Windows (don't remember, but maybe on other desktops
too) with ENABLE_BASIC_PRINTING and with no PRINT_PREVIEW, using gyp defines.

This is not enabled on any build bots, so that config is probably degraded.
There was attempt to get closer to removing ENABLE_BASIC_PRINTING from desktops
at all and keep print preview only.

[Vitaly Buka (NO REVIEWS), 2016-01-07 04:21:14]

https://codereview.chromium.org/1556463003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/1556463003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1006: #if
defined(ENABLE_BASIC_PRINTING)
Sorry, I tried to say:
it was possible to build "without ENABLE_BASIC_PRINTING and with PRINT_PREVIEW"

[Lei Zhang, 2016-01-07 04:47:50]

https://codereview.chromium.org/1556463003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/1556463003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1006: #if
defined(ENABLE_BASIC_PRINTING)
On 2016/01/07 04:21:14, Vitaly Buka wrote:
> Sorry, I tried to say:
> it was possible to build "without ENABLE_BASIC_PRINTING and with
PRINT_PREVIEW"

With this CL, all the configurations are buildable except for "no printing".
There's no bots to test, and I'm fine with that. I just want to have better
separation, so if we shuffle the code around in the future, it's easier to
identify what goes where.

[Vitaly Buka (NO REVIEWS), 2016-01-07 23:50:23]

lgtm

https://codereview.chromium.org/1556463003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/1556463003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1006: #if
defined(ENABLE_BASIC_PRINTING)
Then I am fine with that too :-)

[Lei Zhang, 2016-01-08 05:06:04]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-01-08 05:06:12]

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

[commit-bot: I haz the power, 2016-01-08 05:14:41]

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

[commit-bot: I haz the power, 2016-01-08 05:14:41]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Lei Zhang, 2016-01-08 05:30:37]

thestig@chromium.org changed reviewers:
+ thakis@chromium.org, tsepez@chromium.org

[Lei Zhang, 2016-01-08 05:30:38]

+thakis for build/
+tsepez for shuffling/ifdefing in IPC messages.

[Nico, 2016-01-08 15:10:22]

before adding all these ifdefs, should ENABLE_BASIC_PRINTING be moved to the new
feature flag system?

build/ lgtm tho

[Tom Sepez, 2016-01-08 17:29:42]

messages LGTM

[Lei Zhang, 2016-01-13 02:50:28]

On 2016/01/08 15:10:22, Nico wrote:
> before adding all these ifdefs, should ENABLE_BASIC_PRINTING be moved to the
new
> feature flag system?

So far only more isolated features have been converted to the new feature flag
system. I don't want to be the one leading the charge on converting more global
features to the new system. There's already 150+ printing ifdefs. I don't think
this CL will make the conversion that more painful since it's mostly in files
that already have printing ifdefs. Hopefully we can refactor some code into
their own files to reduce ifdefs too.

[Nico, 2016-01-13 20:54:44]

okie, lgtm

[Lei Zhang, 2016-01-14 02:29:27]

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

[Lei Zhang, 2016-01-14 02:29:27]

+asvitkine for obsolete histogram

[Alexei Svitkine (slow), 2016-01-14 16:51:17]

lgtm

[Lei Zhang, 2016-01-14 18:20:02]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-01-14 18:20:02]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
thakis@chromium.org, vitalybuka@chromium.org
Link to the patchset: https://codereview.chromium.org/1556463003/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-14 18:21:33]

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

[commit-bot: I haz the power, 2016-01-14 19:30:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-14 19:31:00]

Description was changed from

==========
Mark printing code as basic printing and/or print preview code.
==========

to

==========
Mark printing code as basic printing and/or print preview code.

Committed: https://crrev.com/9b14c590e296463faefd60cee87b31469d2e3b38
Cr-Commit-Position: refs/heads/master@{#369502}
==========

[commit-bot: I haz the power, 2016-01-14 19:31:00]

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