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?
I've noticed this too. I comments that are still in the editable textarea are not saved or sent.
(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.
Not sure what to do here without repro steps.
(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.
(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 :(
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).
(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.
Created attachment 181068 [details] Patch
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? :(
Created attachment 181071 [details] Patch
Comment on attachment 181071 [details] Patch Please rev the version to force the new file to load.
Committed r138653: <http://trac.webkit.org/changeset/138653>