Disable PIP for videos in incognito mode. [chromium/src : master]
Side YILMAZ (Gerrit)
Attention is currently required from: Mounir Lamouri.
Side YILMAZ would like Mounir Lamouri to review this change.
Disable PIP for videos in incognito mode.
This CL block picture in picture mode for incognito.
Bug: 1101935
Change-Id: Iae9727b52dc45c60c169805eaa19867394d7238d
---
M chrome/android/java/src/org/chromium/chrome/browser/media/PictureInPictureController.java
1 file changed, 5 insertions(+), 0 deletions(-)
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/media/PictureInPictureController.java b/chrome/android/java/src/org/chromium/chrome/browser/media/PictureInPictureController.java
index 814df3b..efcda7e 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/media/PictureInPictureController.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/media/PictureInPictureController.java
@@ -24,6 +24,7 @@
import org.chromium.chrome.browser.ActivityTabProvider;
import org.chromium.chrome.browser.fullscreen.FullscreenManager;
import org.chromium.chrome.browser.infobar.InfoBarContainer;
+import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.tab.EmptyTabObserver;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.tab.TabObserver;
@@ -117,6 +118,10 @@
return false;
}
+ if (Profile.fromWebContents(webContents).isOffTheRecord()) {
+ return false;
+ }
+
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) {
recordAttemptResult(METRICS_ATTEMPT_RESULT_NO_SYSTEM_SUPPORT);
return false;
To view, visit change 2653147. To unsubscribe, or for help writing mail filters, visit settings.
Side YILMAZ (Gerrit)
Attention is currently required from: Mounir Lamouri.
1 comment:
Patchset:
Hello Mounir,
I created a fix to block picture in picture mode for incognito.
Could you please take a look?
To view, visit change 2653147. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
Attention is currently required from: Side YILMAZ.
1 comment:
Patchset:
Is there a reason why we want to do that? Why is playing a video in incognito okay but playing the same video in Picture-in-Picture in incognito not okay?
To view, visit change 2653147. To unsubscribe, or for help writing mail filters, visit settings.
Side YILMAZ (Gerrit)
Attention is currently required from: Side YILMAZ.
Side YILMAZ uploaded patch set #3 to this change.
Disable PIP for videos in incognito mode.
This CL block picture in picture mode for incognito, since The main
reason is that the content of incognito should stay in incognito
browser experience.
Bug: 1101935
Change-Id: Iae9727b52dc45c60c169805eaa19867394d7238d
---
M chrome/android/java/src/org/chromium/chrome/browser/media/PictureInPictureController.java
1 file changed, 5 insertions(+), 0 deletions(-)
To view, visit change 2653147. To unsubscribe, or for help writing mail filters, visit settings.
Side YILMAZ (Gerrit)
Attention is currently required from: Side YILMAZ.
Side YILMAZ uploaded patch set #4 to this change.
Disable PIP for videos in incognito mode.
This CL block picture in picture mode for incognito, since the content
of incognito should stay in incognito browser experience.
Bug: 1101935
Change-Id: Iae9727b52dc45c60c169805eaa19867394d7238d
---
M chrome/android/java/src/org/chromium/chrome/browser/media/PictureInPictureController.java
1 file changed, 5 insertions(+), 0 deletions(-)
To view, visit change 2653147. To unsubscribe, or for help writing mail filters, visit settings.
Side YILMAZ (Gerrit)
Attention is currently required from: Mounir Lamouri.
2 comments:
Patchset:
Is there a reason why we want to do that? Why is playing a video in incognito okay but playing the s […]
The main reason is that the content in incognito should stay in incognito browser experience, but not live outside of it. In PIP case, incognito content will be alive even Chrome incognito is not active on foreground. So this is against the incognito principals.
Also when looking from user's perspective, it is likely that the incognito is sensitive and may contain content that user'd prefer to hide while closing browser. Hence we should keep media inside of the Chrome.
As an example we treat the media session similar and hide the content name on notification as here:
https://drive.google.com/file/d/1tjlQFlJL7azeyiVhQkiHDZDPU47EHt-Z/edit
Patchset:
Thanks Mounir for review! I replied at the comment.
The bug 1101935 is assign to you currently, would you mind if I assign to myself?
To view, visit change 2653147. To unsubscribe, or for help writing mail filters, visit settings.
Side YILMAZ (Gerrit)
Attention is currently required from: Ramin Halavati, Mounir Lamouri.
Side YILMAZ would like Ramin Halavati to review this change.
Disable PIP for videos in incognito mode.
This CL block picture in picture mode for incognito, since the content
of incognito should stay in incognito browser experience.
Bug: 1101935
Change-Id: Iae9727b52dc45c60c169805eaa19867394d7238d
---
M chrome/android/java/src/org/chromium/chrome/browser/media/PictureInPictureController.java
1 file changed, 5 insertions(+), 0 deletions(-)
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/media/PictureInPictureController.java b/chrome/android/java/src/org/chromium/chrome/browser/media/PictureInPictureController.java
index 814df3b..efcda7e 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/media/PictureInPictureController.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/media/PictureInPictureController.java
@@ -24,6 +24,7 @@
import org.chromium.chrome.browser.ActivityTabProvider;
import org.chromium.chrome.browser.fullscreen.FullscreenManager;
import org.chromium.chrome.browser.infobar.InfoBarContainer;
+import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.tab.EmptyTabObserver;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.tab.TabObserver;
@@ -117,6 +118,10 @@
return false;
}
+ if (Profile.fromWebContents(webContents).isOffTheRecord()) {
+ return false;
+ }
+
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) {
recordAttemptResult(METRICS_ATTEMPT_RESULT_NO_SYSTEM_SUPPORT);
return false;
To view, visit change 2653147. To unsubscribe, or for help writing mail filters, visit settings.
Side YILMAZ (Gerrit)
Attention is currently required from: Ramin Halavati, Mounir Lamouri.
1 comment:
Patchset:
Hi Ramin,
Can you please review the CL from incognito side?
To view, visit change 2653147. To unsubscribe, or for help writing mail filters, visit settings.
Ramin Halavati (Gerrit)
Attention is currently required from: Side YILMAZ, Mounir Lamouri.
Patch set 4:Code-Review +1
1 comment:
Patchset:
The expressed behavior LGTM.
If user hides the Incognito tab, the video should not continue.
To view, visit change 2653147. To unsubscribe, or for help writing mail filters, visit settings.
Side YILMAZ (Gerrit)
Attention is currently required from: Mounir Lamouri.
1 comment:
Patchset:
Hi Mounir,
Do you still have concerns for hiding PIP for incognito mode?
If so, happy to discuss more on gvc.Thanks,
Side
To view, visit change 2653147. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
Attention is currently required from: Side YILMAZ.
1 comment:
Patchset:
Hi Mounir, […]
Sorry for the delay, I wanted to reach out to Florian before getting back to you as I've seen changes like that coming from the incognito team with little context recently. This CL in particular isn't something I would be strongly opposed to but the feature has been around for 4+ years, we weren't asked to block it in Incognito and Incognito users relying on it would have no other way to get that capabilities back.
In my opinion, this should be discussed outside of the code review because it's not a code problem. Actually, your code looks fine, I would only suggest to add a test if we wanted this to land. A document explaining why this behaviour needs to change now and what the expected user impact would be could help. We can then discuss around that document and get back to the CL afterwards. WDYT?
To view, visit change 2653147. To unsubscribe, or for help writing mail filters, visit settings.
Side YILMAZ (Gerrit)
Attention is currently required from: Mounir Lamouri.
1 comment:
Patchset:
Sorry for the delay, I wanted to reach out to Florian before getting back to you as I've seen change […]
That makes so much sense to me. Actually my intention is using this CL as an opportunity to discuss more, if there is any concern with the proposal.
I have created this doc(https://docs.google.com/document/d/1d7M3ucWPdPBji0SYK3-5H8FfeCVubyP1LMkb_CQMfxo) and put there a pros and cons section. Happy to discuss on the doc and come back to CL later :)
To view, visit change 2653147. To unsubscribe, or for help writing mail filters, visit settings.
Side YILMAZ (Gerrit)
Side YILMAZ abandoned this change.
To view, visit change 2653147. To unsubscribe, or for help writing mail filters, visit settings.