668307 - Minimum loadURI support for remote content

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

This should come as part of bug 662008, but that bug is triggering too many other bugs which shouldn't necessarily hold the initial remote content support in desktop.

The code that calls and sends this message has already landed as bug 663040, we just need to have a frame script listening for the message on the content side.

Once bug 662008 lands we can remove this listener. I'm already creating the file at the same place that Dao wrote in bug 653065 and bug 652510 to make things easier.

Dao: asking feedback to know if there was any reason that you added the loadFrameScript call in prepareForStartup rather than delayedStartup. Should I move it there?

Comment 1

[Dão Gottwald [:dao]]

Comment on attachment 542905 [details] [diff] [review]
Patch

>@@ -1492,16 +1492,18 @@ function delayedStartup(isLoadingBlank, 
>   BrowserOffline.init();
>   OfflineApps.init();
>   IndexedDBPromptHelper.init();
>   gFormSubmitObserver.init();
>   AddonManager.addAddonListener(AddonsMgrListener);
> 
>   gBrowser.addEventListener("pageshow", function(evt) { setTimeout(pageShowEventHandlers, 0, evt); }, true);
> 
>+  messageManager.loadFrameScript("chrome://browser/content/content.js", true);

Why is this in delayedStartup? This seems wrong to me.

>+let Ci = Components.interfaces;
>+
>+addMessageListener("WebNavigation:LoadURI", function(message) {
>+  let webNav = docShell.QueryInterface(Ci.nsIWebNavigation);
>+  let flags = message.json.flags || webNav.LOAD_FLAGS_NONE;
>+
>+  webNav.loadURI(message.json.uri, flags, null, null, null);
>+});

Can webNavigation be defined in the global scope?

Comment 2

[Dão Gottwald [:dao]]

> Dao: asking feedback to know if there was any reason that you added the
> loadFrameScript call in prepareForStartup rather than delayedStartup. Should
> I move it there?

Err, yes, doing it in delayedStartup just seems to be asking for trouble. People should be able to talk to the message manager as soon as browser.xul is loaded.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

My understanding is that the frame script loading is asynchronous, so there's no guarantee that loading it earlier will ensure that it can communicate earlier. But perhaps delayedStartup is waiting too much.

> Can webNavigation be defined in the global scope?

Yeah, I believe so

Comment 4

[Dão Gottwald [:dao]]

Yeah, it may be asynchronous, but we still want it to be available as soon as possible. Initiating the load in delayedStartup seems fundamentally wrong to me.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch v2

Sounds reasonable

Comment 6

[Marco Bonardo [:mak]]

I backed out everything from central since Android and Maemo were unhappy about the push these changes were part of.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

http://hg.mozilla.org/mozilla-central/rev/0e1c0a79effc
http://hg.mozilla.org/mozilla-central/rev/a447e63943e1

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> http://hg.mozilla.org/mozilla-central/rev/a447e63943e1

?

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Ci was already defined in that context, and it was generating noise on tests in the previous push, so i pushed a quick fix

Comment 10

[Dão Gottwald [:dao]]

I backed this out because of bug 658738 comment 49.

(In reply to comment #9)
> Ci was already defined in that context,

Why was it already defined?

Comment 11

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #10)
> I backed this out because of bug 658738 comment 49.

Was it really necessary to back it out instead of dealing with it like the other bugs from 658738?  is it really leaking on more situations or just cresting more domwindows during the tests that leak? 
the presence of frame script sometimes can create more domwindows, see bug 666753


> Why was it already defined?
i don't know

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> (In reply to comment #10)
> > I backed this out because of bug 658738 comment 49.
> 
> Was it really necessary to back it out instead of dealing with it like the
> other bugs from 658738?

The current process in that bug isn't sustainable. We need to resolve it and disallow regressions.

> is it really leaking on more situations or just
> cresting more domwindows during the tests that leak?

The number of windows remained the same, but additional docshells were kept alive, which seems like a serious bug.

> the presence of frame script sometimes can create more domwindows, see bug
> 666753
> 
> 
> > Why was it already defined?
> i don't know

We need to understand this.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Attachment: V2

Workaround for the docshell leak (filed as bug 671101), fix for the namespace clash in the framescript context.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 545560 [details] [diff] [review]
V2

Dave, the change that we talked about in extensions-content.js (dao already reviewed the browser code)

Can you also review the same change for satchel/formSubmitListener.js or should I ask zpao?

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 545560 [details] [diff] [review]
V2

r=me for all the toolkit parts

Comment 16

[:Felipe Gomes (needinfo for replies!)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b5ea2677daf6

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/b5ea2677daf6