Bug 52253 - remember diffstate for review tool
Summary: remember diffstate for review tool
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-11 14:14 PST by Ojan Vafai
Modified: 2011-01-11 15:57 PST (History)
3 users (show)

See Also:


Attachments
Patch (1.94 KB, patch)
2011-01-11 14:16 PST, Ojan Vafai
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2011-01-11 14:14:35 PST
remember diffstate for review tool
Comment 1 Ojan Vafai 2011-01-11 14:16:27 PST
Created attachment 78598 [details]
Patch
Comment 2 Ojan Vafai 2011-01-11 14:26:39 PST
This builds on the patch in bug 52226.
Comment 3 Adam Barth 2011-01-11 14:36:34 PST
Comment on attachment 78598 [details]
Patch

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

> Websites/bugs.webkit.org/code-review.js:399
> +      localStorage.diffstate = difftype;

Can we use setItem and getItem?  Also, we should use a name like code-review-diffstate that scopes the state to the code review tool.
Comment 4 Adam Barth 2011-01-11 14:50:35 PST
Comment on attachment 78598 [details]
Patch

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

This patch is fine.  It could use slightly more polish though.

> Websites/bugs.webkit.org/code-review.js:744
> +    updateDiffState();

updateDiffState sounds like I can call this whenever I like, but this is really a load-time function.  Maybe loadDiffState?

> Websites/bugs.webkit.org/code-review.js:749
> +    if (localStorage.diffstate != 'sidebyside')
> +      return;

It seems like we should be more explicit here.  Maybe check that we've gotten one of the two expected values and then take action.

> Websites/bugs.webkit.org/code-review.js:753
> +    $('.FileDiff').each(function() {
> +      convertFileDiff('sidebyside', this);
> +    });

This chunk of code seems like it should be shared with the side-by-side link, right?
Comment 5 Ojan Vafai 2011-01-11 15:57:01 PST
Committed r75565: <http://trac.webkit.org/changeset/75565>