175792 - <page>s and <dialog>s don't need dialogOverlay

Status: RESOLVED FIXED

(SeaMonkey :: Composer, defect)

Description

[[e-mail]]

I noticed that the following files use dialogOverlay - this seems superfluous.
pref-publish.xul
pref-applications-edit.xul
turboDialog.xul

Comment 1

[Serge Gautherie (:sgautherie)]

(Filter "spam" on 'prefs-nobody-20080612'.)

Comment 2

[Edmund Wong (:ewong)]

Is this bug still valid on trunk?

Comment 3

[[e-mail]]

Certainly those files either no longer exist or no longer use dialogOverlay, however there are still a number of files left that do use dialogOverlay and probably shouldn't.

Comment 4

[Philip Chee]

Two remaining uses of dialogOverlay.xul in /editor/ui
http://mxr.mozilla.org/comm-central/source/editor/ui/dialogs/content/EdInsertTOC.xul
http://mxr.mozilla.org/comm-central/source/editor/ui/dialogs/content/EdInsertTOC.xul

Comment 5

[Edmund Wong (:ewong)]

Taking this for a spin.

Comment 6

[Edmund Wong (:ewong)]

Attachment: Removed instance of dialogOverlay.xul

Comment 7

[Edmund Wong (:ewong)]

Attachment: Removed instance of dialogOverlay.xul (v2)

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

FTR: Note that editor/ is under TB control, i.e. you'll need an sr from bienvenu or Standard8 in the end and whoever checks this in will need to check whether the TB tree is open.

Comment 9

[Serge Gautherie (:sgautherie)]

This bug was filed when only SeaMonkey existed.
Edmund, would you care to fix the other c-c (and maybe m-c) occurrences, after fixing SeaMonkey?

Comment 10

[Ian Neal]

(In reply to comment #8)
> FTR: Note that editor/ is under TB control, i.e. you'll need an sr from
> bienvenu or Standard8 in the end and whoever checks this in will need to check
> whether the TB tree is open.

Actually those bits that are shared are under TB control, see http://mxr.mozilla.org/comm-central/source/editor/ui/jar.mn#82 onwards for what is only used by SM.

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #10)
> http://mxr.mozilla.org/comm-central/source/editor/ui/jar.mn#82 onwards for what

(I don't know either...)

Then:
EdDialogTemplate.* isn't referenced anywhere. Maybe it should be used or deleted?
EdInsertTOC.xul is shared.

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 519400 [details] [diff] [review]
Removed instance of dialogOverlay.xul (v2)

I assume dialogOverlay.js needs to be removed too...

Comment 13

[Ian Neal]

(In reply to comment #11)
> (In reply to comment #10)
> > http://mxr.mozilla.org/comm-central/source/editor/ui/jar.mn#82 onwards for what
> 
> (I don't know either...)
> 
> Then:
> EdDialogTemplate.* isn't referenced anywhere. Maybe it should be used or
> deleted?
> EdInsertTOC.xul is shared.

If you look at the EdDialogTemplate.xul file itself, lines 45 and 56 are particularly interesting. Probably need to check with kaze on whether the EdDialogTemplate.* files can be removed but that is outside the scope of this bug.

Comment 14

[Ian Neal]

(In reply to comment #12)
> Comment on attachment 519400 [details] [diff] [review]
> Removed instance of dialogOverlay.xul (v2)
> 
> I assume dialogOverlay.js needs to be removed too...

Correct for this patch.

Comment 15

[Ian Neal]

Comment on attachment 519400 [details] [diff] [review]
Removed instance of dialogOverlay.xul (v2)

EdDialogTemplate.* is used as a template for any EdDialog*.* files, so probably best to keep it updated until kaze says otherwise.

As Serge said, need to remove the dialogOverlay.js lines in both files too i.e.
<script type="application/javascript" src="chrome://global/content/dialogOverlay.js"/>

r=me with that fixed.

You will need an additional r or sr from bienvenu or standard8

Comment 16

[Ian Neal]

(In reply to comment #9)
> This bug was filed when only SeaMonkey existed.
> Edmund, would you care to fix the other c-c (and maybe m-c) occurrences, after
> fixing SeaMonkey?

Looking through:
the two in suite/mailnews (one is still a window with class="dialog") probably need a new bug dependent on bug 197315
the one in suite/browser again is a window with class="dialog" could be included in suite/mailnews bug or a separate one
the ones in calendar probably need their own bug.
the ones in mailnews should be covered in bug 226956
the ones in toolkit (some of which have a window with class="dialog") should have a new bug.

Comment 17

[Edmund Wong (:ewong)]

Attachment: Removed instances of dialogOverlay.(xul|js) [Checkin: comment 23]

Comment 18

[Philip Chee]

> the one in suite/browser again is a window with class="dialog" could be
> included in suite/mailnews bug or a separate one
metadata.xul is blocked by the need for a way to have a dialog with no dialog buttons. See Bug 250609 Comment 0 and Bug 250609 Comment 17

Comment 19

[Ian Neal]

Comment on attachment 519603 [details] [diff] [review]
Removed instances of dialogOverlay.(xul|js) [Checkin: comment 23]

r=me

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 519603 [details] [diff] [review]
Removed instances of dialogOverlay.(xul|js) [Checkin: comment 23]

Note that it looks like EdDialogTemplate.xul is unused:

http://mxr.mozilla.org/comm-central/search?string=EdDialogTemplate&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Comment 21

[Ian Neal]

(In reply to comment #18)
> > the one in suite/browser again is a window with class="dialog" could be
> > included in suite/mailnews bug or a separate one
> metadata.xul is blocked by the need for a way to have a dialog with no dialog
> buttons. See Bug 250609 Comment 0 and Bug 250609 Comment 17

Seems that got fixed somewhere, so now works with buttons="none" attribute. Nothing stopping all those remaining windows being converted to dialogs it seems.

Comment 22

[Philip Chee]

> Note that it looks like EdDialogTemplate.xul is unused:
See Comment 13

Comment 23

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 519603 [details] [diff] [review]
Removed instances of dialogOverlay.(xul|js) [Checkin: comment 23]

http://hg.mozilla.org/comm-central/rev/c5424d36a8b1