Closed Bug 498619 Opened 12 years ago Closed 12 years ago

Pages list on History menu disappear when restore submenus are closed.

Categories

(Firefox :: Bookmarks & History, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: hidenosuke, Assigned: mak)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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!
Assignee: nobody → mak77
Blocks: 324430
Status: NEW → ASSIGNED
Severity: normal → critical
Keywords: regression
Priority: -- → P1
Attached patch patch v1.0 (obsolete) — Splinter Review
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.
Attachment #383578 - Flags: review?(dietrich)
Summary: Page list on History menu is disappeared → Pages list on History menu disappear when restore submenus are closed.
OS: Linux → All
Hardware: x86 → All
(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.
Duplicate of this bug: 499511
Duplicate of this bug: 499471
(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
Attachment #383578 - Attachment is obsolete: true
Attachment #383578 - Flags: review?(dietrich)
Attached patch patch v1.1 (obsolete) — Splinter Review
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.
Attachment #384485 - Flags: review?(dao)
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.
Attached patch patch v1.2 (obsolete) — Splinter Review
yeah, much better.
Attachment #384485 - Attachment is obsolete: true
Attachment #384494 - Flags: review?(dao)
Attachment #384485 - Flags: review?(dao)
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.
Attachment #384494 - Flags: review?(dao) → review+
Attached patch patch v1.3Splinter Review
addressed comment.
Attachment #384494 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/af1418d8f0a5
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
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
Status: RESOLVED → VERIFIED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.