Last Comment Bug 611029 - Move abSelectAddressesDialog.xul to /suite/mailnews
: Move abSelectAddressesDialog.xul to /suite/mailnews
Status: RESOLVED FIXED
[good first bug]
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: General (show other bugs)
: Trunk
: All All
: -- minor (vote)
: seamonkey2.1b2
Assigned To: Edmund Wong (:ewong)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-10 09:03 PST by Jens Hatlak (:InvisibleSmiley)
Modified: 2010-12-14 16:49 PST (History)
2 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Moved abSelectAddressesDialog.(xul|js) and addressbook-panel.xul to suite/ (4.02 KB, patch)
2010-11-24 07:20 PST, Edmund Wong (:ewong)
mnyromyr: review-
Details | Diff | Splinter Review
Moved abSelectAddressesDialog.(xul|js) and addressbook-panel.(xul|js) to suite/ (v2) (3.92 KB, patch)
2010-11-25 07:44 PST, Edmund Wong (:ewong)
mnyromyr: review+
standard8: superreview+
Details | Diff | Splinter Review

Description Jens Hatlak (:InvisibleSmiley) 2010-11-10 09:03:07 PST
/mailnews/addrbook/content/abSelectAddressesDialog.xul is SM-specific, should be moved to /suite/mailnews. Same for abSelectAddressesDialog.js and probably also addressbook-panel.xul.
Comment 1 Edmund Wong (:ewong) 2010-11-22 08:08:53 PST
From my preliminary findings, I'm not sure about moving addressbook-panel.xul 
since it is referenced in http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/content/abResultsPane.js#202.
Comment 2 Jens Hatlak (:InvisibleSmiley) 2010-11-22 14:59:35 PST
(In reply to comment #1)
> From my preliminary findings, I'm not sure about moving addressbook-panel.xul 
> since it is referenced in
> http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/content/abResultsPane.js#202.

And that file includes and uses abSelectAddressesDialog.js:
- DirPaneSelectionChangeMenulist()
- onEnterInSearchBar()

What confuses me is that the file also includes abSelectAddressesDialog.dtd which is only present under suite/, so I wonder whether TB only calls GetSelectedAbCards (in abResultsPane.js) in contexts where no sidebar-box element is present (which would render that code part dead for TB). I guess you'll have to check the four TB occurrences:
http://mxr.mozilla.org/comm-central/ident?i=GetSelectedAbCards
Or ask one of Mnyromyr, Standard8, bienvenu, etc.
Comment 3 Karsten Düsterloh 2010-11-23 15:01:53 PST
abSelectAddressesDialog.js is only used by abSelectAddressesDialog.xul and addressbook-panel.xul, and those two are only used by SM, hence all three can move into /suite.

TB's addressing sidebar in the mail editor is abContactsPanel.xul, which loads abResultsPane.js *on the inside*, hence the check for "sidebar-box" will always fail. Other uses (addressbook main window, address search window) don't even have a "sidebar-box" either.
(And even if, gCurFrame is only defined in SM's sidebarOverlay.js.)

In fact, the code block in question doesn't work in SM as well at the moment, because we don't have the sidebar back in the Messenger yet... 
(No need to touch that here, it's out of scope for this bug.)
Comment 4 Edmund Wong (:ewong) 2010-11-24 07:20:06 PST
Created attachment 492995 [details] [diff] [review]
Moved abSelectAddressesDialog.(xul|js) and addressbook-panel.xul to suite/
Comment 5 Jens Hatlak (:InvisibleSmiley) 2010-11-24 08:59:52 PST
Comment on attachment 492995 [details] [diff] [review]
Moved abSelectAddressesDialog.(xul|js) and addressbook-panel.xul to suite/

I think addressbook-panel.js can be moved, too.

FYI: Once you have r+ from Karsten, you also need to get a review (sr, in this case) from a TB dev (usually Standard8 or bienvenu) since /mailnews is under their control.
Comment 6 Karsten Düsterloh 2010-11-24 11:46:58 PST
Comment on attachment 492995 [details] [diff] [review]
Moved abSelectAddressesDialog.(xul|js) and addressbook-panel.xul to suite/

>diff --git a/mailnews/mailnews.js b/mailnews/mailnews.js
>-// see abCommon.js and abSelectAddressesDialog.js
>+// see abCommon.js
> pref("mailnews.ui.select_addresses_results.version", 1);

Please don't change this.
The pref is only used by (or rather: known to) SM and should be moved by bug 496429 anyway.

(In reply to comment #5)
> I think addressbook-panel.js can be moved, too.

Oh, yes, of course. Please add this as well.
Comment 7 Edmund Wong (:ewong) 2010-11-25 07:44:29 PST
Created attachment 493263 [details] [diff] [review]
Moved abSelectAddressesDialog.(xul|js) and addressbook-panel.(xul|js) to suite/ (v2)
Comment 8 Karsten Düsterloh 2010-11-25 15:17:59 PST
Comment on attachment 493263 [details] [diff] [review]
Moved abSelectAddressesDialog.(xul|js) and addressbook-panel.(xul|js) to suite/ (v2)

Yep, that's it.
Mark, please see comment #3 for reasons why TB isn't affected.
Comment 9 Karsten Düsterloh 2010-12-13 13:42:00 PST
Landed as <http://hg.mozilla.org/comm-central/rev/d1e3e4add0fe>.

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