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

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: philip.chee, Assigned: philip.chee)

Tracking

(Blocks 1 bug)

34 Branch
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

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

Updated

4 years ago
Summary: Refactor more. Followup to Bug 403800 Refactor netError.dtd/netErrorApp.dtd → More Refactoring: Followup to Bug 403800 Refactor netError.dtd/netErrorApp.dtd
Assignee

Comment 1

4 years ago
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.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Assignee

Comment 2

4 years ago
> 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.
Attachment #8660458 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Blocks: 653386
Assignee

Comment 3

4 years ago
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)
Attachment #8660462 - Flags: review?(l10n)
Update, I've started looking at this, but I want to have an in-depth look.
Assignee

Comment 5

4 years ago
(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
Flags: needinfo?(l10n)
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.
Flags: needinfo?(l10n)
Assignee

Comment 7

4 years ago
> 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.
Attachment #8660462 - Attachment is obsolete: true
Attachment #8660462 - Flags: review?(l10n)
Attachment #8669426 - Flags: review?(l10n)
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?
Assignee

Comment 10

4 years ago
> 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...
Assignee

Comment 11

4 years ago
Ping?
Flags: needinfo?(l10n)
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.
Attachment #8669426 - Flags: review?(l10n) → review+

Updated

4 years ago
Flags: needinfo?(l10n)

Comment 14

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b957faece566
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.