Closed Bug 612030 Opened 14 years ago Closed 14 years ago

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

Categories

(SeaMonkey :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1b2

People

(Reporter: steffen.wilberg, Assigned: ewong)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 6 obsolete files)

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].
Depends on: 606289
Whiteboard: [good first bug]
Assigning to myself.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attached patch Remove region.dtd from suite/* (obsolete) — Splinter Review
Attachment #491176 - Flags: review?(iann_bugzilla)
Fixed a version # bug.
Attachment #491176 - Attachment is obsolete: true
Attachment #491186 - Flags: review?(iann_bugzilla)
Attachment #491176 - Flags: review?(iann_bugzilla)
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...
Attachment #491186 - Flags: review?(iann_bugzilla)
> +      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.
> You also need to port bug 428765.
Ah right. Didn't see that.
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.
Summary: follow-up to removal of region.dtd → follow-up to removal of region.dtd [fix about.xhtml, mail start.xhtml etc.]
Attachment #491186 - Attachment is obsolete: true
Attachment #491523 - Flags: review?(iann_bugzilla)
Attachment #491523 - Attachment description: Remove region.dtd and fix about.xhtml, mail start.xhtml etc. → Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v3)
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") {
(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!
Attachment #491523 - Attachment is obsolete: true
Attachment #491523 - Flags: review?(iann_bugzilla)
Attachment #491730 - Flags: review?(iann_bugzilla)
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.]
(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.
Attachment #491730 - Flags: review?(iann_bugzilla)
Added back the version appending code into the script part of about.xhtml.
Attachment #491730 - Attachment is obsolete: true
Attachment #494238 - Flags: review?(iann_bugzilla)
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.]
(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.
(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.
(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.
Attachment #494238 - Attachment is obsolete: true
Attachment #496478 - Flags: review?(iann_bugzilla)
Attachment #494238 - Flags: review?(iann_bugzilla)
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 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
Attachment #496478 - Flags: review?(iann_bugzilla) → review+
Fixed "about:blank"
Attachment #496478 - Attachment is obsolete: true
Attachment #498315 - Flags: review+
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!
Attachment #498315 - Attachment description: Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v7) → Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v7) [Checkin: comment 23]
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: