Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
182333003:
Build correct URL for cacheinvalidation endpoint. (Closed)
|
Created:
6 years, 10 months ago by pavely Modified:
6 years, 9 months ago Reviewers:
rlarocque CC:
chromium-reviews, tim+watch_chromium.org, haitaol+watch_chromium.org, maniscalco+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@RollCacheInvalidations Visibility:
Public. |
DescriptionBuild correct URL for cacheinvalidation endpoint.
Build URL for sending client-to-server cacheinvalidation messages.
BUG=325020
R=rlarocque@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255825
Patch Set 1 #
Total comments: 4
Patch Set 2 : Review feedback #Patch Set 3 : Add URLSafe base64. Conditionally compile for Android. #
Total comments: 1
Patch Set 4 : Feedback #Patch Set 5 : fix build #
Messages
Total messages: 31 (0 generated)
This change is blocked on latest protobufs from cacheinvalidation. I'll land it after the other one is done. Just want to get code review done. The code is similar to AndroidMessageSenderService.buildUrl (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/cachei...)
https://codereview.chromium.org/182333003/diff/1/sync/notifier/gcm_network_ch... File sync/notifier/gcm_network_channel.cc (right): https://codereview.chromium.org/182333003/diff/1/sync/notifier/gcm_network_ch... sync/notifier/gcm_network_channel.cc:198: network_endpoint_id.set_client_address(buffer); Couldn't you get the same serialized result by making the client_address an embedded message protobuf of type EndpointId? I'm guessing these are not your protos, so there's not much you could do to change it. I'm curious to know if there's any justification for it. https://codereview.chromium.org/182333003/diff/1/sync/notifier/gcm_network_ch... sync/notifier/gcm_network_channel.cc:199: network_endpoint_id.SerializeToString(&buffer); I'd prefer to see a different buffer for this serialization. As it is, it's hard to tell whether or not this overwrite is intentional.
https://codereview.chromium.org/182333003/diff/1/sync/notifier/gcm_network_ch... File sync/notifier/gcm_network_channel.cc (right): https://codereview.chromium.org/182333003/diff/1/sync/notifier/gcm_network_ch... sync/notifier/gcm_network_channel.cc:198: network_endpoint_id.set_client_address(buffer); On 2014/02/26 22:55:46, rlarocque wrote: > Couldn't you get the same serialized result by making the client_address an > embedded message protobuf of type EndpointId? > > I'm guessing these are not your protos, so there's not much you could do to > change it. I'm curious to know if there's any justification for it. If network_address is not NetworkAddress::ANDROID then client_address will have different protobuf serialized into it. It is the way how server decodes address. https://codereview.chromium.org/182333003/diff/1/sync/notifier/gcm_network_ch... sync/notifier/gcm_network_channel.cc:199: network_endpoint_id.SerializeToString(&buffer); On 2014/02/26 22:55:46, rlarocque wrote: > I'd prefer to see a different buffer for this serialization. As it is, it's > hard to tell whether or not this overwrite is intentional. Done.
lgtm
I've added URLSafe base64 converters and conditionally compile code with android specific protos. PTAL.
lgtm https://codereview.chromium.org/182333003/diff/40001/sync/notifier/gcm_networ... File sync/notifier/gcm_network_channel_unittest.cc (right): https://codereview.chromium.org/182333003/diff/40001/sync/notifier/gcm_networ... sync/notifier/gcm_network_channel_unittest.cc:321: #ifndef ANDROID nit: This looked a little unusual to me. I did some grepping and foind that #if !defined(FOO) is much more common than #ifndef FOO. You should change this to follow the convention.
The CQ bit was checked by pavely@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/182333003/60001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/182333003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on ios_rel_device_ninja for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
The CQ bit was checked by pavely@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/182333003/70001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/182333003/70001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by pavely@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/182333003/70001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by pavely@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/182333003/70001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by pavely@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/182333003/70001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by pavely@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavely@chromium.org/182333003/70001
Message was sent while issue was closed.
Change committed as 255825 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
