1553188 - Rename browser.xul to browser.xhtml

Status: RESOLVED FIXED

(Firefox :: General, task, P1)

Description

[Brian Grinstead [:bgrins]]

After Bug 1534407, we still have browser.xul in tree (and browser.xhtml preprocesses it into itself). Once we are sure that's stuck, we should go ahead and hg mv browser.xul to browser.xhtml.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1553188 - Part 1 - remove browser.xhtml from hg

We would typically just do:

hg mv browser/base/content/browser.xul browser/base/content/browser.xhtml

But since browser.xhtml is already tracked this would prevent history from
bring preserved from browser.xul. What seems to do the trick is doing this
in two steps where we first do:

hg remove browser/base/content/browser.xhtml

And then separately do the hg mv. This means the browser is broken in between
the two commits, but they are intended to be pushed together and aren't meant
to be bisected.

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1553188 - Part 2 - Rename browser.xul to browser.xhtml

This also removes the MOZ_BROWSER_XUL build option

Depends on D32918

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: Bug 1553188 - Part 5 - Update remaining references to browser.xul

These are generally:

• Code comments to browser.xhtml
• Testcases, assertions that were mostly using browser.xul as a generic chrome URL
• References to the browser.xul path in tree

Depends on D32920

Comment 4

[Brian Grinstead [:bgrins]]

Mark, as far as I can tell our xul handling for eslint is the only file path that we support preprocessing (https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/tools/lint/eslint/eslint-plugin-mozilla/lib/processors/xul.js#45 / https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js#32). This is leading to lint errors when renaming browser.xul to browser.xhtml around the # character. I do see a few existing chrome XHTML paths with preprocessing so I'm guessing those aren't linted or otherwise they would hit the same errors: https://searchfox.org/mozilla-central/search?q=%5C*.*xhtml&regexp=true&path=jar.

What do you think makes sense to do? I was considering renaming that preprocessor to something like preprocessed-xml-document.js and load it for all xhtml+xul.

Comment 5

[Mark Banner (:standard8)]

Looking at the other xhtml files, the preprocessing is not within the script tags. Hence it seems to just get ignored. I suspect eslint-plugin-html is probably just looking for the script tags rather than processing the xhtml fully, but I've not looked to be sure.

Whilst we could add support for preprocessing, I'm a bit concerned on a number of fronts. The xul preprocessor doesn't support automatic fixing, so if we just ported it, then we'd loose that on xhtml files - which I don't really want to do. I'm also not sure if we'd hit some of the multi-processor issues in ESLint that aren't fixed until the new v6 they're working on is released.

In any case, I have other questions about this script section.

Firstly, why do we need inline script in browser.xhtml? Ideally, I think we should be moving away from that (apart from tests maybe). In a world where we move towards es6 modules, we'd ideally have just one module in a script tag per .xhtml/html and everything would spawn from that. Which would make it a lot easier for working out globals (that's going a bit too far for now, but I still think we should be moving scripts out of xhtml/html files where it makes sense).

Secondly, I'm wondering why we need the preprocessing for these scripts in browser.xul/xhtml. Both are objects that aren't initialised nor used until _delayedStartup where we do exactly the same checks, but at run-time. They are also only accessed within _delayedStartup and then manage themselves.

We could load those files in _delayedStartup when actually needed. DevelopmentHelpers could even be made a jsm that is lazily loaded and just passed the document.

I think in this case at least, I'd rather avoid the preprocessing here than trying to get the ESLint support working.

Comment 6

[Brian Grinstead [:bgrins]]

OK, so we don't need to remove all preprocessing, only those that are within inline script tags? And if we can do that (either by not using inline scripts or not using the preprocessor), then we can potentially drop the entire xul.js lint file?

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: Bug 1553188 - Part 4 - Support eslint for browser.xhtml

Since it's not a xul document anymore we can't rely on the xul.js lint preprocessor.
This means we need to remove preprocessor attributes from inline scripts, and tell
lint about the browser window environment.

Depends on D32920

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: Bug 1553188 - Part 3 - Remove the ability to use browser.xul

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: Bug 1553188 - Part 2 - hg mv browser.xul to browser.xhtml

Depends on D33193

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: Bug 1553188 - Part 1 - Move browser.xul to browser.xhtml

We would typically just do:

hg mv browser/base/content/browser.xul browser/base/content/browser.xhtml

But since browser.xhtml is already tracked this would prevent history from
bring preserved from browser.xul. What seems to do the trick is doing this
in two steps where we first do:

hg remove browser/base/content/browser.xhtml

And then separately do the hg mv. This means the browser is broken in between
the two commits, but they are intended to be pushed together and aren't meant
to be bisected.

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: Bug 1553188 - Part 2 - Rename browser.xul to browser.xhtml

Depends on D33204

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: Bug 1553188 - Part 2 - Remove the ability to use browser.xul

Depends on D33205

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: Bug 1553188 - Part 3 - Support eslint for browser.xhtml

Since it's not a xul document anymore we can't rely on the xul.js lint preprocessor.
This means we need to remove preprocessor attributes from inline scripts, and tell
lint about the browser window environment.

Depends on D33206

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: Bug 1553188 - Part 4 - Update remaining references to browser.xul

These are generally:

• Code comments to browser.xhtml
• Testcases, assertions that were mostly using browser.xul as a generic chrome URL
• References to the browser.xul path in tree

Depends on D33207

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef4ae83cd3f7
Part 1 - Move browser.xul to browser.xhtml;r=bdahl
https://hg.mozilla.org/integration/mozilla-inbound/rev/084374e8dade
Part 2 - Remove the ability to use browser.xul;r=bdahl
https://hg.mozilla.org/integration/mozilla-inbound/rev/36bbf3fd6b6c
Part 3 - Support eslint for browser.xhtml;r=Standard8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1adf382a3e65
Part 4 - Update remaining references to browser.xul;r=bdahl

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e893c5dab96
Fix renaming browser.xul to browser.xhtml name change in devtools test. CLOSED TREE

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/ef4ae83cd3f7
https://hg.mozilla.org/mozilla-central/rev/084374e8dade
https://hg.mozilla.org/mozilla-central/rev/36bbf3fd6b6c
https://hg.mozilla.org/mozilla-central/rev/1adf382a3e65
https://hg.mozilla.org/mozilla-central/rev/7e893c5dab96

Comment 18

[Dão Gottwald [:dao]]

https://phabricator.services.mozilla.com/D33204#982475

The diff on phab looks bad because it didn't recognize the rename in the patch. I will push this to inbound so the history gets preserved.

It looks like this didn't work at all? *sigh* Is there any way we can restore blame for this file?

Comment 19

[Mark Banner (:standard8)]

The first changeset might need adding to .hg-annotate-ignore-revs / .git-blame-ignore-revs, though I don't know if searchfox respects those yet.

The revision history is there - see the base link on the second to last changeset here: https://hg.mozilla.org/mozilla-central/log/tip/browser/base/content/browser.xhtml

Comment 20

[Brian Grinstead [:bgrins]]

The blame does exist - see https://hg.mozilla.org/mozilla-central/annotate/tip/browser/base/content/browser.xhtml. Searchfox isn't show it though. I put more details in https://groups.google.com/d/msg/firefox-dev/7geUpMu5Hxk/5VJhoentCAAJ. I'll file a bug on searchfox.