Closed Bug 1493662 Opened 11 months ago Closed 11 months ago

nsIMsgMailNewsUrl.loadURI no longer scriptable after bug 1472087

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(thunderbird63 fixed, thunderbird64 fixed)

RESOLVED FIXED
Thunderbird 64.0
Tracking Status
thunderbird63 --- fixed
thunderbird64 --- fixed

People

(Reporter: neil, Assigned: neil)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

nsIMsgMailNewsUrl.loadURI exists to provide a scriptable way of calling nsIDocShell.loadURI which is needed to preserve the nsIMsgMailNewsUrl being loaded. However bug 1472087 deCOMtaminated nsIDocShellLoadInfo and the bustage fix in 1475387 was only a build fix and didn't restore scriptable access to nsIDocShell.loadURI because it left nsIMsgMailNewsUrl.loadURI in an unscriptable state.
Attached patch Possible patch (obsolete) — Splinter Review
This version is a spot fix that preserves access to the load type, which I believe is the only thing that the load info was being used for.
Attachment #9011444 - Flags: review?(jorgk)
Attached patch Alternative patch (obsolete) — Splinter Review
This more extensive version removes all references to nsDocShellLoadInfo from mailnews, as we can probably get away with only supporting thirteen load types.
Attachment #9011445 - Flags: review?(jorgk)
You'd better ignore the uuid changes (I've only just seen your bug 1492889 comment).
Comment on attachment 9011444 [details] [diff] [review]
Possible patch

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

(In reply to neil@parkwaycc.co.uk from comment #0)
> ... it left nsIMsgMailNewsUrl.loadURI in an unscriptable state.
That happens if you leave the apprentice do the work. At least the patient is still alive unlike in the days when the sorcerers had the rule ;-)

Looks OK to me, obviously breaking it in the first place shows my ignorance :-( - So please once again educate the reviewer.

::: mailnews/base/util/nsMsgMailNewsUrl.cpp
@@ +793,5 @@
>  
> +// EXTRA_LOAD_FLAGS assumes that nsIWebNavigation is in scope...
> +enum {
> +  LOAD_FLAGS_FIRST_LOAD = nsIWebNavigation::LOAD_FLAGS_FIRST_LOAD,
> +  LOAD_FLAGS_ALLOW_POPUPS = nsIWebNavigation::LOAD_FLAGS_ALLOW_POPUPS

These don't appear to be used.

@@ +800,1 @@
>  NS_IMETHODIMP nsMsgMailNewsUrl::LoadURI(nsIDocShell* docShell,

Who calls that? It's not called within TB, is it?
Attachment #9011444 - Flags: review?(jorgk) → review+
Comment on attachment 9011445 [details] [diff] [review]
Alternative patch

I prefer this one since it gets rid of the unneeded loadInfo and simplifies things. Which one do you prefer?

I'm still not sure who calls nsMsgMailNewsUrl::LoadURI(). Some add-on, yes?

I can remove the UUID change when landing.
Attachment #9011445 - Flags: review?(jorgk) → review+
(In reply to Jorg K from comment #4)
> > +// EXTRA_LOAD_FLAGS assumes that nsIWebNavigation is in scope...
> > +enum {
> > +  LOAD_FLAGS_FIRST_LOAD = nsIWebNavigation::LOAD_FLAGS_FIRST_LOAD,
> > +  LOAD_FLAGS_ALLOW_POPUPS = nsIWebNavigation::LOAD_FLAGS_ALLOW_POPUPS
> 
> These don't appear to be used.
EXTRA_LOAD_FLAGS is a macro, and refers directly to those constants. Currently it's only used in nsDocShell.cpp where those constants are already in scope. See https://searchfox.org/mozilla-central/rev/ffe6eaf2f032e58ec3b0650a87df2c62ae4ca441/docshell/base/nsDocShell.cpp#648

> Who calls that? It's not called within TB, is it?
No, this is for use by extensions. (Which is why it needs to be scriptable!)

(In reply to Jorg K from comment #5)
> I prefer this one since it gets rid of the unneeded loadInfo and simplifies
> things. Which one do you prefer?
I guess simpler code is better, right?
Comment on attachment 9011444 [details] [diff] [review]
Possible patch

OK, apparently we're using the other one.
Attachment #9011444 - Attachment is obsolete: true
Comment on attachment 9011445 [details] [diff] [review]
Alternative patch

I can uplift this if you care about TB 63 beta.
Attachment #9011445 - Flags: approval-comm-beta+
Added HG header and fixed some over-long lines. And then ...

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=10423587efbfcd5c6904d43a8ca86de37e7db4d9
Attachment #9011445 - Attachment is obsolete: true
Attachment #9011981 - Flags: review+
Attachment #9011981 - Flags: approval-comm-beta+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d900f4b75757
Bug 1475387 follow-up: Remove use of nsDocShellLoadInfo and make nsIMsgMailNewsUrl.loadURI scriptable again. r=jorgk
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.