Closed
Bug 955597
Opened 11 years ago
Closed 11 years ago
Call getChatRoomDefaultFieldValues lazily for awesometab roomInfos
Categories
(Instantbird Graveyard :: Other, defect)
Instantbird Graveyard
Other
Tracking
(Not tracked)
RESOLVED
FIXED
1.5
People
(Reporter: aleth, Assigned: nhnt11)
References
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
florian
:
review+
clokep
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 2157 at 2013-09-06 12:55:00 UTC ***
getChatRoomDefaultFieldValues is currently called for every channel on RPL_LIST, slowing things down. This can just as well be done lazily when the user actually wants to join the channel, at least for IRC.
Comment 1•11 years ago
|
||
*** Original post on bio 2157 at 2013-09-06 14:03:23 UTC ***
This probably involves subclassing the PossibleConversation code and storing the channel name and account, then calling getChatRoomDefaultFieldValues on the channel name when it needs to access those values.
Assignee | ||
Comment 2•11 years ago
|
||
*** Original post on bio 2157 as attmnt 2849 at 2013-09-07 22:30:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354619 -
Flags: review?(aleth)
Comment 3•11 years ago
|
||
Comment on attachment 8354619 [details] [diff] [review]
Patch
*** Original change on bio 2157 attmnt 2849 at 2013-09-08 23:20:28 UTC ***
I'd like to review this.
Attachment #8354619 -
Flags: review?(clokep)
Comment 4•11 years ago
|
||
Comment on attachment 8354619 [details] [diff] [review]
Patch
*** Original change on bio 2157 attmnt 2849 at 2013-09-08 23:30:47 UTC ***
This seems to make the assumption that chat room field values always just needs the name instead of just adding a getter and subclassing RoomInfo to have an IRC specific version that only takes the name.
Attachment #8354619 -
Flags: review?(clokep) → review-
Assignee | ||
Comment 5•11 years ago
|
||
*** Original post on bio 2157 as attmnt 2851 at 2013-09-08 23:32:00 UTC ***
This makes chatRoomFieldValues a getter.
Attachment #8354621 -
Flags: review?(aleth)
Assignee | ||
Updated•11 years ago
|
Attachment #8354621 -
Flags: review?(clokep)
Assignee | ||
Comment 6•11 years ago
|
||
Comment on attachment 8354619 [details] [diff] [review]
Patch
*** Original change on bio 2157 attmnt 2849 at 2013-09-08 23:32:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354619 -
Attachment is obsolete: true
Attachment #8354619 -
Flags: review?(aleth)
Assignee | ||
Comment 7•11 years ago
|
||
*** Original post on bio 2157 as attmnt 2852 at 2013-09-08 23:37:00 UTC ***
Missed a return in the previous patch.
Attachment #8354622 -
Flags: review?(aleth)
Assignee | ||
Updated•11 years ago
|
Attachment #8354622 -
Flags: review?(clokep)
Assignee | ||
Comment 8•11 years ago
|
||
Comment on attachment 8354621 [details] [diff] [review]
Patch 2
*** Original change on bio 2157 attmnt 2851 at 2013-09-08 23:37:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354621 -
Attachment is obsolete: true
Attachment #8354621 -
Flags: review?(clokep)
Attachment #8354621 -
Flags: review?(aleth)
Comment 9•11 years ago
|
||
Comment on attachment 8354622 [details] [diff] [review]
Patch 3
*** Original change on bio 2157 attmnt 2852 at 2013-09-09 12:09:33 UTC ***
>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
>--- a/chat/protocols/irc/ircBase.jsm
>+++ b/chat/protocols/irc/ircBase.jsm
>@@ -27,16 +27,30 @@ Cu.import("resource:///modules/ircHandle
> Cu.import("resource:///modules/ircUtils.jsm");
> Cu.import("resource:///modules/jsProtoHelper.jsm");
>
> XPCOMUtils.defineLazyGetter(this, "DownloadUtils", function() {
> Components.utils.import("resource://gre/modules/DownloadUtils.jsm");
> return DownloadUtils;
> });
>
>+function ircRoomInfo(aName, aTopic, aParticipantCount, aAccountId) {
>+ this.name = aName;
>+ this.topic = aTopic;
>+ this.participantCount = aParticipantCount;
>+ this.accountId = aAccountId;
I think this would be easier to store a reference to the account here and then add a getter for accountId (and simplify the getter for chatRoomFieldValues).
>diff --git a/instantbird/components/ibConvStatsService.js b/instantbird/components/ibConvStatsService.js
>+ get _displayName() this._roomInfo.name,
>+ get _accountId() this._roomInfo.accountId,
>+ get _statusText() {
>+ return "(" + this._roomInfo.participantCount + ") " +
>+ (this._roomInfo.topic || _instantbird("noTopic"));
Does this need to be localized?
Attachment #8354622 -
Flags: review?(clokep) → review-
Comment 10•11 years ago
|
||
*** Original post on bio 2157 at 2013-09-09 12:22:31 UTC ***
(In reply to comment #7)
> >diff --git a/instantbird/components/ibConvStatsService.js b/instantbird/components/ibConvStatsService.js
> >+ get _displayName() this._roomInfo.name,
> >+ get _accountId() this._roomInfo.accountId,
> >+ get _statusText() {
> >+ return "(" + this._roomInfo.participantCount + ") " +
> >+ (this._roomInfo.topic || _instantbird("noTopic"));
> Does this need to be localized?
My understanding is that putting the participant count at the beginning of the topic sucks and we agreed it should be replaced by something else as soon as we have time to polish the awesometab UI (the current focus is on getting the correct results displayed quickly enough, and I don't think we should let us be distracted by something else until it works). This is bug 955593 (bio 2154).
I don't think we need to localize the current placeholder.
Assignee | ||
Comment 11•11 years ago
|
||
*** Original post on bio 2157 as attmnt 2854 at 2013-09-09 20:50:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354624 -
Flags: review?(aleth)
Assignee | ||
Updated•11 years ago
|
Attachment #8354624 -
Flags: review?(clokep)
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 8354622 [details] [diff] [review]
Patch 3
*** Original change on bio 2157 attmnt 2852 at 2013-09-09 20:50:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354622 -
Attachment is obsolete: true
Attachment #8354622 -
Flags: review?(aleth)
Comment 13•11 years ago
|
||
Comment on attachment 8354624 [details] [diff] [review]
Patch 4
*** Original change on bio 2157 attmnt 2854 at 2013-09-09 21:36:01 UTC ***
Comments given on IRC: http://log.bezut.info/instantbird/130909/#m296
Attachment #8354624 -
Flags: review-
Assignee | ||
Comment 14•11 years ago
|
||
*** Original post on bio 2157 as attmnt 2856 at 2013-09-09 22:02:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354626 -
Flags: review?(clokep)
Assignee | ||
Updated•11 years ago
|
Attachment #8354626 -
Flags: review?(florian)
Assignee | ||
Comment 15•11 years ago
|
||
Comment on attachment 8354624 [details] [diff] [review]
Patch 4
*** Original change on bio 2157 attmnt 2854 at 2013-09-09 22:02:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354624 -
Attachment is obsolete: true
Attachment #8354624 -
Flags: review?(clokep)
Attachment #8354624 -
Flags: review?(aleth)
Assignee | ||
Comment 16•11 years ago
|
||
Comment on attachment 8354626 [details] [diff] [review]
Patch 5
*** Original change on bio 2157 attmnt 2856 at 2013-09-09 22:03:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354626 -
Attachment is patch: true
Attachment #8354626 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 17•11 years ago
|
||
Comment on attachment 8354626 [details] [diff] [review]
Patch 5
*** Original change on bio 2157 attmnt 2856 at 2013-09-09 22:04:28 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354626 -
Flags: review?(aleth)
Assignee | ||
Comment 18•11 years ago
|
||
*** Original post on bio 2157 as attmnt 2857 at 2013-09-09 22:17:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354627 -
Flags: review?(aleth)
Assignee | ||
Updated•11 years ago
|
Attachment #8354627 -
Flags: review?(florian)
Assignee | ||
Updated•11 years ago
|
Attachment #8354627 -
Flags: review?(clokep)
Assignee | ||
Comment 19•11 years ago
|
||
Comment on attachment 8354626 [details] [diff] [review]
Patch 5
*** Original change on bio 2157 attmnt 2856 at 2013-09-09 22:17:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354626 -
Attachment is obsolete: true
Attachment #8354626 -
Flags: review?(florian)
Attachment #8354626 -
Flags: review?(clokep)
Attachment #8354626 -
Flags: review?(aleth)
Comment 20•11 years ago
|
||
Comment on attachment 8354627 [details] [diff] [review]
Patch 6
*** Original change on bio 2157 attmnt 2857 at 2013-09-09 22:32:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354627 -
Flags: review?(florian) → review+
Comment 21•11 years ago
|
||
Comment on attachment 8354627 [details] [diff] [review]
Patch 6
*** Original change on bio 2157 attmnt 2857 at 2013-09-09 23:50:37 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354627 -
Flags: review?(clokep) → review+
Comment 22•11 years ago
|
||
*** Original post on bio 2157 at 2013-09-09 23:55:46 UTC ***
http://hg.instantbird.org/instantbird/rev/f0cd4b1b5222
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
Comment 23•11 years ago
|
||
Comment on attachment 8354627 [details] [diff] [review]
Patch 6
*** Original change on bio 2157 attmnt 2857 at 2013-09-09 23:56:47 UTC ***
Clearing aleth's review, we checked it in without him as he hadn't r-ed any of the previous versions.
Attachment #8354627 -
Flags: review?(aleth)
You need to log in
before you can comment on or make changes to this bug.
Description
•