1624984 - Add logging for Picture-in-Picture

Status: RESOLVED FIXED

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

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 2

[Mark Banner (:standard8)]

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.

Comment 3

[Brian Grinstead [:bgrins]]

(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.

Comment 4

[David]

Attachment: WIP: Bug 1624984 Adding logging to Picture-in-Picture

Comment 5

[David]

https://treeherder.mozilla.org/push-health/push?revision=34b3c17a24f678d882f96c140ee1cf3aef3ac0c5&repo=try&testGroup=pr

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 7

[sayuree]

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?

Comment 8

[kpatenio]

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?

Comment 9

[Molly Howell (she/her) (mostly inactive)]

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.

Comment 10

[sayuree]

Attachment: Bug 1624984 - Added logs for PiP. r=#pip-reviewers

Depends on D142536

Comment 11

[sayuree]

(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!

Comment 12

[sayuree]

Attachment: Bug 1624984 - Updated PiP logs. r=mhowell

Depends on D142935

Comment 13

[sayuree]

Attachment: Bug 1624984 - added and edited logs for PiP. r=mhowell

Comment 14

[sayuree]

Attachment: Bug 1624984 - added and edited logs for PiP. r=mhowell

Comment 15

[kpatenio]

@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.

Comment 16

[sayuree]

Attachment: Bug 1624984 - added and edited logs for PiP. r=mhowell

Comment 17

[sayuree]

(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.

Comment 18

[Molly Howell (she/her) (mostly inactive)]

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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a32adae13aeb
added and edited logs for PiP. r=mhowell

Comment 20

[Iulian Moraru]

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