Bug 105252 – REGRESSION: Review tool sometimes doesn't include some comments in preview & posts

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Website

[Ryosuke Niwa]

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?

[Tony Chang]

I've noticed this too.  I comments that are still in the editable textarea are not saved or sent.

[Ryosuke Niwa]

(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]

Not sure what to do here without repro steps.

[Tony Chang]

(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]

(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]

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)]

(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]

Created attachment 181068 [details]
Patch

[Ryosuke Niwa]

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]

Created attachment 181071 [details]
Patch

[Tony Chang]

Comment on attachment 181071 [details]
Patch

Please rev the version to force the new file to load.

[Ojan Vafai]

Committed r138653: <http://trac.webkit.org/changeset/138653>