Add ENABLE_PRINTING and disable it on Android.

Add ENABLE_PRINTING and disable it on Android.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141353

[sky, 2012-06-06 20:51:58]

LGTM

https://chromiumcodereview.appspot.com/10540033/diff/1/chrome/browser/browser...
File chrome/browser/browser_about_handler.cc (right):

https://chromiumcodereview.appspot.com/10540033/diff/1/chrome/browser/browser...
chrome/browser/browser_about_handler.cc:49: #if defined(ENABLE_PRINTING)
Move this to to the bottom with the rest of the conditionals.

[Xianzhu, 2012-06-06 20:57:01]

https://chromiumcodereview.appspot.com/10540033/diff/1/chrome/browser/browser...
File chrome/browser/browser_about_handler.cc (right):

https://chromiumcodereview.appspot.com/10540033/diff/1/chrome/browser/browser...
chrome/browser/browser_about_handler.cc:49: #if defined(ENABLE_PRINTING)
On 2012/06/06 20:51:58, sky wrote:
> Move this to to the bottom with the rest of the conditionals.

Done.

[Xianzhu, 2012-06-06 22:03:03]

In the new patch set, also changed some gyp files to exclude printing when
enable_printing!=1.

[Elliot Glaysher, 2012-06-06 22:05:23]

profile lgtm

[brettw, 2012-06-06 22:48:05]

We don't want ifdefs that we plan on keeping around forever. What is the reason
for not compiling this code? Is it space? I don't see a huge amount of code
being removed by this patch. If it's that some things just can't work, we should
have an interface where the Android version does nothing or won't compile rather
than having a mess of ifdefs around.

[Xianzhu, 2012-06-07 01:45:27]

On 2012/06/06 22:48:05, brettw wrote:
> We don't want ifdefs that we plan on keeping around forever. What is the
reason
> for not compiling this code? Is it space? I don't see a huge amount of code
> being removed by this patch. If it's that some things just can't work, we
should
> have an interface where the Android version does nothing or won't compile
rather
> than having a mess of ifdefs around.

The main reason of excluding printing on Android is binary size. We don't want
unused features compiled into the package. We exclude not only the code guarded
with ifdefs, but also most files under printing/, chrome/printing/ and
chrome/ui/webui/print_preview/.

I agree with you about using interfaces to replace the ifdefs, but this needs a
big refactoring to the existing printing code (both backend and the ui). Would
you agree to make that a separate task?

[Lei Zhang, 2012-06-07 01:47:43]

On 2012/06/07 01:45:27, Xianzhu wrote:
> On 2012/06/06 22:48:05, brettw wrote:
> > We don't want ifdefs that we plan on keeping around forever. What is the
> reason
> > for not compiling this code? Is it space? I don't see a huge amount of code
> > being removed by this patch. If it's that some things just can't work, we
> should
> > have an interface where the Android version does nothing or won't compile
> rather
> > than having a mess of ifdefs around.
> 
> The main reason of excluding printing on Android is binary size. We don't want
> unused features compiled into the package. We exclude not only the code
guarded
> with ifdefs, but also most files under printing/, chrome/printing/ and
> chrome/ui/webui/print_preview/.

There's also a lot of html/css/js in chrome/browser/resources/print_preview.

[brettw, 2012-06-08 18:19:03]

lgtm

[Lei Zhang, 2012-06-08 19:08:40]

http://codereview.chromium.org/10540033/diff/5001/chrome/browser/browser_abou...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/10540033/diff/5001/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:81: chrome::kChromeUIPrintHost,
Can you also change the reference to kChromeUIPrintHost to be behind
ENABLE_PRINTING in chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc?

I think if you do that, then you can put kChromeUIPrintHost behind
ENABLE_PRINTING as well.

http://codereview.chromium.org/10540033/diff/5001/printing/printing.gyp
File printing/printing.gyp (right):

