New issue
Advanced search Search tips

Issue 911267 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 856201



Sign in to add a comment

Meta bug for PIP activation issue

Project Member Reported by osh...@chromium.org, Dec 3

Issue description

I found a few case that PIP can still be activated.
I'll send a fix soon.
 
Components: Blink>Media>PictureInPicture UI>Shell>WindowManager
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 5

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

commit 14b73df744547f1a0443a92540d9304a567b7743
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Wed Dec 05 07:53:26 2018

PIP activation fix.

* The initial state is also wrong because widget overwite the
activatable state.
* Deactivate when it exits PIP.
  This may not be an issue for ChromePIP, but nothing prevent it from
  swithing state so it's better to move that logic to windowstate.
* Cnsolidates the PIP activation logic
in Chrome PIP and Android PIP into WindowState.

Bug:  911267 
Test: covered by unittests
Change-Id: Idf447771ccaa1d4ad95260c4c44f182946f64c0b
Reviewed-on: https://chromium-review.googlesource.com/c/1357819
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Eliot Courtney <edcourtney@chromium.org>
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613901}
[modify] https://crrev.com/14b73df744547f1a0443a92540d9304a567b7743/ash/BUILD.gn
[add] https://crrev.com/14b73df744547f1a0443a92540d9304a567b7743/ash/wm/pip/pip_unittest.cc
[modify] https://crrev.com/14b73df744547f1a0443a92540d9304a567b7743/ash/wm/window_state.cc
[modify] https://crrev.com/14b73df744547f1a0443a92540d9304a567b7743/chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc
[modify] https://crrev.com/14b73df744547f1a0443a92540d9304a567b7743/chrome/browser/ui/views/overlay/overlay_window_views.cc
[modify] https://crrev.com/14b73df744547f1a0443a92540d9304a567b7743/components/exo/client_controlled_shell_surface.cc
[modify] https://crrev.com/14b73df744547f1a0443a92540d9304a567b7743/components/exo/shell_surface_base.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 5

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

commit 1e388b0752dd47ca0f5c7168127d9824af9b6d0b
Author: Pavol Marko <pmarko@chromium.org>
Date: Wed Dec 05 14:00:07 2018

Revert "PIP activation fix."

This reverts commit 14b73df744547f1a0443a92540d9304a567b7743.

Reason for revert: Suspected to cause build failures in network_service_browser_tests (PictureInPictureWindowControllerBrowserTest.CreationAndVisibilityAndActivation).
Example failure:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.12%20Tests/17330

[24104:4611:1205/045208.445953:WARNING:notification_platform_bridge_mac.mm(521)] AlertNotificationService: XPC connection invalidated.
../../chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc:203: Failure
Value of: platform_util::IsWindowActive(native_window)
  Actual: false
Expected: true

Original change's description:
> PIP activation fix.
> 
> * The initial state is also wrong because widget overwite the
> activatable state.
> * Deactivate when it exits PIP.
>   This may not be an issue for ChromePIP, but nothing prevent it from
>   swithing state so it's better to move that logic to windowstate.
> * Cnsolidates the PIP activation logic
> in Chrome PIP and Android PIP into WindowState.
> 
> Bug:  911267 
> Test: covered by unittests
> Change-Id: Idf447771ccaa1d4ad95260c4c44f182946f64c0b
> Reviewed-on: https://chromium-review.googlesource.com/c/1357819
> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
> Reviewed-by: Eliot Courtney <edcourtney@chromium.org>
> Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#613901}

TBR=oshima@chromium.org,mlamouri@chromium.org,edcourtney@chromium.org

Change-Id: I5c52bbbb07e76f96571309bcdeb5839502a176b3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  911267 
Reviewed-on: https://chromium-review.googlesource.com/c/1363198
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613958}
[modify] https://crrev.com/1e388b0752dd47ca0f5c7168127d9824af9b6d0b/ash/BUILD.gn
[delete] https://crrev.com/4619e61d0c042ddfecc652c75ea797c39f4f457f/ash/wm/pip/pip_unittest.cc
[modify] https://crrev.com/1e388b0752dd47ca0f5c7168127d9824af9b6d0b/ash/wm/window_state.cc
[modify] https://crrev.com/1e388b0752dd47ca0f5c7168127d9824af9b6d0b/chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc
[modify] https://crrev.com/1e388b0752dd47ca0f5c7168127d9824af9b6d0b/chrome/browser/ui/views/overlay/overlay_window_views.cc
[modify] https://crrev.com/1e388b0752dd47ca0f5c7168127d9824af9b6d0b/components/exo/client_controlled_shell_surface.cc
[modify] https://crrev.com/1e388b0752dd47ca0f5c7168127d9824af9b6d0b/components/exo/shell_surface_base.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 5

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

commit ea9a040e5b7974b5317c15948f4bd72644aad94b
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Wed Dec 05 22:29:41 2018

PIP activation fix.

This is reland of crrev.com/c/1357819. I changed the expectation for
MACOSX_10_12.

* The initial state is also wrong because widget overwite the
activatable state.
* Deactivate when it exits PIP.
  This may not be an issue for ChromePIP, but nothing prevent it from
  swithing state so it's better to move that logic to windowstate.
* Cnsolidates the PIP activation logic
in Chrome PIP and Android PIP into WindowState.

