Closed
Bug 1275198
Opened 9 years ago
Closed 9 years ago
Add SafeBrowsing v4 protobuf related files
Categories
(Toolkit :: Safe Browsing, defect)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: hchang, Assigned: hchang)
References
Details
Attachments
(2 files, 9 obsolete files)
1.90 KB,
patch
|
francois
:
review+
|
Details | Diff | Splinter Review |
514.01 KB,
patch
|
hchang
:
review+
|
Details | Diff | Splinter Review |
SB v4 update and hash completion relies on protobuf so we need these files.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → hchang
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 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•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8755806 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 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 | ||
Comment 7•9 years ago
|
||
Attachment #8755828 -
Attachment is obsolete: true
Attachment #8755829 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8756677 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8756679 -
Flags: review?(francois)
Assignee | ||
Updated•9 years ago
|
Attachment #8756808 -
Flags: review?(francois)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8755740 -
Attachment is obsolete: true
Attachment #8756808 -
Attachment is obsolete: true
Attachment #8756808 -
Flags: review?(francois)
Assignee | ||
Comment 11•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8756679 -
Attachment is obsolete: true
Attachment #8756679 -
Flags: review?(francois)
Assignee | ||
Updated•9 years ago
|
Attachment #8757909 -
Flags: review?(francois)
Assignee | ||
Updated•9 years ago
|
Attachment #8757910 -
Flags: review?(francois)
Comment 12•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8757910 -
Flags: review?(francois) → review+
Assignee | ||
Comment 13•9 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)
Comment 14•9 years ago
|
||
(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•9 years ago
|
||
Attachment #8757909 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8758516 -
Flags: review?(francois)
Assignee | ||
Comment 16•9 years ago
|
||
Hi Francois,
I updated patch part 1 as you suggested. Could you have a review again? Thanks!
Assignee | ||
Updated•9 years ago
|
Blocks: safebrowsingv4
Comment 17•9 years ago
|
||
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•9 years ago
|
||
Attachment #8758516 -
Attachment is obsolete: true
Assignee | ||
Updated•9 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 | ||
Comment 19•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 20•9 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•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f04aaf38f90b
https://hg.mozilla.org/mozilla-central/rev/71d424246fe1
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•