The default bug view has changed. See this FAQ.

Minimum loadURI support for remote content

RESOLVED FIXED in Firefox 8

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 8
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 542905 [details] [diff] [review]
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?
Attachment #542905 - Flags: review?(dolske)
Attachment #542905 - Flags: feedback?(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?
> 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.

Updated

6 years ago
Attachment #542905 - Flags: feedback?(dao) → feedback-
(Assignee)

Comment 3

6 years ago
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
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.
(Assignee)

Comment 5

6 years ago
Created attachment 543110 [details] [diff] [review]
Patch v2

Sounds reasonable
Attachment #542905 - Attachment is obsolete: true
Attachment #543110 - Flags: review?(dao)
Attachment #542905 - Flags: review?(dolske)

Updated

6 years ago
Attachment #543110 - Flags: review?(dao) → review+
I backed out everything from central since Android and Maemo were unhappy about the push these changes were part of.
(Assignee)

Comment 7

6 years ago
http://hg.mozilla.org/mozilla-central/rev/0e1c0a79effc
http://hg.mozilla.org/mozilla-central/rev/a447e63943e1
Assignee: nobody → felipc
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
(In reply to comment #7)
> http://hg.mozilla.org/mozilla-central/rev/a447e63943e1

?
(Assignee)

Comment 9

6 years ago
Ci was already defined in that context, and it was generating noise on tests in the previous push, so i pushed a quick fix
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

6 years ago
(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
(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.
(Assignee)

Updated

6 years ago
Depends on: 671101
(Assignee)

Comment 13

6 years ago
Created attachment 545560 [details] [diff] [review]
V2

Workaround for the docshell leak (filed as bug 671101), fix for the namespace clash in the framescript context.
Attachment #543110 - Attachment is obsolete: true
Attachment #545560 - Flags: review?(dao)

Updated

6 years ago
Attachment #545560 - Flags: review?(dao) → review+
(Assignee)

Comment 14

6 years ago
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?
Attachment #545560 - Flags: review?(dtownsend)
Comment on attachment 545560 [details] [diff] [review]
V2

r=me for all the toolkit parts
Attachment #545560 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 16

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/b5ea2677daf6
http://hg.mozilla.org/mozilla-central/rev/b5ea2677daf6
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.