Closed
Bug 1275198
Opened 8 years ago
Closed 8 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•8 years ago
|
Assignee: nobody → hchang
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 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•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8755806 -
Attachment is obsolete: true
Assignee | ||
Comment 5•8 years ago
|
||
Assignee | ||
Comment 6•8 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•8 years ago
|
||
Attachment #8755828 -
Attachment is obsolete: true
Attachment #8755829 -
Attachment is obsolete: true
Assignee | ||
Comment 8•8 years ago
|
||
Assignee | ||
Comment 9•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8756677 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8756679 -
Flags: review?(francois)
Assignee | ||
Updated•8 years ago
|
Attachment #8756808 -
Flags: review?(francois)
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8755740 -
Attachment is obsolete: true
Attachment #8756808 -
Attachment is obsolete: true
Attachment #8756808 -
Flags: review?(francois)
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8756679 -
Attachment is obsolete: true
Attachment #8756679 -
Flags: review?(francois)
Assignee | ||
Updated•8 years ago
|
Attachment #8757909 -
Flags: review?(francois)
Assignee | ||
Updated•8 years ago
|
Attachment #8757910 -
Flags: review?(francois)
Comment 12•8 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•8 years ago
|
Attachment #8757910 -
Flags: review?(francois) → review+
Assignee | ||
Comment 13•8 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•8 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•8 years ago
|
||
Attachment #8757909 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8758516 -
Flags: review?(francois)
Assignee | ||
Comment 16•8 years ago
|
||
Hi Francois, I updated patch part 1 as you suggested. Could you have a review again? Thanks!
Assignee | ||
Updated•8 years ago
|
Blocks: safebrowsingv4
Comment 17•8 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•8 years ago
|
||
Attachment #8758516 -
Attachment is obsolete: true
Assignee | ||
Updated•8 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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d88580442bea
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 20•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f04aaf38f90b https://hg.mozilla.org/mozilla-central/rev/71d424246fe1
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•