[webkit-dev] code review tool changes

[Oliver Hunt]

I just discovered that the review tool doesn't save the general comments, only inline ones (http://webkit.org/b/54121).  This just got me when I clicked on the style bot to see what style checks had failed and lost my feedback :(

This was particularly annoying as i had command-clicked the link (to open in a new tab) and the link actually forced the current page to navigate instead - http://webkit.org/b/54113

--Oliver

On Feb 8, 2011, at 2:39 PM, Ojan Vafai wrote:

> On Wed, Feb 9, 2011 at 7:59 AM, Kenneth Russell <kbr at google.com> wrote:
> On Tue, Feb 1, 2011 at 6:02 PM, Kenneth Russell <kbr at google.com> wrote:
> > On Tue, Feb 1, 2011 at 5:09 PM, Ojan Vafai <ojan at chromium.org> wrote:
> >> There's been a slew of changes to the code review tool. It's probably a good
> >> time to send a summary now that I don't intend to add significant new
> >> features.
> >> -Side-by-side diffs: You can view the entire diff or individual files in
> >> side-by-side diff. If you change the entire diff, we'll store that in
> >> localstorage and load diffs in side-by-side by default.
> >> -Comments and diff navigation: n/p keys will navigate to the next/previous
> >> comment. j/k will navigate to the next/previous diff.
> >> -Draft comments persist: draft comments are now stored in localstorage, so
> >> they will persist across reloads, crashes, etc. Since it's in localstorage
> >> it's stored per-machine.*
> >
> > Thank you for this in particular. Several times I've accidentally
> > navigated away from a review and lost work.
> 
> I just temporarily lost Internet connectivity while uploading a
> (large) review and when I came back to the review (same browser, etc.)
> all of my comments were lost. Is this functionality expected to work
> with Chrome 9 stable?
> 
> When you click the publish button, we clear all the saved drafts. :( Otherwise we'd continue showing the draft comments alongside the published ones. I suppose I should find a way to make that happen when the publish actually succeeds instead. 
>  
> 
> Thanks,
> 
> -Ken
> 
> > -Ken
> >
> >> -Expand diff context: You can expand the lines above/below a diff to see
> >> more context.**
> >> Hope this works well for you all. Obviously, file bugs if something isn't
> >> working.
> >> Ojan
> >> * Taking the next step and storing them online for non-security bugs (e.g.
> >> in S3 or appengine) would be a simple and welcome addition if someone feels
> >> moved.
> >> ** This currently only works if the patch includes the svn revision it was
> >> created at (i.e. it was created with SVN) or applies cleanly to trunk.
> >> Including the SVN revision in git diffs shouldn't be too hard. Again, if you
> >> feel moved to fix this, ping me.
> >> _______________________________________________
> >> webkit-dev mailing list
> >> webkit-dev at lists.webkit.org
> >> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
> >>
> >>
> >
> 
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20110209/649bff43/attachment.html>