1121224 - Have each file in a change link to dxr, if possible

Status: RESOLVED INVALID

(MozReview Graveyard :: General, enhancement, P2)

Description

[Mike Conley (:mconley) (:⚙️)]

Part of reviewing is getting more context - context that dxr / mxr makes easier to find by allowing you to quickly navigate around in the code and find out who calls what.

We should make it easier for our reviewers to use those tools from within MozReview. Somehow.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Can dxr link to a particular version of a file? If not, mxr might be better option.

Comment 2

[Gregory Szorc [:gps]]

MXR is at its end of life. We'll need to find a way to make this work with DXR.

Comment 3

[Erik Rose [:erik][:erikrose]]

DXR doesn't currently have revisioned links, but they're coming sometime this year. Q2?

Comment 4

[Mark Côté [:mcote]]

We have too many P1s, so I'm spreading out the priorities.  P3 -> P4, P2 -> P3, and some portion of P1s will become P2.

Comment 5

[Nils Ohlmeier [:drno]]

Not sure if feasible, but I would think that I simply highlight some text in mozreview and then right click which should then open a pop-up menu with the option to search for that string in dxr. That would mimic how I'm currently copy and pasting string between mzoreview and dxr when doing reviews.

Comment 6

[Gregory Szorc [:gps]]

This was blocked on having a custom repo provider in Review Board so we could track additional metadata for each repository (in this case the DXR "repo" to point back to). This bug is now unblocked.

Lowest hanging fruit is inserting a link to the file on DXR, which is what this bug initially requested (I think there are other bugs for more advanced DXR integration).

Comment 7

[Dan Minor [:dminor]]

Mauro - sorry, I missed what you were asking about in IRC. If you want to store the DXR link, you can add it as a field to the custom hosting service we use for mozilla repos [1]. The values just show up in the extra_data for the repository, like this [2].

[1] https://dxr.mozilla.org/hgcustom_version-control-tools/source/pylib/mozreview/mozreview/hostingservice/hmo_repository.py
[2] https://dxr.mozilla.org/hgcustom_version-control-tools/source/pylib/mozreview/mozreview/autoland/resources.py#136

Comment 8

[Gregory Szorc [:gps]]

Bumping up the priority since this is now a much easier task since we have a custom hosting provider, DXR results for multiple revisions, and a number of people have mentioned DXR integration as potentially having a huge impact.

Comment 9

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: add dxr_repository_url to hmo repository (bug 1121224); r?dminor

This will be used to link files contained in a review request to dxr.mozilla.org

Review commit: https://reviewboard.mozilla.org/r/31437/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31437/

Comment 10

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: add link to files on dxr (bug 1121224); r?smacleod

The dxr link should be only available for files already existing and not for new files.
It's still possible that a file created very recently and modified by a review request is not present
yet on dxr.mozilla.org because it still has to be indexed.

Review commit: https://reviewboard.mozilla.org/r/31439/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31439/

Comment 11

[Dan Minor [:dminor]]

Comment on attachment 8709416 [details]
MozReview Request: mozreview: add dxr_repository_url to hmo repository (bug 1121224); r?dminor

https://reviewboard.mozilla.org/r/31437/#review28121

lgtm

::: pylib/mozreview/mozreview/hostingservice/hmo_repository.py:47
(Diff revision 1)
> +    dxr_repository_url = forms.CharField(

I feel like "code search" might be a better way of putting this than repository here, so it would be dxr_codesearch_url. Feel free to drop if you disagree, naming things has never been my strong point :)

::: pylib/mozreview/mozreview/tests/test-hostingservice.py:86
(Diff revision 1)
> +        el = self.browser.find_element_by_id('id_dxr_repository_url')

Unless I'm missing something, it looks like you don't create this element until the next commit in the series, so this test needs to be moved there.

::: testing/vcttesting/unittest.py:372
(Diff revision 1)
> +        el = self.browser.find_element_by_id('id_dxr_repository_url')

As above, I don't think this element exists yet.

Comment 12

[Mauro Doglio [:mdoglio]]

https://reviewboard.mozilla.org/r/31439/#review28129

::: pylib/mozreview/mozreview/tests/test-dxr-link.py:61
(Diff revision 1)
> +                         'rev/UNKNOWN/foo')

I don't know how to get a real revision instead of UNKNOWN. :smacleod do you have any idea?

Comment 13

[Mauro Doglio [:mdoglio]]

https://reviewboard.mozilla.org/r/31437/#review28121

> Unless I'm missing something, it looks like you don't create this element until the next commit in the series, so this test needs to be moved there.

I thought this element was created automatically in the admin site when you add a new field to the custom hosting provider.

Comment 14

[Erik Rose [:erik][:erikrose]]

With regard to "code search", just note that, while DXR can now show files of any revision, it indexes only the latest for search.

Comment 15

[Mauro Doglio [:mdoglio]]

Comment on attachment 8709416 [details]
MozReview Request: mozreview: add dxr_repository_url to hmo repository (bug 1121224); r?dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31437/diff/1-2/

