Closed Bug 1284887 Opened 8 years ago Closed 8 years ago

Replace references to MXR.mozilla.org in the codebase with DXR.

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: mhoye, Assigned: Towkir)

Details

Attachments

(1 file, 2 obsolete files)

Rebased patch
53.85 KB, patch
bkelly
: review+
Details | Diff | Splinter Review 
Now that MXR has been taken offline and replaced with DXR, references to mxr.mozilla.org in the codebase should be updated to point to DXR.

https://dxr.mozilla.org/mozilla-central/search?q=mxr.mozilla.org&redirect=false

This looks like a good first bug for a new contributor.
A couple observations:

The MXR links with a line number but without a revision number (eg .../foo.js#123 instead of .../foo.js?rev=xxx#123) are fundamentally wrong. They might have been correct at the moment they were originally added, but as those files change the line number will increasingly be wrong, since it will link to whatever is _currently_ at that line.

I'd suggest just changing anything without a specific line number and revision to just a non-link reference to the file. (http://mxr.mozilla.org/whatever/foo.js --> foo.js). (Or in some cases, depending on context, removing it entirely). There's not much value to trying to linkify when tools like SearchFox/DXR (to say nothing of IDEs) make it easy enough to search the codebase.
Component: MXR → General
Product: Webtools → Firefox
While I agree we should cut line numbers out (probably rev or not) I disagree about (http://mxr.mozilla.org/whatever/foo.js --> foo.js), independent of tooling support. There are a lot of identically-named files in the tree, even if you exclude tests. That said, this isn't my call to make, perhaps obviously - whose would it be?
Hello! i just saw this from the twitter feed! 
I like to try this out if its something a newbie can do; would be nice to know where to start from.
Putting links to a code indexing site in source code seems like a poor choice to me. MXR was only ever indexing the latest version of something; DXR has support for permalinks, but there's still no guarantee that links to any given thing will be durable. It really seems like one should be linking to a VCS or bug tracking system, which are intended to be long term stores of historical information (and context!), rather than a regenerated index.

What am I missing?
Hey, Bull500 - thanks for your interest; It looks like it will take a bit of discussion for us to sort out exactly what needs to be done here. Once we've figured out what that is, we'd be grateful for your help.
> I'd suggest just changing anything without a specific line number
> and revision to just a non-link reference to the file.

I'd suggest anything with a specific line number and no revision should be changed to use the revision that the link text itself was added in.

> MXR was only ever indexing the latest version of something

Indexing, yes.  Showing, no.  It could show permalinks to particular revisions of particular files, complete with highlighting some subset of lines, etc.  More flexibly than dxr, in fact (e.g. you could scroll to line 8 and highlight lines 10-15, say).

> What am I missing?

The fact that all the VCS web views around suck (e.g. have no way to highlight a particular set of lines in a file)?

The fact that we're conflating "generated index" and "non-sucky web view of a source tree at a particular revision" in both mxr and dxr is a bit unfortunate, I agree.  Those are really somewhat independent pieces of functionality.
OK, Bull500 - that looks like an answer to me, but it's going to mean you need to do some archaeology to do the whole bug.

Let's do this in two parts: Get a copy of the Firefox source and start with the links that don't have the line reference. Once that's done, we can start into the more tedious part of getting revisions right.

That sound ok?
Hi mike, I am submitting a patch soon with the 'links without line references'
Assignee: nobody → 3ugzilla
what about this one ?
this does not say a specific number but indicates some location 
https://dxr.mozilla.org/mozilla-central/source/devtools/shared/jsbeautify/src/beautify-js.js#975 [something like "around line ......"]
Attached patch dxr_on_mxr_v1.patch (obsolete) — — Splinter Review 
patch contains changes without these lines (as I had some confusions here):
https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit4/src/org/mozilla/android/sync/test/TestRecord.java#192-193,198-199

and line from comment 9 is replaced as it says "around line ...." which is not a specific line number.
Attachment #8770850 - Flags: review?(mhoye)
Comment on attachment 8770850 [details] [diff] [review]
dxr_on_mxr_v1.patch

Bumping this over to Dolske.
Attachment #8770850 - Flags: review?(mhoye) → review?(dolske)
Comment on attachment 8770850 [details] [diff] [review]
dxr_on_mxr_v1.patch

Review of attachment 8770850 [details] [diff] [review]:
-----------------------------------------------------------------

In a few places you're linking to (well, really, the old MXR links were linking to) "http://dxr.mozilla.org/mozilla" instead of "http://dxr.mozilla.org/mozilla-central/". Subtle difference, but /mozilla is an ancient frozen snapshot (iirc from when we split from CVS to Hg?). So let's use the mozilla-central links everywhere for consistency. (I noted the instances I found.)

Sorry for the delay, I'll get to the revised patch with these changes faster.

::: addon-sdk/source/python-lib/cuddlefish/version_comparator.py
@@ +9,5 @@
>        https://developer.mozilla.org/En/NsIVersionComparator
>  
>      For more information, also see:
>  
> +      http://dxr.mozilla.org/mozilla/source/xpcom/glue/nsVersionComparator.cpp

Please use dxr.mozilla.org/mozilla-central/... here.

::: layout/base/tests/test_bug386575.xhtml
@@ +12,5 @@
>  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=386575">Mozilla Bug 386575</a>
>  <p id="display">
>  
>  
> +<!-- the image is http://dxr.mozilla.org/mozilla/source/layout/reftests/bugs/solidblue.png -->

Please use dxr.mozilla.org/mozilla-central/... here.

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_sort.js
@@ +18,5 @@
>          "http://spreadfirefox.com/",
>          "http://planet.mozilla.org/",
>          "https://developer.mozilla.org/",
>          "http://hg.mozilla.org/",
> +        "http://dxr.mozilla.org/",

Technically the change here (and browser_passwordmgrdlg.js) isn't needed, because these are just arbitrary URLs used in a test. But fine, let's just purge all memory of MXR. :)

::: toolkit/content/license.html
@@ +5287,5 @@
>  
>  <pre>
>  Different parts of the US English dictionary (SCOWL) are subject to the
>  following licenses as  shown below. For additional details, sources, credits,
> +and public domain references, see <a href="http://dxr.mozilla.org/mozilla/source/extensions/spellcheck/locales/en-US/hunspell/README.txt?raw=1">README.txt</a>.

Well, this is awkward. This file doesn't actually exist now. Bug 479334 removed it 6 years ago.

Please update this link to:

https://dxr.mozilla.org/mozilla-central/source/extensions/spellcheck/locales/en-US/hunspell/README_en_US.txt

(and no raw=1)

::: toolkit/locales/en-US/chrome/global/intl.properties
@@ +36,5 @@
>  # This preference controls the initial setting of the language drop-down menu
>  # in the Content > Fonts & Colors > Advanced preference panel.
>  #
>  # Set it to the value of one of the menuitems in the "selectLangs" menulist in
> +# http://dxr.mozilla.org/mozilla/source/browser/components/preferences/fonts.xul

Please use dxr.mozilla.org/mozilla-central/... here.

::: toolkit/modules/sessionstore/XPathGenerator.jsm
@@ +91,5 @@
>     * @returns an XPath query to all savable form field nodes
>     */
>    get restorableFormNodes() {
>      // for a comprehensive list of all available <INPUT> types see
> +    // http://dxr.mozilla.org/mozilla-central/search?string=kInputTypeTable

Annoyingly, the search syntax is different now:

https://dxr.mozilla.org/mozilla-central/search?q=kInputTypeTable&redirect=false

::: xpcom/threads/SharedThreadPool.h
@@ +33,5 @@
>  // and mixing MSCOM objects between the two is terrible for performance, and can
>  // cause some functions to fail. So be careful when using Win32 APIs on a
>  // SharedThreadPool, and avoid sharing objects if at all possible.
>  //
> +// [1] http://dxr.mozilla.org/mozilla-central/search?string=coinitialize

Search syntax is different now:

https://dxr.mozilla.org/mozilla-central/search?q=coinitialize&redirect=false
Attachment #8770850 - Flags: review?(dolske) → review-
Attached patch mxr_to_dxr_v1.patch (obsolete) — — Splinter Review 
According to your feedback here is the updated patch.

Thanks
Attachment #8770850 - Attachment is obsolete: true
Attachment #8777905 - Flags: review?(dolske)
Comment on attachment 8777905 [details] [diff] [review]
mxr_to_dxr_v1.patch

Review of attachment 8777905 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Thanks for the patch!
Attachment #8777905 - Flags: review?(dolske) → review+
What's the plan for the other MXR links (per comment 10, sounds like you're just ignoring those for now). Should we keep this bug open after landing this, for another patch from you?
I was asking for your opinion on this, I can submit a patch, see comment 9 too
but I needed your feedback
Sorry, I updated the line mentioned on comment 9 
so now, only the one in comment 10 is remaining.
Shall I submit another patch now or after landing this one ?
Flags: needinfo?(dolske)
I think you should get the current patch landed ASAP so it doesn't bitrot. A second patch fixing the remaining instances is fine.

(In reply to [:Towkir] Ahmed from comment #10)

> patch contains changes without these lines (as I had some confusions here):
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/
> background/junit4/src/org/mozilla/android/sync/test/TestRecord.java#192-193,
> 198-199

These don't strictly need to be updated, since they're just dummy URLs used in a test.
Flags: needinfo?(dolske)
okay then
Keywords: checkin-needed
Flagging for a reminder only
Flags: needinfo?(wkocher)
This appears to have bitrotted :(. Please rebase when you get a chance.
Flags: needinfo?(wkocher)
Keywords: checkin-needed
Attached patch Rebased patch — — Splinter Review 
Some android files got moved around, which could make rebasing confusing, so here's a rebased patch all ready-made. I'll land this in a second so it doesn't get bitrotted again. 

Sorry for the delay getting this landed the first time around. :)
Attachment #8777905 - Attachment is obsolete: true
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4af6ac9ead4a
Replaced references to mxr.mozilla.org in the codebase with dxr.mozilla.org r=dolske
Comment on attachment 8781314 [details] [diff] [review]
Rebased patch

These technically change webidl files, so I'm getting rejected by the hook because there's no DOM peer review, but all of these changes are only in the comments in those files, so I'm going to land this now using the "a=release" exception to bypass the need for review and beg for forgiveness later. :)

Bkelly, feel free to rubberstamp this or reprimand me as you see fit.
Attachment #8781314 - Flags: review?(bkelly)
Comment on attachment 8781314 [details] [diff] [review]
Rebased patch

Review of attachment 8781314 [details] [diff] [review]:
-----------------------------------------------------------------

I only looked at the one webidl change.  LGTM.  Thanks!
Attachment #8781314 - Flags: review?(bkelly) → review+
https://hg.mozilla.org/mozilla-central/rev/4af6ac9ead4a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
[bugday-1284887] bug verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: