Refactor safe-browsing build-config definitions.

Refactor safe-browsing build-config definitions.

Centralize the configuration and break things down into individual sub-systems
to enable rather than refering to feature bundles.

SAFE_BROWSING_SERVICE - enable the browser to talk to the server.
SAFE_BROWSING_DATABASE - maintain local database(s) synced from the server.
SAFE_BROWSING_CSD - client-side phishing detection support.

BUG=none

[Scott Hess - ex-Googler, 2014-07-15 23:35:12]

This is the change I was speaking of earlier.  Putting things into common.gypi
did help, but ... BUILD.gn is still widely-distributed, and I haven't found the
key insight to perform the same trick there.

[Scott Hess - ex-Googler, 2014-07-15 23:35:59]

On 2014/07/15 23:35:12, Scott Hess wrote:
> This is the change I was speaking of earlier.  Putting things into common.gypi
> did help, but ... BUILD.gn is still widely-distributed, and I haven't found
the
> key insight to perform the same trick there.

Apologies - I'm not certain this is ready for review, but I'm starting to feel
like my brain is going to crawl out of it's head and run away, so I am on the
lookout for any concerns that might guide things.

[Ryan Sleevi, 2014-07-15 23:46:10]

Brett can talk more about GN, but you've already got the features flag (
http://src.chromium.org/viewvc/chrome/trunk/src/build/config/features.gni?rev...
), so you likely want to tweak build/config/BUILD.GN to set the defines.

https://codereview.chromium.org/397853002/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/397853002/diff/1/build/common.gypi#newcode5557
build/common.gypi:5557: # SAFE_BROWSING_SERVICE - browser manages a
safe-browsing service.
You like want to move this to 2456/2853 for consistency. Note those blocks are
already in target_defaults

[Scott Hess - ex-Googler, 2014-07-17 23:51:46]

Amazingly, I had entirely missed that the BUILD.gn files would be entirely as
joyful as the gyp files.  Sigh.  Looping in Brett to commentate on those
changes.

https://codereview.chromium.org/397853002/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/397853002/diff/1/build/common.gypi#newcode5557
build/common.gypi:5557: # SAFE_BROWSING_SERVICE - browser manages a
safe-browsing service.
On 2014/07/15 23:46:10, Ryan Sleevi wrote:
> You like want to move this to 2456/2853 for consistency. Note those blocks are
> already in target_defaults

Makes sense.  Done.

[brettw, 2014-07-18 00:00:54]

The GN changes look correct so LGTM, but I'm sad to see so many more global
flags added to every build line.

[Ryan Sleevi, 2014-07-18 00:04:16]

Brett,

I thought there was a cleaner way in the brave-new-GN world using some config
magics?

My comment to Scott about the GYP design of this was about the exposure of these
#defines into header files, and that GYP (and Chrome) have historically done bad
with under-specified dependencies.

If GN offers a way to prevent header leakage, then it seems using dependent
configs would be cleaner, but I wasn't sure if it did / if that'd be secure.

