560254 - Use recursive dtd inclusion for private browsing dtd's

Status: RESOLVED WONTFIX

(Firefox :: Private Browsing, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As it can be seen in the example given above, DTD declarations for external entities can be moved directly into the DTD itself, instead of having to declare them inside the XUL file.

The affected file is aboutPrivateBrowsing.dtd

Comment 1

[(no longer active)]

Attachment: Patch (v1)

I also went ahead and removed the neterror.dtd reference which is not used in this page.

Comment 2

[(no longer active)]

BTW, I should mention that this fix is useful for MozMill guys, so that they can make their test suite not depend on the locale of the build.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

I don't understand why this helps them. Can you elaborate?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The problem in aboutprivatebrowsing.dtd are the exernal entities. If browser.dtd and brand.dtd are not specified in the above dtd, the parser will not automatically include and resolve those entities. We would have to collect all dtd's when running this check on our own. It makes it more complex to check for entity values and I think it's even better to have it that way in the product itself.

See also bug 504635 for the helper function for Mozmill which we also want to port to Firefox core code. As talked with Benjamin, it's something which is not existent yet and could help a lot. Even for Mochitests.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

What is mozmill doing that requires loading single dtds arbitrarily? I'm looking for a higher-level description of what you're doing here, really :)

Comment 6

[(no longer active)]

It's trying to grab the strings directly from the dtd files (like we can do with the string bundle service for properties files) so that MozMill tests can also be run on localized builds.

With files such as aboutPrivateBrowsing.dtd, MozMill's simple technique of just grabbing the dtd file won't work, because it uses entities which are linked to from outside that file, so it would need a very complex logic which would possibly include figuring out which files the dtd is being included from, loading those files, figuring out which entities those files import as well, and then grabbing those and parsing them in addition to the dtd file in question.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #5)
> What is mozmill doing that requires loading single dtds arbitrarily? I'm
> looking for a higher-level description of what you're doing here, really :)

A really trivial example are access keys and shortcuts which we have to retrieve from DTD files to synthesize the correct events.

Comment 8

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/7a349c725653

Comment 9

[(no longer active)]

Comment on attachment 440005 [details] [diff] [review]
Patch (v1)

Henrik told me that he needs this patch on 1.9.1 and 1.9.2.  Axel, does this require l10n changes?  If yes, do we have any way of doing that automatically, so that localizers don't have to update their stable branches?

Comment 10

[Benjamin Smedberg]

We are not going to take this patch on the stable branches. There's too much to lose, and not enough to gain.

Comment 11

[(no longer active)]

(In reply to comment #10)
> We are not going to take this patch on the stable branches. There's too much to
> lose, and not enough to gain.

Henrik, what would we actually lose without this patch on branches?

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #11)
> Henrik, what would we actually lose without this patch on branches?

It's fine. We don't really need it on the branches. It's just some extra work we will have to do for those Mozmill tests. But it's good to see it fixed on trunk. Thanks!

Comment 13

[Axel Hecht [:Pike]]

What :bs said, we're not going to take this on stable branches.

This would basically break add-ons compat and would render any language pack used in the wild broken.

Comment 14

[Axel Hecht [:Pike]]

Comment on attachment 440005 [details] [diff] [review]
Patch (v1)

I'm actually not that fond of this coding pattern.

It's adding a good deal of complexity to the localized file, and in most cases we can just work around the problems. Like, just feeding the &brandShortName; definition and xhtml11.dtd to the tests at all times, or search for referenced entities and insert those as empty text.

Comment 15

[Axel Hecht [:Pike]]

FYI, look at https://l10n-stage-sj.mozilla.org/dashboard/compare?run=41929, first section. That innocent "brandDTD" is all I have to report that rather complex structure in the source. It's easier to not spread that pattern than to come up with a new concept in our toolchain to make that less cumbersome.

Comment 16

[(no longer active)]

Hmm, do you have any better suggestions for dtd files which need entities not in brand.dtd/xhtml11.dtd?  (Like the dtd in question in this bug)

Comment 17

[Axel Hecht [:Pike]]

Well, yes, keep the DTD inclusion in the calling file, i.e., aboutPrivateBrowsing.xhtml.

We might get better with l20n at some point, but with DTDs, it's just tough love either way, let's keep the tough love in the code that's not spread out to several dozen localization teams.

Comment 18

[(no longer active)]

Henrik, what do you think?  As far as I'm concerned, we could just back this patch out and WONTFIX this, if you can handle it on your side. :-)

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Given the unforeseen reason which have been pointed out by Axel, yes please do so.

Comment 20

[(no longer active)]

Backed out:

http://hg.mozilla.org/mozilla-central/rev/60cb76eb0ee6