Refactor netError.dtd to simplify per-application overrides

RESOLVED FIXED

Status

()

Core
General
RESOLVED FIXED
11 years ago
3 years ago

People

(Reporter: johnath, Assigned: Robert Kaiser)

Tracking

(Blocks: 1 bug, {helpwanted})

Trunk
helpwanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
Opened as a result of post-checkin discussion in bug 401575.

netError.dtd is provided by the platform with default strings which applications can pick up as-is, or override to suit their purposes.  Some of those strings are much more likely to be customized than others, though.  This means that an application vendor who wants to, say, override the SSL certificate error strings to provide more detailed override instructions or additional markup is forced to override the whole file.  This means more work for their localizers than is strictly necessary.

DTDs aren't an optimal place to be tweaking functionality in the first place, but they get the job done with the exception of these headaches, so it would be nice to make a modest change here that preserves the current state of things, but gives downstream apps more control.

bz's thought in bug 401575 was that netError.dtd be split into two files, one with the relatively static content, one with the content more likely to be overridden.  Both would be included by netError.xhtml, and app developers could override just the latter of the two (or both, if need be.)
I would be pretty happy to review a patch for this!
Keywords: helpwanted
(Assignee)

Comment 2

11 years ago
Created attachment 289004 [details] [diff] [review]
move securityOverride into a netErrorApp.dtd

As long as the approach is easy like this, I'm happy to do it :)

This patch is moving securityOverride from netError.dtd into a new netErrorApp.dtd that can be overridden without overriding all of netError.dtd :)
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #289004 - Flags: review?(bzbarsky)
Attachment #289004 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 3

11 years ago
Comment on attachment 289004 [details] [diff] [review]
move securityOverride into a netErrorApp.dtd

>+  <!ENTITY % netErrorAppDTD
>+    SYSTEM "chrome://global/locale/netErrorApp.dtd">
>+  %netErrorAppDTD;

How does adding this file and moving the declaration there interact with applications like firefox that already have those entities defined in their netError.dtd override?  Will the override rule?  Or will there be some kind of entity conflict?
(Assignee)

Comment 4

11 years ago
(In reply to comment #3)
> How does adding this file and moving the declaration there interact with
> applications like firefox that already have those entities defined in their
> netError.dtd override?  Will the override rule?  Or will there be some kind of
> entity conflict?

Good catch, we'll have a conflict there unless Firefox (or whoever) overrides both of them. It can just override netErrorApp.dtd with a blank file though.

I should provide a patch that at least doesn't break us. Should the Firefox part just add a blank override (probably using /toolkit/empty-file) or should we also split the override files?

Any other place than /browser where I should add such an override?
(Reporter)

Comment 5

11 years ago
So IIRC, both browser/ and camino/ currently override netError.dtd and would be broken if they weren't updated.  It seems like there are two approaches to consider:

If we want to include both files up front from the XHTML, it seems like the "ideal" way to patch this would be to, for each of those two, add the other override, and move the corresponding entities over in each case (i.e. so that the browser/ and camino/ overrides for netErrorApp.dtd contained their respective "securityOverride" entities.) Then firefox and camino can decide for themselves whether they want to re-org their files and only override netErrorApp at some future point.

The other alternative, would be to include the netErrorApp.dtd from within netError.dtd.  At this point, browser/ and camino/ wouldn't have to change at all because they would be overriding the top-level DTD, and hence have no need for netErrorApp - but other implementors like SeaMonkey could stick to overriding just the app-specific file.  I guess you could get Boris' take on which one he prefers, but the more I think about it, the more I think this is a cleaner way to go.
(Assignee)

Comment 6

11 years ago
Created attachment 290583 [details] [diff] [review]
patch v2: chain netErrorApp.dtd to netError.dtd

OK, here's a patch for the DTD chaining way of this. (Yes, it works!)

The good thing here is that apps overriding the whole netError.dtd aren't broken with this approach, they just can ignore the chained file and include everything in one big override file.

The bad thing with this is that it's a bit fragile wrt L10n: Localizers need to keep the chaining perfectly intact or the whole mechanism for partly overriding stuff breaks down in their language, and it's probably a not-often-tested case to have a bad cert, so they probably won't realize their breakage before a release.
Attachment #289004 - Attachment is obsolete: true
Attachment #290583 - Flags: review?(bzbarsky)
Attachment #290583 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 7

11 years ago
Comment on attachment 290583 [details] [diff] [review]
patch v2: chain netErrorApp.dtd to netError.dtd

Requesting approval. This eases other apps dealing with netError overrides, esp. for the security exception thing, and has no actual impact on Firefox, as it overrides netError.dtd as whole, including what's chained into netErrorApp.dtd
Attachment #290583 - Flags: approval1.9?
(Assignee)

Updated

11 years ago
Blocks: 405862

Updated

11 years ago
Attachment #290583 - Flags: approval1.9? → approval1.9+
(Reporter)

Comment 8

11 years ago
Copying l10n in on this in response to comment 6, in case there are any modifications or suggestions Axel can offer to mitigate the concern, if there is one.
(Assignee)

Comment 9

11 years ago
Checked in. As one can see in the patch, I have added comments for app devs and localizers, so I hope if they read those, everything should be clear, I fear a bit about cases where L10n tools might destroy that chain reference, though. I'll post to mozilla.dev.l10n about that.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 10

11 years ago
I'm not really sure if this was the right thing to do.

It seems that there is no "good candidate" for stuff that an application would want to overwrite or not, at least to me.

This seems to be just working for SeaMonkey and this one addition, I don't see how this would scale for just SeaMonkey, let alone beyond to other apps.
(Assignee)

Comment 11

11 years ago
Actually, I consider it a hack to override any toolkit file in an app, and another hack to put buttons into the netError page via a DTD.
Applying the first hack in the way of overriding the whole netError.dtd just to be able to use the second hack in SeaMonkey is much worse than the also a bit hackish approach we chose here, and I guess that other apps like Songbird also want the button for adding exceptions in the netError page. Better to have them override only that part than all error strings, if we already need to go into all that hackery in the first place.
(Reporter)

Updated

9 years ago
Blocks: 486826

Updated

3 years ago
Blocks: 653386

Updated

3 years ago
Blocks: 1204338
You need to log in before you can comment on or make changes to this bug.