[Auto-PiP] Add auto-PiP permission prompt logic to Android DocPiP [chromium/src : main]
Phil Yan (Gerrit)
Phil Yan voted Commit-Queue+0![]( "Open in Gerrit")
| Commit-Queue | +0 |
Related details
- Frank Liberato
- Tommy Steimel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato added 3 comments![]( "Open in Gerrit")
AutoPictureInPicturePermissionController.clearAutoPipTriggered(webContents);why is it clearing the triggered flag? if this doc pip window is being created in response to an autopip signal, shouldn't it remain set for the lifetime of the window?
as an aside, the state management feels a little fragile. i'm not sure why - i haven't diagrammed it all out but multiple calls into set / clear the state from here and elsewhere makes me wonder. maybe it's okay, but it's hard to tell.
@Testi notice that you're an owner of other files in this directory but not this one -- is it because it's a new file, or OWNERS has a weird pattern?
picture_in_picture::AutoPictureInPicturePermissionControllerBridge::nit: you might want AutoPicture...Bridge::OnPrimaryPageChanged and let that java side sort out the details.
Related details
- Phil Yan
- Tommy Steimel
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Phil Yan (Gerrit)
Phil Yan added 3 comments![]( "Open in Gerrit")
AutoPictureInPicturePermissionController.clearAutoPipTriggered(webContents);why is it clearing the triggered flag? if this doc pip window is being created in response to an autopip signal, shouldn't it remain set for the lifetime of the window?
as an aside, the state management feels a little fragile. i'm not sure why - i haven't diagrammed it all out but multiple calls into set / clear the state from here and elsewhere makes me wonder. maybe it's okay, but it's hard to tell.
The reasoning for clearing the flag here is to avoid showing the prompt for manually triggered PiP.
However, after double checking `auto_picture_in_picture_tab_helper.cc`, I realized that I previously missed the existing `AreAutoPictureInPicturePreconditionsMet()` which serves the **exact same** purpose as the new triggered flag. It's still true that we cannot directly rely on `IsInAutoPictureInPicture()`, which is set to true only *after* pip is created, because the prompt view is added to the activity during the pip initialization process. But we can use `AreAutoPictureInPicturePreconditionsMet()`(exposed via JNI) here and let the native side handle its lifecycle instead of reinventing the wheel.
To lean on the cautious side, in the new `isAutoPictureInPictureInUse` JNI method I just added, I checked `tab_helper->AreAutoPictureInPicturePreconditionsMet() || tab_helper->IsInAutoPictureInPicture()` to keep in consistency with the implementation in `auto_picture_in_picture_tab_helper.cc` although I think `IsInAutoPictureInPicture()` should always be false at the time the doc pip activity checks for it in `initializeCompositor`.
i notice that you're an owner of other files in this directory but not this one -- is it because it's a new file, or OWNERS has a weird pattern?
Looks like it's because the owner file is ultimately mapped to media/OWNERS which I'm not part of. I'm currently in chrome/browser/picture_in_picture/OWNERS.
chrome/android/junit/src/org/chromium/chrome/browser/media/OWNERS ->
components/browser_ui/media/OWNERS ->
media/OWNERS.
Do you think it's okay to add me or picture_in_picture/OWNERS in one of these layers?
picture_in_picture::AutoPictureInPicturePermissionControllerBridge::nit: you might want AutoPicture...Bridge::OnPrimaryPageChanged and let that java side sort out the details.
- Frank Liberato
- Tommy Steimel
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato voted Code-Review+1![]( "Open in Gerrit")
- Phil Yan
- Tommy Steimel
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Steimel (Gerrit)
Tommy Steimel added 1 comment![]( "Open in Gerrit")
static void OnPrimaryPageChanged(content::WebContents& web_contents);Instead of this, could we just use the existing Android-side WebContentsObserver pattern for it?
Related details
- Phil Yan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Phil Yan (Gerrit)
Phil Yan added 2 comments![]( "Open in Gerrit")
@TestPhil Yani notice that you're an owner of other files in this directory but not this one -- is it because it's a new file, or OWNERS has a weird pattern?
Looks like it's because the owner file is ultimately mapped to media/OWNERS which I'm not part of. I'm currently in chrome/browser/picture_in_picture/OWNERS.
chrome/android/junit/src/org/chromium/chrome/browser/media/OWNERS ->
components/browser_ui/media/OWNERS ->
media/OWNERS.Do you think it's okay to add me or picture_in_picture/OWNERS in one of these layers?
Updated chrome/android/junit/src/org/chromium/chrome/browser/media/OWNERS to include `per-file *PictureInPicture*.java=file://chrome/browser/picture_in_picture/OWNERS`
static void OnPrimaryPageChanged(content::WebContents& web_contents);Instead of this, could we just use the existing Android-side WebContentsObserver pattern for it?
- Frank Liberato
- Tommy Steimel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Steimel (Gerrit)
Tommy Steimel voted Code-Review+1![]( "Open in Gerrit")
- Frank Liberato
- Phil Yan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |