Closed
Bug 457333
Opened 16 years ago
Closed 6 years ago
De-RDF and de-fork subscribe
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(thunderbird_esr60 fixed, thunderbird60 fixed, thunderbird61 wontfix, thunderbird62 fixed)
RESOLVED
FIXED
Thunderbird 62.0
People
(Reporter: jcranmer, Assigned: jcranmer)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 7 obsolete files)
48.59 KB,
patch
|
Details | Diff | Splinter Review | |
6.04 KB,
patch
|
jorgk-bmo
:
review+
jorgk-bmo
:
approval-comm-beta+
jorgk-bmo
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
This bug is to de-RDF and de-fork the subscribe dialog. Attached is a patch which has my work to date. More history can be seen at the kill-rdf repo.
Assignee | ||
Comment 1•16 years ago
|
||
Comment on attachment 340663 [details] [diff] [review] A general idea Oops, wrong diff.
Attachment #340663 -
Attachment is obsolete: true
Assignee | ||
Comment 2•16 years ago
|
||
As the attachment name says...
Assignee | ||
Comment 3•16 years ago
|
||
This is the final product, modulo some severe threadline issues.
Attachment #340664 -
Attachment is obsolete: true
Assignee | ||
Comment 4•16 years ago
|
||
Finally, I have something that works to the point of reviewability. Before balking at the size, keep a few things in mind: 1. A total of six files are being removed, comprising in sum 2637 lines. 2. This essentially rewrites much of subscribe.js and all useful content of nsISubscribableServer.js. The changes to subscribe.xul may be better understood by comparing the new file with the old TB version. In general, the basic model changes from having the interface know the state to having the interface tell the dialog its state. The dialog also rocks much better with async connections (try running it on a server with a large amount of newsgroups on a really slow connection and note that it is usable while getting newsgroups!). The only problem is that large sync loads are more painful. In particular, loading the news host file wants to be done on another thread. There is one thing which this patch will regress (acceptably, IMHO): selecting servers to subscribe to in search mode and then exiting search mode will "reset" those subscriptions until you go back into search mode. I plan on redoing the search + subscribe feature before TB 3 anyways (asking the server to implement nsITreeView and keep copies of stuff itself is a waste)... This is more than what is available at the tip of jminta's kill-rdf repo, since I intend to have this land separately, as it does not affect any other kill-rdf work (asides from showing a folderLookupService regression).
Attachment #341769 -
Attachment is obsolete: true
Attachment #341776 -
Flags: superreview?(neil)
Attachment #341776 -
Flags: review?(bienvenu)
Comment 5•16 years ago
|
||
(In reply to comment #4) > The dialog also rocks much better with async connections (try running > it on a server with a large amount of newsgroups on a really slow > connection and note that it is usable while getting newsgroups!). The > only problem is that large sync loads are more painful. In particular, > loading the news host file wants to be done on another thread. My ISP's news server carries over 85000 groups, and loading them took an age and there were several slow script warning prompts along the way. By comparison, without the patch the subscribe dialog loaded instantaneously.
Comment 6•16 years ago
|
||
Comment on attachment 341776 [details] [diff] [review] Patch, version 1 Also the themeing changes don't seem to work quite right - the unsubscribable items had a dot in the subscribe column.
Attachment #341776 -
Flags: superreview?(neil) → superreview-
Assignee | ||
Comment 7•16 years ago
|
||
< Side note: I swear I dreamed something about writing a response to this comment a while back... > (In reply to comment #5) > My ISP's news server carries over 85000 groups, and loading them took an age > and there were several slow script warning prompts along the way. By > comparison, without the patch the subscribe dialog loaded instantaneously. It sounds like this patch would want to shunt the host file loading off the main thread, then, instead of delaying it to another patch. (In reply to comment #6) > Also the themeing changes don't seem to work quite right - the unsubscribable > items had a dot in the subscribe column. Which theme is this? I checked my changes on the default theme for Linux on TB (qute) and SM (classic, I think), and both seemed to work...
Comment 8•16 years ago
|
||
loading hostinfo.dat is probably a much more common operation than building up the list of newsgroups, so making it slower seems like a move in the wrong direction. And moving it onto its own thread sounds like a drastic work-around for an underlying problem that might be more easily fixed by just using timeouts and loading the hostinfo.dat file in chunks.
Assignee | ||
Comment 9•16 years ago
|
||
(In reply to comment #8) > loading hostinfo.dat is probably a much more common operation than building up > the list of newsgroups, so making it slower seems like a move in the wrong > direction. And moving it onto its own thread sounds like a drastic work-around > for an underlying problem that might be more easily fixed by just using > timeouts and loading the hostinfo.dat file in chunks. Actually, LoadHostInfoFile is only called by StartPopulating, so it's a sync call happening on the UI thread only happening when someone opens the subscribe dialog. That said, I forgot about being able to use timeouts instead (which is how nsNNTPProtocol downloads the list).
Assignee | ||
Comment 10•16 years ago
|
||
This speeds up the dialog considerably, and makes news host file loading asynchronous. Also, this fixes the SM CSS problem that Neil saw. Finally, this fixed a regression in IMAP i18n folder names. There may be one or two other minor fixes in here, but those are the bigger ones.
Attachment #341776 -
Attachment is obsolete: true
Attachment #342611 -
Flags: superreview?(neil)
Attachment #342611 -
Flags: review?(bienvenu)
Attachment #341776 -
Flags: review?(bienvenu)
Assignee | ||
Comment 11•16 years ago
|
||
This patch contains a few small fixes and one big fix. The big fix: news load now loads all files The small fixes: * I found the cause of the leak. And I fixed it. * Expanding a news folder while loading hostinfo doesn't stop loading hostinfo * A bit more error-tolerant * Removed a few redundant method calls. * Moved the chunking to 500 items instead of 1000, as the 500 is less hesitant I could do more thorough cleanup with respect to the search stuff, but that will come when I make search + subscribe sane, hopefully before b1.
Attachment #342611 -
Attachment is obsolete: true
Attachment #342823 -
Flags: superreview?(neil)
Attachment #342823 -
Flags: review?(bienvenu)
Attachment #342611 -
Flags: superreview?(neil)
Attachment #342611 -
Flags: review?(bienvenu)
Comment 12•16 years ago
|
||
Comment on attachment 342823 [details] [diff] [review] Patch, version 3 As measured on a 3GHz dual-core processor: Time to load 47496 groups: 31s (unresponsive) Time to load in 97 batches: 97.5s (responsive) Time to load with old code: 5s (unresponsive) >+ if (gSubscribableServer.subscribeListener != gSubscribeTreeView) >+ gSubscribableServer.subscribeListener = gSubscribeTreeView; What's the point of the test? >+ dump("Stack trace:\n" + ex.stack); undefined >+ try { >+ // We just need to know it's doable; actually setting the server will be >+ // done later >+ folder.server.QueryInterface(Components.interfaces.nsISubscribableServer); >+ gServerURI = folder.server.serverURI; >+ } >+ catch (ex) { >+ //dump("not a subscribable server\n"); >+ gSubscribableServer = null; >+ gServerURI = null; >+ } Should use instanceof instead of try { QueryInterface } Note that our current UI won't let you open the Subscribe dialog unless you have selected a subscribable server. >+ StateChanged(name,state); Nit: might as well add a space after the comma > if (inSearchMode) > SetStateFromRow(k, state); > else { >- var rowRes = gSubscribeTree.builderView.getResourceAtIndex(k); >- var name = GetRDFProperty(rowRes, "Name"); >- SetState(name, state); >+ var column = {id: "subscribedColumn"}; >+ if (gSubscribeTreeView.isEditable(k, column)) >+ gSubscribeTreeView.setCellValue(k, column, "" + state); Followup to make both views use the same code to set the state? .+let Ci = Components.interfaces; ... >+function makeRowElement(name, leafName, level) { >+ let row = {}; >+ row.name = name; >+ row.leafName = leafName; >+ row.parent = null; >+ row.children = []; >+ row.open = false; >+ row.level = level; >+ return row; >+} Consider making this a constructor, rather than a function i.e. call it with new rowElement(...) and move the null/false constants to the prototype. >+ this.setCellValue(aRow, {id: "subscribedColumn"}, !row.subscribed + ""); Why the + ""? Does xpconnect not get this cast right? >+ this._batchAdditions.push([aName, aSubscribed, aSubscribable, aOverwrite]); push(arguments); ? >+ if (this._batchAdditions.length > 500) { >= (otherwise you get 501 in a batch...) >+ for each (var args in this._batchAdditions) Can't use for ... in with an array. >+ // Custom binary search that returns where the index would be. Didn't you adapt the other binary search to do this too? >+ if (mapIndex == gSubscribeTreeView._rowMap.length) >+ gSubscribeTreeView._rowMap.push(row); >+ else >+ gSubscribeTreeView._rowMap.splice(mapIndex, 0, row); Don't need to special-case this. >+ return -low - 1; IMHO ~x is neater than -x - 1 >+ let row = makeRowElement(fullName, midName, level); >+ row.subscribed = false; >+ row.canSubscribe = false; IMHO these should default to false in the rowElement prototype. >+ // We have a match! Overwrite only if necessary >+ if (aOverwrite && (row.subscribed != aSubscribed || >+ row.canSubscribe != aSubscribable)) { Don't you trust the overwrite flag? >+ addNode(nameParts, child.children, child, child.open, level + 1); >+ return; This tail recursion is a somewhat unusual way of writing a loop... >+ if (this._tree) >+ this._tree.beginUpdateBatch(); >+ for each (var args in this._batchAdditions) >+ this._realAddFolder(args[0], args[1], args[2], args[3]); >+ this._batchAdditions = []; >+ if (this._tree) >+ this._tree.endUpdateBatch(); Nit: duplicated code. >+ isContainer: function (aIndex) { >+ return !!this._rowMap[aIndex].children.length; >+ }, Hmm, how does this work for IMAP, I thought we only populate as necessary. >+ this._tree.rowCountChanged(aIndex + 1, index - aIndex - 1); >+ this._tree.invalidateRange(aIndex + 1, index - aIndex - 1); rowCountChanged does invalidate the added rows! You might want to invalidate the twisty though. >+ this._tree.rowCountChanged(aIndex + 1, -(i - (aIndex + 1))); aIndex + 1 - i >+ return this._rowMap[aRow].subscribed ? "true" : "false"; xpconnect casting issue again? >+ var atomSvc = Components.classes["@mozilla.org/atom-service;1"] >+ .getService(Components.interfaces.nsIAtomService); Why not use the global atomService? + aProps.AppendElement(atomSvc.getAtom("Subscribable-" + row.canSubscribe)); + + if (aCol.id == "nameColumn") + aProps.AppendElement(atomSvc.getAtom(this._serverType)); Might it be worth getting these atoms globally? (It's a pity there aren't predefined properties for uneditable cells.)
Attachment #342823 -
Flags: superreview?(neil) → superreview-
Assignee | ||
Comment 13•16 years ago
|
||
(In reply to comment #12) > >+ if (gSubscribableServer.subscribeListener != gSubscribeTreeView) > >+ gSubscribableServer.subscribeListener = gSubscribeTreeView; > What's the point of the test? > > >+ dump("Stack trace:\n" + ex.stack); > undefined It's what I've used for whenever someone throws a JS Error at me... > > if (inSearchMode) > > SetStateFromRow(k, state); > > else { > >- var rowRes = gSubscribeTree.builderView.getResourceAtIndex(k); > >- var name = GetRDFProperty(rowRes, "Name"); > >- SetState(name, state); > >+ var column = {id: "subscribedColumn"}; > >+ if (gSubscribeTreeView.isEditable(k, column)) > >+ gSubscribeTreeView.setCellValue(k, column, "" + state); > Followup to make both views use the same code to set the state? That'll be part of the subscribe search cleanup, where I intend to make both search and non-search use gSubscribeTreeView. > >+ this.setCellValue(aRow, {id: "subscribedColumn"}, !row.subscribed + ""); > Why the + ""? Does xpconnect not get this cast right? I'm pretty sure that xpconnect did 0/1 for JS bools as strings, but I'll double check. > >+ if (this._batchAdditions.length > 500) { > >= (otherwise you get 501 in a batch...) ... oops ... > >+ // Custom binary search that returns where the index would be. > Didn't you adapt the other binary search to do this too? This is a standard binary search extension to return more detailed information instead of -1 (at least, I based this off of Java's Arrays.binarySearch). In this case, however, I don't bother trying to look for it in the list since I already know it's not in there. > >+ if (mapIndex == gSubscribeTreeView._rowMap.length) > >+ gSubscribeTreeView._rowMap.push(row); > >+ else > >+ gSubscribeTreeView._rowMap.splice(mapIndex, 0, row); > Don't need to special-case this. Duly noted. > >+ // We have a match! Overwrite only if necessary > >+ if (aOverwrite && (row.subscribed != aSubscribed || > >+ row.canSubscribe != aSubscribable)) { > Don't you trust the overwrite flag? The overwrite only tells us that we /may/ overwrite it, not that we need overwrite it. > >+ isContainer: function (aIndex) { > >+ return !!this._rowMap[aIndex].children.length; > >+ }, > Hmm, how does this work for IMAP, I thought we only populate as necessary. IMAP looks at everything two levels at a time, so it's really populating grandchildren as we expand the nodes. > >+ var atomSvc = Components.classes["@mozilla.org/atom-service;1"] > >+ .getService(Components.interfaces.nsIAtomService); > Why not use the global atomService? Forgot about that as I was rampaging earlier stuff; will do. > + aProps.AppendElement(atomSvc.getAtom("Subscribable-" + row.canSubscribe)); > + > + if (aCol.id == "nameColumn") > + aProps.AppendElement(atomSvc.getAtom(this._serverType)); > Might it be worth getting these atoms globally? Ditto.
Assignee | ||
Comment 14•16 years ago
|
||
(In reply to comment #12) > >+ this.setCellValue(aRow, {id: "subscribedColumn"}, !row.subscribed + ""); > Why the + ""? Does xpconnect not get this cast right? xpconnect gets the cast right, but I'm calling this via JS invocation, where I'm expecting a string "true" or "false". And true != "true".
Assignee | ||
Comment 15•16 years ago
|
||
Improved patch. Also fixed regression that made news not show full names.
Attachment #342823 -
Attachment is obsolete: true
Attachment #344841 -
Flags: superreview?(neil)
Attachment #344841 -
Flags: review?(bienvenu)
Attachment #342823 -
Flags: review?(bienvenu)
Comment 16•15 years ago
|
||
where are we with this patch? last I heard, there were still significant performance issues with loading the subscribe UI with it.
Assignee | ||
Comment 17•15 years ago
|
||
(In reply to comment #16) > where are we with this patch? last I heard, there were still significant > performance issues with loading the subscribe UI with it. That is correct. I had something that was much more performant when I pushed the JS implementation back into C++, but that was a rather ugly implementation. I guess I never got around to clearing review requests...
Assignee | ||
Updated•15 years ago
|
Attachment #344841 -
Attachment is obsolete: true
Attachment #344841 -
Flags: superreview?(neil)
Attachment #344841 -
Flags: review?(bienvenu)
Assignee | ||
Comment 18•15 years ago
|
||
Comment on attachment 344841 [details] [diff] [review] Patch, version 4 Canceling requests.
Updated•12 years ago
|
Target Milestone: Thunderbird 3.0b1 → ---
Comment 19•12 years ago
|
||
(In reply to Joshua Cranmer [:jcranmer] from comment #4) > The changes to subscribe.xul may be better understood by comparing the new > file with the old TB version. The patch (version 4) has changes to mailnews/base/resources/content/subscribe.js and mailnews/base/resources/content/subscribe.xul. In current comm-central, there is only mail/base/content/subscribe.js and suite/mailnews/subscribe.js. Apparently, a lot has changed since then. Can someone help how the change to the various subscribe.js and subscribe.xul can be best de-bitrotted? Joshua, maybe you still have "the old TB version" against which one should compare? Best regards, Jens
Comment 20•6 years ago
|
||
TB and SM subscribe dialog was merged in bug 1425962, made to work without the RDF backend and <template> XUL element. It seems we can now remove the whole nsSubscribeDatasource files and cut some datasource references from nsSubscribableServer. Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=30c8d6588f0de357a3373f34b492d1589c33fb63
Attachment #8944167 -
Flags: review?(Pidgeot18)
Comment 21•6 years ago
|
||
I have aggressively removed GetChildren() method from the SubscribableServer as it seem unused now. It may be it will be needed for the tree implementation. But it will need a different non-RDF reimplamentation (you have something already in your derdf-patches) so then I can put it back.
Comment 22•6 years ago
|
||
This salvages whatever is still needed from jcranmer's patches 'subscribedefork' and 'subscribe-fatrip'. Most of those patches are already in the tree due to the quick fix we had to implement in bug 1425962.
Attachment #8982348 -
Flags: review?(jorgk)
Comment 23•6 years ago
|
||
The current version of jcranmer's patches is at https://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/patches-derdf/ . After part 1 that I attached we still need to apply the patches named subscribe-invert and subscribe-tree.
Comment 24•6 years ago
|
||
Comment on attachment 8944167 [details] [diff] [review] 457333-removeSubscribeDS.patch [landed in bug 1495101] This can be done as the last step after the dialog is fully converted.
Attachment #8944167 -
Attachment description: 457333-removeSubscribeDS.patch → 457333-removeSubscribeDS.patch [re-evaluate later]
Attachment #8944167 -
Flags: review?(Pidgeot18)
Comment 25•6 years ago
|
||
Comment on attachment 8982348 [details] [diff] [review] 457333-1.patch Looks like some boy-scout clean-up with little change.
Attachment #8982348 -
Flags: review?(jorgk) → review+
Updated•6 years ago
|
Keywords: leave-open
Comment 26•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/ecc150ade2f2 Cleanup subscribe dialog code. r=jorgk
Comment 27•6 years ago
|
||
Comment on attachment 8982348 [details] [diff] [review] 457333-1.patch We need this or else the patch from bug 1466705 doesn't apply.
Attachment #8982348 -
Flags: approval-comm-esr60+
Attachment #8982348 -
Flags: approval-comm-beta+
Comment 28•6 years ago
|
||
TB 60 beta 8 (BETA_60_CONTINUATION branch): https://hg.mozilla.org/releases/comm-beta/rev/b0a1a743051f736a85129948ef441d9c6d2a51ae
status-thunderbird60:
--- → fixed
status-thunderbird61:
--- → affected
status-thunderbird62:
--- → fixed
status-thunderbird_esr60:
--- → affected
Target Milestone: --- → Thunderbird 62.0
Comment 29•6 years ago
|
||
I think we should apply more patches in another bug since otherwise we get a big version confusion.
Comment 30•6 years ago
|
||
Great and then all the history stays buried here and the bug summary is nothing about what was landed :(
Comment 31•6 years ago
|
||
Well, you can file the follow-up with "Bug 457333 follow-up: ..." and also include this in the commit message. Landing patches across versions in a single bug becomes messy real soon, and since I have to manage it, it won't happen.
Updated•6 years ago
|
Attachment #8944167 -
Attachment description: 457333-removeSubscribeDS.patch [re-evaluate later] → 457333-removeSubscribeDS.patch [landed in bug 1495101]
You need to log in
before you can comment on or make changes to this bug.
Description
•