441707 - Sourt out virtualfolderproperties include story

Status: RESOLVED WORKSFORME

(Thunderbird :: Mail Window Front End, defect)

Description

[Joey Minta]

Attachment: patch v1

Same story, different dialog.  Don't ask me why people felt the need to include these functions in commandglue.js when they're only called from within the dialog.  I've moved them over in a cleaned up form.  Also, dialogOverlay.xul is obsolete, so we really shouldn't be using it, ever.

Comment 1

[Joey Minta]

Oh, I should note that this patch is going to conflict with the other global cleanup patches (see Bug 440616), but those are blocked by the searchdialog patch, so I'm not holding my breath.

Comment 2

[David :Bienvenu]

Comment on attachment 326603 [details] [diff] [review]
patch v1

I tried editing a virtual folder's search terms and got this error:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "getSearchTermString is not defined" {file: "
chrome://messenger/content/virtualFolderProperties.js" line: 181}]' when calling
 method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XP
C_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************

this used to be in commandglue.js and isn't in virtualFolderProperties.js...

My tree is pretty much clean at this point - perhaps there's an interaction with the other patch in your tree?

Comment 3

[Wayne Mery (:wsmwk)]

david, good time to revisit this?

(In reply to comment #2)
> (From update of attachment 326603 [details] [diff] [review])
> I tried editing a virtual folder's search terms and got this error:
> 
> ************************************************************
> * Call to xpconnect wrapped JSObject produced this error:  *
> [Exception... "'[JavaScript Error: "getSearchTermString is not defined" {file:
> "
> chrome://messenger/content/virtualFolderProperties.js" line: 181}]' when
> calling
>  method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021
> (NS_ERROR_XP
> C_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
> ************************************************************
> 
> this used to be in commandglue.js and isn't in virtualFolderProperties.js...
> 
> My tree is pretty much clean at this point - perhaps there's an interaction
> with the other patch in your tree?

Comment 4

[David :Bienvenu]

I suspect asuth's refactoring makes this moot.

Comment 5

[Wayne Mery (:wsmwk)]

asuth, do you agree?

(In reply to comment #4)
> I suspect asuth's refactoring makes this moot.

Comment 6

[Wayne Mery (:wsmwk)]

nomis, alta, is this something you could pick up?

(In reply to comment #4)
> I suspect asuth's refactoring makes this moot.

davida sez "not yet"

Comment 7

[Wayne Mery (:wsmwk)]

might this cause any real world problems for users?
or our future development?

Comment 8

[:aceman]

I'll try to look at this.

Comment 9

[:aceman]

It looks like the patch is outdated now. The functions CreateVirtualFolder and getSearchTermString to move are nowhere to be seen (in /mail or /mailnews). They are defined nowhere, just called from one test file.

Should I still apply the rest of the changes in the patch? Some of them would be applicable, but I don't know if they make sense.

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Wayne, aceman, my apologies on not responding to this bug when pinged a year ago-ish.

My refactoring moved the logic in question to:
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/virtualFolderWrapper.js

Presumably this sorted out the duplicate copies of the logic too.

This seems to leave some minor dialog related changes, specifically the removal of some includes, the removal of
  doSetOKCancel(onOK, onCancel);
and the offsetting addition of
  ondialogcancel="return onCancel();"
This suggests that jminta was eliminating the redundancy of there also existing an ondialogaccept property.

I do not think we can safely remove those includes given the amount of time that passed without re-evaluating them.  Since virtualFolderProperties.xul is unlikely to ever be a performance bottleneck, I don't think it's worth it unless part of a greater cleanup effort that's done very carefully.

I'm going to WFM this bug since its core goal was addressed, but if anyone disagrees, feel free to reopen.

Comment 11

[:aceman]

Thanks for explanation.