Bug 105252

Summary: REGRESSION: Review tool sometimes doesn't include some comments in preview & posts
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: WebKit WebsiteAssignee: Ojan Vafai <ojan>
Status: RESOLVED FIXED    
Severity: Critical CC: apavlov, eric, ojan, tony
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch tony: review+

Ryosuke Niwa
Reported 2012-12-17 21:43:19 PST
It appears that review tool now completely ignores new comments. I have to reload the page in order for the preview & the actual comments to reflect some of comments I added. Maybe comments that have not been saved are not included?
Attachments
Patch (9.16 KB, patch)
2013-01-02 14:27 PST, Ojan Vafai 		no flags
DetailsFormatted DiffDiff
Patch (9.21 KB, patch)
2013-01-02 14:40 PST, Ojan Vafai 		tony: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Tony Chang
Comment 1 2012-12-18 09:47:56 PST
I've noticed this too. I comments that are still in the editable textarea are not saved or sent.
Ryosuke Niwa
Comment 2 2012-12-18 09:49:54 PST
(In reply to comment #1) > I've noticed this too. I comments that are still in the editable textarea are not saved or sent. Hm... that might be a slightly different issue. What I'm observing is that the inserts comments are saved but not included in preview or sent.
Ojan Vafai
Comment 3 2012-12-18 10:05:54 PST
Not sure what to do here without repro steps.
Tony Chang
Comment 4 2012-12-18 13:17:09 PST
(In reply to comment #2) > (In reply to comment #1) > > I've noticed this too. I comments that are still in the editable textarea are not saved or sent. > > Hm... that might be a slightly different issue. What I'm observing is that the inserts comments are saved but not included in preview or sent. Oh, I was guessing. Maybe I am remembering to hit "OK" and the comments aren't getting sent. I'm not sure how to debug. I'll try to do reviews with an open JS console, I guess.
Ryosuke Niwa
Comment 5 2012-12-18 13:19:50 PST
(In reply to comment #3) > Not sure what to do here without repro steps. Yeah, it appears to be some sort of heisen bug or involves race condition :(
Ryosuke Niwa
Comment 6 2012-12-23 22:31:06 PST
I reproduced this bug on https://bugs.webkit.org/attachment.cgi?id=180637&action=review once when I was commenting on DeleteSelectionCommand.cpp change at lines 483-484. Here's my analysis. The problem is that forEachLine prematurely stops iteration at #line32 when it's called by fillInReviewForm because #line33 doesn't exist on this page (it appears that this doesn't reproduce reliably either).
Alexander Pavlov (apavlov)
Comment 7 2012-12-27 07:40:24 PST
(In reply to comment #6) > I reproduced this bug on https://bugs.webkit.org/attachment.cgi?id=180637&action=review once when I was commenting on DeleteSelectionCommand.cpp change at lines 483-484. > > Here's my analysis. The problem is that forEachLine prematurely stops iteration at #line32 when it's called by fillInReviewForm because #line33 doesn't exist on this page (it appears that this doesn't reproduce reliably either). Another manifestation, same cause: I expanded the diff (20 above, 20 below...), which inserted a number of ".ExpansionLine"s, but removed ".Line.LineContainer.context" having a line number! Thus, a hole in the line number sequence appeared, and that was the reason for forEachLine premature bail-out. Thus, all comments after the first expanded source chunk were lost.
Ojan Vafai
Comment 8 2013-01-02 14:27:55 PST
Created attachment 181068 [details] Patch
Ryosuke Niwa
Comment 9 2013-01-02 14:37:25 PST
Comment on attachment 181068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181068&action=review Looks reasonable. > Websites/bugs.webkit.org/code-review.js:75 > var g_displayed_draft_comments = false; > + var g_next_line_id = 0; Mixing camelCase and underscored names? :(
Ojan Vafai
Comment 10 2013-01-02 14:40:53 PST
Created attachment 181071 [details] Patch
Tony Chang
Comment 11 2013-01-02 14:44:21 PST
Comment on attachment 181071 [details] Patch Please rev the version to force the new file to load.
Ojan Vafai
Comment 12 2013-01-02 14:47:36 PST
Committed r138653: <http://trac.webkit.org/changeset/138653>
Note You need to log in before you can comment on or make changes to this bug.