Closed
Bug 1273412
Opened 8 years ago
Closed 8 years ago
"Table/List name" (v2) to "Threat type" (v4) conversion
Categories
(Toolkit :: Safe Browsing, defect)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
DUPLICATE
of bug 1275507
People
(Reporter: hchang, Assigned: hchang)
References
Details
(Whiteboard: #sbv4-m0)
Attachments
(2 files, 3 obsolete files)
3.81 KB,
patch
|
Details | Diff | Splinter Review | |
2.79 KB,
patch
|
francois
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → hchang
Assignee | ||
Updated•8 years ago
|
Blocks: safebrowsingv4
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8757803 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8757869 -
Attachment description: Test case → Part 2: The test case
Assignee | ||
Updated•8 years ago
|
Attachment #8758598 -
Flags: review?(francois)
Assignee | ||
Updated•8 years ago
|
Attachment #8757869 -
Flags: review?(francois)
Assignee | ||
Comment 4•8 years ago
|
||
Hi Francois, Could you review this patch for the threat type and list name conversion? I made it depend on Bug 1275198 since it uses enum defined in safebrowsing.pb.h. Thanks :)
Comment 5•8 years ago
|
||
Comment on attachment 8758598 [details] [diff] [review] Part 1: Implement the conversion Review of attachment 8758598 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp @@ +252,5 @@ > NS_IMETHODIMP > +nsUrlClassifierUtils::ConvertThreatTypeToListName(uint32_t aThreatType, > + nsACString& aListName) > +{ > + const char* MAPPING_TABLE[] = { Do you think it makes sense to use a struct array and a for loop like in the other function? It's such a short array that I suspect the performance impact is negligible and it would avoid hard-coding the int values for the various threat types.
Attachment #8758598 -
Flags: review?(francois)
Comment 6•8 years ago
|
||
Comment on attachment 8757869 [details] [diff] [review] Part 2: The test case Review of attachment 8757869 [details] [diff] [review]: ----------------------------------------------------------------- In both of these patches, we should use "goog-*-proto" instead of "goog-*-shavar".
Attachment #8757869 -
Flags: review?(francois) → review-
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8758598 -
Attachment is obsolete: true
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8757869 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8769541 -
Flags: review?(francois)
Assignee | ||
Updated•8 years ago
|
Attachment #8769543 -
Flags: review?(francois)
Assignee | ||
Comment 9•8 years ago
|
||
Hi Francois, Thanks for your review. I've updated the conversion implementation and changed the list name to "goog4-*-proto". (or you think just use goog-*-proto?). Could you please review again? Thanks!
Assignee | ||
Updated•8 years ago
|
Whiteboard: #sbv4-m0
Comment 10•8 years ago
|
||
Comment on attachment 8769541 [details] [diff] [review] Part 1: Implement the conversion Review of attachment 8769541 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp @@ +206,5 @@ > +static const struct { > + const char* mListName; > + uint32_t mThreatType; > +} THREAT_TYPE_CONV_TABLE[] = { > + { "goog4-phish-proto", SOCIAL_ENGINEERING_PUBLIC}, Is ..._PUBLIC the same one that Chrome uses and the one that's in the private spec we have? Because for V2 there are two versions of the phishing list. One that's public and one that is more complete but private. Our API key has access to the private list.
Attachment #8769541 -
Flags: review?(francois)
Comment 11•8 years ago
|
||
Comment on attachment 8769543 [details] [diff] [review] Part 2: The test case Review of attachment 8769543 [details] [diff] [review]: ----------------------------------------------------------------- Only one thing to fix: "goog4" should be "goog"
Attachment #8769543 -
Flags: review?(francois) → review+
Assignee | ||
Comment 12•8 years ago
|
||
Per offline discussion with Francois, we decided to merge the patches to Bug 1275507 so I am closing this bug.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Updated•8 years ago
|
No longer blocks: safebrowsingv4
You need to log in
before you can comment on or make changes to this bug.
Description
•