Last Comment Bug 813725 - Top of RSS item viewing window frozen
: Top of RSS item viewing window frozen
Status: RESOLVED FIXED
: regression
Product: Thunderbird
Classification: Client Software
Component: Folder and Message Lists (show other bugs)
: 17 Branch
: All All
: -- normal (vote)
: Thunderbird 23.0
Assigned To: alta88
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-20 13:40 PST by epaul137ml
Modified: 2014-02-22 20:20 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (3.59 KB, patch)
2013-04-24 21:24 PDT, alta88
mkmelin+mozilla: review-
Details | Diff | Review
patch (3.59 KB, patch)
2013-04-26 07:47 PDT, alta88
no flags Details | Diff | Review
updated (3.59 KB, patch)
2013-04-26 12:21 PDT, alta88
mkmelin+mozilla: review+
Details | Diff | Review
for checkin (3.68 KB, patch)
2013-04-26 13:35 PDT, alta88
alta88: review+
Details | Diff | Review

Description epaul137ml 2012-11-20 13:40:13 PST
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20121119183901

Steps to reproduce:

I opened a RSS entry viewing window by double clicking on a RSS item. This displayed the RSS item in a new window.


Actual results:

After viewing and deleting the RSS item, the bottom of the window updated to display the next RSS item but the top of the window is still showing the title and http link to the item that was deleted. After viewing and deleting multiple items, the top of window still displays the information from the first item that was displayed in the window. Closing the window and doubling clicking the next RSS item, starts the issue over.


Expected results:

The top of the window should follow the bottom of the window. The window operated correctly in Thunderbird 16 and below.
Comment 1 epaul137ml 2012-11-20 14:47:55 PST
The problem only occurs if you reading RSS items from the top down. If you are reading items from the bottom up, the top of the window updates properly.
Comment 2 alta88 2013-04-23 14:27:54 PDT
hmm, this happens on message deletion in either the standalone window or 3pane (regardless if the message opened initially is first or last in threadpane) only if Message->Open Feed message is Web Page.  the standalone window headers never update even though the right web page loads (for next message after deletion).
Comment 3 alta88 2013-04-24 21:24:37 PDT
Created attachment 741662 [details] [diff] [review]
patch
Comment 4 Magnus Melin 2013-04-25 11:57:24 PDT
Comment on attachment 741662 [details] [diff] [review]
patch

Review of attachment 741662 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/folderDisplay.js
@@ +1262,5 @@
>      let viewIndex = this.view.dbView.currentlyDisplayedMessage;
>      let msgHdr = (viewIndex != nsMsgViewIndex_None) ?
>                     this.view.dbView.getMsgHdrAt(viewIndex) : null;
>  
> +    if ((this instanceof FolderDisplayWidget) &&

I don't think it's acceptable to do instanceof here. 
Override in StandaloneFolderDisplayWidget as needed instead?
Comment 5 alta88 2013-04-25 13:11:22 PDT
why exactly?  StandaloneFolderDisplayWidget calls FolderDisplayWidget proto.  having to recreate the displayMessageChanged() function in SFDW without these 2 lines seems quite inefficient.
Comment 6 Magnus Melin 2013-04-25 23:14:55 PDT
StandaloneFolderDisplayWidget is supposed to handle standalone window differences from FolderDisplayWidget. If we start testing for standalone inside FolderDisplayWidget it's a slippery slope... 

You don't necessarily need to copy the implementation. You could create a helper method in FolderDisplayWidget that handles setting the content, then override that metod in StandaloneFolderDisplayWidget to do nothing. 

Or maybe there's better ways - i'm not sure i understand what the problem was?
Comment 7 alta88 2013-04-26 07:47:37 PDT
Created attachment 742368 [details] [diff] [review]
patch

well, i guess i still don't understand why it's (too) slippery.  yes, there are numerous other methods, but there are 2 subclasses of FDW in the codebase, and i wanted to do it in < 1 line.. ;)

fortunately it turns out the subclasses already call the super with a null aTabInfo (for the same reasons) and this will work perfectly (better).
Comment 8 Magnus Melin 2013-04-26 12:02:18 PDT
Comment on attachment 742368 [details] [diff] [review]
patch

Review of attachment 742368 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to be some problem with the diff

$ hg qpush -v
applying feedWindow.patch
patching file mail/base/content/folderDisplay.js
bad hunk #1 @@ -1258,17 +1258,20 @@ FolderDisplayWidget.prototype = {
 (18 17 20 20)
patch failed, rejects left in working dir
errors during apply, please fix and refresh feedWindow.patch
Comment 9 alta88 2013-04-26 12:21:03 PDT
Created attachment 742505 [details] [diff] [review]
updated


updated..
Comment 10 Magnus Melin 2013-04-26 13:03:20 PDT
Comment on attachment 742505 [details] [diff] [review]
updated

Review of attachment 742505 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/folderDisplay.js
@@ +1269,1 @@
>        FeedMessageHandler.setContent(msgHdr, false);

I think this if should have braces since thers's the comment in there.

also ie -> i.e. and "a tabInfo"
Comment 11 alta88 2013-04-26 13:35:23 PDT
Created attachment 742535 [details] [diff] [review]
for checkin
Comment 12 Ryan VanderMeulen [:RyanVM] 2013-04-27 18:49:45 PDT
https://hg.mozilla.org/comm-central/rev/13b289848f1d
Comment 13 marvinhk 2013-05-01 23:50:42 PDT
will this be deployed to earlybird or beta soon?
or this will be known issue until tb23?
I'd like to check out Bug 816414 which is said to have this as root cause.
not sure if I should wait for tb22 beta upon code merge or nightly directly.

Note You need to log in before you can comment on or make changes to this bug.