Introduce nsIURIClassifier.getFeatureByName() and get rid of nsIURIClassifier.asyncClassifyLocalWithTables

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P1
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(2 attachments)

There is only 2 places where we use asyncClassifyLocalWithTables:

browser-contentblocking.js and specialpowersAPI.js (for testing only).

We can get rid of this method introducing getFeaturesByName() and createFeatureWithTables().
Attachment #9032371 - Flags: review?(dlee)
Comment on attachment 9032371 [details] [diff] [review]
part 1 - byName

Gijs, can you take a look at the frontend code?
Attachment #9032371 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 9032371 [details] [diff] [review]
part 1 - byName

Review of attachment 9032371 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed

::: browser/base/content/browser-contentblocking.js
@@ +158,2 @@
>      return new Promise(resolve => {
> +      let feature = classifierService.getFeatureByName("tracking-protection");

Nit: pull this out of the `new Promise` bit please.

::: toolkit/components/url-classifier/tests/unit/test_features.js
@@ +1,1 @@
> +add_test(async _ => {

This needs a CC0 license, and "use strict";

@@ +2,5 @@
> +  Services.prefs.setBoolPref("browser.safebrowsing.passwords.enabled", true);
> +
> +  let classifier = Cc["@mozilla.org/url-classifier/dbservice;1"]
> +                     .getService(Ci.nsIURIClassifier);
> +  ok(!!classifier);

Nit: please add a message.

@@ +27,5 @@
> +
> +  let uri = Services.io.newURI("https://example.com");
> +
> +  let results = await new Promise(resolve => {
> +    let feature = classifier.getFeatureByName("tracking-protection");

Nit: move this outside of the new promise, and remove `let`
Attachment #9032371 - Flags: review?(gijskruitbosch+bugs) → review+
Status: NEW → ASSIGNED
Priority: -- → P1
Blocks: 1513046
Attachment #9032372 - Flags: review?(dlee) → review+
Comment on attachment 9032371 [details] [diff] [review]
part 1 - byName

Review of attachment 9032371 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

::: netwerk/url-classifier/UrlClassifierFeatureWithTables.h
@@ +12,5 @@
> +#include "nsString.h"
> +
> +namespace mozilla {
> +
> +class UrlClassifierFeatureWithTables : public nsIUrlClassifierFeature {

Do you think if we use a name like "UrlClassifierFeatureCustomTables" could make it easier
to know that this class is also a feature just like UrlClassifierFeatureTrackingAnnotation.cpp and others?

You can just ignore this comment if you think the current one is better or it doesn't make any difference.
Attachment #9032371 - Flags: review?(dlee) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf02de018ec5
Introduce nsIURIClassifier.getFeatureByName() and nsIURIClassifier.createFeatureWithTables(), r=dimi
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e04d5fd29aa
Get rid of nsIURIClassifier.asyncClassifyLocalWithTables, r=dimi
https://hg.mozilla.org/mozilla-central/rev/cf02de018ec5
https://hg.mozilla.org/mozilla-central/rev/4e04d5fd29aa
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.