[webkit-reviews] review denied: [Bug 52866] Store draft comments in localStorage : [Attachment 79689] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 52866: Store draft comments in localStorage
https://bugs.webkit.org/show_bug.cgi?id=52866

Attachment 79689: Patch
https://bugs.webkit.org/attachment.cgi?id=79689&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79689&action=review

I'd put a "born-on" date in the draft comment JSON so we can GC draft comments
that are older than, say, a month.  Would be nice to see one more iteration.

> Websites/bugs.webkit.org/code-review.js:154
> +    $(JSON.parse(serialized_comments)).each(function() {
> +	 addDraftComment(this.start_line_id, this.end_line_id, this.contents);
> +    });

We need some better error handling here in case localStorage gets corrupted
somehow.

> Websites/bugs.webkit.org/code-review.js:171
> +    localStorage.setItem('draft-comments-for-attachment-' + attachment_id,
JSON.stringify(comment_store));

Do we need to handle the case where we're out of storage space?  Maybe catch
whatever exception that is and offer to clear previous draft comments?

> Websites/bugs.webkit.org/code-review.js:1099
> -  $('.comment .discard').live('click', discardComment);
> +  $('.comment .discard').live('click', function() {
discardComment($(this).parents('.comment')); });

Maybe make this function into handleDiscardCommentClick?  Seems prettier to
just bind a function with a name in case it grows later.

> Websites/bugs.webkit.org/code-review.js:1105
> -	 discardComment.call(this);
> +	 discardComment.call(comment_block);

Did you test this part?  We don't want call here, which will bind comment_block
to |this| in discardComment.  We just want to call the function normally.

> Websites/bugs.webkit.org/code-review.js:1115
> +  $('.comment .ok').live('click', function() {
> +    freezeComment($(this).parents('.comment'));
> +    saveDraftComments();

Maybe you're right that we can use this anonymous functions.  I'd move
"discard" and "ok" to be next to each other though.