Closed
Bug 386159
Opened 17 years ago
Closed 17 years ago
Menu-view performance improvements
Categories
(Firefox :: Bookmarks & History, defect, P2)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 3 alpha6
People
(Reporter: asaf, Assigned: asaf)
References
Details
Attachments
(1 file)
8.32 KB,
patch
|
dietrich
:
review+
|
Details | Diff | Splinter Review |
* We should do full rebuilds only once the menu is opened * We should not live complex-queries containers open (unlike folder container which are cheap)
Assignee | ||
Comment 1•17 years ago
|
||
Assignee | ||
Updated•17 years ago
|
Flags: blocking-firefox3?
Priority: -- → P2
Target Milestone: --- → Firefox 3 alpha6
Updated•17 years ago
|
Summary: Menu-view performace improvements → Menu-view performance improvements
Comment 2•17 years ago
|
||
Comment on attachment 270158 [details] [diff] [review] patch > <method name="onPopupShowing"> > <body><![CDATA[ > if (!this._resultNode) > this._init(); > > if (!this._resultNode.containerOpen) >- this._resultNode.containerOpen = true; >+ this._resultNode.containerOpen = true; nit: whitespace >@@ -892,18 +912,22 @@ > if (event.target == this) > this.onPopupShowing(); > </handler> > > <handler event="popuphidden"> > if (event.target != this) > return; > >- // UI performance: keep the resultnode open so we don't rebuild its >- // contents whenever the popup is reopened. >+ // UI performance: folder queries are cheap, keep the resultnode open >+ // so we don't rebuild its contents whenever the popup is reopened. >+ if (!PlacesUtils.nodeIsFolder(this._resultNode)) { >+ this._resultNode.containerOpen = false; >+ this._built = false; >+ } won't this change make the history menu load slower as it has to do a full rebuild every time? >Index: browser/components/places/content/toolbar.xml >=================================================================== >RCS file: /cvsroot/mozilla/browser/components/places/content/toolbar.xml,v >retrieving revision 1.95 >diff -u -p -8 -r1.95 toolbar.xml >--- browser/components/places/content/toolbar.xml 28 Jun 2007 06:50:46 -0000 1.95 >+++ browser/components/places/content/toolbar.xml 28 Jun 2007 09:46:14 -0000 >@@ -624,29 +624,29 @@ > while (parent) { > if (parent == container) > return true; > parent = parent.parent; > } > return false; > } > >- var viewerToRebuild = null; >+ var menuToRebuild = null; > for (var i=0; i < this._self._containerNodesMap.length; i++) { > var node = this._self._containerNodesMap[i].resultNode; > > if (node == aContainer) >- viewerToRebuild = this._self._containerNodesMap[i].domNode._viewer; >+ menuToRebuild = this._self._containerNodesMap[i].domNode._viewer; > if (isChildOf(node, aContainer)) > this._self._containerNodesMap.splice(i,1); > } >- >+ > if (aContainer.containerOpen) { >- if (viewerToRebuild) >- viewerToRebuild._self._rebuild(); >+ if (menuToRebuild) >+ menuToRebuild._self._built = false; > else > this._self._rebuild(); > } > }, > > invalidateAll: function TV_V_invalidateAll() { > this._self._containerNodesMap.splice(0); > this._self._rebuild(); why this change? it's the viewer, not the menu element that the variable is referring to, right? also, why change this in the toolbar and not in the menu?
Comment 3•17 years ago
|
||
Comment on attachment 270158 [details] [diff] [review] patch ok, cleared up the history menu perf question w/ mano over irc. r=me, assuming the toolbar.xml naming change is removed or explained.
Attachment #270158 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 4•17 years ago
|
||
dietrich> i'm reviewing the menu change now [19:22] <dietrich> why shouldn't we live-update menus for query nodes? [19:24] <Mano> dietrich: expensive [19:25] <Mano> dietrich: folder updates are cheap [19:25] <dietrich> expensive how? high volume of viewer activity? more expensive than rebuilding? [19:25] <Mano> for queries we run sql queries very often [19:25] <Mano> dietrich: high sql usage. [19:26] <Mano> most queries re-query on every single visit [19:27] <dietrich> ugh, ok that would explain it [19:27] <Mano> dietrich: given that our longest query is ten items long.... [19:28] <dietrich> ok, can you put that info in the bug for reference? [19:31] |<-- stevewon has left moznet (Quit: Leaving)
Comment 5•17 years ago
|
||
> won't this change make the history menu load slower as it has to do a full > rebuild every time? We can make that particular menu really fast, see bug #385397. See also https://bugzilla.mozilla.org/show_bug.cgi?id=385397#c6 for more info about the re-query on every single visit problem.
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Comment 6•17 years ago
|
||
checked in just the menu.xml parts Checking in browser/components/places/content/menu.xml; /cvsroot/mozilla/browser/components/places/content/menu.xml,v <-- menu.xml new revision: 1.79; previous revision: 1.78 done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: blocking-firefox3+ → blocking-firefox3?
Resolution: --- → FIXED
Assignee | ||
Updated•17 years ago
|
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 8•17 years ago
|
||
Checking in browser/components/places/content/menu.xml; /cvsroot/mozilla/browser/components/places/content/menu.xml,v <-- menu.xml new revision: 1.81; previous revision: 1.80 done Checking in browser/components/places/content/toolbar.xml; /cvsroot/mozilla/browser/components/places/content/toolbar.xml,v <-- toolbar.xml new revision: 1.96; previous revision: 1.95 done
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: blocking-firefox3?
Comment 9•15 years ago
|
||
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.
Description
•