Consolidate the ways that we reference "browser.xul" across the tree

RESOLVED FIXED in Firefox 63

Status

()

P3
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

8 months ago
I'd like to consolidate the ways that we access the main browser document URL so that it'll be easier to track which issues are remaining when migrating it to XHTML.

Here are the ways we currently access the main browser document URL:

- hardcoded "chrome://browser/content/browser.xul"
- hardcoded "chrome://browser/content/" which redirects to "chrome://browser/content/browser.xul"
- Services.prefs.getCharPref("browser.chromeURL") -> "chrome://browser/content/"
- getBrowserURL() from utilityOverlay.js

When checking an existing window we also sometimes use the windowtype attribute:

- document.documentElement.getAttribute("windowtype") == "navigator:browser"
- Services.wm.getMostRecentWindow("navigator:browser")
(Assignee)

Comment 1

8 months ago
A couple of questions:

1) Is there some benefit to referencing "chrome://browser/content/" and relying on the redirect to browser.xul? If we could stop doing that (i.e. change the "browser.chromeURL" pref to the full doc URL) that would let us use the pref in places where we are using the full URL for various checks.
2) Any preferred way to get the URL? I'm leaning towards the pref and removing hardcoded strings. If we're worried about performance from pref lookups I guess we could make a JSM that exposes a getBrowserURL() function instead of in utilityOverlay.js (which isn't available in JSMs).
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dao+bmo)
(In reply to Brian Grinstead [:bgrins] from comment #1)
> A couple of questions:
> 
> 1) Is there some benefit to referencing "chrome://browser/content/" and
> relying on the redirect to browser.xul?

I don't think so.

