Bug 63963 - Would be nice if the review-tool offered to show the corresponding header
Summary: Would be nice if the review-tool offered to show the corresponding header
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-05 17:58 PDT by Eric Seidel (no email)
Modified: 2011-10-13 18:39 PDT (History)
4 users (show)

See Also:


Attachments
Untested, probably broken (844 bytes, patch)
2011-07-05 22:23 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (1.96 KB, patch)
2011-10-13 16:49 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (3.68 KB, patch)
2011-10-13 17:16 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2011-07-05 17:58:58 PDT
Would be nice if the review-tool offered to show the corresponding header

Similar to how XCode has a "show me the corresponding file" button for headers/cpps, would be nice if the review-tool knew how to show the corresponding header/cpp (probably in an iframe) to the current file under review.
Comment 1 Ojan Vafai 2011-07-05 18:04:43 PDT
Sounds great. We already have all the code in there to pull source files from trak. I'm trying to think what the UI would look like. We could add the header file to the diff, but just not have it allow comments?
Comment 2 Adam Barth 2011-07-05 20:58:32 PDT
In the links that appear on the right (like "blame") you could have a link called "header" that went to trac.
Comment 3 Adam Barth 2011-07-05 22:23:33 PDT
Created attachment 99793 [details]
Untested, probably broken
Comment 4 Ojan Vafai 2011-07-06 00:18:59 PDT
Comment on attachment 99793 [details]
Untested, probably broken

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

Code seems fine to me. Do we also want to show link to the .cpp file for .h files?

> Websites/bugs.webkit.org/code-review.js:628
> +      stem = file_name.substr(0, file_name.length - 'cpp'.length);

Need to declare "stem".
Comment 5 Adam Barth 2011-07-06 00:28:13 PDT
We'll probably also want to handle ".mm" and possibly FooMac.mm => Foo.h.  I'm not sure how much of that to bite off in the first iteration.
Comment 6 Adam Barth 2011-10-13 16:49:47 PDT
Created attachment 110933 [details]
Patch
Comment 7 Ojan Vafai 2011-10-13 16:55:35 PDT
Comment on attachment 110933 [details]
Patch

Code looks fine. Can you add a test? code-review-test.html
Comment 8 Adam Barth 2011-10-13 17:02:32 PDT
> Can you add a test?

What fun would that be!  :)
Comment 9 Adam Barth 2011-10-13 17:16:38 PDT
Created attachment 110940 [details]
Patch
Comment 10 WebKit Review Bot 2011-10-13 18:39:31 PDT
Comment on attachment 110940 [details]
Patch

Clearing flags on attachment: 110940

Committed r97435: <http://trac.webkit.org/changeset/97435>
Comment 11 WebKit Review Bot 2011-10-13 18:39:36 PDT
All reviewed patches have been landed.  Closing bug.