654720 - Could not read extensions chrome manifest file (972ce4c6-7e08-4474-a285-3208198ce6fd = default theme), SeaMonkey workaround

Status: VERIFIED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Serge Gautherie (:sgautherie)]

Port (TB) bug 654672, as there is no progress in (Toolkit) bug 586610.

Bug 586610 comment 14:
{
Nomis101      2011-02-21 14:28:49 PST

I can do the same for suite as well.
}
That would be nice :-)

Comment 1

[Nomis101]

Attachment: Workaround for SM

I haven't tested this one!

Comment 2

[Nomis101]

I have no idea about the SM review policy, so please choose a reviewer for this patch as you think.

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 530057 [details] [diff] [review]
Workaround for SM

Review of attachment 530057 [details] [diff] [review]:

Untested, but f+ with nits fixed.

::: suite/installer/removed-files.in
@@ +607,5 @@
   extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/
   extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/icon.png
   extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/install.rdf
   extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/preview.png
+  extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/chrome.manifest

Good point! (SM has an '#ifdef MOZ_OMNIJAR' that TB doesn't have.)

::: suite/installer/package-manifest.in
@@ +446,5 @@
 #else
 @BINPATH@/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/icon.png
 @BINPATH@/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/install.rdf
 @BINPATH@/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/preview.png
+@BINPATH@/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/chrome.manifest

Nit: in all files, please insert in alphabetical order.

::: suite/themes/classic/Makefile.in
@@ +50,5 @@
 
 FILES = \
 	$(srcdir)/preview.png \
 	$(srcdir)/icon.png \
+       $(srcdir)/chrome.manifest \

Nit: misaligned. (and modern too.)

Comment 4

[Nomis101]

Attachment: SM Workaround (nits fixed)

So, now everything should be in alphabetic order. :-)

The misalignment thing is strange, it was not misaligned in the file, but in the patch. Now it should be properly aligned, I hope...

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 530095 [details] [diff] [review]
SM Workaround (nits fixed)

Review of attachment 530095 [details] [diff] [review]:

Untested, but f+ with (1st) nits fixed.

::: suite/themes/classic/Makefile.in
@@ +48,5 @@
 
 DEFINES += -DSEAMONKEY_VERSION=$(SEAMONKEY_VERSION)
 
 FILES = \
+       $(srcdir)/chrome.manifest \

Nit: patch should use a tab, not spaces. (modern too.)

@@ +49,5 @@
 DEFINES += -DSEAMONKEY_VERSION=$(SEAMONKEY_VERSION)
 
 FILES = \
+       $(srcdir)/chrome.manifest \
+	$(srcdir)/icon.png \

Nit: I don't mind touching existing line :-) But I'm not sure whether reviewer will or not: you'll see.

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

I think I see this as: "Could not find jar manifest entry 'chrome.manifest'."

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> I think I see this as: "Could not find jar manifest entry 'chrome.manifest'."

[Mozilla/5.0 (Windows NT 5.0; rv:6.0a1) Gecko/20110430 Firefox/6.0a1 SeaMonkey/2.2a1pre] (nightly)

Yeah, error message has probably changed in the meantime.

Comment 9

[[e-mail]]

Comment on attachment 530095 [details] [diff] [review]
SM Workaround (nits fixed)

Doesn't Modern already provide a chrome.manifest?

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)
> Doesn't Modern already provide a chrome.manifest?

Right! SM 2.x [e-mail] does contain a chrome.manifest.
Iirc, I had checked that only Classic (= default) theme was affected when I first noticed this (Core) bug.

Comment 11

[[e-mail]]

Comment on attachment 530095 [details] [diff] [review]
SM Workaround (nits fixed)

> FILES = \
>+       $(srcdir)/chrome.manifest \
>+	$(srcdir)/icon.png \
> 	$(srcdir)/preview.png \
>-	$(srcdir)/icon.png \
> 	$(NULL)
Nit: Please use a tab for consistency with the other lines.

>diff --git a/suite/themes/modern/Makefile.in b/suite/themes/modern/Makefile.in
>diff --git a/suite/themes/modern/chrome.manifest b/suite/themes/modern/chrome.manifest
Please remove the Modern changes. r=me with that fixed.

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Caution: Given that the TB guys backed out their solution (see bug 654672 comment 3) and investigation is pending, this should at least not land on branch yet.

Comment 13

[Nomis101]

Yes, the TB patch was backed out, because of a problem with mac builds. I try to fix the TB patch and after that I make a new SM one.

Comment 14

[Nomis101]

Attachment: New patch

In this patch, I used tabs, I removed the modern change and I included the fix for the TB bustage.

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #14)
> Created attachment 530688 [details] [diff] [review] [review]
> New patch
> 
> In this patch, I used tabs, I removed the modern change and I included the
> fix for the TB bustage.

As stefanh noted on IRC, there's a copy/paste mistake in the last few lines:

+++ b/mail/themes/qute/mail/chrome.manifest
      ^^^^

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

(Actually the whole line is referring to TB, not just what I highlighted.)

Comment 17

[Nomis101]

Attachment: The fix [Checkin: comment 18]

Oops!

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 530727 [details] [diff] [review]
The fix [Checkin: comment 18]

http://hg.mozilla.org/comm-central/rev/e2565adeaf34
http://hg.mozilla.org/releases/comm-2.0/rev/8d5044303cf7

Comment 19

[rsx11m]

Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.2pre) Gecko/20110507 SeaMonkey/2.1pre, no more manifest-related message on startup; thanks Nomis!