(The concern is about two targets including the same header, but having
different #defines, thus different object size layouts)

[Scott Hess - ex-Googler, 2014-07-18 00:15:46]

On 2014/07/18 00:00:54, brettw wrote:
> The GN changes look correct so LGTM, but I'm sad to see so many more global
> flags added to every build line.

I would certainly rather have them only exported from something more specific,
chrome/common for instance.  AFAICT, the only way to do that with gyp would be
to have a target with appropriate direct_dependent_settings, and intermediate
users having export_dependent_settings as appropriate.  Ryan pointed out that
that wouldn't provide good warnings if someone just dropped a conditional
statement somewhere which wasn't wired up, yet.  If there's a slicker way to do
it for gn, I'm happy to adapt to it, just point me at an example case I can
follow!

I am going to have to audit all of the FULL_SAFE_BROWSING cases, which often are
code for "desktop", so it may be that some of them can be refactored to not use
conditional compilation at all.  I don't know if enough can be removed to remove
the need for broad access to the defines, though.

[Scott Hess - ex-Googler, 2014-07-18 00:17:13]

Also, I would greatly prefer restricting this to chrome/ if it can be done
effectively.  I'll be upset if anyone ever uses the defines outside of chrome/.

[brettw, 2014-07-18 19:49:30]

GN should work about the same as GYP in this respect (otherwise conversion would
be too hard).

The best way I can think of is to find the lowest-level library (chrome_common?)
and add an all dependent settings on it.

Long term I'd like to change all the defines to be 0 when false and 1 when true
so you can tell if it's undefined you don't have the ability to read that
define. But that's for another time.

[Ryan Sleevi, 2014-07-24 22:47:50]

To circle back on this: This LGTM, buttt....

As far as I can tell, you (thankfully) don't use these #ifdef's in really any
headers, just the .cc. In that case, you could get away with
'direct_dependent_settings' and making sure they flow appropriately. If anything
fails, it'd be a link time failure, because someone forgot to express the
dependency graph. But it wouldn't affect object sizes or have 'silent' compiles
succeed, which might cause runtime crashes.

That is, with the exception of the messages. However, you could unconditionally
include the message generators, and rather than #error-ing out, just not define
any messages. If anyone tried to #include that header and not define the
messages, things just "wouldn't work".

Your current implementation errs on the side of safety, and assumes the worst of
people attempting to modify the GYP files - a safe assumption if ever - so LGTM.
Just noting the alternative.s

https://codereview.chromium.org/397853002/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_host/safe_browsing_resource_throttle_factory.h
(right):

https://codereview.chromium.org/397853002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_host/safe_browsing_resource_throttle_factory.h:34:
#endif
Does it make sense to
1) Move the implementation to the .cc
2) Forward declare SafeBrowsingResourceThrottleFactory?

You'd then be able to get rid of this #ifdef entirely.

https://codereview.chromium.org/397853002/diff/20001/chrome/common/common_mes...
File chrome/common/common_message_generator.h (right):

https://codereview.chromium.org/397853002/diff/20001/chrome/common/common_mes...
chrome/common/common_message_generator.h:43: #if defined(SAFE_BROWSING_SERVICE)
This is the one thing I can see forcing this to be a global define

[Scott Hess - ex-Googler, 2014-07-29 22:52:31]

https://codereview.chromium.org/397853002/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_host/safe_browsing_resource_throttle_factory.h
(right):

https://codereview.chromium.org/397853002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_host/safe_browsing_resource_throttle_factory.h:34:
#endif
On 2014/07/24 22:47:50, Ryan Sleevi wrote:
> Does it make sense to
> 1) Move the implementation to the .cc
> 2) Forward declare SafeBrowsingResourceThrottleFactory?
> 
> You'd then be able to get rid of this #ifdef entirely.

