New issue
Advanced search Search tips

Issue 688607 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Provide a button to remove all player in about://media-internals

Project Member Reported by xhw...@chromium.org, Feb 4 2017

Issue description

Use case: I am going to start a new playback, and I want to see the stats in about://media-internals. So I opened about://media-internals before I start to play, only to find there are already 50+ players listed in the page.  Issue 605961  or issue 551065 should be able help control the number of players we see on the page. But I really wish I could have a magic button "Clear All Players" to just remove them all.

This would be similar to the "clear" command in bash terminal.
 

Comment 1 by w...@chromium.org, Feb 4 2017

Cc: w...@chromium.org
Cc: -w...@chromium.org
Owner: w...@chromium.org
Status: Assigned (was: Untriaged)
watk@, feel free to re-assign to media-internals owner. I just need remove this bug from untriaged plate.

Comment 3 by w...@chromium.org, Feb 7 2017

Owner: sande...@chromium.org
xhwang@, once such a "clear" command is executed, I assume the intent is that any further log messages from the cleared players would resurrect those players in media-internals' player list (but not resurrect those players' older logs), right?
The currently proposed design would remove them permanently.
I have a prototype CL at: https://chromiumcodereview.appspot.com/2672193003/

What this does is:
- It clears all current players on the page, but doesn't touch the C++ side.
- After a player is cleared, new logs for the cleared players will be dropped (I think) and will not resurrect any cleared players.
- If a user refreshes the media-internals page, or open a new one, we'll get all players back.

It works for what I want. But I guess it's a bit confusing for users/devs that when you clear it you only clear it from the current view, and didn't really "delete" it. That's why in the CL I call it "hide". But this is still a bit confusing.

Suggestions?

Maybe we can send a command back to C++ to really clear the players, assuming this won't affect our UMA reporting (haven't looked closely at the C++ part recently).
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4ee2eeb27fb5cb49b1d5fa470528c16738d0274d

commit 4ee2eeb27fb5cb49b1d5fa470528c16738d0274d
Author: xhwang <xhwang@chromium.org>
Date: Tue Feb 07 22:56:51 2017

media: Add "Hide players" button in about://media-internals

What this does is:
- Adds a button to hide all current players on the page, but doesn't touch the C++ side to actually delete those players.
- After a player is hidden, new logs for that player will be dropped and will not resurrect any hidden players.
- If a user refreshes the media-internals page, or open a new one, we'll get all players back.

BUG= 688607 

Review-Url: https://codereview.chromium.org/2672193003
Cr-Commit-Position: refs/heads/master@{#448764}

[modify] https://crrev.com/4ee2eeb27fb5cb49b1d5fa470528c16738d0274d/content/browser/resources/media/manager.js
[modify] https://crrev.com/4ee2eeb27fb5cb49b1d5fa470528c16738d0274d/content/browser/resources/media/media_internals.html

Project Member

Comment 9 by bugdroid1@chromium.org, Feb 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eb328e2bf72447674d553e61c948c1462c07c753

commit eb328e2bf72447674d553e61c948c1462c07c753
Author: shimazu <shimazu@chromium.org>
Date: Wed Feb 08 01:34:32 2017

Revert of media: Add "Hide players" button in about://media-internals (patchset #2 id:20001 of https://codereview.chromium.org/2672193003/ )

Reason for revert:
Two following tests fail:
-
 WebUIResourceBrowserTest.MediaInternals_Integration
- WebUIResourceBrowserTest.MediaInternals_Manager

Original issue's description:
> media: Add "Hide players" button in about://media-internals
>
> What this does is:
> - Adds a button to hide all current players on the page, but doesn't touch the C++ side to actually delete those players.
> - After a player is hidden, new logs for that player will be dropped and will not resurrect any hidden players.
> - If a user refreshes the media-internals page, or open a new one, we'll get all players back.
>
> BUG= 688607 
>
> Review-Url: https://codereview.chromium.org/2672193003
> Cr-Commit-Position: refs/heads/master@{#448764}
> Committed: https://chromium.googlesource.com/chromium/src/+/4ee2eeb27fb5cb49b1d5fa470528c16738d0274d

TBR=sandersd@chromium.org,xhwang@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 688607 

Review-Url: https://codereview.chromium.org/2682013002
Cr-Commit-Position: refs/heads/master@{#448838}

[modify] https://crrev.com/eb328e2bf72447674d553e61c948c1462c07c753/content/browser/resources/media/manager.js
[modify] https://crrev.com/eb328e2bf72447674d553e61c948c1462c07c753/content/browser/resources/media/media_internals.html

Project Member

Comment 10 by bugdroid1@chromium.org, Feb 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/46844f6e8ad264cd4b20a3d75c0d3a8c303333bb

commit 46844f6e8ad264cd4b20a3d75c0d3a8c303333bb
Author: xhwang <xhwang@chromium.org>
Date: Wed Feb 08 20:56:46 2017

Reland 'media: Add "Hide players" button in about://media-internals' with fix

This relands 4ee2eeb27fb5cb49b1d5fa470528c16738d0274d with fix.

Original CL description:

What this does is:
- Adds a button to hide all current players on the page, but doesn't
  touch the C++ side to actually delete those players.
- After a player is hidden, new logs for that player will be dropped
  and will not resurrect any hidden players.
- If a user refreshes the media-internals page, or open a new one,
  we'll get all players back.

Fix:

In manager.js, handle the case where the button may not exist, e.g. in
tests.

TBR=sandersd@chromium.org
BUG= 688607 

Review-Url: https://codereview.chromium.org/2682803004
Cr-Commit-Position: refs/heads/master@{#449087}

[modify] https://crrev.com/46844f6e8ad264cd4b20a3d75c0d3a8c303333bb/content/browser/resources/media/manager.js
[modify] https://crrev.com/46844f6e8ad264cd4b20a3d75c0d3a8c303333bb/content/browser/resources/media/media_internals.html

Status: Fixed (was: Assigned)
Closing as fixed. The feature was never fully implemented, but the correct fix (limit to 10 players on reload) covers the use case.

Sign in to add a comment