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)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1275507

People

(Reporter: hchang, Assigned: hchang)

References

Details

(Whiteboard: #sbv4-m0)

Attachments

(2 files, 3 obsolete files)

      No description provided.
Assignee: nobody → hchang
Attached patch Patch (obsolete) — Splinter Review
Attached patch Part 2: The test case (obsolete) — Splinter Review
Blocks: 1276595
Blocks: 1275507
Attached patch Part 1: Implement the conversion (obsolete) — Splinter Review
Attachment #8757803 - Attachment is obsolete: true
Attachment #8757869 - Attachment description: Test case → Part 2: The test case
Depends on: 1275198
Attachment #8758598 - Flags: review?(francois)
Attachment #8757869 - Flags: review?(francois)
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 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 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-
Attachment #8758598 - Attachment is obsolete: true
Attachment #8757869 - Attachment is obsolete: true
Attachment #8769541 - Flags: review?(francois)
Attachment #8769543 - Flags: review?(francois)
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!
Whiteboard: #sbv4-m0
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 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+
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
No longer blocks: safebrowsingv4
You need to log in before you can comment on or make changes to this bug.