450257 - Move SeaMonkey-specific files in manager/ to comm-central/suite

Status: RESOLVED FIXED

(SeaMonkey :: Security, defect)

Description

[Stefan [:stefanh]]

There are currently a couple of files in security/manager/pki/resources/content that SeaMonkey is the only consumer of. The files are: PageInfoOverlay.xul, PrefOverlay.xul, pref-certs.xul, pref-masterpass.xul, pref-ssl.xul, pref-validation.xul (and the corresponding js/.dtd files). I think it'd be best to move those files over to comm-central, especially now when most of them have to be re-written anyway (prefpane migration). Last time I talked to kaie about this, he was ok with the move.

Comment 1

[Robert Kaiser]

Attachment: create new files in comm-central

This part of the patch creates the new files in comm-central and integrates them into the build. It leaves them in pippki.jar for now, though I believe they should move into comm.jar in the long term - but that's a SeaMonkey-specific problem then. I'm also not sure about the MOZ_PSM ifdefs as we have hardwired security UI without such switches in other places.
pref-validation.dtd only has the stuff needed for the prefpanel here, pref-security.[js|dtd] have been renamed to pref-certs.[js|dtd] as they're actually only used there, and I created a second overlay for the new prefwindow as secPrefsOverlay (like mailPrefsOverlay) as we couldn't use the one file for both old and new due to using the same <treechildren> element ID in both pref trees.

Comment 2

[Robert Kaiser]

Attachment: remove old files from mozilla-central

This part of the patch removes the files from mozilla-central. I spotted that PageInfoOverlay was completely unused so I removed it (like Firefox, we hardwire the security tab into the page info dialog now), and the still-used rest of pref-validation.dtd is renamed to just validation.dtd.

I tested that both Firefox and SeaMonkey work as before after those two patches.

Comment 3

[Robert Kaiser]

Attachment: create new files in comm-central, v1.1

I just realized I hadn't tracked some files in my patch, here's the same patch with those files. Also, I guess Neil should take a look at this on the comm-central side.

Comment 4

[[e-mail]]

Comment on attachment 336356 [details] [diff] [review]
create new files in comm-central, v1.1

>+#ifdef MOZ_PSM
>+  locale/@AB_CD@/pippki/pref-certs.dtd                                      (%chrome/pippki/pref-certs.dtd)
>+  locale/@AB_CD@/pippki/pref-masterpass.dtd                                 (%chrome/pippki/pref-masterpass.dtd)
>+  locale/@AB_CD@/pippki/pref-ssl.dtd                                        (%chrome/pippki/pref-ssl.dtd)
>+  locale/@AB_CD@/pippki/pref-validation.dtd                                 (%chrome/pippki/pref-validation.dtd)
>+  locale/@AB_CD@/pippki/secPrefsOverlay.dtd                                 (%chrome/pippki/secPrefsOverlay.dtd)
>+#endif
I don't think that putting these panels in pippki will work with a libxul
build, although I can't think of anywhere obvious to put them other than
chrome://communicator/content/foo/ where foo is either security or pippki.

My other nit is the name secPrefsOverlay.* - we can manage to spell out
editorPrefsOverlay so why not security (or maybe you would prefer pippki)?

Comment 5

[Robert Kaiser]