Comment 16

[Mauro Doglio [:mdoglio]]

Comment on attachment 8709417 [details]
MozReview Request: mozreview: add link to files on dxr (bug 1121224); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31439/diff/1-2/

Comment 17

[Mauro Doglio [:mdoglio]]

Comment on attachment 8709416 [details]
MozReview Request: mozreview: add dxr_repository_url to hmo repository (bug 1121224); r?dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31437/diff/2-3/

Comment 18

[Mauro Doglio [:mdoglio]]

Comment on attachment 8709417 [details]
MozReview Request: mozreview: add link to files on dxr (bug 1121224); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31439/diff/2-3/

Comment 19

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/31437/#review28913

::: pylib/mozreview/mozreview/fields.py:334
(Diff revision 3)
> +class DxrFileLinkField(BaseReviewRequestField):
> +    """This field provides a mapping between file names and links to dxr."""

This should not be a field. The intended use of fields is to store data as part of a review request/draft. If we're not actually taking advantage of this storage, or even rendering as part of the fields on the review request, we really should be using something else.

I think a `TemplateHook` should be suitable for loading the data you need.

::: pylib/mozreview/mozreview/fields.py:363
(Diff revision 3)
> +        return get_template('mozreview/dxr_file_link_data.html').render(

This file doesn't exist in this commit.

Comment 20

[Steven MacLeod [:smacleod]]

Comment on attachment 8709417 [details]
MozReview Request: mozreview: add link to files on dxr (bug 1121224); r?smacleod

https://reviewboard.mozilla.org/r/31439/#review28915

::: pylib/mozreview/mozreview/extension.py:125
(Diff revision 3)
> +                                 'mozreview/js/init_dxrfilelink.js',

This will be loaded on the reviews page as well as the diffs page. Have you checked that it isn't interacting strangely with the diff comments in reviews?

Part of me thinks we might want to have a diff page only bundle with the js just for that page (such as the filediffreviewer stuff).

::: pylib/mozreview/mozreview/extension.py:257
(Diff revision 3)
>          ReviewRequestFieldsHook(self, 'main', [FileDiffReviewerField])

This also should be made into a `TemplateHook` rather than a review request field. That's beyond the scope of this change but could you at minimum file a bug to take care of it later?

::: pylib/mozreview/mozreview/static/mozreview/css/review.less:143
(Diff revision 3)
> +span.dxr-link-wrapper {
> +    margin-left: 20px;
> +    font-size: 8pt;
> +    font-weight: normal;
> +    text-decoration: underline;
> +    & a {
> +        &:standard {color: #0000EE;}
> +        &:visited {color: #551A8B;}
> +        &:active {color: #EE0000;}
> +    }
> +
> +}

I would love some screenshots :)

::: pylib/mozreview/mozreview/tests/test-dxr-link.py:36
(Diff revision 3)
> +            lr = self.create_basic_repo('[e-mail]', 'mjane')
> +            lr.write('foo', 'first change')
> +            lr.run(['commit', '-m', 'Initial commit'])
> +            lr.run(['phase', '--public', '-r', '.'])
> +            lr.write('foo', 'second change')
> +            lr.run(['commit', '-m', 'Bug 1 - Another test try; r?jsmith'])
> +            lr.run(['push', '--config', 'reviewboard.autopublish=true'])

We're definitely going to want to test cases with filediffs which represent:
a) move
b) move and modify
b) copy
c) copy and modify
d) modify (which you're doing)

These can all render / act differently and I wouldn't be surprised if we break down (maybe with filenames to grab etc.) in some of these cases.

::: pylib/mozreview/mozreview/tests/test-dxr-link.py:61
(Diff revision 3)
> +                         'rev/UNKNOWN/foo')

What revision to use really depends on where we want to link to, and what dxr is indexing. Best case would be DXR indexing the review repository but I have no idea if that's feasible.

1) Link to the review repositories revision for the commit associated with the current diff: The commit id will be present in the extra data for the review request, `p2rb.commit_id` I think.
2) Link to the review repositories parent commit of the current commit (Which could also be a commit under review): The diffset has a `base_commit_id` property which contains this revision
3) Link to the review repositories parent commit for the entire series (While this commit might also be present in mozilla-central, I'd guess it usually isn't): The diffset for the first commit's `base_commit_id` property, or the squashed diffset's `base_commit_id`
4) Link to the "first public ancestor" on mozilla central. We store `p2rb.first_public_ancestor` in the extra data for each commit, and the squashed review request, which is the identifier for the first commit which is phase public when walking up the DAG. This *could* be the parent, this *could* be some far ancestor. While it's also possible this exists only on the review repo, it's much more likely to actually be present on mozilla-central (It's also possible that there are quite a few commits between this and the review series).


TBH, all of the above options have issues, but the one I think that's least likely to break is 4. It's also most likely to be decieving when it doesn't break.

Comment 21

[BMO Automation]

MozReview is now obsolete. Please use Phabricator instead. Closing this bug.