Closed Bug 1275198 Opened 8 years ago Closed 8 years ago

Add SafeBrowsing v4 protobuf related files

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: hchang, Assigned: hchang)

References

Details

Attachments

(2 files, 9 obsolete files)

SB v4 update and hash completion relies on protobuf so we need these files.
Assignee: nobody → hchang
Blocks: 1274112
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.
Attached patch Patch (using v2) (obsolete) — Splinter Review
Attachment #8755806 - Attachment is obsolete: true
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...
Blocks: 1275507
Attachment #8755828 - Attachment is obsolete: true
Attachment #8755829 - Attachment is obsolete: true
Attachment #8756677 - Attachment is obsolete: true
Attachment #8756679 - Flags: review?(francois)
Attachment #8756808 - Flags: review?(francois)
Blocks: 1276595
Attachment #8755740 - Attachment is obsolete: true
Attachment #8756808 - Attachment is obsolete: true
Attachment #8756808 - Flags: review?(francois)
Attachment #8756679 - Attachment is obsolete: true
Attachment #8756679 - Flags: review?(francois)
Attachment #8757909 - Flags: review?(francois)
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+
(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)
Attachment #8757909 - Attachment is obsolete: true
Attachment #8758516 - Flags: review?(francois)
Hi Francois, 

I updated patch part 1 as you suggested. Could you have a review again? Thanks!
Blocks: 1273412
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+
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+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/f04aaf38f90b
https://hg.mozilla.org/mozilla-central/rev/71d424246fe1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1275195
No longer blocks: 1275507
You need to log in before you can comment on or make changes to this bug.