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

RESOLVED FIXED in seamonkey2.1b2

Status

SeaMonkey
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Steffen Wilberg, Assigned: ewong)

Tracking

Trunk
seamonkey2.1b2

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

7 years ago
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].
(Reporter)

Updated

7 years ago
Depends on: 606289

Updated

7 years ago
Whiteboard: [good first bug]
(Assignee)

Comment 1

7 years ago
Assigning to myself.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
(Assignee)

Comment 2

7 years ago
Created attachment 491176 [details] [diff] [review]
Remove region.dtd from suite/*
Attachment #491176 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 3

7 years ago
Created attachment 491186 [details] [diff] [review]
Remove region.dtd from suite/* (v2)

Fixed a version # bug.
Attachment #491176 - Attachment is obsolete: true
Attachment #491186 - Flags: review?(iann_bugzilla)
Attachment #491176 - Flags: review?(iann_bugzilla)
(Reporter)

Comment 4

7 years ago
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)

Comment 5

7 years ago
> +      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

7 years ago
> 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.

Updated

7 years ago
Summary: follow-up to removal of region.dtd → follow-up to removal of region.dtd [fix about.xhtml, mail start.xhtml etc.]
(Assignee)

Comment 8

7 years ago
Created attachment 491523 [details] [diff] [review]
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v3)
Attachment #491186 - Attachment is obsolete: true
Attachment #491523 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

7 years ago
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") {
(Assignee)

Comment 10

7 years ago
(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!
(Assignee)

Comment 11

7 years ago
Created attachment 491730 [details] [diff] [review]
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v4)
Attachment #491523 - Attachment is obsolete: true
Attachment #491523 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

7 years ago
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.]
(Assignee)

Comment 13

7 years ago
(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.
(Assignee)

Updated

7 years ago
Attachment #491730 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 14

7 years ago
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.
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.]
(Assignee)

Comment 16

7 years ago
(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.
(Assignee)

Comment 17

7 years ago
(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

7 years ago
(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.
(Assignee)

Comment 19

7 years ago
Created attachment 496478 [details] [diff] [review]
Remove region.dtd and fix about.xhtml, mail start.xhtml etc. (v6)
Attachment #494238 - Attachment is obsolete: true
Attachment #496478 - Flags: review?(iann_bugzilla)
Attachment #494238 - Flags: review?(iann_bugzilla)
(Reporter)

Comment 20

7 years ago
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

7 years ago
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+
(Assignee)

Comment 22

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

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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b2
You need to log in before you can comment on or make changes to this bug.