Steps to reproduce: 1. Open two or three pages 2. Open some page in new window and close it 3. Point History -> Recently Closed Windows 4. Move mouse upward on History menu Actual result: Page list on History menu is disappeared. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090616 Minefield/3.6a1pre

regression from bug 324430?

ugh, yes, i know what happens, we serve popupshowing and popuphidden for submenus. Trunk only, thanks!

we should not serve events for submenus, there is no reason really. I also moved the sessionstore getter in browser.js since is not used by Places code and does not need to stay in browser-places.js. I don't think a test is needed in this case, it's a specific menu and case, and issues are immediately visible by any user. I fear a test would be too specific about code implementation.

(In reply to comment #3) > Created an attachment (id=383578) [details] > patch v1.0 > > we should not serve events for submenus, there is no reason really. > I also moved the sessionstore getter in browser.js since is not used by Places > code and does not need to stay in browser-places.js. > > I don't think a test is needed in this case, it's a specific menu and case, and > issues are immediately visible by any user. I fear a test would be too specific > about code implementation. I tried this patch. It seems to be OK for me. Thanks.

(In reply to comment #3) > I also moved the sessionstore getter in browser.js since is not used by Places > code and does not need to stay in browser-places.js. This is probably slower, and doesn't make a lot of sense to me, as browser-places.js is a component part of browser.js.

In fact, it looks like toggleRecentlyClosedTabs, populateUndoSubmenu, toggleRecentlyClosedWindows and populateUndoWindowSubmenu shpould be in browser-places.js...

why should it be slower? also i think that has been done for code separation, session restore code is not part of Places, even if that file is a component part of it.

(In reply to comment #9) > why should it be slower? Because it penetrates the object post-creation. > also i think that has been done for code separation, session restore code is > not part of Places, even if that file is a component part of it. I think it was done for misguided reasons. Strictly speaking, none of browser-places.js is part of Places -- it's rather brwoser-specific Places hooks. There's no reason why nsISessionStore shouldn't be used in browser-places.js. Also note that toggleRecentlyClosedTabs and toggleRecentlyClosedWindows are called only from browser-places.js. The way this is organized right now is quite weird. If you really want that code separated, it should probably be two distinct objects.

Well, the slowdown we're talking about is probably by some nanosecond, code clarity is probably a better win. Btw, i tried to follow current design, if Dietrich is fine with that i can move everything inside browser-places.js or split into 2 objects.

Well, it's marginal and most likely unmeasurable, but a deferred __defineGetter__ is certainly less efficient. But more importantly, I wouldn't consider HistoryMenu being split over multiple files a win in terms of code clarity. Seems like a loss-loss situation.

Comment on attachment 383578 [details] [diff] [review] patch v1.0 Going to move some code in browser-places.js

Since Dietrich approved the code move to browser-places.js, the code is not heavily Places dependent, and i have over-saturated Dietrich's reviews queue, moving review to Dao. This moves all methods into HistoryMenu helper object, i did not touch anything inside those methods.

hm, sounds like i could also move undoCloseMiddleClick

Comment on attachment 384485 [details] [diff] [review] patch v1.1 >+ var menuPopup = aEvent.target; >+ // Don't serve events for submenus. nit: "serve" doesn't fit in this context, as far as my limited English experience goes. "handle" might be better. >+ if (menuPopup.parentNode.id != "history-menu") >+ return; I think "if (event.target != event.currentTarget)" would be preferable. currentTarget is the element that the event listener was added for. Looks good overall. I agree that it would make sense to move undoCloseMiddleClick as well.

yeah, much better.

Comment on attachment 384494 [details] [diff] [review] patch v1.2 >+ m.addEventListener("click", this.undoCloseMiddleClick, false); This way, "this" will be different from "HistoryMenu" inside of undoCloseMiddleClick. I'd at least prefix it with an underscore to make it somewhat clearer that it's not a proper method and only there for internal use.

addressed comment.

I confirmed the problem is fixed with today's build. Thanks.

Verified fixed with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090703 Minefield/3.6a1pre ID:20090703031521

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv