Closed Bug 948238 Opened 6 years ago Closed 6 years ago

Read browser.tabs.remote once at startup and never again

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch browser-tabs-remote (obsolete) — Splinter Review
The goal here is to fix the value of browser.tabs.remote at startup so that changes don't take effect until restart. Right now we cache the value in some places and read it directly in others. That can lead to weird behavior when different browser components don't agree on the value.

One slight problem here is that content processes read the pref when they start up, which may be much later than when the main process starts. However, this will only affect the case where the user turns off the pref and then starts a new content process, which will get browser.tabs.remote=false. It would be nice to fix this, but I don't want to have to do IPC or something, which could also affect b2g.

Eventually we probably won't want to be checking this pref in content processes, but that will be a bit more work.
Attachment #8345058 - Flags: review?(benjamin)
Comment on attachment 8345058 [details] [diff] [review]
browser-tabs-remote

>-          if (Services.prefs.getBoolPref("browser.tabs.remote")) {
>+          let remote = Cc["@mozilla.org/xre/app-info;1"].
>+                       getService(Ci.nsIXULRuntime).
>+                       browserTabsRemote;

For stuff like this where Services is available you can get at nsIXULRuntime via "Services.appinfo", so "Services.appinfo.browserTabsRemote" might be all you need.
> For stuff like this where Services is available you can get at nsIXULRuntime
> via "Services.appinfo", so "Services.appinfo.browserTabsRemote" might be all
> you need.

OK, I can do that.
Comment on attachment 8345058 [details] [diff] [review]
browser-tabs-remote

the Services. thing is correct, we should do that. I don't need to re-review that part.

Making XRE_GetBrowserTabsRemote an exported XRE function is unnecessary. nsXULAppAPI.h is intended for embedders who need access to functions outside of libxul. I'd tell you to put a declaration of mozilla::BrowserTabsRemote() in nsAppRunner.h, but that header is a mess and including it would probably break unified builds. So you have a few options:

* declare that function in a new header in toolkit/xre
* declare that function in a %{C++ block in nsIXULRuntime.idl
* declare that function in some existing header that is safe to use everywhere: probably nsXPCOMPrivate.h is your best bet.

I'd like to re-review that part of this.
Attachment #8345058 - Flags: review?(benjamin) → review-
Attached patch cpp-partSplinter Review
Attachment #8345058 - Attachment is obsolete: true
Attachment #8345433 - Flags: review?(benjamin)
Attachment #8345433 - Flags: review?(benjamin) → review+
Ugh. I also broke ASAN, since it doesn't enable the crashreporter.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa625c970c1
Blocks: 1068189
No longer blocks: 1068189
You need to log in before you can comment on or make changes to this bug.