http://codereview.chromium.org/10540033/diff/5001/printing/printing.gyp#newco...
printing/printing.gyp:249: ['use_cups==1 and enable_printing==1', {
I don't think you need this. |use_cups| is already set to 0 in
build/common.gypi.

http://codereview.chromium.org/10540033/diff/5001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10540033/diff/5001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1175: #if defined(USE_SKIA) &&
defined(ENABLE_PRINTING)
Why not just make PrintPage() simply return false if ENABLE_PRINTING is not set?
Similarly PrintBegin() can return 0, PrintEnd() can just return, and
PrintPageHelper() can be #ifdef'd out.

[Xianzhu, 2012-06-08 22:50:43]

Thanks Lei. In the new patch set I also exclude the printing resources.

http://codereview.chromium.org/10540033/diff/5001/chrome/browser/browser_abou...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/10540033/diff/5001/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:81: chrome::kChromeUIPrintHost,
On 2012/06/08 19:08:41, Lei Zhang wrote:
> Can you also change the reference to kChromeUIPrintHost to be behind
> ENABLE_PRINTING in
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc?

Done. 

> I think if you do that, then you can put kChromeUIPrintHost behind
> ENABLE_PRINTING as well.

Normally we don't disable the declarations when upstreaming Android specific
code that disables some features to reduce the number of ifdefs.

http://codereview.chromium.org/10540033/diff/5001/printing/printing.gyp
File printing/printing.gyp (right):

http://codereview.chromium.org/10540033/diff/5001/printing/printing.gyp#newco...
printing/printing.gyp:249: ['use_cups==1 and enable_printing==1', {
On 2012/06/08 19:08:41, Lei Zhang wrote:
> I don't think you need this. |use_cups| is already set to 0 in
> build/common.gypi.

Done.

http://codereview.chromium.org/10540033/diff/5001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10540033/diff/5001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1175: #if defined(USE_SKIA) &&
defined(ENABLE_PRINTING)
On 2012/06/08 19:08:41, Lei Zhang wrote:
> Why not just make PrintPage() simply return false if ENABLE_PRINTING is not
set?
> Similarly PrintBegin() can return 0, PrintEnd() can just return, and
> PrintPageHelper() can be #ifdef'd out.

Done.

[Lei Zhang, 2012-06-08 23:24:52]

FYI, there's more print resources in chrome/app/*.grd.

http://codereview.chromium.org/10540033/diff/5001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10540033/diff/5001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1175: #if defined(USE_SKIA) &&
defined(ENABLE_PRINTING)
On 2012/06/08 22:50:43, Xianzhu wrote:
> On 2012/06/08 19:08:41, Lei Zhang wrote:
> > Why not just make PrintPage() simply return false if ENABLE_PRINTING is not
> set?
> > Similarly PrintBegin() can return 0, PrintEnd() can just return, and
> > PrintPageHelper() can be #ifdef'd out.
> 
> Done.

This is fine, but you can also turn the other printing methods in this file into
dummy methods when printing is disabled. You can do it now or in a separate CL
later.

http://codereview.chromium.org/10540033/diff/10004/chrome/browser/ui/tab_cont...
File chrome/browser/ui/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/10540033/diff/10004/chrome/browser/ui/tab_cont...
chrome/browser/ui/tab_contents/tab_contents.cc:124:
print_view_manager_.reset(new printing::PrintViewManager(this));
I believe you can combine this with the other #if defined(ENABLE_PRINTING) on
line 156 under one #if.

http://codereview.chromium.org/10540033/diff/10004/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

http://codereview.chromium.org/10540033/diff/10004/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:239: #if
defined(ENABLE_PRINTING)
I would write this as:

#if !defined(OS_ANDROID)
AAA
CCC
#endif
#if defined(ENABLE_PRINTING)
BBB
#endif

instead of:


#if !defined(OS_ANDROID)
AAA
#endif
#if defined(ENABLE_PRINTING)
BBB
#endif
#if !defined(OS_ANDROID)
CCC
#endif

[Xianzhu, 2012-06-09 00:51:30]

> FYI, there's more print resources in chrome/app/*.grd.

Will do it in a separate change after making clear how excluding the strings
affects Android build.

http://codereview.chromium.org/10540033/diff/5001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10540033/diff/5001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1175: #if defined(USE_SKIA) &&
defined(ENABLE_PRINTING)
On 2012/06/08 23:24:52, Lei Zhang wrote:
> On 2012/06/08 22:50:43, Xianzhu wrote:
> > On 2012/06/08 19:08:41, Lei Zhang wrote:
> > > Why not just make PrintPage() simply return false if ENABLE_PRINTING is
not
> > set?
> > > Similarly PrintBegin() can return 0, PrintEnd() can just return, and
> > > PrintPageHelper() can be #ifdef'd out.
> > 
> > Done.
> 
> This is fine, but you can also turn the other printing methods in this file
into
> dummy methods when printing is disabled. You can do it now or in a separate CL
> later.

Now added ifdef in PrintPDFOutput() which also uses printing:: symbols. Didn't
change that before because it won't break build without enable_printing.

http://codereview.chromium.org/10540033/diff/10004/chrome/browser/ui/tab_cont...
File chrome/browser/ui/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/10540033/diff/10004/chrome/browser/ui/tab_cont...
chrome/browser/ui/tab_contents/tab_contents.cc:124:
print_view_manager_.reset(new printing::PrintViewManager(this));
On 2012/06/08 23:24:52, Lei Zhang wrote:
> I believe you can combine this with the other #if defined(ENABLE_PRINTING) on
> line 156 under one #if.

Done.

http://codereview.chromium.org/10540033/diff/10004/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

http://codereview.chromium.org/10540033/diff/10004/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:239: #if
defined(ENABLE_PRINTING)
On 2012/06/08 23:24:52, Lei Zhang wrote:
> I would write this as:
> 
> #if !defined(OS_ANDROID)
> AAA
> CCC
> #endif
> #if defined(ENABLE_PRINTING)
> BBB
> #endif
> 
> instead of:
> 
> 
> #if !defined(OS_ANDROID)
> AAA
> #endif
> #if defined(ENABLE_PRINTING)
> BBB
> #endif
> #if !defined(OS_ANDROID)
> CCC
> #endif

Done.

[Lei Zhang, 2012-06-09 00:57:33]

lgtm

[commit-bot: I haz the power, 2012-06-09 01:20:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/10540033/6018

[commit-bot: I haz the power, 2012-06-09 06:04:04]

Change committed as 141353