Bug 52866 – Store draft comments in localStorage

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ojan Vafai]

Store draft comments in localStorage

[Ojan Vafai]

Created attachment 79689 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 79689 [details]
Patch

You're blowing my mind!

[Adam Barth]

Comment on attachment 79689 [details]
Patch

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.

[Ojan Vafai]

Created attachment 79821 [details]
Patch

[Ojan Vafai]

Addressed all comments. Also, made this comment saving into a standalone class so that I could more easily unittest it.

[Ojan Vafai]

ping

[Adam Barth]

Comment on attachment 79821 [details]
Patch

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-attachment-2\ngetItem:draft-comments-for-attachment-3\ngetItem:draft-comments-for-attachment-4\nremoveItem:draft-comments-for-attachment-1\nremoveItem:draft-comments-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()?

[Ojan Vafai]

Comment on attachment 79821 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79821&action=review

>> Websites/bugs.webkit.org/code-review-test.html:2
>> +<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.

Yeah, I'm not sure either. I thought about putting them in LayoutTests, but it seemed like a bad thing for a test in LayoutTests to reach out to a JS file in Websites.

>> 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-attachment-2\ngetItem:draft-comments-for-attachment-3\ngetItem:draft-comments-for-attachment-4\nremoveItem:draft-comments-for-attachment-1\nremoveItem:draft-comments-for-attachment-2\nremoveItem:draft-comments-for-attachment-3\nsetItem:draft-comments-for-attachment-1234,{MOCK JSON}');
> 
> This would be prettier in python.

It certainly would.

>> 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.

I dunno. WDYT? I think it makes the code more readable. WRT unix_hacker, sorry, it's hard for me to get in the habit of that style, although we really should switch this whole file over to webkit style one day. :)

>> Websites/bugs.webkit.org/code-review.js:156
>> +      return;
> 
> Shouldn't the DraftCommentSaver do this work?

Hm...it probably should do some of this. I've avoided having it touch the DOM to make it easier to unittest. But it certainly could have a parsedComments or something that gets an array of the parsed JSON. Will do.

>> Websites/bugs.webkit.org/code-review.js:186
>> +  window['DraftCommentSaver'] = DraftCommentSaver;
> 
> Can we make this conditional on being in the unit test?

Yes.

>> Websites/bugs.webkit.org/code-review.js:227
>> +    if (!bool) {
> 
> bool is kind of a lame variable name.  remove_comments?

Heh. Sorry. Wrote that in a fit of laziness.

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

yeah, that's def better.

>> 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.

oh, good point. that's certainly more readable.

>> Websites/bugs.webkit.org/code-review.js:1026
>> +    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()?

There can't be more than one inline listener. But, I suppose we should be able to add a capturing event listener and make the code a bit less gross. I'll play with it.

[Ojan Vafai]

Comment on attachment 79821 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79821&action=review

>>> Websites/bugs.webkit.org/code-review.js:1026
>>> +    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()?
> 
> There can't be more than one inline listener. But, I suppose we should be able to add a capturing event listener and make the code a bit less gross. I'll play with it.

Figured out why using addEventListener didn't work before. When you submit from JS (i.e. form.submit()) it doesn't fire the submit event. Anyways, I've changed it to use addEventListener and also do the same functionality before we call submit via JS.

[Ojan Vafai]

Committed: http://trac.webkit.org/changeset/77330