1493662 - nsIMsgMailNewsUrl.loadURI no longer scriptable after bug 1472087

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[[e-mail]]

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.

Comment 1

[[e-mail]]

Attachment: Possible patch

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.

Comment 2

[[e-mail]]

Attachment: Alternative patch

This more extensive version removes all references to nsDocShellLoadInfo from mailnews, as we can probably get away with only supporting thirteen load types.

Comment 3

[[e-mail]]

You'd better ignore the uuid changes (I've only just seen your bug 1492889 comment).

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 9011444 [details] [diff] [review]
Possible patch

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

(In reply to [e-mail] 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?

Comment 5

[Jorg K (CEST = GMT+2)]

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.

Comment 6

[[e-mail]]

(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 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 9011444 [details] [diff] [review]
Possible patch

OK, apparently we're using the other one.

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 9011445 [details] [diff] [review]
Alternative patch

I can uplift this if you care about TB 63 beta.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1493662.diff - patch for landing

Added HG header and fixed some over-long lines. And then ...

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=10423587efbfcd5c6904d43a8ca86de37e7db4d9

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d900f4b75757
Bug 1475387 follow-up: Remove use of nsDocShellLoadInfo and make nsIMsgMailNewsUrl.loadURI scriptable again. r=jorgk

Comment 11

[Jorg K (CEST = GMT+2)]

Beta (TB 63):
https://hg.mozilla.org/releases/comm-beta/rev/2fadf4650af71853ce9f7f7e2ccb80cc92cfbee4