Closed Bug 1514697 Opened 11 months ago Closed 10 months ago

Lazy loading for URL-Classifier features

Categories

(Toolkit :: Safe Browsing, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch feature_lazy.patch (obsolete) — Splinter Review
Attachment #9031812 - Flags: review?(dlee)
Priority: -- → P1
Depends on: 1511436
Comment on attachment 9031812 [details] [diff] [review]
feature_lazy.patch

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

Hi Baku,
Do we want to check preference before initializing Features in this patch?

tp : "privacy.trackingprotection.enabled" and "privacy.trackingprotection.pbmode.enabled"
tp annotate: "privacy.trackingprotection.annotate_channels"
flash: "plugins.flashBlock.enabled"
(In reply to Dimi Lee[:dimi][:dlee] from comment #2)
> 
> Hi Baku,
> Do we want to check preference before initializing Features in this patch?

If we want to do this, I'll suggest include the changes in this patch, otherwise, we will have to wait until 2019 :)
> Do we want to check preference before initializing Features in this patch?

This is already done.

UrlClassifierFeatureFlash::MaybeCreate checks StaticPrefs::plugins_flashBlock_enabled()

UrlClassifierFeatureTrackingAnnotation::MaybeCreate checks StaticPrefs::privacy_trackingprotection_annotate_channels()

UrlClassifierFeatureTrackingProtection::MaybeCreate checks loadContext->UseTrackingProtection(), which calls nsDocShell::GetUseTrackingProtection
Flags: needinfo?(dlee)
Now I see! This is the wrong patch. I already applied these changes.
Attachment #9031812 - Attachment is obsolete: true
Attachment #9031812 - Flags: review?(dlee)
Attachment #9032375 - Flags: review?(dlee)
Flags: needinfo?(dlee)
Comment on attachment 9032375 [details] [diff] [review]
feature_lazy.patch

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

Looks good!

::: netwerk/url-classifier/UrlClassifierFeatureFlash.cpp
@@ +48,5 @@
>            EmptyCString())  // aPrefSkipHosts
>        ,
>        mDenying(sFlashFeaturesMap[aId].mDenying) {}
>  
> +/* static */ void UrlClassifierFeatureFlash::MaybeInitialize() {

Check XRE_IsParentProcess() here and other places?

@@ +106,5 @@
>  
>    bool is3rdParty =
>        nsContentUtils::IsThirdPartyWindowOrChannel(nullptr, aChannel, nullptr);
>  
> +  MaybeInitialize();

Put this before bool is3rdParty =

@@ +120,5 @@
>  }
>  
>  /* static */ already_AddRefed<nsIUrlClassifierFeature>
>  UrlClassifierFeatureFlash::GetIfNameMatches(const nsACString& aName) {
> +  MaybeInitialize();

MOZ_ASSERT(sFlashFeaturesMap[0].mFeature) to sync coding style with other features
If we only init features for the parent process then we should just null return after MaybeInitial
Attachment #9032375 - Flags: review?(dlee) → review+
Blocks: 1513046
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfd83fc53601
Lazy loading for URL-Classifier features, r=dimi
https://hg.mozilla.org/mozilla-central/rev/dfd83fc53601
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.