Last Comment Bug 668307 - Minimum loadURI support for remote content
: Minimum loadURI support for remote content
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Firefox 8
Assigned To: :Felipe Gomes (needinfo me!)
:
: Dão Gottwald [:dao]
Mentors:
Depends on: 671101
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-29 12:14 PDT by :Felipe Gomes (needinfo me!)
Modified: 2011-07-14 09:31 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (4.64 KB, patch)
2011-06-29 12:14 PDT, :Felipe Gomes (needinfo me!)
dao+bmo: feedback-
Details | Diff | Splinter Review
Patch v2 (4.56 KB, patch)
2011-06-30 05:45 PDT, :Felipe Gomes (needinfo me!)
dao+bmo: review+
Details | Diff | Splinter Review
V2 (4.86 KB, patch)
2011-07-12 18:17 PDT, :Felipe Gomes (needinfo me!)
dao+bmo: review+
dtownsend: review+
Details | Diff | Splinter Review

Description :Felipe Gomes (needinfo me!) 2011-06-29 12:14:34 PDT
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?
Comment 1 Dão Gottwald [:dao] 2011-06-29 23:25:29 PDT
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] 2011-06-29 23:29:33 PDT
> 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 me!) 2011-06-30 05:26:14 PDT
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] 2011-06-30 05:39:43 PDT
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 me!) 2011-06-30 05:45:41 PDT
Created attachment 543110 [details] [diff] [review]
Patch v2

Sounds reasonable
Comment 6 Marco Bonardo [::mak] 2011-07-01 07:46:58 PDT
I backed out everything from central since Android and Maemo were unhappy about the push these changes were part of.
Comment 8 Dão Gottwald [:dao] 2011-07-09 00:06:05 PDT
(In reply to comment #7)
> http://hg.mozilla.org/mozilla-central/rev/a447e63943e1

?
Comment 9 :Felipe Gomes (needinfo me!) 2011-07-09 08:24:40 PDT
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] 2011-07-09 08:35:30 PDT
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 me!) 2011-07-09 13:47:09 PDT
(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] 2011-07-09 18:23:50 PDT
(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 me!) 2011-07-12 18:17:33 PDT
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.
Comment 14 :Felipe Gomes (needinfo me!) 2011-07-13 11:32:46 PDT
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] 2011-07-13 11:50:11 PDT
Comment on attachment 545560 [details] [diff] [review]
V2

r=me for all the toolkit parts
Comment 16 :Felipe Gomes (needinfo me!) 2011-07-13 12:28:44 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/b5ea2677daf6

Note You need to log in before you can comment on or make changes to this bug.