Adds LogDnsClient::NotifyWhenNotThrottled

Adds LogDnsClient::NotifyWhenNotThrottled

Allows a callback to be registered that will be invoked once the
LogDnsClient has stopped throttling queries.

BUG=624894
Committed: https://crrev.com/0cb5c9c3ad3a29b0cfd9235cae02c051515fa8c5
Cr-Commit-Position: refs/heads/master@{#426472}

[Rob Percival, 2016-09-30 11:59:05]

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

[Rob Percival, 2016-09-30 11:59:23]

robpercival@chromium.org changed reviewers:
+ eranm@chromium.org, rsleevi@chromium.org

[Rob Percival, 2016-09-30 11:59:24]

PTAL

[commit-bot: I haz the power, 2016-09-30 11:59:26]

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, 2016-09-30 12:38:15]

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

[commit-bot: I haz the power, 2016-09-30 12:38:16]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2016-10-03 23:56:04]

https://codereview.chromium.org/2380293002/diff/1/components/certificate_tran...
File components/certificate_transparency/log_dns_client.cc (right):

https://codereview.chromium.org/2380293002/diff/1/components/certificate_tran...
components/certificate_transparency/log_dns_client.cc:413: FROM_HERE,
base::Bind(callback, weak_ptr_factory_.GetWeakPtr()));
From a design perspective: Why asynchronously? Why not synchronously?

https://codereview.chromium.org/2380293002/diff/1/components/certificate_tran...
File components/certificate_transparency/log_dns_client.h (right):

https://codereview.chromium.org/2380293002/diff/1/components/certificate_tran...
components/certificate_transparency/log_dns_client.h:47:
base::Callback<void(base::WeakPtr<LogDnsClient> sender)>;
From an API design standpoint, it's never desirable to expose base::WeakPtr<> as
part of a public API contract, because that adds significantly to the complexity
of reasoning about lifetime management.

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Oy3uuiTSwDk/5UDyR...
has a much longer explanation of why this is.

To this specific issue, however, if the caller cares about the state of which
LogDnsClient is used, they should be the one smuggling the pointer. When using a
base::Callback pattern (as opposed to a Delegate interface), because the caller
can curry all necessary arguments, your API contract doesn't and shouldn't
specify what's meaningful to the caller, but allow them to determine that
themselves. This can also help reduce compile-time and generated code.

[Rob Percival, 2016-10-04 18:52:59]

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

[Rob Percival, 2016-10-04 18:53:03]

https://codereview.chromium.org/2380293002/diff/1/components/certificate_tran...
File components/certificate_transparency/log_dns_client.cc (right):

https://codereview.chromium.org/2380293002/diff/1/components/certificate_tran...
components/certificate_transparency/log_dns_client.cc:413: FROM_HERE,
base::Bind(callback, weak_ptr_factory_.GetWeakPtr()));
On 2016/10/03 23:56:04, Ryan Sleevi (slow) wrote:
> From a design perspective: Why asynchronously? Why not synchronously?

An abundance of caution, but actually this can be done synchronously without
issue.

https://codereview.chromium.org/2380293002/diff/1/components/certificate_tran...
File components/certificate_transparency/log_dns_client.h (right):

https://codereview.chromium.org/2380293002/diff/1/components/certificate_tran...
components/certificate_transparency/log_dns_client.h:47:
base::Callback<void(base::WeakPtr<LogDnsClient> sender)>;
On 2016/10/03 23:56:04, Ryan Sleevi (slow) wrote:
> From an API design standpoint, it's never desirable to expose base::WeakPtr<>
as
> part of a public API contract, because that adds significantly to the
complexity
> of reasoning about lifetime management.
> 
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Oy3uuiTSwDk/5UDyR...
> has a much longer explanation of why this is.
> 
> To this specific issue, however, if the caller cares about the state of which
> LogDnsClient is used, they should be the one smuggling the pointer. When using
a
> base::Callback pattern (as opposed to a Delegate interface), because the
caller
> can curry all necessary arguments, your API contract doesn't and shouldn't
> specify what's meaningful to the caller, but allow them to determine that
> themselves. This can also help reduce compile-time and generated code.

Done.

[commit-bot: I haz the power, 2016-10-04 18:53:24]

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

[Rob Percival, 2016-10-04 18:53:46]

