Closed Bug 1274105 Opened 5 years ago Closed 5 years ago

Refactor classifierHelper.js

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: dlee, Assigned: dlee)

References

Details

Attachments

(1 file, 1 obsolete file)

There are bunch of things i would like to refactor, will fix them in this bug
Following is the idea:

1. clean up add chunk by using expiration instead of sub chunk
- This can avoid we keep having add/sun chunk storing in database because expiration will remove add/sub chunk directly

2. using promise instead of callback
- Using promise for asynchronous API seems a better idea for code design

3. rename
Blocks: 1272239
Comment on attachment 8754176 [details]
MozReview Request: Bug 1274105 - Refactor classifierHelper.js. r?francois

Hi gcp,
Francouis is busy this week, could you help review this one ? thanks
Attachment #8754176 - Flags: review?(francois) → review?(gpascutto)
Comment on attachment 8754176 [details]
MozReview Request: Bug 1274105 - Refactor classifierHelper.js. r?francois

https://reviewboard.mozilla.org/r/53808/#review51220

::: toolkit/components/url-classifier/tests/mochitest/classifierHelper.js:6
(Diff revision 1)
>  if (typeof(classifierHelper) == "undefined") {
>    var classifierHelper = {};
>  }
>  
>  const CLASSIFIER_COMMON_URL = SimpleTest.getTestFileURL("classifierCommon.js");
> -var classifierCommonScript = SpecialPowers.loadChromeScript(CLASSIFIER_COMMON_URL);
> +var gScript = SpecialPowers.loadChromeScript(CLASSIFIER_COMMON_URL);

Any reason for changing the descriptive name into a non-descriptive one?
Attachment #8754176 - Flags: review?(gpascutto)
(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)
> ::: toolkit/components/url-classifier/tests/mochitest/classifierHelper.js:6
> (Diff revision 1)
> >  if (typeof(classifierHelper) == "undefined") {
> >    var classifierHelper = {};
> >  }
> >  
> >  const CLASSIFIER_COMMON_URL = SimpleTest.getTestFileURL("classifierCommon.js");
> > -var classifierCommonScript = SpecialPowers.loadChromeScript(CLASSIFIER_COMMON_URL);
> > +var gScript = SpecialPowers.loadChromeScript(CLASSIFIER_COMMON_URL);
> 
> Any reason for changing the descriptive name into a non-descriptive one?

When reference DXR and grep |SpecialPowers.loadChromeScript|, i found almost every
one uses script, gscript in this case, so i sync the coding style. If you prefer the descriptive name i will change it back.
Flags: needinfo?(gpascutto)
Comment on attachment 8754176 [details]
MozReview Request: Bug 1274105 - Refactor classifierHelper.js. r?francois

https://reviewboard.mozilla.org/r/53808/#review51432
Attachment #8754176 - Flags: review+
(In reply to Dimi Lee[:dimi][:dlee] from comment #6)

> When reference DXR and grep |SpecialPowers.loadChromeScript|, i found almost
> every one uses script, gscript in this case, so i sync the coding style.

Ok, sounds like a good enough reason to me.
Flags: needinfo?(gpascutto)
update bug description to correct reviewer
Attachment #8754176 - Attachment is obsolete: true
Attachment #8756300 - Flags: review+
(In reply to Dimi Lee[:dimi][:dlee] from comment #1)
> 1. clean up add chunk by using expiration instead of sub chunk
> - This can avoid we keep having add/sun chunk storing in database because
> expiration will remove add/sub chunk directly

You can only do this if you know all servers work that way, though, i.e. if you agree to break SafeBrowsing v2 support entirely.
https://hg.mozilla.org/mozilla-central/rev/6926b3233f9c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1329366
You need to log in before you can comment on or make changes to this bug.