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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted 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 79821: Patch
https://bugs.webkit.org/attachment.cgi?id=79821&action=review

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

I'm very excited about the tests in this patch.  Some minor style /
organizational nits below.

> Websites/bugs.webkit.org/code-review-test.html:2
> +<div>Tests for some of the easily unittestable parts of code-review.js. You
should see a series of "PASSED" lines.</div>
> +<div>FIXME: Run these as part of the layout test suite?</div>

Crazy!	I'm not sure if this is the ideal location for this file, but I'm not
sure what's better.

> Websites/bugs.webkit.org/code-review-test.html:92
> +ASSERT_EQUAL(ls.json(), 'QuotaExceeded on
setItem:draft-comments-for-attachment-1234,{MOCK
JSON}\ngetItem:draft-comments-for-attachment-1\ngetItem:draft-comments-for-atta
chment-2\ngetItem:draft-comments-for-attachment-3\ngetItem:draft-comments-for-a
ttachment-4\nremoveItem:draft-comments-for-attachment-1\nremoveItem:draft-comme
nts-for-attachment-2\nremoveItem:draft-comments-for-attachment-3\nsetItem:draft
-comments-for-attachment-1234,{MOCK JSON}');

This would be prettier in python.

> Websites/bugs.webkit.org/code-review.js:72
> +  var g_displayedDraftComments = false;

Should we add g_ to the other global variables?  Also, we've been using
unix_hacker for variable names.

> Websites/bugs.webkit.org/code-review.js:156
> +    g_displayedDraftComments = true;
> +    var serialized_comments =
localStorage.getItem(DraftCommentSaver._keyPrefix + attachment_id);
> +    if (!serialized_comments)
> +	 return;

Shouldn't the DraftCommentSaver do this work?

> Websites/bugs.webkit.org/code-review.js:186
> +  // Export this so we can unittest it.
> +  window['DraftCommentSaver'] = DraftCommentSaver;

Can we make this conditional on being in the unit test?

> Websites/bugs.webkit.org/code-review.js:227
> +    var bool = this._prompt('Local storage quota is full. Remove draft
comments from all reviews to make room?');
> +    if (!bool) {

bool is kind of a lame variable name.  remove_comments?

> Websites/bugs.webkit.org/code-review.js:228
> +	 this._saveComments = false;

We don't really have a strong style guide here, but I would have expected
_unix_hacker for private member variable names.

> Websites/bugs.webkit.org/code-review.js:241
> +    return prompt('Local storage quota is full. Remove draft comments from
all reviews to make room?');;

extra semicolon.  "all previous reviews" ?

> Websites/bugs.webkit.org/code-review.js:270
> +    var draft_comment_keys = [];

draft_comment_keys => keys_to_delete ?

> Websites/bugs.webkit.org/code-review.js:273
> +	 if (key.indexOf(DraftCommentSaver._keyPrefix) == 0) {

Totally a style nit, but I would have reversed this condition and used a
continue, mostly to conserve nesting depth.

> Websites/bugs.webkit.org/code-review.js:1026
> +    var old_submit = form.onsubmit;
> +    form.onsubmit = function() {

This is kind of yuck.  What if there's more than one listener?	Can't we just
use addEventListener and then not cancel the event?  Better yet, why not just
use bind()?