Patchset #2 (id:20001) has been deleted

[Rob Percival, 2016-10-04 18:54:47]

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

[commit-bot: I haz the power, 2016-10-04 18:55:14]

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, 2016-10-04 18:58:27]

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

[commit-bot: I haz the power, 2016-10-04 18:58:28]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Rob Percival, 2016-10-05 10:36:18]

Patchset #4 (id:80001) has been deleted

[Rob Percival, 2016-10-05 11:57:30]

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

[commit-bot: I haz the power, 2016-10-05 11:57:46]

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, 2016-10-05 12:35:32]

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

[commit-bot: I haz the power, 2016-10-05 12:35:34]

Dry run: This issue passed the CQ dry run.

[Rob Percival, 2016-10-05 15:29:04]

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

[commit-bot: I haz the power, 2016-10-05 15:29:25]

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, 2016-10-05 16:17:34]

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

[commit-bot: I haz the power, 2016-10-05 16:17:35]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2016-10-07 15:22:55]

https://codereview.chromium.org/2380293002/diff/120001/components/certificate...
File components/certificate_transparency/log_dns_client.cc (right):

https://codereview.chromium.org/2380293002/diff/120001/components/certificate...
components/certificate_transparency/log_dns_client.cc:465:
not_throttled_callbacks_.clear();
Because the base::Callback could result in deletion of |this| (since it's a
callback and you have no guarantee over what the callback behaviour will be),
I'd just recommend the following

std::list<base::Closure> callbacks = std::move(not_throttled_callbacks_);
for (const auto& callback : callbacks) {
  callback.Run();
}

That is, force it into a stack temporary

[Ryan Sleevi, 2016-10-07 15:25:00]

Sorry, meant to LGTM it % that nit

[Eran Messeri, 2016-10-20 12:02:17]

lgtm

[Rob Percival, 2016-10-20 12:36:13]

Patchset #6 (id:140001) has been deleted

[Rob Percival, 2016-10-20 12:43:13]

https://chromiumcodereview.appspot.com/2380293002/diff/120001/components/cert...
File components/certificate_transparency/log_dns_client.cc (right):

https://chromiumcodereview.appspot.com/2380293002/diff/120001/components/cert...
components/certificate_transparency/log_dns_client.cc:465:
not_throttled_callbacks_.clear();
On 2016/10/07 15:22:55, Ryan Sleevi (at conf til 10-21 wrote:
> Because the base::Callback could result in deletion of |this| (since it's a
> callback and you have no guarantee over what the callback behaviour will be),
> I'd just recommend the following
> 
> std::list<base::Closure> callbacks = std::move(not_throttled_callbacks_);
> for (const auto& callback : callbacks) {
>   callback.Run();
> }
> 
> That is, force it into a stack temporary

Done.

[Rob Percival, 2016-10-20 12:43:21]

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

[commit-bot: I haz the power, 2016-10-20 12:43:42]

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, 2016-10-20 14:05:36]

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

[commit-bot: I haz the power, 2016-10-20 14:05:37]

Dry run: This issue passed the CQ dry run.

[Rob Percival, 2016-10-20 14:20:09]

The CQ bit was checked by robpercival@chromium.org

[Rob Percival, 2016-10-20 14:20:10]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
rsleevi@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2380293002/#ps180001 (title: "Addresses
Ryan's comment")

[commit-bot: I haz the power, 2016-10-20 14:20:30]

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, 2016-10-20 14:28:53]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2016-10-21 13:18:08]

Description was changed from

==========
Adds LogDnsClient::NotifyWhenNotThrottled

Allows a callback to be registered that will be invoked once the
LogDnsClient has stopped throttling queries.

BUG=624894
==========

to

==========
Adds LogDnsClient::NotifyWhenNotThrottled

Allows a callback to be registered that will be invoked once the
LogDnsClient has stopped throttling queries.

BUG=624894

Committed: https://crrev.com/0cb5c9c3ad3a29b0cfd9235cae02c051515fa8c5
Cr-Commit-Position: refs/heads/master@{#426472}
==========

[commit-bot: I haz the power, 2016-10-21 13:18:11]

Patchset 7 (id:??) landed as
https://crrev.com/0cb5c9c3ad3a29b0cfd9235cae02c051515fa8c5
Cr-Commit-Position: refs/heads/master@{#426472}