Closed Bug 1495184 Opened Last year Closed Last year

Port bug 1493563 - Adapt to changes in nsIWebProgressListener.onSecurityChange()

Categories

(MailNews Core :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: jorgk, Assigned: jorgk)

References

Details

Attachments

(4 files, 5 obsolete files)

No description provided.
Summary: Port bug 1493563 - nsMsgContentPolicy.cpp:1060:21: error: out-of-line definition of 'OnSecurityChange' does not match any declaration in 'nsMsgContentPolicy' → Port bug 1493563 - Adapt to changes in nsIWebProgressListener.onSecurityChange()
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Comment on attachment 9013073 [details] [diff] [review]
1495184-onSecurityChange.patch - C++ part

Let's split the C++ part out. The JS part also affects gData where we'll need some backward compatible solution. I'll do a trunk version first.
Attachment #9013073 - Attachment description: 1495184-onSecurityChange.patch - WIP - C++ part only → 1495184-onSecurityChange.patch - C++ part
Attachment #9013073 - Flags: review?(acelists)
Straight forward, mostly the call is ignored anyway. MMD won't be happy, but I can't think right now who you would define two callbacks with different parameters. One had hoped that M-C would have inserted the new parameters after the existing ones, but they went for reshuffling :-(
Attachment #9013074 - Flags: review?(makemyday)
Attachment #9013074 - Flags: review?(acelists)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/17f0a541d7ac
Port 1493563: Adapt to API changes in nsIWebProgressListener.onSecurityChange(), C++ part. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/759a21c4e0f9
Port 1493563: Adapt to API changes in nsIWebProgressListener.onSecurityChange(), JS part. rs=bustage-fix
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
Comment on attachment 9013074 [details] [diff] [review]
1495184-onSecurityChange-JS.patch - JS part

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

It's unfortunate that the order of the arguments was changed on the interface. r+ with fixes mentioned below to retain backward compatibility.

::: calendar/providers/gdata/content/browserRequest.js
@@ +33,5 @@
>  
>      onStatusChange: function(aWebProgress, aRequest, aStatus, aMessage) {
>      },
>  
> +    onSecurityChange: function(aWebProgress, aRequest, aOldState, aState, aContentBlockingLogJSON) {

Please make this

function(aWebProgress, aRequest, aOldState, aState=null, aContentBlockingLogJSON=null) {

Since on the new interface only the last argument is optional, you can do a null check on fourth argument and decide based on that whether the third or fourth argument is the current state, which is what is needed here.

let state = arguments[3] === null ? aOldState : aState;

Then, change the later use from aState to state.

Please add a comment that this is done because a not backward compatible interface change and this workaround could be removed once TB minimum version for gadata is raised to or above 64.
Attachment #9013074 - Flags: review?(makemyday) → review+
Attached patch 1495184-follow-up-gdata.patch (obsolete) — Splinter Review
I gave the parameters "neutral" names, but I can use aOldState and aState is you prefer.
Attachment #9013081 - Flags: review?(makemyday)
Comment on attachment 9013073 [details] [diff] [review]
1495184-onSecurityChange.patch - C++ part

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

Thanks
Attachment #9013073 - Flags: review?(acelists) → review+
Comment on attachment 9013073 [details] [diff] [review]
1495184-onSecurityChange.patch - C++ part

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

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +4081,5 @@
>    return NS_OK;
>  }
>  
>  /* void onSecurityChange (in nsIWebProgress aWebProgress, in nsIRequest aRequest, in unsigned long state); */
> +NS_IMETHODIMP nsMsgComposeSendListener::OnSecurityChange(nsIWebProgress *aWebProgress, nsIRequest *aRequest,

Please also update the comment.
Comment on attachment 9013074 [details] [diff] [review]
1495184-onSecurityChange-JS.patch - JS part

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

OK, but there are many more places you do not update, like in .xml file and in tests.
Attachment #9013074 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #10)
> Please also update the comment.
I'll remove it when landing the Calendar follow-up. Doesn't cause bustage right now ;-)

(In reply to :aceman from comment #11)
> OK, but there are many more places you do not update, like in .xml file and
> in tests.
Right.
Attached patch 1495184-follow-up-tests.patch (obsolete) — Splinter Review
Is this all or have I missed more? I don't think any change is necessary in XML files.
Attachment #9013086 - Flags: review?(acelists)
Attached patch 1495184-follow-up-tests.patch (obsolete) — Splinter Review
Sorry, wrong commit message.
Attachment #9013086 - Attachment is obsolete: true
Attachment #9013086 - Flags: review?(acelists)
Attachment #9013087 - Flags: review?(acelists)
(In reply to Jorg K (GMT+2) from comment #13)
> I don't think any change is necessary in XML files.
Sorry, I see it now. Coming.
Is this complete now?
Attachment #9013087 - Attachment is obsolete: true
Attachment #9013087 - Flags: review?(acelists)
Attachment #9013088 - Flags: review?(acelists)
Comment on attachment 9013088 [details] [diff] [review]
1495184-follow-up-tests.patch (v2)

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

Looks better now, thanks.
Attachment #9013088 - Flags: review?(acelists) → review+
Comment on attachment 9013081 [details] [diff] [review]
1495184-follow-up-gdata.patch

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

Thanks, r+wc

::: calendar/providers/gdata/content/browserRequest.js
@@ +38,5 @@
> +    // From mozilla64 the function has five parameters, the third is the old state,
> +    // the fourth one the state.
> +    // Once support for versions below 64 is dropped, this workaround can be removed.
> +    onSecurityChange: function(aWebProgress, aRequest, aState1, aState2 = null, aContentBlockingLogJSON = null) {
> +        let state = arguments[3] === null ? aState1 : aState2;

I prefer the more verbose argument names you used before. And please move the comment into the function before defining state.
Attachment #9013081 - Flags: review?(makemyday) → review+
Sorry to give this another round. I don't think we need to default to null and use arguments[3] here. Arguments not supplied by the caller are simply undefined and that is falsy. So
  let state = aState ? aState : aOldState;
is all that's required. Purists could ask: And what about being passed null as aState in mozilla64? In this case the perfect solution would be:
  let state = typeof aState === "undefined" ? aOldState : aState;

Do you prefer the latter?
Attachment #9013081 - Attachment is obsolete: true
Attachment #9013100 - Flags: review?(makemyday)
Second variation I mentioned.
Attachment #9013110 - Flags: review?(makemyday)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ae62e27f1265
Follow-up: Remove unneeded comments in nsMsgCompose.cpp, fix four tests, update XML files. r=aceman
Even better:
  let state = arguments.length > 3 ? aState : aOldState;
Attachment #9013143 - Flags: review?(makemyday)
Attachment #9013110 - Flags: review?(makemyday) → review+
Attachment #9013100 - Attachment is obsolete: true
Attachment #9013100 - Flags: review?(makemyday)
Attachment #9013143 - Attachment is obsolete: true
Attachment #9013143 - Flags: review?(makemyday)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/93d3584af75b
Follow-up: Make gData's onSecurityChange() backward compatible. r=MakeMyDay DONTBUILD
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.