(In reply to comment #4)
> I don't think that putting these panels in pippki will work with a libxul
> build, although I can't think of anywhere obvious to put them other than
> chrome://communicator/content/foo/ where foo is either security or pippki.

With libxul it's no problem, but with full XULRunner it is. :)
And I agree we should get them somewhere into comm.jar and probably chrome://communicator/ in the long term, but that should be something we can then fully do on the comm-central side (we even can do hg renames and keep history).

> My other nit is the name secPrefsOverlay.* - we can manage to spell out
> editorPrefsOverlay so why not security (or maybe you would prefer pippki)?

OK, I'll make that securityPrefsOverlay then.

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 336300 [details] [diff] [review]
create new files in comm-central

I guess this patch is obsolete, it looks very similar to your v1.1 patch. removing review request and marking obsolete

Comment 7

[Kai Engert (:KaiE:)]

Feel free to keep MOZ_PSM, if you want.
But I think it's ok to drop it.
PSM is no longer an optional part of the build, it's required.


> create new files in comm-central
> 
> This part of the patch creates the new files in comm-central and integrates
> them into the build. It leaves them in pippki.jar for

I'm surprised this works.
You move the files to suite/locales/en-US/chrome/pippki/

But mozilla/security/manager/pki/resources/jar.mn
still drives packaging pippki.jar,
then later in suite/security/jar.mn
you add more files to the existing jar.

I think this should be avoided.
I think SeaMonkey should have those in an application specific jar file.

Comment 8

[Kai Engert (:KaiE:)]

> pref-validation.dtd only has the stuff needed for the prefpanel here,

You split validation into two separate files.
I wonder if there are any strings that get used in both places,
it would be good to test that both places have all the strings they need.

Just a couple of days ago I reviewed another patch that seemd to touch the same files,
and it appears the changes overlap.


> pref-security.[js|dtd] have been renamed to pref-certs.[js|dtd] as they're
> actually only used there

I don't understand what you mean by "only used", but I don't mind renaming the files.

Comment 9

[Robert Kaiser]

Packaging files into a single jar from multiple jar.mn files is standard practice and used all over our trees.

Anyway, if you feel good about dropping MOZ_PSM, it's probably also best to do away with the overlay and put the listitems into preferences.xul itself. I would have liked to do the move from pippki.jar into comm.jar in a followup, but I can fold it into this work as well, even though it probably will increase the race-condition-like state with pref migration. It just makes me wonder why we need to keep the files in a security/ directory apart from the other pref panels at all, though.

(In reply to comment #8)
> You split validation into two separate files.
> I wonder if there are any strings that get used in both places,
> it would be good to test that both places have all the strings they need.

I actually have verified that, both by code inspection and testing of the actual windows in both Firefox and SeaMonkey.

> > pref-security.[js|dtd] have been renamed to pref-certs.[js|dtd] as they're
> > actually only used there
> 
> I don't understand what you mean by "only used", but I don't mind renaming the
> files.

They're not referenced by any other XUL file than pref-certs.xul.

Comment 10

[Kai Engert (:KaiE:)]

(In reply to comment #9)
> Packaging files into a single jar from multiple jar.mn files is standard
> practice and used all over our trees.

I didn't know that, thanks for clarification.


> Anyway, if you feel good about dropping MOZ_PSM, it's probably also best to do
> away with the overlay and put the listitems into preferences.xul itself. I
> would have liked to do the move from pippki.jar into comm.jar in a followup,

Please continue to follow your original plan, if that is simpler. Doing it in two steps sounds ok to me.

> It just makes me wonder why
> we need to keep the files in a security/ directory apart from the other pref
> panels at all, though.

It would be good to have them separately, so people are reminded that it is a good idea to talk to security people when changing this code. Also, it makes it easier to watch changes. Yes, maybe I'm overly cautious here. Having separate directories can also make it easier to declare code ownership.


> I actually have verified that, both by code inspection and testing of the
> actual windows in both Firefox and SeaMonkey.

Great, thanks a lot for doing so!


> They're not referenced by any other XUL file than pref-certs.xul.

Ah, makes sense now, thanks!

Comment 11

[Robert Kaiser]

Attachment: create new files in comm-central, v2

OK, here's an updated patch implementing my previous comments. Re-requesting review and forwarding sr.

Comment 12

[Kai Engert (:KaiE:)]

Comment on attachment 336529 [details] [diff] [review]
create new files in comm-central, v2

r=kaie

I assume you're moving things, not changing logic.

Comment 13

[Kai Engert (:KaiE:)]

Comment on attachment 336301 [details] [diff] [review]
remove old files from mozilla-central

r=kaie

Assuming all this code gets moved, not deleted, except the one obsolete file you had mentioned.

Comment 14

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/e8fdf415c571 and http://hg.mozilla.org/mozilla-central/rev/80c81a0db7c8