Add SafeBrowsing v4 protobuf related files

RESOLVED FIXED in Firefox 50

Status

()

Toolkit
Safe Browsing
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hchang, Assigned: hchang)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(2 attachments, 9 obsolete attachments)

(Assignee)

Description

2 years ago
SB v4 update and hash completion relies on protobuf so we need these files.
(Assignee)

Updated

2 years ago
Assignee: nobody → hchang
(Assignee)

Comment 1

2 years ago
Created attachment 8755740 [details] [diff] [review]
0001-Bug-1275198-Adds-generated-protobuf-files-based-on-s.patch
(Assignee)

Updated

2 years ago
Blocks: 1274112
(Assignee)

Comment 2

2 years ago
Reading through the .proto file, it doesn't seem to use much proto3 feature/syntax. Probably we don't need upgrading protobuf to 3.0 at all.
(Assignee)

Comment 3

2 years ago
Created attachment 8755806 [details] [diff] [review]
Patch (using v2)
(Assignee)

Comment 4

2 years ago
Created attachment 8755828 [details] [diff] [review]
Part 1: Add generated files and their source
Attachment #8755806 - Attachment is obsolete: true
(Assignee)

Comment 5

2 years ago
Created attachment 8755829 [details] [diff] [review]
Part 2: Extremely simple test case
(Assignee)

Comment 6

2 years ago
Just attached patches in currently used protobuf. Not sure if protobuf 2.6.1 would be compatible with data serialized by 3.0. I hope it is...
(Assignee)

Updated

2 years ago
Blocks: 1275507
(Assignee)

Comment 7

2 years ago
Created attachment 8756677 [details] [diff] [review]
Part 1: Add generated files and their source
Attachment #8755828 - Attachment is obsolete: true
Attachment #8755829 - Attachment is obsolete: true
(Assignee)

Comment 8

2 years ago
Created attachment 8756679 [details] [diff] [review]
Part 2: Extremely simple test case
(Assignee)

Comment 9

2 years ago
Created attachment 8756808 [details] [diff] [review]
Part 1: Add generated files and their source
(Assignee)

Updated

2 years ago
Attachment #8756677 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8756679 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8756808 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Blocks: 1276595
(Assignee)

Comment 10

2 years ago
Created attachment 8757909 [details] [diff] [review]
Part 1: Add generated files and their source
Attachment #8755740 - Attachment is obsolete: true
Attachment #8756808 - Attachment is obsolete: true
Attachment #8756808 - Flags: review?(francois)
(Assignee)

Comment 11

2 years ago
Created attachment 8757910 [details] [diff] [review]
Part 2: Extremely simple test case
(Assignee)

Updated

2 years ago
Attachment #8756679 - Attachment is obsolete: true
Attachment #8756679 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8757909 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8757910 - Flags: review?(francois)
Comment on attachment 8757909 [details] [diff] [review]
Part 1: Add generated files and their source

Review of attachment 8757909 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but since it's unmodified upstream chromium code, let's move it to a new subdirectory: toolkit/components/url-classifier/chromium/

You could also add README in there to document how you have generated these files and where you got them (i.e the URL of the chromium repo).
Attachment #8757909 - Flags: review?(francois)
Attachment #8757910 - Flags: review?(francois) → review+
(Assignee)

Comment 13

2 years ago
(In reply to François Marier [:francois] from comment #12)
> Comment on attachment 8757909 [details] [diff] [review]
> Part 1: Add generated files and their source
> 
> Review of attachment 8757909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, but since it's unmodified upstream chromium code, let's
> move it to a new subdirectory: toolkit/components/url-classifier/chromium/
> 
> You could also add README in there to document how you have generated these
> files and where you got them (i.e the URL of the chromium repo).

Precisely speaking, it's been added one line:

"package mozilla.safebrowsing;" 

to avoid the naming pollution.

Do you think we should still put it to 'chromium' subdirectory?
Flags: needinfo?(francois)
(In reply to Henry Chang [:henry] from comment #13)
> Precisely speaking, it's been added one line:
> 
> "package mozilla.safebrowsing;" 
> 
> to avoid the naming pollution.

Then we'll want to mention that in the README :)

> Do you think we should still put it to 'chromium' subdirectory?

I think so, just to make it obvious we didn't write it and to emphasize that we should periodically update it from the Chromium source.
Flags: needinfo?(francois)
(Assignee)

Comment 15

2 years ago
Created attachment 8758516 [details] [diff] [review]
Part 1: Add generated files and their source
Attachment #8757909 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8758516 - Flags: review?(francois)
(Assignee)

Comment 16

2 years ago
Hi Francois, 

I updated patch part 1 as you suggested. Could you have a review again? Thanks!
(Assignee)

Updated

2 years ago
Blocks: 1273412
(Assignee)

Updated

2 years ago
Blocks: 1167038
Comment on attachment 8758516 [details] [diff] [review]
Part 1: Add generated files and their source

Review of attachment 8758516 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the small tweaks to the README

::: toolkit/components/url-classifier/chromium/README.txt
@@ +1,3 @@
> +# Overview
> +
> +'safebrowsing.proto' is modified from [1] with the following line addedd:

typo: added

@@ +7,5 @@
> +to avoid naming pollution. We use this source file along with protobuf compiler (protoc) to generate safebrowsing.pb.h/cc for safebrowsing v4 update and hash completion. The current generated files are compiled by protoc 2.6.1 since the protobuf library in gecko is not upgraded to 3.0 yet.
> +
> +# Update
> +
> +If you want to update to the upstream,

I would say "latest upstream version"

@@ +18,5 @@
> +(Note that we should use protoc v2.6.1 to compile. You can find the compiled protoc in [3] if you don't have one.)
> +
> +[1] https://chromium.googlesource.com/chromium/src.git/+/9c4485f1ce7cac7ae82f7a4ae36ccc663afe806c/components/safe_browsing_db/safebrowsing.proto
> +[2] https://chromium.googlesource.com/chromium/src.git/+/master/components/safe_browsing_db/safebrowsing.proto
> +[3] http://repo1.maven.org/maven2/com/google/protobuf/protoc
\ No newline at end of file

Let's make that last link HTTPS :)

We should probably link to the real site too: https://github.com/google/protobuf/releases/tag/v2.6.1
Attachment #8758516 - Flags: review?(francois) → review+
(Assignee)

Comment 18

2 years ago
Created attachment 8760605 [details] [diff] [review]
Part 1: Add generated files and their source (carry r+)
Attachment #8758516 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8760605 - Attachment description: Part 1: Add generated files and their source → Part 1: Add generated files and their source (carry r+)
Attachment #8760605 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 20

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/f04aaf38f90b
Part 1: Add safebrowsing protobuf related files in proto2 format. r=francois.
https://hg.mozilla.org/integration/fx-team/rev/71d424246fe1
Part 2: Simple test cases. r=francois.
Keywords: checkin-needed

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f04aaf38f90b
https://hg.mozilla.org/mozilla-central/rev/71d424246fe1
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.