> If we could stop doing that (i.e.
> change the "browser.chromeURL" pref to the full doc URL) that would let us
> use the pref in places where we are using the full URL for various checks.
> 2) Any preferred way to get the URL? I'm leaning towards the pref and
> removing hardcoded strings. If we're worried about performance from pref
> lookups I guess we could make a JSM that exposes a getBrowserURL() function
> instead of in utilityOverlay.js (which isn't available in JSMs).

I don't think a pref is quite the right tool for what we want to do here. How about a #define exposed in AppConstants.jsm?
Flags: needinfo?(dao+bmo)

Comment 3

8 months ago
(In reply to Dão Gottwald [::dao] from comment #2)
> I don't think a pref is quite the right tool for what we want to do here.
> How about a #define exposed in AppConstants.jsm?

This seems like the right solution to me, assuming this works while we're iterating on making the XHTML thing work. If we need to alter it at runtime or something, we could move `getBrowserURL()` to BrowserUtils, make it rely on a lazy pref getter for now, consolidate on that, and then migrate that to AppConstants once we've switched to XHTML.

comment #0 also talks about the windowtype attribute, but comment #1 has no questions about it... I have always kind of assumed that'd work in HTML with very little work (ie if we set the same attribute). I'd be OK with using documentURI or similar if that makes things easier?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 4

8 months ago
(In reply to :Gijs (he/him) from comment #3)
> comment #0 also talks about the windowtype attribute, but comment #1 has no
> questions about it... I have always kind of assumed that'd work in HTML with
> very little work (ie if we set the same attribute). I'd be OK with using
> documentURI or similar if that makes things easier?

I should have mentioned that - [windowtype] and Services.wm.* works on HTML windows as long as the attrs are set on the root element (as does size/location persistence as of Bug 1453788).
(Assignee)

Comment 5

8 months ago
(In reply to :Gijs (he/him) from comment #3)
> (In reply to Dão Gottwald [::dao] from comment #2)
> > I don't think a pref is quite the right tool for what we want to do here.
> > How about a #define exposed in AppConstants.jsm?

Since AppConstants.jsm is in toolkit/ we'd lose a bit of separation by referring to browser/ URLs directly, although I don't think it would really hurt practically.

> This seems like the right solution to me, assuming this works while we're
> iterating on making the XHTML thing work. If we need to alter it at runtime
> or something, we could move `getBrowserURL()` to BrowserUtils, make it rely
> on a lazy pref getter for now, consolidate on that, and then migrate that to
> AppConstants once we've switched to XHTML.

I don't think there's any use case to alter it at runtime, but I still like this idea as a starting point. I think I'd make it a getter on BrowserUtils (maybe `get browserURL`) instead of a function call.

Comment 6

8 months ago
(In reply to Brian Grinstead [:bgrins] from comment #5)
> (In reply to :Gijs (he/him) from comment #3)
> > (In reply to Dão Gottwald [::dao] from comment #2)
> > > I don't think a pref is quite the right tool for what we want to do here.
> > > How about a #define exposed in AppConstants.jsm?
> 
> Since AppConstants.jsm is in toolkit/ we'd lose a bit of separation by
> referring to browser/ URLs directly, although I don't think it would really
> hurt practically.

Without wanting to speak for Dão, I think what was meant is that you define a build-time config thing (that other toolkit apps could override) that you only expose in AppConstants the same way we expose other stuff.
(Assignee)

Comment 7

8 months ago
(In reply to :Gijs (he/him) from comment #6)
> (In reply to Brian Grinstead [:bgrins] from comment #5)
> > (In reply to :Gijs (he/him) from comment #3)
> > > (In reply to Dão Gottwald [::dao] from comment #2)
> > > > I don't think a pref is quite the right tool for what we want to do here.
> > > > How about a #define exposed in AppConstants.jsm?
> > 
> > Since AppConstants.jsm is in toolkit/ we'd lose a bit of separation by
> > referring to browser/ URLs directly, although I don't think it would really
> > hurt practically.
> 
> Without wanting to speak for Dão, I think what was meant is that you define
> a build-time config thing (that other toolkit apps could override) that you
> only expose in AppConstants the same way we expose other stuff.

Ah, that does make sense. I'll first look into exposing the existing pref value through BrowserUtils to see how hard it is to update all callers, then next look at a build change to expose the URL through the preprocessor instead.
(Assignee)

Updated

8 months ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
(Assignee)

Comment 8

8 months ago
Thinking about this more: if this wasn't already a pref I wouldn't suggest we make it a pref - there's no reason for this to be configured at runtime. That said, I'd slightly prefer to stick with the pref in this bug since it'll be easier to plug into what we already have, and will have the side effect of keeping it easy to prototype changing the URL in an artifact build.

The main downside I can think of doing the build change in a separate bug is more churn (i.e. BrowserUtils.browserURL->AppConstants.browserURL). But if we do the work here to unify how we access it, that would turn into a  relatively simple find/replace.

If there's no objection to doing it that way, I'll file a follow up to define the browser URL at build time.
(Assignee)

Comment 9

8 months ago
Hm, putting this into BrowserUtils causes "browser/base/content/test/performance/browser_startup.js | should have no unexpected modules loaded before profile selection - Got 1, expected 0" on Windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2d69c1e0b4073d5ca8551a378241a2a3b74e9c&selectedJob=188983123. Haven't looked into specifically why, but that may be enough motivation to move this straight into AppConstants.
(Assignee)

Updated

8 months ago
Blocks: 1473165
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

8 months ago
Brian, thanks for the heads-up. How is this relevant to us. Any tip would be appreciated.
(Assignee)

Comment 15

8 months ago
I'm not sure if I've done the build changes right (part 1). Could you please give some feedback? In particular:

1) In AppConstants.jsm I ultimately ended up having to not quote the value (`MOZ_BROWSER_CHROME_URL: @MOZ_BROWSER_CHROME_URL@`) to avoid double quotes around it, while all the others are quoted (`MOZ_WIDGET_TOOLKIT: "@MOZ_WIDGET_TOOLKIT@"`). But if I try to not include quotes in the AC_DEFINE_UNQUOTED call, then the access from c++ causes a build error.
2) If other apps currently define the browser.chromeURL pref [0] (like mobile/), I guess we'll need to make build changes there as well. In that case would that be done in mobile/android/confvars.sh? Or is there a better way to define the value as a default in toolkit/ given that browser/ and mobile/ use the same value?

[0] https://searchfox.org/mozilla-central/search?q=browser.chromeURL&path=
Flags: needinfo?(gps)
(Assignee)

Comment 16

8 months ago
(In reply to Jorg K (GMT+2) from comment #14)
> Brian, thanks for the heads-up. How is this relevant to us. Any tip would be
> appreciated.

What specifically needs to be done depends on the outcome of Comment 15, but ultimately we are planning to switch the  browser.chromeURL pref to AppConstants.MOZ_BROWSER_CHROME_URL (see part 2 for example). So anywhere that sets or accesses the pref value should change - for example: https://searchfox.org/comm-central/search?q=browser.chromeURL&path=. There's probably follow ups that could happen also to migrate anything that references messengercompose.xul to point to the build variable, but that wouldn't be required.
AC_DEFINE and AC_DEFINE_UNQUOTED are used to populate entries in mozilla-config.h. AC_DEFINE gets converted to `#define $1 "$2"` and AC_DEFINE_UNQUOTED is turned into `#define $1 $2`. From there, the C preprocessor has at them. Also, pre-processed files (such as FINAL_TARGET_PP_FILES or EXTRA_PP_JS_FILES (such as AppConstants.jsm) pull in these defines during evaluation.

The example you gave of MOZ_WIDGET_TOOLKIT is actually using a separate method of defining things: set_config() (in moz.configure), which is similar to AC_SUBST in m4-based configure. This is a different mechanism from set_define() / AC_DEFINE[_UNQUOTED]. Defines are intended for C/C++ via mozilla-config.h. Config items are exposed to moz.build files, make files, and can be accessed by individual processes invoked by the build backend. set_config() / AC_SUBST do not imply set_define() / AC_DEFINE[_UNQUOTED] and those variables aren't exposed to the preprocessor unless an explicit call is made. In the case of MOZ_WIDGET_TOOLKIT, toolkit/modules/moz.build is executing a loop that assigns MOZ_WIDGET_TOOLKIT (and other variables) to DEFINES, thus supplementing the available constants for preprocessor invocations in just that directory. The values will be raw / unquoted when evaluated by the preprocessor. That's why quotes are added in e.g. AppConstants.jsm.

Hopefully this information gives you the context you need to make the C++ work. (Hint: you probably want to use AC_DEFINE_UNQUOTED and add the quotes where this value is consumed by the preprocessor. Or you want to declare a quoted and unquoted variation of the variable and consume the appropriate one.)
Flags: needinfo?(gps)
(Assignee)

Comment 18

8 months ago
(In reply to Gregory Szorc [:gps] from comment #17)
> Hopefully this information gives you the context you need to make the C++
> work. (Hint: you probably want to use AC_DEFINE_UNQUOTED and add the quotes
> where this value is consumed by the preprocessor. Or you want to declare a
> quoted and unquoted variation of the variable and consume the appropriate
> one.)

Thank you. Yes that helps - I decided to go for two separate variables instead of quoting it from the C++ file. After spending some time trying to figure out how to properly expand and quote the from the file, it seems easier to do a one-liner and AC_DEFINE_UNQUOTED both variations.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

8 months ago
mozreview-review
Comment on attachment 8993802 [details]
Bug 1476333 - Define the browser chrome URL as BROWSER_CHROME_URL so it can be accessed from AppConstants instead of a pref;

https://reviewboard.mozilla.org/r/258484/#review265900
Attachment #8993802 - Flags: review?(gps) → review+

Comment 22

8 months ago
mozreview-review
Comment on attachment 8993802 [details]
Bug 1476333 - Define the browser chrome URL as BROWSER_CHROME_URL so it can be accessed from AppConstants instead of a pref;

https://reviewboard.mozilla.org/r/258484/#review266012

::: toolkit/modules/AppConstants.jsm:301
(Diff revision 3)
>    MOZ_BING_API_CLIENTID: "@MOZ_BING_API_CLIENTID@",
>    MOZ_BING_API_KEY: "@MOZ_BING_API_KEY@",
>    MOZ_GOOGLE_API_KEY: "@MOZ_GOOGLE_API_KEY@",
>    MOZ_MOZILLA_API_KEY: "@MOZ_MOZILLA_API_KEY@",
>  
> +  MOZ_BROWSER_CHROME_URL: "@MOZ_BROWSER_CHROME_URL@",

I don't care much about the name of the define, but can we get rid of the MOZ_ prefix for the AppConstants property?

Comment 23

8 months ago
mozreview-review
Comment on attachment 8993803 [details]
Bug 1476333 - Refer to AppConstants.BROWSER_CHROME_URL to get the browser URL from the frontend;

https://reviewboard.mozilla.org/r/258486/#review266076

r=me with the rename Dão suggested.

Should we also remove getBrowserURL() ? You removed the pref here but not the helper...

Also, are there places that use getBrowserURL() that can now stop loading utilityOverlay.js (like maybe the hidden window)? (Answer might well be "no", but worth checking IMO.)

::: browser/components/syncedtabs/test/xpcshell/test_TabListComponent.js
(Diff revision 3)
>        PlacesCommandHook: {
>          bookmarkLink() { return Promise.resolve(); }
>        },
>        PlacesUtils: { bookmarksMenuFolderId: "id" }
>      },
> -    getBrowserURL() {},

Shouldn't we add AppConstants onto the window mock, given the component gets the URL from there?
Attachment #8993803 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 24

8 months ago
(In reply to :Gijs (he/him) from comment #23)
> Comment on attachment 8993803 [details]
> Bug 1476333 - Refer to AppConstants.MOZ_BROWSER_CHROME_URL to get the
> browser URL from the frontend;
> 
> https://reviewboard.mozilla.org/r/258486/#review266076
> 
> r=me with the rename Dão suggested.

Going to check and see if we can use BROWSER_CHROME_URL everywhere instead of MOZ_BROWSER_CHROME_URL.

> Should we also remove getBrowserURL() ? You removed the pref here but not
> the helper...

It's already removed from browser/base/content/utilityOverlay.js on this version (https://reviewboard.mozilla.org/r/258486/diff/3/#file8292622).
  
> Also, are there places that use getBrowserURL() that can now stop loading
> utilityOverlay.js (like maybe the hidden window)? (Answer might well be
> "no", but worth checking IMO.)

They all seem to be loaded in / referencing a browser window (https://searchfox.org/mozilla-central/search?q=getBrowserURL%28%29&path=). Given the lack of test coverage on non-browser windows for now, I'm going to assume that we need to keep the same set of scripts loaded in non-browser and browser windows (and that other utilityOverlay functions are used in browser menuitem commands).

> ::: browser/components/syncedtabs/test/xpcshell/test_TabListComponent.js
> (Diff revision 3)
> >        PlacesCommandHook: {
> >          bookmarkLink() { return Promise.resolve(); }
> >        },
> >        PlacesUtils: { bookmarksMenuFolderId: "id" }
> >      },
> > -    getBrowserURL() {},
> 
> Shouldn't we add AppConstants onto the window mock, given the component gets
> the URL from there?

Last I checked the tests passed without it, but I'll confirm and update if needed.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

8 months ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f4181dab3ad8
Define the browser chrome URL as BROWSER_CHROME_URL so it can be accessed from AppConstants instead of a pref;r=gps
https://hg.mozilla.org/integration/autoland/rev/dd386b5b9fa7
Refer to AppConstants.BROWSER_CHROME_URL to get the browser URL from the frontend;r=Gijs
(Assignee)

Updated

8 months ago
Depends on: 1478128

Comment 30

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f4181dab3ad8
https://hg.mozilla.org/mozilla-central/rev/dd386b5b9fa7
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(Assignee)

Updated

7 months ago
Blocks: 1484759
You need to log in before you can comment on or make changes to this bug.