Bug 63963

Summary:

Would be nice if the review-tool offered to show the corresponding header

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

Tools / Tests

Assignee:

Adam Barth <abarth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, aroben, ojan, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Untested, probably broken	none
Patch	none
Patch	none

Eric Seidel (no email)

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

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Ojan Vafai

Comment 1 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?

Adam Barth

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

Adam Barth

Comment 3 2011-07-05 22:23:33 PDT

Created attachment 99793 [details] Untested, probably broken

Ojan Vafai

Comment 4 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".

Adam Barth

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

Adam Barth

Comment 6 2011-10-13 16:49:47 PDT

Created attachment 110933 [details] Patch

Ojan Vafai

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

Adam Barth

Comment 8 2011-10-13 17:02:32 PDT

> Can you add a test? What fun would that be! :)

Adam Barth

Comment 9 2011-10-13 17:16:38 PDT

Created attachment 110940 [details] Patch

WebKit Review Bot

Comment 10 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>

WebKit Review Bot

Comment 11 2011-10-13 18:39:36 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.