Introduced incognito custom tabs (ICT). [chromium/src : master]
Pavel Shmakov (Gerrit)
Pavel Shmakov uploaded patch set #2 to this change.
Introduced incognito custom tabs (ICT).
Notes:
1. The feature is hidden behind a switch enable_incognito_custom_tabs. Without it, the behavior should remain as before, i.e. only allow ICTs for payments flow.
2. ICT can be launched by adding extra com.google.android.apps.chrome.EXTRA_OPEN_NEW_INCOGNITO_TAB and only by first-party apps,
unless overriden for testing purposes with allow_incognito_custom_tabs_from_third_party.
3. The toolbar of ICT has grey theme color, but this can be overriden by custom tab parameters.
4. ICT has menu item "Open in Incognito Chrome", which reparents the tab into Chrome. App picker is not shown.
5. "Close all incognito tabs" notification takes ICTs into account.
6. Taking screenshots of ICTs is not allowed, and no screenshots of them are seen in recents.
crbug/871738
Change-Id: Ia5c7e9cd31bcacc5e8e30bc43d7b0f234ac007c4
---
M chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java
M chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
A chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoCleanupUtils.java
M chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java
M chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoTabSnapshotController.java
M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModelImplCreator.java
M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
M chrome/android/java/strings/android_chrome_strings.grd
M chrome/android/java_sources.gni
13 files changed, 206 insertions(+), 103 deletions(-)
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Peter Conn (Gerrit)
This is really good, thanks!
11 comments:
Could you make it a bit more explicit that Incognito Custom Tabs will always show Open in Chrome Incognito, whereas normal Custom Tabs may open in a different browser, if the user has a default?
Actually, can you also put a note of that on the bug - I feel this is an important behaviour to highlight.
Patch Set #2, Line 18:
crbug/871738You should write
Bug: 871738
Then Gerrit will link it for you (and comment on that bug when this code lands).
File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:
Patch Set #2, Line 199:
thirdShould this be "first-party"?
Patch Set #2, Line 201:
ENABLE_INCOGNITO_CUSTOM_TABSAlso, there are two separate things these flags are doing:
1. Enabling the feature altogether (eg, letting Incognito Custom Tabs) exist in the first place.
2. Allowing developers to white-list them for 3rd party apps for development.Chrome Switches is suitable for the second use - letting developers use the command line to do special things for development. For the first usage (enabling/disabling a feature we're not sure we want to launch yet), we use ChromeFeatureList.
This is cool in that we have server side configuration that we can use to enable/disable features - so if we put Incognito Custom Tabs behind a Chrome Feature (not a command line flag), we can leave the code in and then at a later point push some config to devices to enable it (or alternatively, if we find there's a massive bug, we can disable it for users without having to get them to update Chrome).
So, ultimately, use ChromeSwitches for things that developers locally may want to use, and use ChromeFeatureList for features that we would want to enable/disable for our users. (You can also enable and disable features at the command line if you want).
File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:
Why do we need to relax this constraint? This is checking that no third-party opens an incognito Tab in ChromeTabbedActivity, we don't want to allow that. We want to allow Custom Tabs to open Incognito Tabs in ChromeTabbedActivity, but that shouldn't trigger this?
File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:
Patch Set #2, Line 1050:
publicpublic classes and methods should have javadoc.
Patch Set #2, Line 156:
menu_open_in_incognito_chromeAgain, line length.
You should have got a warning when you ran `git cl upload` about formatting and stuff.
Patch Set #2, Line 626:
CommandLineAs I mentioned before, this should change to something like:
is (ChromeFeatureList.isEnabled(ChromeFeatureList.ENABLE_INCOGNITO_CUSTOM_TABS) ...
Patch Set #2, Line 17:
import javax.annotation.Nullable;We have three different versions of @Nullable that we can import:
javax.annotation.Nullable
android.support.Nullable
android.support.annotation.NullableWe should use the last one. There's a bit of justification here: https://bugs.chromium.org/p/chromium/issues/detail?id=771683
Patch Set #2, Line 38:
TabModelSelectorWe've got a 100 character max line length.
You can run `git cl format .` in the base of your repository and it will format everything automatically for you (it's designed to work with C++, it does an OK job with Java, though I find myself having to stop it from messing up code involving annotations).
File chrome/android/java_sources.gni:
Patch Set #2, Line 645:
IncognitoCleanupUtilsKeep this list in alphabetical order.
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Peter Conn (Gerrit)
Adding Bernhard in for a second set of eyes and OWNERs
Bernhard Bauer (Gerrit)
7 comments:
Patch Set #2, Line 13:
but this can be overriden by custom tab parametersI wonder whether we should always make them incognito-grey. It still wouldn't solve the problem of incognito spoofing, but that exists for tabbed Chrome as well (and hopefully we'll address it for both at the same time).
File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:
Patch Set #2, Line 199:
apps launchNit: "Allows [first-party apps] to launch incognito custom tabs"
Patch Set #2, Line 201:
enable_incognito_custom_tabsCommand line switches use kebab-case, not snake_case.
Patch Set #2, Line 310:
isIncognito());Wrap at 100 columns.
File chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoCleanupUtils.java:
Patch Set #2, Line 27:
public class IncognitoCleanupUtils {Add a private empty constructor so that this class can't be instantiated.
Patch Set #2, Line 182:
if (activity instanceof CustomTabActivity) {Would it make more sense to use polymorphism and add a base method to ChromeActivity that is implemented by CustomTabActivity and ChromeTabbedActivity?
File chrome/android/java/strings/android_chrome_strings.grd:
Please add the attribute translateable="false" while the string is not approved yet so that we don't unnecessarily kick off translation for it.
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Bernhard Bauer (Gerrit)
1 comment:
Patch Set #2, Line 622:
isTrustedIntent()Oh, also: Sadly, isTrustedIntent() isn't as, uh, trusted as we want it to be (see https://crbug.com/832124). We might have to check whether CustomTabsConnection.getClientPackageNameForSession() is a first-party app.
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Shmakov (Gerrit)
Pavel Shmakov uploaded patch set #3 to this change.
Introduced incognito custom tabs (ICT).
Notes:
1. The feature is hidden behind a switch enable_incognito_custom_tabs. Without it, the behavior should remain as before, i.e. only allow ICTs for payments flow.
2. ICT can be launched by adding extra com.google.android.apps.chrome.EXTRA_OPEN_NEW_INCOGNITO_TAB and only by first-party apps,
unless overriden for testing purposes with allow_incognito_custom_tabs_from_third_party.
3. The toolbar of ICT has grey theme color, but this can be overriden by custom tab parameters.
4. ICT has menu item "Open in Incognito Chrome", which reparents the tab into Chrome. App picker is not shown.
Thus, unlike normal custom tabs, which either show an app-picker, or open default browser, ICT always opens Chrome.
5. "Close all incognito tabs" notification takes ICTs into account.
6. Taking screenshots of ICTs is not allowed, and no screenshots of them are seen in recents.
Bug: 871738
Change-Id: Ia5c7e9cd31bcacc5e8e30bc43d7b0f234ac007c4
---
M chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java
M chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
A chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoCleanupUtils.java
M chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java
M chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoTabSnapshotController.java
M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModelImplCreator.java
M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
M chrome/android/java/strings/android_chrome_strings.grd
M chrome/android/java_sources.gni
13 files changed, 206 insertions(+), 103 deletions(-)
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Shmakov (Gerrit)
17 comments:
Patch Set #2, Line 13:
but this can be overriden by custom tab parameters
I wonder whether we should always make them incognito-grey. […]
My guess is as long as we allow only first-party apps to open these tabs, it's would be ok to allow any customization.
Could you make it a bit more explicit that Incognito Custom Tabs will always show Open in Chrome Inc […]
Done
You should write […]
Done
File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:
Patch Set #2, Line 199:
first
Should this be "first-party"?
Done
Patch Set #2, Line 199:
apps launchNit: "Allows [first-party apps] to launch incognito custom tabs"
Done
Patch Set #2, Line 201:
enable-incognito-custom-tabs
Command line switches use kebab-case, not snake_case.
Done
Patch Set #2, Line 201:
ENABLE_INCOGNITO_CUSTOM_TABS
Also, there are two separate things these flags are doing: […]
This was a feature initially, but unfortunately features can't be resolved until native has loaded. Which is problematic in this case: we wouldn't be able to tell whether the tab is incognito until native load.
Bernhard suggested we make it a switch for now.
File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:
Why do we need to relax this constraint? This is checking that no third-party opens an incognito Tab […]
The assert does trigger, because externalAppId refers to the app that contained the custom tab.
File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:
public classes and methods should have javadoc.
Done
File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:
Patch Set #5, Line 277:
getWindow().addFlags(WindowManager.LayoutParams.FLAG_SECURE);I removed usage of IncognitoTabSnapshotController, because it doesn't deal with reparenting well: the screenshot of the page can be seen on original app in recents.
Actually, adding a window flag is all that is needed.
Again, line length. […]
Done
Wrap at 100 columns.
Done
File chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoCleanupUtils.java:
Add a private empty constructor so that this class can't be instantiated.
Done
Would it make more sense to use polymorphism and add a base method to ChromeActivity that is impleme […]
Definitely. Actually, I'll replace all casts to CustomTabActivity introduced in this cl.
We have three different versions of @Nullable that we can import: […]
Done
We've got a 100 character max line length. […]
Done
File chrome/android/java_sources.gni:
Patch Set #2, Line 645:
IncognitoUtils.java",
Keep this list in alphabetical order.
Done
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Shmakov (Gerrit)
Pavel Shmakov uploaded patch set #6 to this change.
Introduced incognito custom tabs (ICT).
Notes:
1. The feature is hidden behind a switch enable-incognito-custom-tabs. Without it, the behavior should remain as before, i.e. only allow ICTs for payments flow.
2. ICT can be launched by adding extra com.google.android.apps.chrome.EXTRA_OPEN_NEW_INCOGNITO_TAB and only by first-party apps.
First-party apps also need to be maintaining a connection with CustomTabsService.
These limitations can be relaxed for testing purposes with the switch allow-incognito-custom-tabs-from-third-party.
3. The toolbar of ICT has grey theme color, but this can be overriden by custom tab parameters.
4. ICT has menu item "Open in Incognito Chrome", which reparents the tab into Chrome. App picker is not shown.
Thus, unlike normal custom tabs, which either show an app-picker, or open default browser, ICT always opens Chrome.
5. "Close all incognito tabs" notification takes ICTs into account.
6. Taking screenshots of ICTs is not allowed, and no screenshots of them are seen in recents.
Bug: 871738
Change-Id: Ia5c7e9cd31bcacc5e8e30bc43d7b0f234ac007c4
---
M chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java
M chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
M chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
M chrome/android/java/src/org/chromium/chrome/browser/browserservices/BrowserSessionDataProvider.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
M chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java
A chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoTabHost.java
A chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoTabHostRegistry.java
A chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoUtils.java
M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabParamsManager.java
A chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabsIncognitoTabHost.java
M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModelImplCreator.java
M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
M chrome/android/java/strings/android_chrome_strings.grd
M chrome/android/java_sources.gni
18 files changed, 380 insertions(+), 121 deletions(-)
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Shmakov (Gerrit)
3 comments:
The assert does trigger, because externalAppId refers to the app that contained the custom tab.
Actually you're right, this assert doesn't trigger when reparenting, I'll restore it
Oh, also: Sadly, isTrustedIntent() isn't as, uh, trusted as we want it to be (see https://crbug. […]
I'll use CustomTabsConnection.getClientPackageNameForSession() and also deprecate isTrustedIntent method.
File chrome/android/java/strings/android_chrome_strings.grd:
Please add the attribute translateable="false" while the string is not approved yet so that we don't […]
Done
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Peter Conn (Gerrit)
7 comments:
File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:
Patch Set #6, Line 2005:
IncognitoTabHostRegistryThere's a bit of a mismatch here - you're registering it in onResume and then unregistering it in onDestroyInternal (which is called at onDestroy). Any reason?
Patch Set #6, Line 270:
ChromeSwitchesThis should be ChromeFeatureList.isEnabled, right?
File chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoTabHostRegistry.java:
Patch Set #6, Line 17:
volatileMost of the code that touches the UI is run on the UI thread (we do run some code in parallel, but everything that touches the Android UI is run on one thread, for sanity), and we can generally assume that the code we write will only be run on one thread, unless we explicitly know otherwise. Where you're registering and unregistering the IncognitoTabHosts (in the CustomTabActivity and ChromeTabbedActivity) we're on the UI thread, so thread safety isn't needed.
I'd get rid of volatile and the synchronized block below since I don't think thread safety buys us anything there.
Patch Set #6, Line 23:
IncognitoTabHostRegistryIf you wanted to keep this, I'd recommend the Initialization-on-demand holder idiom [1], which does this a bit more neatly. Or failing that, AtomicReference [2].
[1] https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom
[2] https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/atomic/AtomicReference.htmlPatch Set #6, Line 30:
mHostsSo my understanding of this class is that getInstance() is threadsafe (and therefore implies that it can be called on multiple threads), however the static IncognitoTabHostRegistry holds an ArrayList, which is not threadsafe.
If you wanted to make IncoginitoTabHostRegistry threadsafe (which again, I think isn't necessary) you'd have to synchronize all the accessors of mHosts as well. Otherwise two different threads could get the static IncognitoTabHostRegistry, call register at the exact same time and mess up the ArrayList.
File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabParamsManager.java:
Patch Set #6, Line 20:
IncognitoTabHostRegistryAh, I see why you wanted to make the thing thread-safe now. This feels a bit ugly to me - partially that we're having to make a class thread-safe just to use this one static initializer, and partially that all the instance of this class does is deal with static variables anyway (eg, getAsyncTabParams).
Why can't we register the AsyncTabsIncognitoTabHost in the IncognitoTabHostRegistry? That way we don't have to do any synchronization and we're thread-safe. I know it messes the dependencies up a bit though. Any other ideas?
File chrome/android/java/strings/android_chrome_strings.grd:
Patch Set #6, Line 1193:
IDS_DATA_REDUCTION_INITIAL_TITLEHave you rebased/merged with master? It looks like you're picking up some changes from other CLs.
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Bernhard Bauer (Gerrit)
8 comments:
Patch Set #2, Line 13:
purposes with the switch allow-incognito-custom-tMy guess is as long as we allow only first-party apps to open these tabs, it's would be ok to allow […]
I would argue the other way around: We don't expect a different color, so there is no need to support it.
File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:
Patch Set #2, Line 199:
apps launchDone
Missing a "to" in the middle.
Patch Set #8, Line 42:
private boolean mIsTrustedIntentWithSession;Can this be final? For that matter, maybe it should be mIsFirstParty, because this won't be true for intents coming from Chrome itself.
Patch Set #8, Line 117:
crbug/832124
File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:
Patch Set #8, Line 1414:
// Should only be registered if the custom tab is incognito.Assert that in the constructor then?
Patch Set #8, Line 172:
private void closeIncognitoTabs() {You could move this into IncognitoUtils?
File chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoTabHost.java:
Small nit: superfluous empty line.
File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabsIncognitoTabHost.java:
Patch Set #8, Line 16:
public class AsyncTabsIncognitoTabHost implements IncognitoTabHost {And you could move this class into AsyncTabParamsManager where it's used.
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Shmakov (Gerrit)
17 comments:
Patch Set #2, Line 13:
purposes with the switch allow-incognito-custom-t
I would argue the other way around: We don't expect a different color, so there is no need to suppor […]
Ok; anyway, custom color would probably be very confusing for users.
File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:
Patch Set #2, Line 199:
apps launch
Missing a "to" in the middle.
Done
File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:
Patch Set #6, Line 2005:
mTabModalHandler.desThere's a bit of a mismatch here - you're registering it in onResume and then unregistering it in on […]
Actually, preInflationStartup() is called from onCreate(). So, as I understand, it's "paired" with onDestroyInternal().
Patch Set #8, Line 42:
private boolean mIsTrustedIntentWithSession;
Can this be final? For that matter, maybe it should be mIsFirstParty, because this won't be true for […]
mIsFirstParty would be misleading, because if first party app doesn't bind to the service, mIsFirstParty would be false. Also, it might be not clear whether Chrome counts as "first party" or not.
mIsTrustedIntentWithSession implies that there should be a "session", which is not true for intents from Chrome itself. This name is kind of awkward, but I don't see a better one.
Patch Set #8, Line 117:
crbug/832124
https://crbug. […]
Done
File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:
Patch Set #5, Line 277:
getWindow().addFlags(WindowManager.LayoutParams.FLAG_SECURE);
I removed usage of IncognitoTabSnapshotController, because it doesn't deal with reparenting well: th […]
Done
File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:
Patch Set #8, Line 1414:
// Should only be registered if the custom tab is incognito.Assert that in the constructor then?
Done
As I mentioned before, this should change to something like: […]
Discussed in this thread:
https://chromium-review.googlesource.com/c/chromium/src/+/1171225/2/chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java#201
Patch Set #6, Line 270:
ChromeSwitches
This should be ChromeFeatureList. […]
As I mentioned in reply to another comment, making this a feature is problematic:
https://chromium-review.googlesource.com/c/chromium/src/+/1171225/2/chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java#201
Patch Set #8, Line 172:
private void closeIncognitoTabs() {You could move this into IncognitoUtils?
Yes. I'll keep the "ThreadUtils.runOnUiThreadBlocking" part here though, as it's specific to the notification.
File chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoTabHost.java:
Small nit: superfluous empty line.
Done
File chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoTabHostRegistry.java:
Patch Set #6, Line 17:
volatile
Most of the code that touches the UI is run on the UI thread (we do run some code in parallel, but e […]
I got rid of synchronization
Patch Set #6, Line 23:
IncognitoTabHostRegistry
If you wanted to keep this, I'd recommend the Initialization-on-demand holder idiom [1], which does […]
Ok
Patch Set #6, Line 30:
mHosts
So my understanding of this class is that getInstance() is threadsafe (and therefore implies that it […]
Ok
File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabParamsManager.java:
Patch Set #6, Line 20:
IncognitoTabHostRegistry
Ah, I see why you wanted to make the thing thread-safe now. […]
I guess I'll lazily register in AsyncTabParamsManager::add. I believe that method is called from ui thread only, otherwise sAsyncTabParams would get messed up.
Another route is getting rid of statics, which is always a good idea. But this would introduce too many changes.
File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabsIncognitoTabHost.java:
Patch Set #8, Line 16:
public class AsyncTabsIncognitoTabHost implements IncognitoTabHost {And you could move this class into AsyncTabParamsManager where it's used.
Patch Set #6, Line 1193:
IDS_DATA_REDUCTION_INITIAL_TITLEHave you rebased/merged with master? It looks like you're picking up some changes from other CLs.
Yes, I did rebase; the diff with master looks fine
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Bernhard Bauer (Gerrit)
Nice! Just nits now:
3 comments:
File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:
Tiny nit: no colon
Patch Set #8, Line 42:
private final boolean mIsTrustedIntentWithSession;
mIsFirstParty would be misleading, because if first party app doesn't bind to the service, mIsFirstParty would be false.
That's fine -- if the client doesn't bind to the service, we have no way of verifying they are a first party app.
Also, it might be not clear whether Chrome counts as "first party" or not.
mIsTrustedIntentWithSession implies that there should be a "session", which is not true for intents from Chrome itself. This name is kind of awkward, but I don't see a better one.
Exactly -- currently the code checks for a client package name, and that is going to be empty for intents from Chrome. In terms of behavior, that's fine, because we can have an additional check for intents from Chrome somewhere else, so we can just update the member name to reflect this.
File chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoTabHostRegistry.java:
Teeny tiny nit: superfluous space.
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Shmakov (Gerrit)
1 comment:
Patch Set #8, Line 42:
private final boolean mIsTrustedIntentWithSession;
That's fine -- if the client doesn't bind to the service, we have no way of verifying they are a first party app.
But by naming mIsFirstParty we would state "this intent is not from first party" in that case, while the truth is "we don't know if it's first party". I think this distinction is important enough to make it clear.
I suggest naming it "mIsVerifiedFirstPartyIntent". Verbose, but more precise.
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Bernhard Bauer (Gerrit)
1 comment:
Patch Set #8, Line 42:
private final boolean mIsTrustedIntentWithSession;
> That's fine -- if the client doesn't bind to the service, we have no way of verifying they are a f […]
Sure, that works.
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Shmakov (Gerrit)
Pavel Shmakov uploaded patch set #11 to this change.
Introduced incognito custom tabs (ICT).
Notes:
1. The feature is hidden behind a switch enable-incognito-custom-tabs. Without it, the behavior should remain as before, i.e. only allow ICTs for payments flow.
2. ICT can be launched by adding extra com.google.android.apps.chrome.EXTRA_OPEN_NEW_INCOGNITO_TAB and only by first-party apps.
First-party apps also need to be maintaining a connection with CustomTabsService.
These limitations can be relaxed for testing purposes with the switch allow-incognito-custom-tabs-from-third-party.
3. The toolbar of ICT has grey theme color, and this can not be overriden by custom tab parameters.
4. ICT has menu item "Open in Incognito Chrome", which reparents the tab into Chrome. App picker is not shown.
Thus, unlike normal custom tabs, which either show an app-picker, or open default browser, ICT always opens Chrome.
5. "Close all incognito tabs" notification takes ICTs into account.
6. Taking screenshots of ICTs is not allowed, and no screenshots of them are seen in recents.
Bug: 871738
Change-Id: Ia5c7e9cd31bcacc5e8e30bc43d7b0f234ac007c4
---
M chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java
M chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
M chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
M chrome/android/java/src/org/chromium/chrome/browser/browserservices/BrowserSessionDataProvider.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
M chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java
A chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoTabHost.java
A chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoTabHostRegistry.java
A chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoUtils.java
M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabParamsManager.java
M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModelImplCreator.java
M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
M chrome/android/java/strings/android_chrome_strings.grd
M chrome/android/java_sources.gni
17 files changed, 381 insertions(+), 122 deletions(-)
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Shmakov (Gerrit)
3 comments:
File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:
Tiny nit: no colon
Done
Patch Set #8, Line 42:
private final boolean mIsVerifiedFirstPartyIntent;Sure, that works.
Done
File chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoTabHostRegistry.java:
Teeny tiny nit: superfluous space.
Done
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Bernhard Bauer (Gerrit)
Thank you!
Patch set 11:Code-Review +1
Peter Conn (Gerrit)
LGTM too!
Patch set 11:Code-Review +1
1 comment:
Patch Set #6, Line 1193:
IDS_DATA_REDUCTION_INITIAL_TITLEHave you rebased/merged with master? It looks like you're picking up some changes from other CLs.
Nevermind, you fixed this!
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Shmakov (Gerrit)
Patch set 11:Commit-Queue +2
Commit Bot (Gerrit)
Try jobs failed on following builders:
android-kitkat-arm-rel on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-kitkat-arm-rel/58243)
Pavel Shmakov (Gerrit)
1 comment:
Patch Set #12, Line 265:
boolean isPaymentRequest = isTrustedIntent() && isOpenedByChrome() && isForPaymentRequest();I messed up the payments flow, because the isVerifiedFirstPartyIntent() check was not passing by Chrome itself, which was discovered by tests.
Now I restored the logic exactly as it was, for the case when command line switches are off.
To view, visit change 1171225. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Rebase" https://chromium-review.googlesource.com/c/1171225/13
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1171225/13
Bot data: {"action": "start", "triggered_at": "2018-08-15T14:40:55.0Z", "cq_cfg_revision": "2d2bef7bb928a763e179d3e6e761d3c3c94c991d", "revision": "025fb555c11e73e3631a3d5178dd2d23c48edd37"}
Pavel Shmakov (Gerrit)
Patch set 13:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Introduced incognito custom tabs (ICT).
Notes:
1. The feature is hidden behind a switch enable-incognito-custom-tabs. Without it, the behavior should remain as before, i.e. only allow ICTs for payments flow.
2. ICT can be launched by adding extra com.google.android.apps.chrome.EXTRA_OPEN_NEW_INCOGNITO_TAB and only by first-party apps.
First-party apps also need to be maintaining a connection with CustomTabsService.
These limitations can be relaxed for testing purposes with the switch allow-incognito-custom-tabs-from-third-party.
3. The toolbar of ICT has grey theme color, and this can not be overriden by custom tab parameters.
4. ICT has menu item "Open in Incognito Chrome", which reparents the tab into Chrome. App picker is not shown.
Thus, unlike normal custom tabs, which either show an app-picker, or open default browser, ICT always opens Chrome.
5. "Close all incognito tabs" notification takes ICTs into account.
6. Taking screenshots of ICTs is not allowed, and no screenshots of them are seen in recents.
Bug: 871738
Change-Id: Ia5c7e9cd31bcacc5e8e30bc43d7b0f234ac007c4
Reviewed-on: https://chromium-review.googlesource.com/1171225
Commit-Queue: Pavel Shmakov <pshm...@chromium.org>
Reviewed-by: Bernhard Bauer <bau...@chromium.org>
Reviewed-by: Peter Conn <pec...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583256}
---
M chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java
M chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
M chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
M chrome/android/java/src/org/chromium/chrome/browser/browserservices/BrowserSessionDataProvider.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
M chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java
A chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoTabHost.java
A chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoTabHostRegistry.java
A chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoUtils.java
M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabParamsManager.java
M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModelImplCreator.java
M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
M chrome/android/java/strings/android_chrome_strings.grd
M chrome/android/java_sources.gni
17 files changed, 384 insertions(+), 122 deletions(-)