Closed Bug 1685808 Opened 5 months ago Closed 4 months ago

Remove expired Scalar `browser.engagement.restored_pinned_tabs_count`

Categories

(Toolkit :: Telemetry, task, P3)

Product:
Toolkit
Component:
Telemetry
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
87 Branch
Tracking Flags:
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)

Bug 1685808 - [Telemetry] Remove expired Scalar browser.engagement.restored_pinned_tabs_count. r=chutten
48 bytes, text/x-phabricator-request
Details | Review

To help Mozilla out with this bug, here's the steps:

  1. Comment here on the bug that you want to volunteer to help. I (or someone else) will assign it to you.
  2. 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:
  3. Start working on this bug. You will need to remove these lines in toolkit/components/telemetry/Scalars.yaml and these lines in toolkit/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.
  4. Build your change with mach build and test your change with mach test toolkit/components/telemetry/tests/. Also check your changes for adherence to our style guidelines by using mach lint
  5. Submit the patch for review. Mark me as a reviewer so I'll get an email to come look at your code.
  6. 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!
  7. ...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.

Hey
I want to work on this bug.

Tanweer Ali

Flags: needinfo?(chutten)

Excellent. I have assigned the bug to you and see you found us on #telemetry. Let me know if I can help out.

Assignee: nobody → tanweerali908
Flags: needinfo?(chutten)

Hey Chutten,
I have just submitted a patch. Can you review it please.
Tanweer

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?

Flags: needinfo?(tanweerali908)

Sorry, I didnt get you

Flags: needinfo?(tanweerali908)

https://phabricator.services.mozilla.com/D102092

Here is the patch which you approved now.

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.

Flags: needinfo?(tanweerali908)

Ok got it. So first I should do hg pull and then hg up central. After that I should work on this bug.

Flags: needinfo?(tanweerali908)

Yes, if you don't mind switching over to remove that other Scalar.

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.

Hey Chris,
It would be easy for me if you just brief me about how to remove browser.engagement.restored_pinned_tabs_count

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?

Status: NEW → ASSIGNED
Summary: Remove expired Scalar `telemetry.process_creation_timestamp_inconsistent` → Remove expired Scalar `browser.engagement.restored_pinned_tabs_count`

Yeah Thanks for that

Hey Chris, I have submitted a patch for this.

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

https://hg.mozilla.org/mozilla-central/rev/eaa5d6a68fd4

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
status-firefox87: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch
Attachment #9198819 - Attachment is obsolete: true
Attachment #9196590 - Attachment is obsolete: true
Attachment #9197587 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.