I cannot find anyone who calls this.  I'm breaking off this change into a
separate CL ( https://codereview.chromium.org/431433002/ ) and will circle back
once that discussion is complete.

[Scott Hess - ex-Googler, 2014-07-30 21:29:57]

On 2014/07/24 22:47:50, Ryan Sleevi wrote:
> To circle back on this: This LGTM, buttt....

Thank you for the good description of alternatives.  There are still a lot of
references to FULL_SAFE_BROWSING, so I'm going to be a bit conservative about
trying to get too targeted until I've crawled through those.  But I agree with
your long-term suggestion of aiming for link-time errors rather than
#ifdef-driven compile-time errors.

https://codereview.chromium.org/397853002/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_host/safe_browsing_resource_throttle_factory.h
(right):

https://codereview.chromium.org/397853002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_host/safe_browsing_resource_throttle_factory.h:34:
#endif
On 2014/07/29 22:52:31, Scott Hess wrote:
> On 2014/07/24 22:47:50, Ryan Sleevi wrote:
> > Does it make sense to
> > 1) Move the implementation to the .cc
> > 2) Forward declare SafeBrowsingResourceThrottleFactory?
> > 
> > You'd then be able to get rid of this #ifdef entirely.
> 
> I cannot find anyone who calls this.  I'm breaking off this change into a
> separate CL ( https://codereview.chromium.org/431433002/ ) and will circle
back
> once that discussion is complete.

OK, apparently Android uses it in some way, so I've shifted the #ifdef into the
implementation (via CL https://codereview.chromium.org/431433002 ).

[Scott Hess - ex-Googler, 2014-07-30 21:36:08]

Lei, looping you in for chrome/ OWNERS because you OWNERS-reviewed the
safe_browsing_resource_throttle_factory change I broke off of this.  My
long-term goal is to be able to develop the different build variants on desktop.

[Lei Zhang, 2014-07-30 21:59:44]

Reviewing...

+mattm

[Lei Zhang, 2014-07-30 22:05:13]

lgtm if mattm approves.

https://codereview.chromium.org/397853002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/397853002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_service.cc:227: #if
defined(SAFE_BROWSING_CSD)
This shouldn't need to be nested inside FULL_SAFE_BROWSING.

[mattm, 2014-07-31 00:51:43]

lgtm with comments

https://codereview.chromium.org/397853002/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/397853002/diff/40001/build/common.gypi#newcod...
build/common.gypi:2867: # TODO(shess): I believe this case is no longer in use.
iOS build is a mystery to me, but I thought they still use safe_browsing=2

https://codereview.chromium.org/397853002/diff/40001/chrome/browser/renderer_...
File chrome/browser/renderer_host/safe_browsing_resource_throttle_factory.cc
(right):

https://codereview.chromium.org/397853002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_host/safe_browsing_resource_throttle_factory.cc:44:
#error Need to define {FULL|MOBILE} SAFE_BROWSING mode.
update error message?

[Lei Zhang, 2014-07-31 00:54:45]

https://codereview.chromium.org/397853002/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/397853002/diff/40001/build/common.gypi#newcod...
build/common.gypi:2867: # TODO(shess): I believe this case is no longer in use.
On 2014/07/31 00:51:43, mattm wrote:
> iOS build is a mystery to me, but I thought they still use safe_browsing=2

Isn't it 0? See build/common.gypi:762.

[mattm, 2014-07-31 00:59:44]

https://codereview.chromium.org/397853002/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/397853002/diff/40001/build/common.gypi#newcod...
build/common.gypi:2867: # TODO(shess): I believe this case is no longer in use.
On 2014/07/31 00:54:45, Lei Zhang wrote:
> On 2014/07/31 00:51:43, mattm wrote:
> > iOS build is a mystery to me, but I thought they still use safe_browsing=2
> 
> Isn't it 0? See build/common.gypi:762.

They must be overriding it somewhere in the internal build.

[Scott Hess - ex-Googler, 2014-07-31 20:50:14]

+rohitrao FYI in case I'm going to destroy you, though I don't expect a full
review.

+tsepez because of chrome/common/safe_browsing/safebrowsing_messages.h .  The
change is #ifdef only, no changes to actual messages.

https://codereview.chromium.org/397853002/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/397853002/diff/40001/build/common.gypi#newcod...
build/common.gypi:2867: # TODO(shess): I believe this case is no longer in use.
On 2014/07/31 00:59:44, mattm wrote:
> On 2014/07/31 00:54:45, Lei Zhang wrote:
> > On 2014/07/31 00:51:43, mattm wrote:
> > > iOS build is a mystery to me, but I thought they still use safe_browsing=2
> > 
> > Isn't it 0? See build/common.gypi:762.
> 
> They must be overriding it somewhere in the internal build.

IMHO, anything people rely on in the Chromium tree needs to be anchored by a
test and/or comments that are in the Chromium tree.  I'll loop in rohit just in
case I'm about to set their world on fire.

[I _think_ this change should break anyone, though, as FULL_SAFE_BROWSING and
MOBILE_SAFE_BROWSING continue to be defined.  Long term I'd prefer to shift away
from ambiguous-bag-of-features defines, though.]

https://codereview.chromium.org/397853002/diff/40001/chrome/browser/renderer_...
File chrome/browser/renderer_host/safe_browsing_resource_throttle_factory.cc
(right):

https://codereview.chromium.org/397853002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_host/safe_browsing_resource_throttle_factory.cc:44:
#error Need to define {FULL|MOBILE} SAFE_BROWSING mode.
On 2014/07/31 00:51:43, mattm wrote:
> update error message?

Done.

[Tom Sepez, 2014-07-31 21:08:10]

Rubberstamp LGTM on safe browsing messages.

[Ryan Sleevi, 2015-05-07 01:09:38]

scott: Dead CL? Mind closing? Alternatively, happy to revisit if you're still
working on this 9 months later :)

[Scott Hess - ex-Googler, 2015-05-07 05:33:16]

On 2015/05/07 01:09:38, Ryan Sleevi wrote:
> scott: Dead CL? Mind closing? Alternatively, happy to revisit if you're still
> working on this 9 months later :)

Actually, Nathan took it over elsewhere.