SafeBrowsing: Move enabled_ to base class DB Manager

Safe Browsing: Move enabled_ to the base DatabaseManager class.

Moves the enabled_ member to the SafeBrowsingDatabaseManager base class and checks enabled_ in CheckApiBlacklistUrl().

Note that this changes the order in which enabled_ gets set in some of the subclasses.

BUG=683791
Review-Url: https://codereview.chromium.org/2648193002
Cr-Commit-Position: refs/heads/master@{#445913}
Committed: https://chromium.googlesource.com/chromium/src/+/f0fa01b10bf10faae9ac8ff604a91c6130377491

[kcarattini, 2017-01-23 05:24:05]

The CQ bit was checked by kcarattini@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 05:24:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kcarattini, 2017-01-23 05:24:21]

Description was changed from

==========
formatting


Safe Browsing: Move enabled_ to the base DatabaseManager class

Moves the enabled_ member to the SafeBrowsingDatabaseManager base class and
checks enabled_ in CheckApiBlacklistUrl().

Note that this changes the order in whcih enabled_ gets set in soem of the
subclasaes.

BUG=683791
==========

to

==========
Safe Browsing: Move enabled_ to the base DatabaseManager class

Moves the enabled_ member to the SafeBrowsingDatabaseManager base class and
checks enabled_ in CheckApiBlacklistUrl().

Note that this changes the order in whcih enabled_ gets set in soem of the
subclasaes.

BUG=683791
==========

[kcarattini, 2017-01-23 05:25:10]

kcarattini@chromium.org changed reviewers:
+ nparker@chromium.org, vakh@chromium.org

[commit-bot: I haz the power, 2017-01-23 05:41:44]

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

[commit-bot: I haz the power, 2017-01-23 05:41:45]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[kcarattini, 2017-01-23 05:44:39]

Description was changed from

==========
Safe Browsing: Move enabled_ to the base DatabaseManager class

Moves the enabled_ member to the SafeBrowsingDatabaseManager base class and
checks enabled_ in CheckApiBlacklistUrl().

Note that this changes the order in whcih enabled_ gets set in soem of the
subclasaes.

BUG=683791
==========

to

==========
Safe Browsing: Move enabled_ to the base DatabaseManager class

Moves the enabled_ member to the SafeBrowsingDatabaseManager base class and
checks enabled_ in CheckApiBlacklistUrl().

Note that this changes the order in which enabled_ gets set in soem of the
subclasaes.

BUG=683791
==========

[kcarattini, 2017-01-23 05:45:04]

Description was changed from

==========
Safe Browsing: Move enabled_ to the base DatabaseManager class

Moves the enabled_ member to the SafeBrowsingDatabaseManager base class and
checks enabled_ in CheckApiBlacklistUrl().

Note that this changes the order in which enabled_ gets set in soem of the
subclasaes.

BUG=683791
==========

to

==========
Safe Browsing: Move enabled_ to the base DatabaseManager class

Moves the enabled_ member to the SafeBrowsingDatabaseManager base class and
checks enabled_ in CheckApiBlacklistUrl().

Note that this changes the order in which enabled_ gets set in some of the
subclasses.

BUG=683791
==========

[kcarattini, 2017-01-23 05:45:21]

Description was changed from

==========
Safe Browsing: Move enabled_ to the base DatabaseManager class

Moves the enabled_ member to the SafeBrowsingDatabaseManager base class and
checks enabled_ in CheckApiBlacklistUrl().

Note that this changes the order in which enabled_ gets set in some of the
subclasses.

BUG=683791
==========

to

==========
Safe Browsing: Move enabled_ to the base DatabaseManager class.

Moves the enabled_ member to the SafeBrowsingDatabaseManager base class and
checks enabled_ in CheckApiBlacklistUrl().

Note that this changes the order in which enabled_ gets set in some of the
subclasses.

BUG=683791
==========

[kcarattini, 2017-01-23 05:45:29]

The CQ bit was checked by kcarattini@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 05:45:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 06:41:38]

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

[commit-bot: I haz the power, 2017-01-23 06:41:39]

Dry run: This issue passed the CQ dry run.

[vakh (use Gerrit instead), 2017-01-23 16:02:49]

Hey Kendra, PVer4 is already running in Dev and Canary (50%).
If your work isn't launching in Stable before M57, I would prefer that we move
the ApiCheck method to PVer4 code instead of making this change.

What do you think?

[kcarattini, 2017-01-23 18:23:59]

Hi,

I'm not sure what you mean by move to pver4 code -- do you mean move it to the
V4LocalDatabaseManager?

Our code will launch in M58. I'd prefer not move it at this point in case there
are any issues with pver4 launching to stable, but happy to move it once the
move is complete.

Kendra

[Nathan Parker, 2017-01-23 23:32:34]

