Remove expired Scalar `browser.engagement.restored_pinned_tabs_count`
Categories
(Toolkit :: Telemetry, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox87 | --- | fixed |
People
(Reporter: chutten, Assigned: tanweerali908, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: [good first bug][lang=c++])
Attachments
(1 file, 3 obsolete files)
To help Mozilla out with this bug, here's the steps:
- Comment here on the bug that you want to volunteer to help. I (or someone else) will assign it to you.
- Download and build the Firefox source code
- If you have any problems, please ask on Element/Matrix in the
#introduction
channel. They're there to help you get started. - You can also read the Developer Guide, which has answers to most development questions:
- If you have any problems, please ask on Element/Matrix in the
- Start working on this bug. You will need to remove these lines in
toolkit/components/telemetry/Scalars.yaml
and these lines intoolkit/components/telemetry/core/TelemetryCommon.cpp
.- If you have any problems with this bug, please comment on this bug and set the needinfo flag for me. Also, you can find me and my teammates on the
#telemetry
channel on Element/Matrix most hours of most days.
- If you have any problems with this bug, please comment on this bug and set the needinfo flag for me. Also, you can find me and my teammates on the
- Build your change with
mach build
and test your change withmach test toolkit/components/telemetry/tests/
. Also check your changes for adherence to our style guidelines by usingmach lint
- Submit the patch for review. Mark me as a reviewer so I'll get an email to come look at your code.
- After a series of reviews and changes to your patch, I'll mark it for checkin or push it to autoland. Your code will soon be shipping to Firefox users worldwide!
- ...now you get to think about what kind of bug you'd like to work on next. Let me know what you're interested in and I can help you find your next contribution.
Assignee | ||
Comment 1•3 years ago
|
||
Hey
I want to work on this bug.
Tanweer Ali
Assignee | ||
Updated•3 years ago
|
Reporter | ||
Comment 2•3 years ago
|
||
Excellent. I have assigned the bug to you and see you found us on #telemetry. Let me know if I can help out.
Assignee | ||
Comment 3•3 years ago
|
||
Assignee | ||
Comment 4•3 years ago
|
||
Hey Chutten,
I have just submitted a patch. Can you review it please.
Tanweer
Assignee | ||
Comment 5•3 years ago
|
||
Reporter | ||
Comment 6•3 years ago
|
||
Well that's odd. Lando couldn't apply your patches. Were you editing the most recent versions of these files?
Oh, something just recently landed that might've gotten in the way... Ack! I'd forgotton, I'd already encouraged :padenot to remove this Scalar when he was adjusting the MsSinceProcessStart code in bug 1205985 (which just landed a few days ago).
Blast, it appears as though I shouldn't have filed this bug after all.
If it's alright with you, we can rework this patch to remove a different Scalar (this one I assure you I haven't accidentally asked someone else to remove : ) ). This time it's restored_pinned_tabs_count
which you can find in Scalars.yaml
and in browser/components/sessionstore/SessionStartup.jsm
What do you think?
Assignee | ||
Comment 8•3 years ago
|
||
https://phabricator.services.mozilla.com/D102092
Here is the patch which you approved now.
Reporter | ||
Comment 9•3 years ago
|
||
I mean that the patch for bug 1205985 which landed just a couple days ago already removed the code that your patch is removing. Your patch is operating on a not-quite-up-to-date version of the code, so it still has it.
Unfortunately it means that D102092 isn't needed any more. Which is 100% my fault for filing this bug after forgetting that I'd already recommended the code be removed in bug 1205985.
But there's another completely different Scalar that you could remove instead, called browser.engagement.restored_pinned_tabs_count
. You could remove it instead, if you'd like.
Assignee | ||
Comment 10•3 years ago
|
||
Ok got it. So first I should do hg pull
and then hg up central
. After that I should work on this bug.
Reporter | ||
Comment 11•3 years ago
|
||
Yes, if you don't mind switching over to remove that other Scalar.
Assignee | ||
Comment 12•3 years ago
|
||
Yeah sure I will do that after fixing BUG 1650304
. Actually I got assigned another Bug and It has been more than one week so After fixing that one I will work on this. Thanks for assigning me this.
Assignee | ||
Comment 13•3 years ago
|
||
Hey Chris,
It would be easy for me if you just brief me about how to remove browser.engagement.restored_pinned_tabs_count
Reporter | ||
Comment 14•3 years ago
|
||
Certainly.
Much like the original task, this is a removal of an expired Scalar from both Scalars.yaml and from the code that puts data into it. Scalars.yaml is still at toolkit/components/telemetry/Scalars.yaml
and you're looking to remove these lines.
As for where that scalar is used, it's used in browser/components/sessionstore/SessionStartup.jsm
. It looks like you only need to remove these lines, but definitely remember to run the linter with mach lint
to make sure there won't be other code that becomes unreachable once you remove the obvious stuff.
Does that help?
Assignee | ||
Comment 15•3 years ago
|
||
Yeah Thanks for that
Assignee | ||
Comment 16•3 years ago
|
||
Depends on D102265
Assignee | ||
Comment 17•3 years ago
|
||
Hey Chris, I have submitted a patch for this.
Assignee | ||
Comment 18•3 years ago
|
||
Depends on D102265
Comment 19•3 years ago
|
||
Pushed by chutten@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eaa5d6a68fd4 [Telemetry] Remove expired Scalar browser.engagement.restored_pinned_tabs_count. r=chutten
Comment 20•3 years ago
|
||
bugherder |
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Description
•