Add more fields to ClientDownloadRequest

Add more fields to ClientDownloadRequest

BUG=639467
Committed: https://crrev.com/19697d12caa1c2d0f5de30c99cfdb49208a1a149
Cr-Commit-Position: refs/heads/master@{#417363}

[Jialiu Lin, 2016-09-07 21:10:22]

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

[commit-bot: I haz the power, 2016-09-07 21:10:51]

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

[Jialiu Lin, 2016-09-07 21:30:28]

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

[Jialiu Lin, 2016-09-07 21:30:54]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-09-07 21:32:37]

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

[Jialiu Lin, 2016-09-07 21:32:53]

jialiul@chromium.org changed reviewers:
+ auk@google.com

[Jialiu Lin, 2016-09-07 21:32:54]

Hi Kathy, 
Let me know if you're OK with this proto change before I make the same change to
csd_client.proto in google3. 

Thanks!

[auk, 2016-09-07 22:00:43]

On 2016/09/07 21:32:54, Jialiu Lin wrote:
> Hi Kathy, 
> Let me know if you're OK with this proto change before I make the same change
to
> csd_client.proto in google3. 
> 
> Thanks!

Can you remind me what is the difference between CLIENT_REDIRECT and
SERVER_REDIRECT?

Also, unrelated to this CL, one thing that caught my eye is that field 32 has
mismatched types between the client and the server. Would this cause parsing
issue?
Chrome:
repeated string alternate_extensions = 32;
CSD:
optional string geographical_country = 32;

[commit-bot: I haz the power, 2016-09-07 22:19:02]

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

[commit-bot: I haz the power, 2016-09-07 22:19:03]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2016-09-07 22:21:31]

On 2016/09/07 at 22:00:43, auk wrote:
> On 2016/09/07 21:32:54, Jialiu Lin wrote:
> > Hi Kathy, 
> > Let me know if you're OK with this proto change before I make the same
change to
> > csd_client.proto in google3. 
> > 
> > Thanks!
> 
> Can you remind me what is the difference between CLIENT_REDIRECT and
SERVER_REDIRECT?
Client redirect refers to webpage using meta refresh or windows.location.href to
redirect to a different page. It's controlled by the webpage, the destination of
the redirect usually can be captured by analyzing the webpage.
On the other hand, server redirects are triggered by server. It usually uses an
HTTP status code (e.g., 301 Moved Permanently, 307 Temporary Redirect) to
response to a request for a particular URL.

It does not make a huge differences for our case, but might be a signal can be
used in the future. 

> 
> Also, unrelated to this CL, one thing that caught my eye is that field 32 has
mismatched types between the client and the server. Would this cause parsing
issue?
> Chrome:
> repeated string alternate_extensions = 32;
> CSD:
> optional string geographical_country = 32;

Ah, nice catch. I'll change the alternate_extensions filed to 35 and added to
csd_client.proto in google 3 too.

[auk, 2016-09-07 22:35:55]

On 2016/09/07 22:21:31, Jialiu Lin wrote:
> On 2016/09/07 at 22:00:43, auk wrote:
> > On 2016/09/07 21:32:54, Jialiu Lin wrote:
> > > Hi Kathy, 
> > > Let me know if you're OK with this proto change before I make the same
> change to
> > > csd_client.proto in google3. 
> > > 
> > > Thanks!
> > 
> > Can you remind me what is the difference between CLIENT_REDIRECT and
> SERVER_REDIRECT?
> Client redirect refers to webpage using meta refresh or windows.location.href
to
> redirect to a different page. It's controlled by the webpage, the destination
of
> the redirect usually can be captured by analyzing the webpage.
> On the other hand, server redirects are triggered by server. It usually uses
an
> HTTP status code (e.g., 301 Moved Permanently, 307 Temporary Redirect) to
> response to a request for a particular URL.
> 
> It does not make a huge differences for our case, but might be a signal can be
> used in the future. 
> 
> > 
> > Also, unrelated to this CL, one thing that caught my eye is that field 32
has
> mismatched types between the client and the server. Would this cause parsing
> issue?
> > Chrome:
> > repeated string alternate_extensions = 32;
> > CSD:
> > optional string geographical_country = 32;
> 
> Ah, nice catch. I'll change the alternate_extensions filed to 35 and added to
> csd_client.proto in google 3 too.

Thanks for the clarification! lgtm :)

[Jialiu Lin, 2016-09-07 22:37:05]

jialiul@chromium.org changed reviewers:
+ nparker@chromium.org

[Jialiu Lin, 2016-09-07 22:37:06]

+nparker@, Please take a look. Thanks!

[Jialiu Lin, 2016-09-08 18:48:36]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2016-09-08 18:48:37]

The patchset sent to the CQ was uploaded after l-g-t-m from auk@google.com
Link to the patchset: https://codereview.chromium.org/2319533003/#ps80001
(title: "trailing spaces")

[commit-bot: I haz the power, 2016-09-08 18:49:14]

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-08 19:34:57]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-09-08 19:37:17]

Description was changed from

==========
Add more fields to ClientDownloadRequest

BUG=639467
==========

to

==========
Add more fields to ClientDownloadRequest

BUG=639467

Committed: https://crrev.com/19697d12caa1c2d0f5de30c99cfdb49208a1a149
Cr-Commit-Position: refs/heads/master@{#417363}
==========

[commit-bot: I haz the power, 2016-09-08 19:37:18]

Patchset 4 (id:??) landed as
https://crrev.com/19697d12caa1c2d0f5de30c99cfdb49208a1a149
Cr-Commit-Position: refs/heads/master@{#417363}

[Nathan Parker, 2016-09-08 20:23:25]

https://codereview.chromium.org/2319533003/diff/80001/chrome/common/safe_brow...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/2319533003/diff/80001/chrome/common/safe_brow...
chrome/common/safe_browsing/csd.proto:383: repeated string alternate_extensions
= 35;
Changing the id... I assume this was this never in use?  Is it in use now?

[Jialiu Lin, 2016-09-08 20:34:19]

On 2016/09/08 at 20:23:25, nparker wrote:
>
https://codereview.chromium.org/2319533003/diff/80001/chrome/common/safe_brow...
> File chrome/common/safe_browsing/csd.proto (right):
> 
>
https://codereview.chromium.org/2319533003/diff/80001/chrome/common/safe_brow...
> chrome/common/safe_browsing/csd.proto:383: repeated string
alternate_extensions = 35;
> Changing the id... I assume this was this never in use?  Is it in use now?

I guess not, since it is not in csd_client.proto in google3. auk@ asked me to
correct this id while I am already changing this proto.