Android: Don't compile printing sources

Android: Don't compile printing sources

We currently don't have the ability to print anything on Android.
* Exclude chrome/browser/printing and chrome/browser/ui/webui/print_preview
  from the GYP rule for Android.
* Guards calls to various print functions.

BUG=http://crbug.com/113487
TEST=


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

[gone, 2012-02-28 00:48:51]

Hey all, we need to add a few guards to get rid of a few printing-related link
errors under Android.  Wasn't sure which person to send it to for the edit to
chrome/browser/profiles/profile_dependency_manager.cc, so please redirect if
necessary.

Thanks!

[Bernhard Bauer, 2012-02-28 09:08:24]

prefs/ LGTM (I guess that's what you wanted me to review?)

[Jói, 2012-02-28 10:04:18]

I'm not very familiar with the printing and print preview support in Chrome in
general; it would probably be best to get someone who is to take a quick look to
help make sure you're turning off all the bits you should.

OTOH, this LGTM from a mechanical standpoint; your changes affect only Android
and I see no errors.

Cheers,
Jói

[Miranda Callahan, 2012-02-28 14:32:58]

On 2012/02/28 10:04:18, Jói wrote:
> I'm not very familiar with the printing and print preview support in Chrome in
> general; it would probably be best to get someone who is to take a quick look
to
> help make sure you're turning off all the bits you should.
> 
> OTOH, this LGTM from a mechanical standpoint; your changes affect only Android
> and I see no errors.
> 
> Cheers,
> Jói

I'm not sure about this ProfileDependencyManager change -- the ifdef will
definitely work, but does the Android code have another strategy about profile
services defined here that involves something other than if-deffing? For
example, I see that the PDM here also asserts that the ThemeServiceFactory is
built -- is this really being used by Android (along with all the other services
listed here)?

[jam, 2012-02-28 16:11:31]

lgtm with nit

also, i think to make readability better, where you now add an ifdef inside a
block of code, it would be better to move that ifdef(android) to be at the end
beside other ifdefs, instead of in the middle of calling the other functions

http://codereview.chromium.org/9447086/diff/1/chrome/browser/ui/tab_contents/...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/9447086/diff/1/chrome/browser/ui/tab_contents/...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:100:
print_view_manager_.reset(new printing::PrintViewManager(this));
nit: bring this down to line 125 so you only have one ifdef

[Yaron, 2012-03-01 01:16:20]

On 2012/02/28 14:32:58, Miranda Callahan wrote:
> On 2012/02/28 10:04:18, Jói wrote:
> > I'm not very familiar with the printing and print preview support in Chrome
in
> > general; it would probably be best to get someone who is to take a quick
look
> to
> > help make sure you're turning off all the bits you should.
> > 
> > OTOH, this LGTM from a mechanical standpoint; your changes affect only
Android
> > and I see no errors.
> > 
> > Cheers,
> > Jói
> 
> I'm not sure about this ProfileDependencyManager change -- the ifdef will
> definitely work, but does the Android code have another strategy about profile
> services defined here that involves something other than if-deffing? For
> example, I see that the PDM here also asserts that the ThemeServiceFactory is
> built -- is this really being used by Android (along with all the other
services
> listed here)?

If-deffing is part of the strategy and I think it's a necessary evil but we're
open to suggestions and improvements. Also, we're trying not to just blanketly
if-def code. We're also refactoring code when necessary to minimize dependencies
on these features in an effort to  keep the code maintainable. However, binary
size is especially important on Android so the general approach has been to
remove entire folders (via gyp) and use as few if-defs as possible in these key
integration points where most features/services are called.

Regarding ThemeServiceFactory: someone else is working on it but this is
happening incrementally. Our first focus is just to get the various unit test
binaries compiling/linking upstream. Once we have that in place we'll focus on
run-time dependencies and actually running tests. Once we have all that in
place, we can safely upstream feature code _and_ test it.

[Miranda Callahan, 2012-03-01 14:03:10]

On 2012/03/01 01:16:20, Yaron wrote:
> On 2012/02/28 14:32:58, Miranda Callahan wrote:
> > On 2012/02/28 10:04:18, Jói wrote:
> > > I'm not very familiar with the printing and print preview support in
Chrome
> in
> > > general; it would probably be best to get someone who is to take a quick
> look
> > to
> > > help make sure you're turning off all the bits you should.
> > > 
> > > OTOH, this LGTM from a mechanical standpoint; your changes affect only
> Android
> > > and I see no errors.
> > > 
> > > Cheers,
> > > Jói
> > 
> > I'm not sure about this ProfileDependencyManager change -- the ifdef will
> > definitely work, but does the Android code have another strategy about
profile
> > services defined here that involves something other than if-deffing? For
> > example, I see that the PDM here also asserts that the ThemeServiceFactory
is
> > built -- is this really being used by Android (along with all the other
> services
> > listed here)?
> 
> If-deffing is part of the strategy and I think it's a necessary evil but we're
> open to suggestions and improvements. Also, we're trying not to just blanketly
> if-def code. We're also refactoring code when necessary to minimize
dependencies
> on these features in an effort to  keep the code maintainable. However, binary
> size is especially important on Android so the general approach has been to
> remove entire folders (via gyp) and use as few if-defs as possible in these
key
> integration points where most features/services are called.
> 
> Regarding ThemeServiceFactory: someone else is working on it but this is
> happening incrementally. Our first focus is just to get the various unit test
> binaries compiling/linking upstream. Once we have that in place we'll focus on
> run-time dependencies and actually running tests. Once we have all that in
> place, we can safely upstream feature code _and_ test it.

I certainly understand the importance of an incremental approach here -- thanks
for the clarification. LGTM for the change, then.

[gone, 2012-03-01 19:57:00]

http://codereview.chromium.org/9447086/diff/1/chrome/browser/ui/tab_contents/...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/9447086/diff/1/chrome/browser/ui/tab_contents/...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:100:
print_view_manager_.reset(new printing::PrintViewManager(this));
On 2012/02/28 16:11:32, John Abd-El-Malek wrote:
> nit: bring this down to line 125 so you only have one ifdef

They seem to be explicitly separated in the code; the top group is for tab
helpers and the other is per-tab observers.  I've moved it to the bottom of its
block, though.

[commit-bot: I haz the power, 2012-03-01 20:55:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/9447086/11001

[commit-bot: I haz the power, 2012-03-01 22:48:06]

Change committed as 124509

[Avi (use Gerrit), 2012-03-03 02:50:14]

http://codereview.chromium.org/9447086/diff/11001/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/9447086/diff/11001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:109: #endif
What's wrong with alphabetical order? :(