[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071502 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4) *|isTextSelection()|: Synchronize/Improve over Thunderbird. *|setWallpaper()|: While there, I "rewrote" it a little.

Comment on attachment 330093 [details] [diff] [review] (Av1) "Remove strres.js" part >Index: nsContextMenu.js >=================================================================== >+ const buttonPressed = promptService.confirmEx(window, promptTitle, promptMsg, >+ (promptService.BUTTON_TITLE_IS_STRING * promptService.BUTTON_POS_0) + >+ (promptService.BUTTON_TITLE_CANCEL * promptService.BUTTON_POS_1), This line doesn't appear to be lined up with the others. >+ promptConfirmButton, null, null, null, {value: 0}); >+ /** >+ * Determines whether the focused window has selected text, and if so >+ * formats the first 15 characters for the label of the context-searchselect >+ * element according to the searchText string. >+ * @return true if there is selected text, false if not >+ */ >+ isTextSelection : function CM_isTextSelection() { >+ var searchSelectText = this.searchSelected(16); >+ const result = searchSelectText != ""; >+ >+ if (result) { >+ searchSelectText = searchSelectText.toString(); >+ if (searchSelectText.length > 15) >+ searchSelectText = searchSelectText.substr(0, 15) + "..."; >+ >+ // Format "Search for <selection>" string to show in menu. >+ // Use |getStringBundle()| from <contentAreaUtils.js>. >+ searchSelectText = getStringBundle().formatStringFromName( >+ "searchText", [searchSelectText], 1); >+ this.setItemAttr("context-searchselect", "label", searchSelectText); >+ } >+ >+ return result; > }, I would say too much change just for the sake of white space tidy up. Just make the string bundle changes.

(In reply to comment #2) > This line doesn't appear to be lined up with the others. Yes, because it's the same parameter (+), whereas the others are different parameters (,). Do you confirm that you want this aligned too ?

(In reply to comment #3) > (In reply to comment #2) > > This line doesn't appear to be lined up with the others. > > Yes, because it's the same parameter (+), whereas the others are different > parameters (,). > Do you confirm that you want this aligned too ? > Sorry, didn't spot the (+), leave as you did it then.

(In reply to comment #2) > I would say too much change just for the sake of white space tidy up. Just make Actually, it is: 1- the string bundle changes. 2- |result| optimization, and |selection| merge into |searchSelectText|. 3- white space tidy up, and synchronization with Thunderbird. > the string bundle changes. I still think it's better to do the whole function "once for all". Do you confirm that you want 1- only ?

(In reply to comment #5) > (In reply to comment #2) > > I would say too much change just for the sake of white space tidy up. Just make > > Actually, it is: > 1- the string bundle changes. > 2- |result| optimization, and |selection| merge into |searchSelectText|. > 3- white space tidy up, and synchronization with Thunderbird. > > > the string bundle changes. > > I still think it's better to do the whole function "once for all". Yes, do it all, there are more non-white space changes than I initially saw.

Av1, with a split line.

Comment on attachment 330397 [details] [diff] [review] (Av1a) "Remove strres.js" part >Index: nsContextMenu.js >=================================================================== >- //Get selected object and convert it to a string to get >- //selected text. Only use the first 15 chars. >- isTextSelection : function() { >+ isTextSelection : function CM_isTextSelection() { Why CM_isTextSelection added other than this what appears in TB? r=me with question answered.

(In reply to comment #8) > >+ isTextSelection : function CM_isTextSelection() { > Why CM_isTextSelection added other than this what appears in TB? This is the only reason: *It helps to synchronize files :-) *I know: Firefox does not have it (yet)... and synchronizing with Firefox is not the purpose of this bug :-| *Iirc, this (kind of) name is used is error console (reports), instead of "anonymous" :-))

You still need an sr on the patch

Comment on attachment 330397 [details] [diff] [review] (Av1a) "Remove strres.js" part IIRC, Ian prefers I ask for sr too on this one.

Comment on attachment 330397 [details] [diff] [review] (Av1a) "Remove strres.js" part >- var winhooks = Components.classes[ "@mozilla.org/winhooks;1" ]. >- getService(Components.interfaces.nsIWindowsHooks); >- >- winhooks.setImageAsWallpaper(this.target, false); >+ Components.classes["@mozilla.org/winhooks;1"] >+ .getService(Components.interfaces.nsIWindowsHooks) >+ .setImageAsWallpaper(this.target, false); This is due to get switched over to the shell service so it's worth patching. I'm also not terribly keen on your var/const changes. >+ isTextSelection : function CM_isTextSelection() { Why the CM_ prefix? >+ const result = searchSelectText != ""; As we're changing this already, we might as well return false early instead. >+ searchSelectText = searchSelectText.toString(); This is in fact already a string. (It used to be a selection object.)

Av1a, with comment 12 suggestion(s). (In reply to comment #12) > This is due to get switched over to the shell service so it's worth patching. I assume you meant _not_ worth. > >+ isTextSelection : function CM_isTextSelection() { > Why the CM_ prefix? Initials of |nsContextMenu.prototype|. That's how I saw it done elsewhere...

(In reply to comment #13) > (In reply to comment #12) > > This is due to get switched over to the shell service so it's worth patching. > I assume you meant _not_ worth. Oops, yes! > > >+ isTextSelection : function CM_isTextSelection() { > > Why the CM_ prefix? > Initials of |nsContextMenu.prototype|. > That's how I saw it done elsewhere... Well, I don't like it my self. Other review comments to follow...

Hmm, I just noticed that although this stops using strres.js it simply calls the stringbundle service directly although the <stringbundle> element has a neater API (e.g. don't need to pass the array length when formatting a string). I guess that's existing code so it would be covered by a separate bug.

Comment on attachment 333644 [details] [diff] [review] (Av1b) "Remove strres.js" part >- (promptService.BUTTON_TITLE_IS_STRING * promptService.BUTTON_POS_0) + >- (promptService.BUTTON_TITLE_CANCEL * promptService.BUTTON_POS_1), >- promptConfirmButton, null, null, null, {value:0}); >+ (promptService.BUTTON_TITLE_IS_STRING * promptService.BUTTON_POS_0) + >+ (promptService.BUTTON_TITLE_CANCEL * promptService.BUTTON_POS_1), >+ promptConfirmButton, null, null, null, {value: 0}); > > if (buttonPressed != 0) > return; > > var winhooks = Components.classes[ "@mozilla.org/winhooks;1" ]. > getService(Components.interfaces.nsIWindowsHooks); >- Hmm, your "undo" isn't quite right ;-) >+ if (searchSelectText == "") This could be tweaked to if (!searchSelectText) >+ searchSelectText = getStringBundle().formatStringFromName( >+ "searchText", >+ [searchSelectText], 1); Still trying to think of a way to wrap this nicely in only two lines...

(In reply to comment #15) > I guess that's existing code so it would be covered by a separate bug. Moved to bug 445831 comment 2 ;-)

Av1b, with comment 14 and comment 16 suggestion(s), plus an unbitrot/unregression I had missed. (In reply to comment #16) > Hmm, your "undo" isn't quite right ;-) Well, I had left this white space cleanup on purpose ;-< > Still trying to think of a way to wrap this nicely in only two lines... Actually, the answer is in the need to revert to the old code :->

Comment on attachment 333654 [details] [diff] [review] (Av1c) "Remove strres.js" part > var buttonPressed = promptService.confirmEx(window, promptTitle, promptMsg, >- (promptService.BUTTON_TITLE_IS_STRING * promptService.BUTTON_POS_0) + >- (promptService.BUTTON_TITLE_CANCEL * promptService.BUTTON_POS_1), >- promptConfirmButton, null, null, null, {value:0}); >+ (promptService.BUTTON_TITLE_IS_STRING * promptService.BUTTON_POS_0) + >+ (promptService.BUTTON_TITLE_CANCEL * promptService.BUTTON_POS_1), >+ promptConfirmButton, null, null, null, {value: 0}); Actually it is correct for all the lines to line up, and it's not worth making whitespace changes for their own sake (I guess this should have been done when gPromptService was removed but it's too late now.) sr=me with this reverted.

Av1c, with comment 19 suggestion(s).

Comment on attachment 334298 [details] [diff] [review] (Av1d) "Remove strres.js" part [Checkin: Comment 21] Pushed to comm-central, changeset edb1732a88c3

Feedback port Av1d to Thunderbird, to stay as much in sync' as possible.

Comment on attachment 335275 [details] [diff] [review] (Bv1-TB) <nsContextMenu.js> [Superseded by bug 463003] The lower part isn't indented properly... But then again, Thunderbird doesn't have any context-searchselect, and the setting search text part of the code should just be removed from that function. (Preferably in a new bug.) In general it seems very odd to me that that part isn't in it's own function, in seamonkey.