Bug:  911267 
TBR: edcourtney@chromium.org
Test: covered by unittests
Change-Id: Ia6b9f3beb9035debfef64fbc85d1ce9482047422
Reviewed-on: https://chromium-review.googlesource.com/c/1357819
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Eliot Courtney <edcourtney@chromium.org>
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#613901}
Reviewed-on: https://chromium-review.googlesource.com/c/1363204
Cr-Commit-Position: refs/heads/master@{#614136}
[modify] https://crrev.com/ea9a040e5b7974b5317c15948f4bd72644aad94b/ash/BUILD.gn
[add] https://crrev.com/ea9a040e5b7974b5317c15948f4bd72644aad94b/ash/wm/pip/pip_unittest.cc
[modify] https://crrev.com/ea9a040e5b7974b5317c15948f4bd72644aad94b/ash/wm/window_state.cc
[modify] https://crrev.com/ea9a040e5b7974b5317c15948f4bd72644aad94b/chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc
[modify] https://crrev.com/ea9a040e5b7974b5317c15948f4bd72644aad94b/chrome/browser/ui/views/overlay/overlay_window_views.cc
[modify] https://crrev.com/ea9a040e5b7974b5317c15948f4bd72644aad94b/components/exo/client_controlled_shell_surface.cc
[modify] https://crrev.com/ea9a040e5b7974b5317c15948f4bd72644aad94b/components/exo/shell_surface_base.cc

Labels: Merge-Request-72
Blocking: 856201
Project Member

Comment 7 by sheriffbot@chromium.org, Dec 6

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-72 Merge-Approved-72
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 7

Labels: -merge-approved-72 merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8e19f673801c7144464519868bb854c77e421acc

commit 8e19f673801c7144464519868bb854c77e421acc
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Fri Dec 07 17:50:49 2018

PIP activation fix.

This is reland of crrev.com/c/1357819. I changed the expectation for
MACOSX_10_12.

* The initial state is also wrong because widget overwite the
activatable state.
* Deactivate when it exits PIP.
  This may not be an issue for ChromePIP, but nothing prevent it from
  swithing state so it's better to move that logic to windowstate.
* Cnsolidates the PIP activation logic
in Chrome PIP and Android PIP into WindowState.

Bug:  911267 
TBR: edcourtney@chromium.org
Test: covered by unittests
Change-Id: Ia6b9f3beb9035debfef64fbc85d1ce9482047422
Reviewed-on: https://chromium-review.googlesource.com/c/1357819
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Eliot Courtney <edcourtney@chromium.org>
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/master@{#613901}
Reviewed-on: https://chromium-review.googlesource.com/c/1363204
Cr-Original-Commit-Position: refs/heads/master@{#614136}(cherry picked from commit ea9a040e5b7974b5317c15948f4bd72644aad94b)
Reviewed-on: https://chromium-review.googlesource.com/c/1367891
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#137}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/8e19f673801c7144464519868bb854c77e421acc/ash/BUILD.gn
[add] https://crrev.com/8e19f673801c7144464519868bb854c77e421acc/ash/wm/pip/pip_unittest.cc
[modify] https://crrev.com/8e19f673801c7144464519868bb854c77e421acc/ash/wm/window_state.cc
[modify] https://crrev.com/8e19f673801c7144464519868bb854c77e421acc/chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc
[modify] https://crrev.com/8e19f673801c7144464519868bb854c77e421acc/chrome/browser/ui/views/overlay/overlay_window_views.cc
[modify] https://crrev.com/8e19f673801c7144464519868bb854c77e421acc/components/exo/client_controlled_shell_surface.cc
[modify] https://crrev.com/8e19f673801c7144464519868bb854c77e421acc/components/exo/shell_surface_base.cc

Status: Fixed (was: Assigned)
Labels: CommitLog-Audit-Violation Merge-Without-Approval
Here's a summary of the rules that were executed: 
 - OnlyMergeApprovedChange: Rule Failed -- Revision 8e19f673801c7144464519868bb854c77e421acc was merged to refs/branch-heads/3626 branch with no merge approval from a TPM! 
Please explain why this change was merged to the branch!
 - AcknowledgeMerge: Notification Required -- 
Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/8e19f673801c7144464519868bb854c77e421acc

Commit: 8e19f673801c7144464519868bb854c77e421acc
Author: oshima@chromium.org
Commiter: oshima@chromium.org
Date: 2018-12-07 17:50:49 +0000 UTC

PIP activation fix.

This is reland of crrev.com/c/1357819. I changed the expectation for
MACOSX_10_12.

* The initial state is also wrong because widget overwite the
activatable state.
* Deactivate when it exits PIP.
  This may not be an issue for ChromePIP, but nothing prevent it from
  swithing state so it's better to move that logic to windowstate.
* Cnsolidates the PIP activation logic
in Chrome PIP and Android PIP into WindowState.

Bug:  911267 
TBR: edcourtney@chromium.org
Test: covered by unittests
Change-Id: Ia6b9f3beb9035debfef64fbc85d1ce9482047422
Reviewed-on: https://chromium-review.googlesource.com/c/1357819
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Eliot Courtney <edcourtney@chromium.org>
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/master@{#613901}
Reviewed-on: https://chromium-review.googlesource.com/c/1363204
Cr-Original-Commit-Position: refs/heads/master@{#614136}(cherry picked from commit ea9a040e5b7974b5317c15948f4bd72644aad94b)
Reviewed-on: https://chromium-review.googlesource.com/c/1367891
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#137}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

Sign in to add a comment