Closed Bug 1204338 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: General, defect)

34 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

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

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Bug 403800 didn't actually work. Fortunately nobody made use of the functionality there. This bug is for making it actually work.
Summary: Refactor more. Followup to Bug 403800 Refactor netError.dtd/netErrorApp.dtd → More Refactoring: Followup to Bug 403800 Refactor netError.dtd/netErrorApp.dtd
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
> 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
Blocks: 653386
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.
(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)
> 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?
> 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...
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+
Flags: needinfo?(l10n)
https://hg.mozilla.org/mozilla-central/rev/b957faece566
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.