Closed Bug 1624984 Opened 2 years ago Closed 1 month ago

Add logging for Picture-in-Picture

Categories

(Toolkit :: Picture-in-Picture, task, P3)

task
Points:
2

Tracking

()

RESOLVED FIXED
101 Branch
Tracking Status
firefox101 --- fixed

People

(Reporter: mconley, Assigned: sabina.zaripova)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fidefe-MR1-2022])

Attachments

(2 files, 4 obsolete files)

Periodically, users will report that the toggle doesn't appear, or doesn't work properly. Sometimes, these problems come and go in a way that makes them hard to reproduce and/or diagnose.

Adding some logging might give us more data to help address these situations.

Priority: -- → P3
Blocks: 1662870

Hey Standard8, do you know what the state-of-the-art currently is wrt logging in our code? What are we supposed to be using? We notice that browser/components/urlbar is using Log.jsm, but Gijs and I both swear we remember some kind of mailing list post that described us moving off of Log.jsm to something better. Does that sound familiar?

Or bgrins, do you know?

Flags: needinfo?(standard8)
Flags: needinfo?(bgrinstead)

You want console.createInstance: https://searchfox.org/mozilla-central/search?q=console.createInstance&path=&case=false&regexp=false

Very similar to Console.jsm, but even better integrated into the console functions.

Flags: needinfo?(standard8)
Flags: needinfo?(bgrinstead)

(In reply to Mark Banner (:standard8) from comment #2)

You want console.createInstance: https://searchfox.org/mozilla-central/search?q=console.createInstance&path=&case=false&regexp=false

Very similar to Console.jsm, but even better integrated into the console functions.

Yes. I had a plan years ago to remove console.jsm by first replacing the implementation under the hood with createInstance, and then ultimately replace consumers to directly use createInstance (Bug 1430810), but unfortunately never completed it. Simplifying/unifying our logging options would be very nice.

I don't remember if we ever directly promoted console.createInstance as a replacement for Log.jsm but for most cases it should be - certainly if you are only logging to stdout / the Browser Console anyway.

Blocks: 1685549
No longer blocks: 1662870
Assignee: nobody → heftydav
Status: NEW → ASSIGNED
Component: Video/Audio Controls → Picture-in-Picture
Version: unspecified → Trunk
Whiteboard: [fidefe-MR1-2022]
Points: --- → 2
Blocks: 1742457

The bug assignee didn't login in Bugzilla in the last 7 months.
:mconley, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: heftydav → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mconley)
No longer blocks: 1742457

Hey, I wanted to submit a patch for this issue, so I wanted to clarify some aspects. Basically, I need to add logging to Picture-In-
Picture, using console.createInstance, right? Specifically, what kind of data or information should be logged?

Hi sayuree,

That's right, the logs would be made using console.createInstance. As for the type of information we need, Molly do you know what is expected here?

Flags: needinfo?(mconley) → needinfo?(mhowell)

I think there are two things I'd like to get here (I'm open to more ideas).

One thing, and the one I would start with, would be context around how we make the decision to show the PIP toggle or not, meaning in particular the logic in and around this function. Which element we're looking at, whether that element should have a toggle or not, why we made that decision the way we did, and so on.

The second thing I think would be great to have would be some logging in the site adapters, to help with troubleshooting when they fail because of changes to the site. For instance these could log if an element that the adapter is looking for by ID or class name doesn't exist, or if a site function it tries to call doesn't exist or throws an exception or something like that.

I'm sure there are other things that could be done, but those are the ones that come to mind immediately.

Flags: needinfo?(mhowell)

Depends on D142536

Assignee: nobody → sabina.zaripova
Status: NEW → ASSIGNED

(In reply to Molly Howell (she/her) [:mhowell] from comment #9)

One thing, and the one I would start with, would be context around how we make the decision to show the PIP toggle or not, meaning in particular the logic in and around this function. Which element we're looking at, whether that element should have a toggle or not, why we made that decision the way we did, and so on.

I have added logs for the count of visible videos in the viewport and whether they are worth tracking, also the element at which the mouse is pointing has been logged. I have not still figured out about the site adapters: did you mean site wrappers that need to adhere to a specific interface? Can you please look at this initial commit and tell me whether I am on the right track. I am a bit confused at the moment and it would be great to receive some direction. Thank you!

Depends on D142935

@sayuree - is your newest patch including the latest changes?

Please use only one patch for the entire bug, rather than creating new ones. You can update an existing patch using hg commit --amend or git commit --amend. You can also "pull" your patch into your local machine using the moz-phab patch <phabricator_patch_id> command.

See this Firefox Source Docs page for more information.

Flags: needinfo?(sabina.zaripova)

(In reply to kpatenio from comment #15)

@sayuree - is your newest patch including the latest changes?

Please use only one patch for the entire bug, rather than creating new ones. You can update an existing patch using hg commit --amend or git commit --amend. You can also "pull" your patch into your local machine using the moz-phab patch <phabricator_patch_id> command.

See this Firefox Source Docs page for more information.

Yes, the newest patch includes the latest changes, I think the problem is that I have used hg commit --amend -m "<some commit message>". I suggest that I do not need to write a new commit message.

Flags: needinfo?(sabina.zaripova)

Ah, yes, you're right, that would be causing this. Here's what's happening: when you submit a patch to Phabricator for review, it gets assigned an identifier, which is the thing that looks like D123456. That identifier then gets added on to the end of the commit message, so that the next time that same patch is submitted with changes made, Phabricator knows where it should put the changes. The problem here is that hg commit --amend -m is overwriting the entire commit message, including removing that identifier. So that's why it's creating a new revision every time, because it doesn't know that there already was one.

So yes, you can just use hg commit --amend without supplying a message. It will probably open a text editor where you can modify the existing message, but you don't have to do that, you can just close the editor without changing anything.

Attachment #9270855 - Attachment is obsolete: true
Attachment #9271048 - Attachment is obsolete: true
Attachment #9271630 - Attachment is obsolete: true
Attachment #9271847 - Attachment is obsolete: true
Pushed by mhowell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a32adae13aeb
added and edited logs for PiP. r=mhowell
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 101 Branch
You need to log in before you can comment on or make changes to this bug.