Rename browser.xul to browser.xhtml

RESOLVED FIXED in Firefox 69

Status

()

task
P1
normal
RESOLVED FIXED
26 days ago
3 days ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

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

Firefox Tracking Flags

(firefox69 fixed)

Details

Attachments

(4 attachments, 7 obsolete attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Assignee

Description

26 days ago

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.

Assignee

Updated

26 days ago
Type: defect → task
Assignee

Updated

23 days ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Assignee

Comment 1

19 days ago

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.

Assignee

Comment 2

19 days ago

This also removes the MOZ_BROWSER_XUL build option

Depends on D32918

Assignee

Comment 3

19 days ago

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

Assignee

Comment 4

18 days ago

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.

Flags: needinfo?(standard8)

Updated

18 days ago
Priority: -- → P1

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.

Flags: needinfo?(standard8)
Assignee

Comment 6

17 days ago

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?

Assignee

Comment 7

17 days ago

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

Attachment #9068216 - Attachment description: Bug 1553188 - Part 3 - Update remaining references to browser.xul → Bug 1553188 - Part 4 - Update remaining references to browser.xul
Attachment #9068213 - Attachment is obsolete: true
Assignee

Comment 9

17 days ago

Depends on D33193

Attachment #9068213 - Attachment is obsolete: false
Attachment #9068802 - Attachment description: Bug 1553188 - Part 1 - Remove the ability to use browser.xul → Bug 1553188 - Part 3 - Remove the ability to use browser.xul
Attachment #9068734 - Attachment description: Bug 1553188 - Part 3 - Support eslint for browser.xhtml → Bug 1553188 - Part 4 - Support eslint for browser.xhtml
Attachment #9068216 - Attachment description: Bug 1553188 - Part 4 - Update remaining references to browser.xul → Bug 1553188 - Part 5 - Update remaining references to browser.xul
Attachment #9068215 - Attachment is obsolete: true
Attachment #9068213 - Attachment is obsolete: true
Attachment #9068734 - Attachment is obsolete: true
Attachment #9068802 - Attachment is obsolete: true
Attachment #9068216 - Attachment is obsolete: true
Attachment #9068803 - Attachment is obsolete: true
Assignee

Comment 10

17 days ago

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.

Assignee

Comment 11

17 days ago

Depends on D33204

Assignee

Comment 13

17 days ago

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

Assignee

Comment 14

17 days ago

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

Attachment #9068818 - Attachment description: Bug 1553188 - Part 1 - remove browser.xhtml from hg → Bug 1553188 - Part 1 - Move browser.xul to browser.xhtml
Attachment #9068820 - Attachment description: Bug 1553188 - Part 3 - Remove the ability to use browser.xul → Bug 1553188 - Part 2 - Remove the ability to use browser.xul
Attachment #9068821 - Attachment description: Bug 1553188 - Part 4 - Support eslint for browser.xhtml → Bug 1553188 - Part 3 - Support eslint for browser.xhtml
Attachment #9068822 - Attachment description: Bug 1553188 - Part 5 - Update remaining references to browser.xul → Bug 1553188 - Part 4 - Update remaining references to browser.xul
Attachment #9068819 - Attachment is obsolete: true

Comment 15

16 days ago
Pushed by bgrinstead@mozilla.com:
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

16 days ago
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e893c5dab96
Fix renaming browser.xul to browser.xhtml name change in devtools test. CLOSED TREE

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?

Flags: needinfo?(bgrinstead)

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

Assignee

Comment 20

3 days ago

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.

Flags: needinfo?(bgrinstead)
Assignee

Updated

3 days ago
See Also: → 1559145
You need to log in before you can comment on or make changes to this bug.