Last Comment Bug 175792 - <page>s and <dialog>s don't need dialogOverlay
: <page>s and <dialog>s don't need dialogOverlay
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Composer (show other bugs)
: Trunk
: All All
: -- minor (vote)
: seamonkey2.1b3
Assigned To: Edmund Wong (:ewong)
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on:
Blocks: 197315
  Show dependency treegraph
 
Reported: 2002-10-21 09:15 PDT by neil@parkwaycc.co.uk
Modified: 2011-03-19 07:04 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Removed instance of dialogOverlay.xul (2.10 KB, patch)
2011-03-15 07:15 PDT, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Removed instance of dialogOverlay.xul (v2) (2.07 KB, patch)
2011-03-15 07:30 PDT, Edmund Wong (:ewong)
iann_bugzilla: review+
Details | Diff | Splinter Review
Removed instances of dialogOverlay.(xul|js) [Checkin: comment 23] (3.43 KB, patch)
2011-03-16 01:05 PDT, Edmund Wong (:ewong)
iann_bugzilla: review+
standard8: review+
mozilla: superreview+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2002-10-21 09:15:21 PDT
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) 2008-06-11 15:02:46 PDT
(Filter "spam" on 'prefs-nobody-20080612'.)
Comment 2 Edmund Wong (:ewong) 2011-03-14 18:46:16 PDT
Is this bug still valid on trunk?
Comment 3 neil@parkwaycc.co.uk 2011-03-15 04:41:48 PDT
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 5 Edmund Wong (:ewong) 2011-03-15 06:43:36 PDT
Taking this for a spin.
Comment 6 Edmund Wong (:ewong) 2011-03-15 07:15:34 PDT
Created attachment 519398 [details] [diff] [review]
Removed instance of dialogOverlay.xul
Comment 7 Edmund Wong (:ewong) 2011-03-15 07:30:16 PDT
Created attachment 519400 [details] [diff] [review]
Removed instance of dialogOverlay.xul (v2)
Comment 8 Jens Hatlak (:InvisibleSmiley) 2011-03-15 07:47:31 PDT
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) 2011-03-15 07:57:00 PDT
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 2011-03-15 07:58:30 PDT
(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) 2011-03-15 08:04:58 PDT
(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) 2011-03-15 08:05:59 PDT
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 2011-03-15 08:21:09 PDT
(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 2011-03-15 08:24:15 PDT
(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 2011-03-15 08:29:45 PDT
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 2011-03-15 09:00:21 PDT
(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) 2011-03-16 01:05:56 PDT
Created attachment 519603 [details] [diff] [review]
Removed instances of dialogOverlay.(xul|js) [Checkin: comment 23]
Comment 18 Philip Chee 2011-03-16 11:14:53 PDT
> 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 2011-03-16 18:58:28 PDT
Comment on attachment 519603 [details] [diff] [review]
Removed instances of dialogOverlay.(xul|js) [Checkin: comment 23]

r=me
Comment 20 Mark Banner (:standard8) 2011-03-17 06:02:46 PDT
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 2011-03-17 16:51:22 PDT
(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 2011-03-18 02:26:18 PDT
> Note that it looks like EdDialogTemplate.xul is unused:
See Comment 13
Comment 23 Jens Hatlak (:InvisibleSmiley) 2011-03-19 07:03:43 PDT
Comment on attachment 519603 [details] [diff] [review]
Removed instances of dialogOverlay.(xul|js) [Checkin: comment 23]

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

Note You need to log in before you can comment on or make changes to this bug.