I think we should land this fix now rather than waiting for Pver4, since as-is
there's bug where Permissions blacklisting can't be disabled.  Also, the
RemoteDatabaseManager will live on after Pver3 is gone.

https://codereview.chromium.org/2648193002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/2648193002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.cc:701:
SafeBrowsingDatabaseManager::StopOnIOThread(shutdown);
I think we should set enabled_=false at the top of this function, since the
client-callback calls in DoStopOnIOThread() and in SBDM::StopOnIOThread() could
call back into the DB Mgr, and they should see that it's disabled..

And it's probably safer to have the symmetry of calling the base class's Start
function before the sub-class's code, and calling the base class's Stop after
the sub-class's code (like how members get initialized).  That would require
splitting the enable=* code out from SBDM::Start/StopOnIOThread().

(You could have a base class SetEnabled(bool) to enforce that it's manipulated
on the IO thread.  That doesn't really add much through.)

[kcarattini, 2017-01-24 03:21:23]

https://codereview.chromium.org/2648193002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/2648193002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.cc:701:
SafeBrowsingDatabaseManager::StopOnIOThread(shutdown);
On 2017/01/23 23:32:34, Nathan Parker wrote:
> I think we should set enabled_=false at the top of this function, since the
> client-callback calls in DoStopOnIOThread() and in SBDM::StopOnIOThread()
could
> call back into the DB Mgr, and they should see that it's disabled..
> 
> And it's probably safer to have the symmetry of calling the base class's Start
> function before the sub-class's code, and calling the base class's Stop after
> the sub-class's code (like how members get initialized).  That would require
> splitting the enable=* code out from SBDM::Start/StopOnIOThread().
> 
> (You could have a base class SetEnabled(bool) to enforce that it's manipulated
> on the IO thread.  That doesn't really add much through.)

Done (I think). Now only the subclasses set enabled_ and base class calls are
int he order you specified. PTAL.

[kcarattini, 2017-01-24 03:22:08]

The CQ bit was checked by kcarattini@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 03:22:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 04:16:42]

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

[commit-bot: I haz the power, 2017-01-24 04:16:43]

Dry run: This issue passed the CQ dry run.

[vakh (use Gerrit instead), 2017-01-24 21:51:36]

lgtm

[Nathan Parker, 2017-01-24 22:51:03]

Cool!  LGTM

https://codereview.chromium.org/2648193002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/database_manager.h (right):

https://codereview.chromium.org/2648193002/diff/40001/components/safe_browsin...
components/safe_browsing_db/database_manager.h:200: // override this method and
set enabled_ to true.
... and call the base class method at the top of it. (similar comment below, but
at the bottom of it)

[kcarattini, 2017-01-25 00:19:00]

Thanks!

https://codereview.chromium.org/2648193002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/database_manager.h (right):

https://codereview.chromium.org/2648193002/diff/40001/components/safe_browsin...
components/safe_browsing_db/database_manager.h:200: // override this method and
set enabled_ to true.
On 2017/01/24 22:51:03, Nathan Parker wrote:
> ... and call the base class method at the top of it. (similar comment below,
but
> at the bottom of it)

Done.

[kcarattini, 2017-01-25 00:19:12]

The CQ bit was checked by kcarattini@chromium.org

[kcarattini, 2017-01-25 00:19:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, vakh@chromium.org
Link to the patchset: https://codereview.chromium.org/2648193002/#ps60001
(title: "Update comments")

[commit-bot: I haz the power, 2017-01-25 00:19:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 02:17:19]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485303552077590,
"parent_rev": "50304478d3ae4394a921bd8bac028895125f7f7d", "commit_rev":
"f0fa01b10bf10faae9ac8ff604a91c6130377491"}

[commit-bot: I haz the power, 2017-01-25 02:18:23]

Description was changed from

==========
Safe Browsing: Move enabled_ to the base DatabaseManager class.

Moves the enabled_ member to the SafeBrowsingDatabaseManager base class and
checks enabled_ in CheckApiBlacklistUrl().

Note that this changes the order in which enabled_ gets set in some of the
subclasses.

BUG=683791
==========

to

==========
Safe Browsing: Move enabled_ to the base DatabaseManager class.

Moves the enabled_ member to the SafeBrowsingDatabaseManager base class and
checks enabled_ in CheckApiBlacklistUrl().

Note that this changes the order in which enabled_ gets set in some of the
subclasses.

BUG=683791

Review-Url: https://codereview.chromium.org/2648193002
Cr-Commit-Position: refs/heads/master@{#445913}
Committed:
https://chromium.googlesource.com/chromium/src/+/f0fa01b10bf10faae9ac8ff604a9...
==========

[commit-bot: I haz the power, 2017-01-25 02:18:24]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f0fa01b10bf10faae9ac8ff604a9...