Setting OptInStateFilter for publishes and subscribes (b/16324352)

Setting OptInStateFilter for publishes and subscribes (b/16324352)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289337

[rkc, 2014-08-12 03:38:19]

https://codereview.chromium.org/459903006/diff/1/components/copresence/rpc/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/459903006/diff/1/components/copresence/rpc/rp...
components/copresence/rpc/rpc_handler.cc:204: void
AllowOptedOutMessages(ReportRequest* request) {
This anonymous namespace is getting huge. We should move these functions out to
a copresence::proto_util namespace in a separate file.

https://codereview.chromium.org/459903006/diff/1/components/copresence/rpc/rp...
components/copresence/rpc/rpc_handler.cc:205:
RepeatedPtrField<PublishedMessage>* messages =
This is a pattern that we'll be following 3 places in the code once
https://codereview.chromium.org/461803003/ lands.

We should have a ProcessPublishes and ProcessSubscribes method which takes a
function pointer as a parameter. These methods would call that function on each
of the publishes/subscribes. 

I don't have time to fix this in my CL, but unless this fix is urgent to get
into M-38, let's fix it here.

[Charlie, 2014-08-12 04:36:14]

https://codereview.chromium.org/459903006/diff/1/components/copresence/rpc/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/459903006/diff/1/components/copresence/rpc/rp...
components/copresence/rpc/rpc_handler.cc:204: void
AllowOptedOutMessages(ReportRequest* request) {
On 2014/08/12 03:38:19, Rahul Chaturvedi wrote:
> This anonymous namespace is getting huge. We should move these functions out
to
> a copresence::proto_util namespace in a separate file.

Added a TODO. Keep in mind that ExtractTokenExchangeStrategy will get deleted as
soon as the server supports the BroadcastScanConfiguration correctly.

https://codereview.chromium.org/459903006/diff/1/components/copresence/rpc/rp...
components/copresence/rpc/rpc_handler.cc:205:
RepeatedPtrField<PublishedMessage>* messages =
On 2014/08/12 03:38:19, Rahul Chaturvedi wrote:
> This is a pattern that we'll be following 3 places in the code once
> https://codereview.chromium.org/461803003/ lands.
> 
> We should have a ProcessPublishes and ProcessSubscribes method which takes a
> function pointer as a parameter. These methods would call that function on
each
> of the publishes/subscribes. 
> 
> I don't have time to fix this in my CL, but unless this fix is urgent to get
> into M-38, let's fix it here.

Another TODO. I think Ben probably doesn't want to wait until November for this
fix to go live.

Let's discuss this a bit offline. I believe RepeatedPtrField is designed to
cooperate with C++11 foreach syntax, so this problem could go away soon on its
own.

[rkc, 2014-08-13 00:55:46]

lgtm

[Charlie, 2014-08-13 01:11:49]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-08-13 01:23:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ckehoe@chromium.org/459903006/40001

[commit-bot: I haz the power, 2014-08-13 09:58:32]

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

[commit-bot: I haz the power, 2014-08-13 09:58:33]

Failed to apply patch for components/copresence/rpc/rpc_handler.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file components/copresence/rpc/rpc_handler.cc
  Hunk #2 FAILED at 120.
  Hunk #3 succeeded at 206 with fuzz 1 (offset 11 lines).
  Hunk #4 FAILED at 284.
  2 out of 4 hunks FAILED -- saving rejects to file
components/copresence/rpc/rpc_handler.cc.rej

Patch:       components/copresence/rpc/rpc_handler.cc
Index: components/copresence/rpc/rpc_handler.cc
diff --git a/components/copresence/rpc/rpc_handler.cc
b/components/copresence/rpc/rpc_handler.cc
index
2ca127ea7955fca154b1e9750389a24db0e5695b..35d5273435ffc89127c7937c0df135d019032f77
100644
--- a/components/copresence/rpc/rpc_handler.cc
+++ b/components/copresence/rpc/rpc_handler.cc
@@ -84,6 +84,7 @@ bool ReportErrorLogged(const ReportResponse& response) {
 }
 
 // Request construction
+// TODO(ckehoe): Move these into a separate file?
 
 template <typename T>
 BroadcastScanConfiguration GetBroadcastScanConfig(const T& msg) {
@@ -119,11 +120,11 @@ BroadcastScanConfiguration ExtractTokenExchangeStrategy(
 
   // Strategies for subscriptions.
   if (request.has_manage_subscriptions_request()) {
-    const RepeatedPtrField<Subscription> messages =
+    const RepeatedPtrField<Subscription> subscriptions =
         request.manage_subscriptions_request().subscription();
-    for (int i = 0; i < messages.size(); ++i) {
+    for (int i = 0; i < subscriptions.size(); ++i) {
       BroadcastScanConfiguration config =
-          GetBroadcastScanConfig(messages.Get(i));
+          GetBroadcastScanConfig(subscriptions.Get(i));
       broadcast_only = broadcast_only || config == BROADCAST_ONLY;
       scan_only = scan_only || config == SCAN_ONLY;
       if (config == BROADCAST_AND_SCAN || (broadcast_only && scan_only))
@@ -194,6 +195,36 @@ ClientVersion* CreateVersion(const std::string& client,
   return version;
 }
 
+OptInStateFilter* CreateOptedInOrOutFilter() {
+  OptInStateFilter* filter = new OptInStateFilter;
+  filter->add_allowed_opt_in_state(copresence::OPTED_IN);
+  filter->add_allowed_opt_in_state(copresence::OPTED_OUT);
+  return filter;
+}
+
+void AllowOptedOutMessages(ReportRequest* request) {
+  // TODO(ckehoe): Collapse this pattern into ProcessPublish()
+  // and ProcessSubscribe() methods.
+
+  RepeatedPtrField<PublishedMessage>* messages =
+      request->mutable_manage_messages_request()->mutable_message_to_publish();
+  for (int i = 0; i < messages->size(); ++i) {
+    PublishedMessage* message = messages->Mutable(i);
+    if (!message->has_opt_in_state_filter())
+      message->set_allocated_opt_in_state_filter(CreateOptedInOrOutFilter());
+  }
+
+  RepeatedPtrField<Subscription>* subscriptions =
+       
request->mutable_manage_subscriptions_request()->mutable_subscription();
+  for (int i = 0; i < subscriptions->size(); ++i) {
+    Subscription* subscription = subscriptions->Mutable(i);
+    if (!subscription->has_opt_in_state_filter()) {
+      subscription->set_allocated_opt_in_state_filter(
+          CreateOptedInOrOutFilter());
+    }
+  }
+}
+
 }  // namespace
 
 // Public methods
@@ -253,6 +284,7 @@ void RpcHandler::SendReportRequest(scoped_ptr<ReportRequest>
request,
 
   request->mutable_update_signals_request()->set_allocated_state(
       GetDeviceCapabilities(*request).release());
+  AllowOptedOutMessages(request.get());
   SendServerRequest(kReportRequestRpcName,
                     app_id,
                     request.Pass(),

[Charlie, 2014-08-13 15:10:14]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-08-13 15:11:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ckehoe@chromium.org/459903006/60001

[commit-bot: I haz the power, 2014-08-13 19:00:27]

Committed patchset #4 (60001) as 289337