Details
- Reviewers
• mconley - Commits
- rMOZILLACENTRAL1c62e73258b4: Bug 1528459 - Show PiP Icon in parent tab of media. r=mconley
- Bugzilla Bug ID
- 1528459
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Code analysis found 6 defects in this patch:
- 6 defects found by mozlint
You can run this analysis locally with:
- ./mach lint --warnings path/to/file (JS/Python/etc)
If you see a problem in this automated review, please report it here.
| browser/base/content/tabbrowser.js | ||
|---|---|---|
| 4668 ↗ | (On Diff #96181) | Error: 'tab' is not defined. [eslint: no-undef] |
| 4677 ↗ | (On Diff #96181) | Error: 'tab' is not defined. [eslint: no-undef] |
| 4678 ↗ | (On Diff #96181) | Error: 'tab' is not defined. [eslint: no-undef] |
| 4681 ↗ | (On Diff #96181) | Error: 'tab' is not defined. [eslint: no-undef] |
| 4682 ↗ | (On Diff #96181) | Error: 'tab' is not defined. [eslint: no-undef] |
| 4689 ↗ | (On Diff #96181) | Error: 'tab' is not defined. [eslint: no-undef] |
| browser/base/content/tabbrowser.js | ||
|---|---|---|
| 4667 ↗ | (On Diff #96181) | So I actually think we can go about this the other direction, and have PictureInPicture.jsm do most of the heavy lifting here. handlePictureInPicture request and unload both make reference to the browser that sent the message to show a PiP window. We can resolve that browser to a tab like so: let win = browser.ownerGlobal; let tab = win.gBrowser.getTabForBrowser(browser); At that point, you can set and remove the attributes in there. What do you think? |
This is looking good, thanks @JSON_voorhees.
So, I haven't tested this yet, but it looks from reading it that a PiP window will hide the mute button in the tab until hover, is that right? Can I assume this is what UX settled on?
| browser/base/content/tabbrowser.css | ||
|---|---|---|
| 23–25 | Traditionally, this kind of styling (as opposed to the "showing and hiding and behavioural" styles) goes inside of browser/themes/shared/tabs.inc.css. Can you move this rule there? Also, why is the !important required? What has greater precedence? Finally, we're going to need to make sure this works in RTL builds. We can do that easily here by using margin-inline-start: 5px instead, which will choose the right direction based on the UI direction. | |
| browser/base/content/tabbrowser.xml | ||
| 1938 | Why does this image need the pictureinpicture attribute inherited? | |
| 1958 | Nit: Double comma Also, I'm not really sure this image needs to inherit these attributes - I think it only needs to inherit the pictureinpicture one that you're using in the stylesheet (xbl:inherits means that these children will clone that set of attributes from their parent). | |
| 1959 | This ID isn't being used, and can probably be removed. | |
| 2320 | Please revert this whitespace change. | |
| browser/themes/shared/tabs.inc.css | ||
| 380 | This looks like commented out code, so let's get rid of it. | |
| 389 | Nit: please remove this commented out code. | |
| toolkit/components/pictureinpicture/PictureInPicture.jsm | ||
| 77 | This will get called in the unload event handler, so we can probably get rid of this. | |
| toolkit/components/pictureinpicture/content/player.js | ||
| 54 ↗ | (On Diff #96311) | This is going to be taken care of in the unload event handler. So I don't think we need to call it here too. |
| toolkit/components/pictureinpicture/PictureInPicture.jsm | ||
|---|---|---|
| 77 | unload seems to not be taking care of this here, or in player.js | |
Bumped pictureinpicture.svg size down a tad to look better in the tab from emanuela's suggestion. It still looks good in the video overlay
This will need to be rebased on top of bug 1551570, and then we can avoid the extra calls to unload.
Otherwise, this looks really good!
| toolkit/components/pictureinpicture/PictureInPicture.jsm | ||
|---|---|---|
| 58–59 | This was removed in https://phabricator.services.mozilla.com/D31081, so will conflict if we attempt to apply this. Can you remove this change in your patch? When you pull from central after bug 1551570 closes, this change will be in there. | |
| 62 | Nit: /**. | |
Thanks, @JSON_voorhees - this works wonderfully. Just one thing I noticed while testing - see comment. You can land with that fixed.
| toolkit/components/pictureinpicture/PictureInPicture.jsm | ||
|---|---|---|
| 68 | While testing, I realized that we might end up here by the tab closing (tab closes, browser goes away, Picture-in-Picture window unloads, which causes clearPipTabIcon to be called), which will cause this to throw, since tab will not exist. And then we'll never clear the this.browser variable, and we'll end up leaking that thing. We should probably check to see if tab exists before attempting to remove the attribute. Can you also add a comment saying something like, at this point, the tab might be gone, so that's why we're doing the check? | |
| toolkit/components/pictureinpicture/PictureInPicture.jsm | ||
|---|---|---|
| 68 | We generally brace one-liners, so can you please make this: if (tab) { tab.removeAttribute("pictureinpicture"); } | |