1475670 - TrackingProtection.init might be too eager

Status: RESOLVED FIXED

(Firefox :: Protections UI, enhancement, P3)

Description

[Johann Hofmann [:johannh]]

This came from a conversation with Gijs and bgrins on IRC.

TrackingProtection.init() is called once per chrome window in onLoad, but we likely don't need to call it that early.

init() adds some pref observers for updating the control center and main menu UI, which we can probably move to delayedStartup.

It also records two telemetry probes that have a bit unclear definition of "session" in the description, but we might not actually record those on every window open. In any case, doesn't sound like it needs to run on onLoad.

The other main thing it does is cache a bunch of strings and nodes. These should absolutely be made lazy getters, even if we can't move the rest out of onLoad for some reason.

I've actually been adding more functionality into this function very recently and it's not going to get any less, so it's probably a good idea to optimize this.

I can take a look at it.

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Hello Johann,

I'd like to work on this issue. I'm new to Tracking Protection.

(In reply to Johann Hofmann [:johannh] from comment #0)
> This came from a conversation with Gijs and bgrins on IRC.
> 
> TrackingProtection.init() is called once per chrome window in onLoad, but we
> likely don't need to call it that early.

Currently, ContentBlocking calls init for all blockers which tracking protection is one of them.

> init() adds some pref observers for updating the control center and main
> menu UI, which we can probably move to delayedStartup.

Where is |delayedStartup|?

> It also records two telemetry probes that have a bit unclear definition of
> "session" in the description, but we might not actually record those on
> every window open. In any case, doesn't sound like it needs to run on onLoad.

Where is it too?

Comment 2

[Johann Hofmann [:johannh]]

Hey there, sorry for the delayed response, I was out sick last week.

I'm generally happy to let you work on this bug, however, a lot of things happened in the mean time (which is the primary reason why I didn't progress on this) and the code looks much different now (however, some of the old issues are still there).

As you might have noticed, we now have a file called browser-contentblocking.js [0] instead of browser-trackingprotection.js. In our mental model Content Blocking is the headline for a whole collection of blocking mechanisms such as Tracking Protection, FastBlock, etc.

The new culprit we're looking at is no longer called TrackingProtection.init but ContentBlocking.init [1]. ContentBlocking.init is called in the onLoad function of each chrome window [2].

There are two main tasks in this bug:

- Optimizing the code inside ContentBlocking.init:

The main thing to do here is take the DOM elements we query in init() [4] and turn them into lazy getters as can be seen here [5].

- Moving ContentBlocking.init into delayedStartup [3] because there's no need to call it so early.

I hope that answers your questions. Let me know if you need anything else.

[0] https://searchfox.org/mozilla-central/source/browser/base/content/browser-contentblocking.js
[1] https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/browser/base/content/browser-contentblocking.js#250
[2] https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/browser/base/content/browser.js#1355
[3] https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/browser/base/content/browser.js#1445
[4] https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/browser/base/content/browser-contentblocking.js#250-260
[5] https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/browser/base/content/browser-contentblocking.js#185-197

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1- contentBlocking

Thanks a lot for your guidance and I'm sorry for that delay. I'm waiting your review for this patch.

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 9023444 [details] [diff] [review]
v1- contentBlocking

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

This looks great, just two minor things before this can land!

Thanks!

::: browser/base/content/browser-contentblocking.js
@@ +149,5 @@
>  
> +  get content() {
> +    delete this.content;
> +    return this.content =
> +      document.querySelector("#identity-popup-content-blocking-content");

Please replace all document.querySelector calls in the code you added with document.getElementyById, which should be much faster.

::: browser/base/content/browser.js
@@ +1495,4 @@
>      BookmarkingUI.init();
>      BrowserSearch.delayedStartupInit();
>      AutoShowBookmarksToolbar.init();
> +    ContentBlocking.init();

You also need to move ContentBlocking.uninit() (https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/browser/base/content/browser.js#1934) to here: https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/browser/base/content/browser.js#1961

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v2- contentBlocking

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 9023921 [details] [diff] [review]
v2- contentBlocking

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

Sorry, I just noticed that we should also make getters out of those two strings here: https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/browser/base/content/browser-contentblocking.js#224-227

You should put those in the strings object and update the references to them: https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/browser/base/content/browser-contentblocking.js#151-161

The rest looks great, thanks!

Comment 7

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v3- contentBlocking

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 9024025 [details] [diff] [review]
v3- contentBlocking

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

That looks awesome, thanks for bearing with me :)

Please fix up the one nit, upload a new patch (you can just "carry over" the r+) and set the checkin-needed flag on the bug. Let me know if you need help with any of that.

Thanks!

::: browser/base/content/browser-contentblocking.js
@@ +211,5 @@
> +    get disabledTooltipText() {
> +      delete this.disabledTooltipText;
> +      return this.disabledTooltipText =
> +        gNavigatorBundle.getString("trackingProtection.icon.disabledTooltip");
> +    }

nit: you will need to add a dangling comma here

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Hi abdelrhman, it looks like this is almost ready to land - there's just one review comment left. Are you still working on this?

Comment 10

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v4- contentBlocking

Sorry for that delay. This bug is ready to be landed now

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e31237189d9
[mq]: contentBlocking.patch r=johannh

Comment 12

[Raul Gurzau (:RaulG)]

Backed out changeset 3e31237189d9 (bug 1475670) for eslint failure on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/3862bfac0fab6039f4ff11de80a42a82d41918ef

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&revision=3e31237189d9f4d69bc3d01e6097cba1ca13188f

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217429553&repo=mozilla-inbound&lineNumber=273

Log snippet: 

[task 2018-12-17T13:48:01.828Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2018-12-17T13:48:01.828Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-12-17T13:48:01.828Z] 
[task 2018-12-17T13:48:01.828Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-12-17T13:53:27.767Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/browser-contentblocking.js:571:1 | Trailing spaces not allowed. (no-trailing-spaces)
[task 2018-12-17T13:53:27.767Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/browser-contentblocking.js:577:1 | Trailing spaces not allowed. (no-trailing-spaces)
[taskcluster 2018-12-17 13:53:28.090Z] === Task Finished ===
[taskcluster 2018-12-17 13:53:28.091Z] Unsuccessful task run with exit code: 1 completed in 644.572 seconds

Comment 13

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v5- contentBlocking

https://treeherder.mozilla.org/#/jobs?repo=try&revision=965c565de1725e3714167acad3b128148e8cd9c1

Comment 14

[:Gijs (he/him)]

Is this ready to go now? Try looks green to me... If so, can you set the checkin-needed keyword? Thank you!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d00dcf698edd
Optimizing the code inside ContentBlocking. r=johannh

Comment 16

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d00dcf698edd