612030 - follow-up to removal of region.dtd [fix about.xhtml, mail start.xhtml etc.]

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Steffen Wilberg]

In bug 606289, I removed chrome://global-region/locale/region.dtd. The only thing in that file was vendorURL, which nobody used.

You'll want to remove the reference to region.dtd from
http://mxr.mozilla.org/comm-central/source/suite/mailnews/start.xhtml#45.

You might also want to move app.vendorURL from
http://mxr.mozilla.org/comm-central/source/suite/locales/en-US/chrome/branding/brand.properties#8
to a pref, to override the default www.mozilla.org link from the logo in the about: page, see attachment 486163 [details] [diff] [review].

Comment 1

[Edmund Wong (:ewong)]

Assigning to myself.

Comment 2

[Edmund Wong (:ewong)]

Attachment: Remove region.dtd from suite/*

Comment 3

[Edmund Wong (:ewong)]

Attachment: Remove region.dtd from suite/* (v2)

Fixed a version # bug.

Comment 4

[Steffen Wilberg]

Comment on attachment 491186 [details] [diff] [review]
Remove region.dtd from suite/* (v2)

>+++ b/suite/common/about.xhtml
>+      var versionNum = Components.classes["@mozilla.org/xre/app-info;1"]
>+                       .getService(Components.interfaces.nsIXULAppInfo)
>+                       .version;
That's not right, you do nothing with it...
You also need to port bug 428765.
Or just unfork about.xhtml...

Comment 5

[Philip Chee]

> +      var versionNum = Components.classes["@mozilla.org/xre/app-info;1"]
> +                       .getService(Components.interfaces.nsIXULAppInfo)
> +                       .version;
> +
> +      var version = document.getElementById("version");
> +
> +      version.appendChild(document.createTextNode("&about.version; " + versionNum));

In our about.xhtml the version is already preprocessed:

#expand <p id="version">&about.version; __MOZ_APP_VERSION__</p>

>  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
> -<!ENTITY % regionDTD SYSTEM "chrome://global-region/locale/region.dtd" >
>  <!ENTITY % startDTD SYSTEM "chrome://messenger/locale/start.dtd" >
>  %brandDTD;
> -%regionDTD;
>  %startDTD;

Might as well take the opportunity to rearrange in interleaved format which is the current style.

<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
%brandDTD;
<!ENTITY % startDTD SYSTEM "chrome://messenger/locale/start.dtd" >
%startDTD;

You also don't check for: |if (vendorURL != "about:blank")| here; but you do in about.xhtml.

Comment 6

[Philip Chee]

> You also need to port bug 428765.
Ah right. Didn't see that.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

I guess/hope the following will be fixed through this:
The SeaMonkey MailNews start page is currently broken (XML Parsing Error: entity startpage.title undefined). The affected file (start.xhtml) loads multiple DTDs and the one that contains the entity (start.dtd) is listed after region.dtd so I guess loading is stopped when region.dtd cannot be found.

Comment 8

[Edmund Wong (:ewong)]

Attachment: Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v3)

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 491523 [details] [diff] [review]
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v3)

Drive-by:
>+      if (releaseNotesURL != "about:balnk") {

Comment 10

[Edmund Wong (:ewong)]

(In reply to comment #9)
> Comment on attachment 491523 [details] [diff] [review]
> Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v3)
> 
> Drive-by:
> >+      if (releaseNotesURL != "about:balnk") {

*sigh*   overlooked this one.  I'll get it fixed.  Thanks!

Comment 11

[Edmund Wong (:ewong)]

Attachment: Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v4)

Comment 12

[[e-mail]]

Comment on attachment 491523 [details] [diff] [review]
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v3)

>+      var versionNum = Components.classes["@mozilla.org/xre/app-info;1"]
>+                                 .getService(Components.interfaces.nsIXULAppInfo)
>+                                 .version;
>+
>+      var version = document.getElementById("version");
>+
>+      version.appendChild(document.createTextNode("&about.version; " + versionNum));
[Not that this worked but I think the latest patch displays no version at all.]

Comment 13

[Edmund Wong (:ewong)]

(In reply to comment #12)
> Comment on attachment 491523 [details] [diff] [review]
> Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v3)
> 
> >+      var versionNum = Components.classes["@mozilla.org/xre/app-info;1"]
> >+                                 .getService(Components.interfaces.nsIXULAppInfo)
> >+                                 .version;
> >+
> >+      var version = document.getElementById("version");
> >+
> >+      version.appendChild(document.createTextNode("&about.version; " + versionNum));
> [Not that this worked but I think the latest patch displays no version at all.]

No.  It didn't.  I'm removing the review request until I get this straightened
out.

Comment 14

[Edmund Wong (:ewong)]

Attachment: Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v5)

Added back the version appending code into the script part of about.xhtml.

Comment 15

[[e-mail]]

Comment on attachment 494238 [details] [diff] [review]
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v5)

>-app.vendorURL=http://www.seamonkey-project.org/
[This change looks wrong to me, but the rest of the patch seems fine.]

Comment 16

[Edmund Wong (:ewong)]

(In reply to comment #15)
> Comment on attachment 494238 [details] [diff] [review]
> Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v5)
> 
> >-app.vendorURL=http://www.seamonkey-project.org/
> [This change looks wrong to me, but the rest of the patch seems fine.]

I must've misunderstood.  Comment 1 mentioned removing app.vendorURL from 
brand.properties.  Isn't that because the vendorURL will be referenced
in the pref.js file?  Clarifications appreciated.

Comment 17

[Edmund Wong (:ewong)]

(In reply to comment #16)

> I must've misunderstood.  Comment 1 mentioned removing app.vendorURL from 
> brand.properties.  Isn't that because the vendorURL will be referenced
> in the pref.js file?  Clarifications appreciated.

Er, I meant comment 0.

Comment 18

[Robert Kaiser]

(In reply to comment #16)
> I must've misunderstood.  Comment 1 mentioned removing app.vendorURL from 
> brand.properties.  Isn't that because the vendorURL will be referenced
> in the pref.js file?  Clarifications appreciated.

No, our pref points to that place, it's a "localized pref". Steffen is no SeaMonkey dev, so he probably doesn't know that. We need to leave it in the .properties file.

Comment 19

[Edmund Wong (:ewong)]

Attachment: Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v6)

Comment 20

[Steffen Wilberg]

Right, I somehow managed to overlook that:
http://mxr.mozilla.org/comm-central/source/suite/browser/browser-prefs.js#416
http://mxr.mozilla.org/l10n-central/search?find=%2F&string=app.vendorURL

Well, I said "your might want to" in comment 0. Looks like you might not want...

Comment 21

[Ian Neal]

Comment on attachment 496478 [details] [diff] [review]
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v6)


>+      if (releaseNotesURL != "about:balnk") {
about:blank

r=me with that fixed

Comment 22

[Edmund Wong (:ewong)]

Attachment: Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v7) [Checkin: comment 23]

Fixed "about:blank"

Comment 23

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 498315 [details] [diff] [review]
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v7) [Checkin: comment 23]

http://hg.mozilla.org/comm-central/rev/5ddf0094e105

Please set the checkin-needed keyword for future patches that are ready for check-in. Thanks!