1204338 - More Refactoring: Followup to Bug 403800 Refactor netError.dtd/netErrorApp.dtd

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Philip Chee]

Bug 403800 didn't actually work. Fortunately nobody made use of the functionality there. This bug is for making it actually work.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Make Bug 403800 do something useful

In order to work the override entity (netErrorApp.dtd) should be at the top of the netError.dtd file. This is because the L10n parser code takes the first matching string and ignores any further matches further down the file.

Comment 2

[Philip Chee]

Attachment: Patch v1.0 Make Bug 403800 do something useful

> In order to work the override entity (netErrorApp.dtd) should be at the top
> of the netError.dtd file. This is because the L10n parser code takes the
> first matching string and ignores any further matches further down the file.

Comment 3

[Philip Chee]

Comment on attachment 8660462 [details] [diff] [review]
Patch v1.0 Make Bug 403800 do something useful

Picking a reviewer at random (from the history of the file)

Comment 4

[Axel Hecht [:Pike]]

Update, I've started looking at this, but I want to have an in-depth look.

Comment 5

[Philip Chee]

(In reply to Axel Hecht [:Pike] from comment #4)
> Update, I've started looking at this, but I want to have an in-depth look.
Ping

Comment 6

[Axel Hecht [:Pike]]

Here's my line of thought that I didn't finish yet:

Why do we put netErrorApp.dtd in netError.dtd and not in netError.xhtml? I'm concerned that the order in the file matters, and I wonder if there's a real use-case for having it in l10n.

Comment 7

[Philip Chee]

Attachment: Patch v2.0 Put netErrorApp.dtd in netError.xhtml

> Why do we put netErrorApp.dtd in netError.dtd and not in netError.xhtml? I'm
> concerned that the order in the file matters, and I wonder if there's a real
> use-case for having it in l10n.
I agree that netErrorApp.dtd should be in netError.xhtml. However even there the order matters and netErrorApp.dtd should preceed netError.dtd. See new patch attached.

Comment 8

[Philip Chee]

Try job https://treeherder.mozilla.org/#/jobs?repo=try&revision=2849bc4e897a&group_state=expanded

Comment 9

[Axel Hecht [:Pike]]

Comment on attachment 8669426 [details] [diff] [review]
Patch v2.0 Put netErrorApp.dtd in netError.xhtml

Review of attachment 8669426 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I like this much better.

I'm wondering, there are a lot of pages importing netError.dtd, https://dxr.mozilla.org/comm-central/search?q=netError.dtd&redirect=false&case=true&limit=63&offset=0.

As you're moving the string into netError.dtd, those shouldn't be hurt, but should they also generally include netErrorApp.dtd to get consistent strings?

Comment 10

[Philip Chee]

> I'm wondering, there are a lot of pages importing netError.dtd,
> https://dxr.mozilla.org/comm-central/search?q=netError.
> dtd&redirect=false&case=true&limit=63&offset=0.
> 
> As you're moving the string into netError.dtd, those shouldn't be hurt, but
> should they also generally include netErrorApp.dtd to get consistent strings?
....................................................................

> mail/locales/jar.mn
> 11 	% override chrome://global/locale/netError.dtd chrome://messenger/locale/netError.dtd
Thunderbird Bug 653386 is what prompted this one. This core patch will make fixing 653386 simpler.

> 	mozilla/browser/locales/jar.mn
> 165 	locale/browser/netError.dtd                (%chrome/overrides/netError.dtd)
> 168 	% override chrome://global/locale/netError.dtd chrome://browser/locale/netError.dtd
Firefox has completely forked netError.*

> 	mozilla/mobile/locales/jar.mn
> 13 	* locale/@AB_CD@/browser/netError.dtd             (%overrides/netError.dtd)
> 14 	% override chrome://global/locale/netError.dtd    chrome://browser/locale/netError.dtd
Ditto for mobile.

> 	suite/common/aboutSessionRestore.xhtml
> 12 	<!ENTITY % netErrorDTD SYSTEM "chrome://global/locale/netError.dtd">
> 48 	<!-- Long Description (Note: See netError.dtd for used XHTML tags) -->
> 	suite/common/certError.xhtml
> 10 	<!ENTITY % netErrorDTD SYSTEM "chrome://global/locale/netError.dtd">
Suite. Currently does not use any overrides and uses the stock standard Core strings. But IanN intends to use netErrorApp.dtd to customize some of our UI.

Various Python files: I have no clue what these do. But the try build didn't show any noticeable problems in that regard.

> 	obj-x86_64-unknown-linux-gnu/dist/bin/chrome/en-US.manifest
> 25 	override chrome://global/locale/netError.dtd chrome://messenger/locale/netError.dtd
I have no idea what this is but it's not in my tree...

Comment 11

[Philip Chee]

Ping?

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 8669426 [details] [diff] [review]
Patch v2.0 Put netErrorApp.dtd in netError.xhtml

Review of attachment 8669426 [details] [diff] [review]:
-----------------------------------------------------------------

r=me sorry for the lag.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b957faece566
https://hg.mozilla.org/integration/mozilla-inbound/rev/97e1e855ec9a

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b957faece566