Closed Bug 1150312 Opened 5 years ago Closed 5 years ago

Check if MOZ_SHARK is still used and if not remove it

Categories

(Firefox Build System :: General, defect)

x86_64
macOS
defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: robert.strong.bugs, Assigned: gioyik, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 1 obsolete file)

I asked a couple of people that are in the know and no one knew whether MOZ_SHARK is still needed. If it isn't it then it can be removed.
Probably worth asking dev.platform first, but if no one jumps up and down we should just remove it and see who complains :)
Whiteboard: [good first bug][mentor=:rstrong]
Still seeing email going by about this: can I get a go/no-go call on this so I can surface it to the community?
Flags: needinfo?(robert.strong.bugs)
I haven't heard anything but speculation that someone somewhere might possibly be using it. In the absence of evidence I say we remove it.
As I understand it there are also alternatives. Go for it
Flags: needinfo?(robert.strong.bugs)
If you or someone else would prefer to mentor this feel free to take over
Sheppy asked in the dev-platform thread whether the tenfourfox people should be contacted for this, and there is evidence that the tenfourfox people have seen the thread (http://tenfourfox.blogspot.jp/2015/04/beware-undead-snow-leopard.html), so I guess they would have complained if they were actively using shark. That said, better safe than sorry, so let's get an explicit statement. Cameron?
Flags: needinfo?(spectre)
Yes, I'm aware. The MOZ_SHARK code appears to be specific to actually controlling Shark, whereas I just use Shark to attach to a running application and take samples, so I don't think removing it should affect TenFourFox work. However, I'd prefer this was not backported to 38ESR just in case, since I'm actively working on that port.
Flags: needinfo?(spectre)
Mentor: robert.strong.bugs
Whiteboard: [good first bug][mentor=:rstrong] → [good first bug]
Hi,

I am seeing many MOZ_SHARK on source code [1]. I want to make a patch for this, so could you tell me which one should be removed?

Thanks

[1]: http://mxr.mozilla.org/mozilla-central/search?string=MOZ_SHARK
Flags: needinfo?(robert.strong.bugs)
MOZ_SHARK is no longer needed so all.
Flags: needinfo?(robert.strong.bugs)
Attached patch 1150312.patch (obsolete) — Splinter Review
Attachment #8638094 - Flags: review?(robert.strong.bugs)
Hi Robert,

I submitted a patch. Could you review it and tell me if is ok?

Thanks
Comment on attachment 8638094 [details] [diff] [review]
1150312.patch

Nice... looks good to me.

Let's also have a build peer review this.
Attachment #8638094 - Flags: review?(robert.strong.bugs)
Attachment #8638094 - Flags: review?(mh+mozilla)
Attachment #8638094 - Flags: feedback+
Thanks, I will assign this bug to myself to keep it following.
Assignee: nobody → gioyik
Comment on attachment 8638094 [details] [diff] [review]
1150312.patch

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

Thank you for your patch.

"Check if MOZ_SHARK is still used and if not remove it" does not describe what the change is about. Please change this to e.g. "Remove MOZ_SHARK".

::: js/src/configure.in
@@ -3231,5 @@
> -    MOZ_PROFILING=1
> -    AC_DEFINE(MOZ_SHARK)
> -fi
> -
> -dnl ========================================================

There's an AC_SUSBT(MOZ_SHARK) leftover at line 3702.
Attachment #8638094 - Flags: review?(mh+mozilla) → review+
Attached patch 1150312.patchSplinter Review
Updated patch
Attachment #8638255 - Flags: review?(mh+mozilla)
Hi Mike,

I added the change you mentioned in the comment and changed the description in the patch. I usually put as description bug title, but I have not problem to change with a more descriptive text.

Let me know if this one is ok and good to land.

Thanks
Attachment #8638255 - Flags: review?(mh+mozilla) → review+
So, should I add checkin-needed keyword now? or there's something else to do?
Flags: needinfo?(mh+mozilla)
That works and thanks again!
Flags: needinfo?(mh+mozilla)
Keywords: checkin-needed
Attachment #8638094 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/926651591417
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Duplicate of this bug: 473563
Duplicate of this bug: 628153
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.