Port bug 1514340 - Move content blocking notifications off on onSecurityChange

RESOLVED FIXED in Thunderbird 66.0

Status

enhancement
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: darktrojan, Assigned: jorgk)

Tracking

Trunk
Thunderbird 66.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Reporter

Description

4 months ago

We're busted by bug 1514340.

Assignee

Comment 1

4 months ago

C++ part. Like in bug 1495184 we need to fix JS implementations as well.

Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Assignee

Comment 2

4 months ago

All the call sites that need treatment can best be seen here:
https://hg.mozilla.org/comm-central/rev/c454b0a4b6f3

Assignee

Comment 3

4 months ago

OK, the mentioned changeset changed 6 C++ and 15 JS/XML files, so here they are.

I wasn't sure what to do in:
mail/base/content/tabmail.xml
calendar/providers/gdata/content/browserRequest.js
chat/content/browserRequest.js
so the respective peers can comment on this. Note r- doesn't do it for a landed patch, so please state the changes you want, or even better, submit a patch.

Perhaps Ehsan can tell us what all this is about.

Attachment #9038167 - Flags: review?(mkmelin+mozilla)
Attachment #9038167 - Flags: review?(makemyday)
Attachment #9038167 - Flags: review?(florian)
Attachment #9038167 - Flags: feedback?(ehsan)
Assignee

Comment 4

4 months ago
Comment on attachment 9038162 [details] [diff] [review]
1521671-OnContentBlockingEvent.patch - C++ part

This one seems straight forward, it just copies OnSecurityChange() and mostly we're not interested.
Attachment #9038162 - Flags: review?(mkmelin+mozilla)
Attachment #9038162 - Flags: feedback?(ehsan)
Assignee

Updated

4 months ago
Keywords: leave-open

Comment 5

4 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/995656d1fb10
Port bug 1514340: Implement nsIWebProgressListener.onContentBlockingEvent(), C++ part. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/f00434eba47a
Port bug 1514340: Implement nsIWebProgressListener.onContentBlockingEvent(), JS part. rs=bustage-fix
Attachment #9038162 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9038167 [details] [diff] [review]
1521671-OnContentBlockingEvent-JS.patch

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

Looks reasonable to me.
Attachment #9038167 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9038167 [details] [diff] [review]
1521671-OnContentBlockingEvent-JS.patch

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

::: chat/content/browserRequest.js
@@ +105,5 @@
>                                       browser.securityUI.tooltipText);
> +  },
> +
> +  onContentBlockingEvent: function(aWebProgress, aRequest, aEvent)
> +  {

nit: It would have been nicer to match the style of the other methods of the same object, ie.
  onContentBlockingEvent: function(/*in nsIWebProgress*/ aWebProgress,
                                   /*in nsIRequest*/ aRequest,
                                   /*in unsigned long*/ aEvent) {

instead of having "{" on its own line.

This comment also applies to the mail/base/content/browserRequest.js fork of this file.

If you do address this comment in a follow-up commit, it could also be a good opportunity to realign the parameters of the other methods. For some reason https://searchfox.org/comm-central/rev/d886cfea9c0d17b49a9aec342853263fe4a034af/chat/content/browserRequest.js#32-34,49-53,61-62,67-69,73-74 is currently misaligned, but it's correctly aligned in the mail/ version of the file.
Attachment #9038167 - Flags: review?(florian)
Assignee

Comment 8

4 months ago

OK, I'll fix the alignment/comment issues. But is the patch OK otherwise, that is, not doing anything?

Comment 9

4 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6c49bd73b927
Follow-up: Fix parameter alignment/comment in mails's and chat's browserRequest.js. rs=white-space-only,comment-only DONTBUILD

(In reply to Jorg K (GMT+1) from comment #8)

OK, I'll fix the alignment/comment issues. But is the patch OK otherwise, that is, not doing anything?

Yes, r=me for the chat/ part, thanks for fixing the bustage!

Comment 11

4 months ago

FWIW a description of the purpose of this refactoring can be found in https://bugzilla.mozilla.org/show_bug.cgi?id=1514340#c2. Sorry I didn't file a bug on comm-central, I should have realized this will break things there too myself. :-(

I will review your patches here in about 30 minutes or so...

Assignee

Comment 12

4 months ago

Thanks, Ehsan. From the description, the notifications have been split in two. So where before weren't interested in onSecurityChanged, we should be interested in the new event either. But there were some cases were onSecurityChanged did something and now the new callback does nothing. But I'm not sure whether we'll even receive onContentBlockingEvent.

Updated

4 months ago
Attachment #9038162 - Flags: feedback?(ehsan) → feedback+

Comment 13

4 months ago
Comment on attachment 9038167 [details] [diff] [review]
1521671-OnContentBlockingEvent-JS.patch

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

::: mail/base/content/specialTabs.js
@@ +171,5 @@
>        this.mTab.security.removeAttribute("level");
>      }
>    },
> +  onContentBlockingEvent(aWebProgress, aRequest, aEvent) {
> +  },

Don't you need to call onContentBlockingEvent on this.mProgressListener similar to the other notifications here?

::: mail/base/content/tabmail.xml
@@ +1777,5 @@
> +        <body><![CDATA[
> +          let browser = aWebProgress.QueryInterface(Ci.nsIDocShellTreeItem)
> +                                    .sameTypeRootTreeItem
> +                                    .chromeEventHandler;
> +          this._callTabListeners("onContentBlockingEvent", [browser, ...arguments]);

_callTabListeners seems to protect itself against non-existing onContentBlockingEvent methods on listener objects, so this is safe.
Attachment #9038167 - Flags: feedback?(ehsan) → feedback+

Comment 14

4 months ago

(In reply to Jorg K (GMT+1) from comment #12)

Thanks, Ehsan. From the description, the notifications have been split in two. So where before weren't interested in onSecurityChanged, we should be interested in the new event either. But there were some cases were onSecurityChanged did something and now the new callback does nothing. But I'm not sure whether we'll even receive onContentBlockingEvent.

FWIW I looked at the call sites where onSecurityChanged used to do something. Except for the previous comment, in all cases the work being done was only specific to the security related notification not the content blocking ones. Since after the refactoring the onSecurityChanged notification would still be called for the security related notifications, seems like nothing in comm-central specifically needs to care about onContentBlockingEvent.

Assignee

Comment 15

4 months ago

(In reply to :Ehsan Akhgari from comment #13)

Don't you need to call onContentBlockingEvent on this.mProgressListener
similar to the other notifications here?

Thank you very much Ehsan. Indeed, it seems like I forgot this.

Attachment #9038344 - Flags: review?(mkmelin+mozilla)
Attachment #9038344 - Flags: review?(mkmelin+mozilla) → review+

Comment 16

4 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ebda63c9e0b1
Follow-up for specialTabs.js. r=mkmelin
Assignee

Updated

4 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 66.0
Version: unspecified → Trunk

Comment 17

4 months ago
Comment on attachment 9038167 [details] [diff] [review]
1521671-OnContentBlockingEvent-JS.patch

Thanks, looks appropriate.
Attachment #9038167 - Flags: review?(makemyday) → review+
You need to log in before you can comment on or make changes to this bug.