Last Comment Bug 612030 - follow-up to removal of region.dtd [fix about.xhtml, mail start.xhtml etc.]
: follow-up to removal of region.dtd [fix about.xhtml, mail start.xhtml etc.]
Status: RESOLVED FIXED
[good first bug]
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b2
Assigned To: Edmund Wong (:ewong)
:
Mentors:
Depends on: 606289
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-13 13:24 PST by Steffen Wilberg
Modified: 2010-12-18 10:59 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Remove region.dtd from suite/* (3.36 KB, patch)
2010-11-17 06:05 PST, Edmund Wong (:ewong)
no flags Details | Diff | Review
Remove region.dtd from suite/* (v2) (3.22 KB, patch)
2010-11-17 07:38 PST, Edmund Wong (:ewong)
no flags Details | Diff | Review
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v3) (4.18 KB, patch)
2010-11-18 07:16 PST, Edmund Wong (:ewong)
no flags Details | Diff | Review
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v4) (3.69 KB, patch)
2010-11-18 17:24 PST, Edmund Wong (:ewong)
no flags Details | Diff | Review
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v5) (4.01 KB, patch)
2010-11-30 17:03 PST, Edmund Wong (:ewong)
no flags Details | Diff | Review
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v6) (3.62 KB, patch)
2010-12-09 05:41 PST, Edmund Wong (:ewong)
iann_bugzilla: review+
Details | Diff | Review
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v7) [Checkin: comment 23] (3.62 KB, patch)
2010-12-17 03:46 PST, Edmund Wong (:ewong)
ewong: review+
Details | Diff | Review

Description Steffen Wilberg 2010-11-13 13:24:15 PST
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) 2010-11-13 20:22:10 PST
Assigning to myself.
Comment 2 Edmund Wong (:ewong) 2010-11-17 06:05:45 PST
Created attachment 491176 [details] [diff] [review]
Remove region.dtd from suite/*
Comment 3 Edmund Wong (:ewong) 2010-11-17 07:38:10 PST
Created attachment 491186 [details] [diff] [review]
Remove region.dtd from suite/* (v2)

Fixed a version # bug.
Comment 4 Steffen Wilberg 2010-11-17 07:47:27 PST
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 2010-11-17 07:54:39 PST
> +      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 2010-11-17 07:56:52 PST
> You also need to port bug 428765.
Ah right. Didn't see that.
Comment 7 Jens Hatlak (:InvisibleSmiley) 2010-11-17 11:02:22 PST
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) 2010-11-18 07:16:49 PST
Created attachment 491523 [details] [diff] [review]
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v3)
Comment 9 Jens Hatlak (:InvisibleSmiley) 2010-11-18 12:55:13 PST
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) 2010-11-18 16:46:18 PST
(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) 2010-11-18 17:24:51 PST
Created attachment 491730 [details] [diff] [review]
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v4)
Comment 12 neil@parkwaycc.co.uk 2010-11-30 12:31:46 PST
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) 2010-11-30 16:52:22 PST
(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) 2010-11-30 17:03:27 PST
Created attachment 494238 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-12-05 10:04:02 PST
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) 2010-12-08 20:12:45 PST
(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) 2010-12-09 04:42:29 PST
(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 (not working on stability any more) 2010-12-09 04:47:58 PST
(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) 2010-12-09 05:41:03 PST
Created attachment 496478 [details] [diff] [review]
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v6)
Comment 20 Steffen Wilberg 2010-12-09 06:23:40 PST
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 2010-12-14 07:34:00 PST
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) 2010-12-17 03:46:56 PST
Created attachment 498315 [details] [diff] [review]
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v7) [Checkin: comment 23]

Fixed "about:blank"
Comment 23 Jens Hatlak (:InvisibleSmiley) 2010-12-18 10:58:58 PST
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!

Note You need to log in before you can comment on or make changes to this bug.