Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1688603004:
AGSA-initiated weblite intents should be rewritten if Chrome can use weblite (Closed)
DescriptionAGSA-initiated weblite intents should be rewritten if Chrome can use weblite
When Chrome receives a navigation intent for a page on googleweblight.com,
Chrome should instead navigate to the url specified in the lite_url query
parameter if all of the following are true:
1) The URL specifies a lite_url parameter
2) Data-Saver is enabled in Chrome
3) the LoFi weblite experiment is enabled
4) the scheme of the url in the lite_url param is HTTP
BUG=584085
Committed: https://crrev.com/30814b95ba3406d644debb4391cfe1b289d29aa4
Cr-Commit-Position: refs/heads/master@{#378414}
Patch Set 1 : #Patch Set 2 : #
Total comments: 22
Patch Set 3 : bengr comments #
Total comments: 21
Patch Set 4 : comments #Patch Set 5 : comments #Patch Set 6 : rebase #Patch Set 7 : move custom tab rewrite to internal #
Total comments: 12
Patch Set 8 : yusufo comments #
Total comments: 14
Patch Set 9 : comments and rebase #
Total comments: 13
Patch Set 10 : bengr comments, blocked on crbug.com/589663 #Patch Set 11 : test fixes, all passing now #
Total comments: 2
Patch Set 12 : bengr comments #Patch Set 13 : rebase #Patch Set 14 : bug fix #
Total comments: 2
Messages
Total messages: 58 (20 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
megjablon@chromium.org changed reviewers: + bengr@chromium.org
PTAL.
https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:89: private static final String PACKAGE_GSA = "com.google.android.googlequicksearchbox"; For an experiment, I suppose this is OK, but a better architecture would embedded a package specific URL rewriter. Add a TODO to do this or fix it here. To fix it, I would add an interface for a rewriter that takes a package name and a url and returns a url, and then I'd add your rewriter to a list of rewriters. https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:283: String url = intent != null ? IntentHandler.getUrlFromIntent(getIntent()) : null; Shouldn't you use intent and not getIntent() here? https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:287: url = DataReductionProxySettings.getInstance().maybeExtractWebliteURL(url); Maybe call this maybeExtractFromWebliteUrl(). At the very least, use "Url" and not "URL." https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java (right): https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java:576: LoadUrlParams loadParams = asyncParams != null ? asyncParams.getLoadUrlParams() : null; Can you make this more compact? E.g.: if (!isIncognito() && asyncParams != null) { LoadUrlParams loadUrlParams = asyncParams.getLoadUrlParams(); if (loadUrlParams != null && loadUrlParams.getUrl() != null) { loadUrlParams.setUrl(...); } } https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:296: if (isWebLiteURL(uri) && areLoFiPreviewsEnabled() && isDataReductionProxyEnabled()) { I would replace isWebLiteURL() with url != null && url.getHost().equals(WEBLITE_HOST_STRING) and get rid of isWebLiteURL. https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:297: String liteURL = getUrlFromWebliteUrl(uri); liteUrl Also, why not just call url.getQueryParameter("lite_url") here and get rid of getUrlFromWebliteUrl(). https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:313: private static boolean areLoFiPreviewsEnabled() { Since native uses this experiment, can you call native for the answer? https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:27: import org.chromium.base.Log; I'd save this change for a different CL. https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java (right): https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java:238: if (url != null) url = DataReductionProxySettings.getInstance().maybeExtractWebliteURL(url); Do you need the null check? https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/ActivityDelegate.java (right): https://codereview.chromium.org/1688603004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/ActivityDelegate.java:145: Uri data = intent.getData(); Do you need this change? https://codereview.chromium.org/1688603004/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsOpenedFromExternalAppTest.java (right): https://codereview.chromium.org/1688603004/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/TabsOpenedFromExternalAppTest.java:553: public void testLaunchWebLiteURL() throws InterruptedException { You probably also want to test that the URL is not rewritten when not in the preview experiment.
megjablon@chromium.org changed reviewers: + dfalcantara@chromium.org
dfalcantara: chrome/android/* https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:89: private static final String PACKAGE_GSA = "com.google.android.googlequicksearchbox"; On 2016/02/16 19:38:06, bengr wrote: > For an experiment, I suppose this is OK, but a better architecture would > embedded a package specific URL rewriter. Add a TODO to do this or fix it here. > To fix it, I would add an interface for a rewriter that takes a package name and > a url and returns a url, and then I'd add your rewriter to a list of rewriters. I'd rather keep this cl as small as possible since it needs to be merged, so I'll add a TODO. https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:283: String url = intent != null ? IntentHandler.getUrlFromIntent(getIntent()) : null; On 2016/02/16 19:38:06, bengr wrote: > Shouldn't you use intent and not getIntent() here? Done. https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:287: url = DataReductionProxySettings.getInstance().maybeExtractWebliteURL(url); On 2016/02/16 19:38:06, bengr wrote: > Maybe call this maybeExtractFromWebliteUrl(). At the very least, use "Url" and > not "URL." Done. https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java:576: LoadUrlParams loadParams = asyncParams != null ? asyncParams.getLoadUrlParams() : null; On 2016/02/16 19:38:06, bengr wrote: > Can you make this more compact? E.g.: > > if (!isIncognito() && asyncParams != null) { > LoadUrlParams loadUrlParams = asyncParams.getLoadUrlParams(); > if (loadUrlParams != null && loadUrlParams.getUrl() != null) { > loadUrlParams.setUrl(...); > } > } Done. https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:296: if (isWebLiteURL(uri) && areLoFiPreviewsEnabled() && isDataReductionProxyEnabled()) { On 2016/02/16 19:38:06, bengr wrote: > I would replace isWebLiteURL() with url != null && > url.getHost().equals(WEBLITE_HOST_STRING) and get rid of isWebLiteURL. Done. https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:297: String liteURL = getUrlFromWebliteUrl(uri); On 2016/02/16 19:38:06, bengr wrote: > liteUrl > > Also, why not just call url.getQueryParameter("lite_url") here and get rid of > getUrlFromWebliteUrl(). Done. https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:313: private static boolean areLoFiPreviewsEnabled() { On 2016/02/16 19:38:06, bengr wrote: > Since native uses this experiment, can you call native for the answer? Done. https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:27: import org.chromium.base.Log; On 2016/02/16 19:38:06, bengr wrote: > I'd save this change for a different CL. This must have snuck back in with a rebase.. I remember cleaning this up. https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java:238: if (url != null) url = DataReductionProxySettings.getInstance().maybeExtractWebliteURL(url); On 2016/02/16 19:38:06, bengr wrote: > Do you need the null check? Yes. If the url isn't found in the intent, null could be passed in. https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/ActivityDelegate.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/ActivityDelegate.java:145: Uri data = intent.getData(); On 2016/02/16 19:38:06, bengr wrote: > Do you need this change? Nope. Don't know how this snuck in here. https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsOpenedFromExternalAppTest.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/100001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/TabsOpenedFromExternalAppTest.java:553: public void testLaunchWebLiteURL() throws InterruptedException { On 2016/02/16 19:38:06, bengr wrote: > You probably also want to test that the URL is not rewritten when not in the > preview experiment. Done.
dfalcantara@chromium.org changed reviewers: + ianwen@chromium.org
+ianwen for CustomTabActivity URL changing.
https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:89: private static final String PACKAGE_GSA = "com.google.android.googlequicksearchbox"; Don't duplicate this string. Just expose the one in IntentHandler. https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:133: String url = IntentHandler.getUrlFromIntent(intent); Did you not need to change this case of getUrlFromIntent()? https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:288: .getClientPackageNameForSession(mSession); 1) Does IntentHandler.determineExternalIntentSource(intent) not give you the same information? 2) If you really have to check the session, ClientManager.getClientPackageNameForSession() can return null. Use TextUtils.equals(). https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:286: && isDataReductionProxyEnabled()) { getHost() can return null. Use TextUtils.equals(WEBLITE_HOSTNAME_STRING, uri.getHost()). https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:287: String liteUrl = uri.getQueryParameter("lite_url"); Extract "lite_url" into a private static final String. https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:288: if (liteUrl != null && Uri.parse(liteUrl).getScheme().equals("http")) return liteUrl; getScheme() can return null. Use TextUtils.equals("http", Uri.parse(liteUrl).getScheme()) instead. https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java:238: if (url != null) url = DataReductionProxySettings.getInstance().maybeExtractWebliteUrl(url); Add a comment about why this line is put here, and only in this specific version of launchUrl*().
https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:89: private static final String PACKAGE_GSA = "com.google.android.googlequicksearchbox"; This string already exists in IntentHandler. https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:282: private String getUrlFromIntent(Intent intent) { Can we put the logic in this method to IntentHandler#getUrlFromIntent()? There are a series of url conversions in that function. And if in CustomTabActivity we use a new getUrlFromIntent(), we might miss some conversions there. This way: 1. you can group all weblite url conversion in one place. 2. no need to change code in tabCreator. https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsOpenedFromExternalAppTest.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/TabsOpenedFromExternalAppTest.java:551: @CommandLineFlags.Add({"enable-spdy-proxy-auth", "data-reduction-proxy-lo-fi=always-on", Could you please also add a test for CustomTabActivity? The tests should be added to CustomTabActivity.java.
https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:89: private static final String PACKAGE_GSA = "com.google.android.googlequicksearchbox"; On 2016/02/16 23:28:45, dfalcantara wrote: > Don't duplicate this string. Just expose the one in IntentHandler. Done. https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:133: String url = IntentHandler.getUrlFromIntent(intent); On 2016/02/16 23:28:45, dfalcantara wrote: > Did you not need to change this case of getUrlFromIntent()? Added. Don't know how I missed this one. https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:282: private String getUrlFromIntent(Intent intent) { On 2016/02/16 23:36:03, Ian Wen wrote: > Can we put the logic in this method to IntentHandler#getUrlFromIntent()? There > are a series of url conversions in that function. And if in CustomTabActivity we > use a new getUrlFromIntent(), we might miss some conversions there. > > This way: > 1. you can group all weblite url conversion in one place. > 2. no need to change code in tabCreator. I initially tried this, but IntentHandler#getUrlFromIntent() is called many times before native is initialized, and since we are checking for a Finch config value, we need native to be initialized. Do you know if there a way to check if native is initialized from IntentHandler? I like the idea of all the conversion being done in one place and was worried about new code missing the url conversion too. https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:288: .getClientPackageNameForSession(mSession); On 2016/02/16 23:28:45, dfalcantara wrote: > 1) Does IntentHandler.determineExternalIntentSource(intent) not give you the > same information? > > 2) If you really have to check the session, > ClientManager.getClientPackageNameForSession() can return null. Use > TextUtils.equals(). 1. determineExternalIntentSource gets the string extra Browser.EXTRA_APPLICATION_ID and can be spoofed. getClientPackageNameForSession uses the Client UID, as returned by Binder.getCallingUid(), to resolve the package name. 2. Removed null check and using TextUtils instead. https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:286: && isDataReductionProxyEnabled()) { On 2016/02/16 23:28:45, dfalcantara wrote: > getHost() can return null. > > Use TextUtils.equals(WEBLITE_HOSTNAME_STRING, uri.getHost()). Done. https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:287: String liteUrl = uri.getQueryParameter("lite_url"); On 2016/02/16 23:28:45, dfalcantara wrote: > Extract "lite_url" into a private static final String. Done. https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:288: if (liteUrl != null && Uri.parse(liteUrl).getScheme().equals("http")) return liteUrl; On 2016/02/16 23:28:45, dfalcantara wrote: > getScheme() can return null. > > Use TextUtils.equals("http", Uri.parse(liteUrl).getScheme()) instead. Done. https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java:238: if (url != null) url = DataReductionProxySettings.getInstance().maybeExtractWebliteUrl(url); On 2016/02/16 23:28:45, dfalcantara wrote: > Add a comment about why this line is put here, and only in this specific version > of launchUrl*(). Done. https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsOpenedFromExternalAppTest.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/TabsOpenedFromExternalAppTest.java:551: @CommandLineFlags.Add({"enable-spdy-proxy-auth", "data-reduction-proxy-lo-fi=always-on", On 2016/02/16 23:36:03, Ian Wen wrote: > Could you please also add a test for CustomTabActivity? The tests should be > added to CustomTabActivity.java. I tried doing this, but the CustomTabActivity checks that the package is AGSA using getClientPackageNameForSession, and I couldn't figure out a way to have the binder fake the intent from another app. Any ideas?
https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:282: private String getUrlFromIntent(Intent intent) { On 2016/02/17 00:17:44, megjablon wrote: > On 2016/02/16 23:36:03, Ian Wen wrote: > > Can we put the logic in this method to IntentHandler#getUrlFromIntent()? There > > are a series of url conversions in that function. And if in CustomTabActivity > we > > use a new getUrlFromIntent(), we might miss some conversions there. > > > > This way: > > 1. you can group all weblite url conversion in one place. > > 2. no need to change code in tabCreator. > > I initially tried this, but IntentHandler#getUrlFromIntent() is called many > times before native is initialized, and since we are checking for a Finch config > value, we need native to be initialized. Do you know if there a way to check if > native is initialized from IntentHandler? I like the idea of all the conversion > being done in one place and was worried about new code missing the url > conversion too. You could try guarding it with LibraryLoader.isInitialized(). That has the side-effect of potentially having the function return two different URLs, depending on whether the library is available, but at least you'd be consistently going through that unified code path...
Patchset #5 (id:160001) has been deleted
https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:282: private String getUrlFromIntent(Intent intent) { On 2016/02/17 00:45:01, dfalcantara wrote: > On 2016/02/17 00:17:44, megjablon wrote: > > On 2016/02/16 23:36:03, Ian Wen wrote: > > > Can we put the logic in this method to IntentHandler#getUrlFromIntent()? > There > > > are a series of url conversions in that function. And if in > CustomTabActivity > > we > > > use a new getUrlFromIntent(), we might miss some conversions there. > > > > > > This way: > > > 1. you can group all weblite url conversion in one place. > > > 2. no need to change code in tabCreator. > > > > I initially tried this, but IntentHandler#getUrlFromIntent() is called many > > times before native is initialized, and since we are checking for a Finch > config > > value, we need native to be initialized. Do you know if there a way to check > if > > native is initialized from IntentHandler? I like the idea of all the > conversion > > being done in one place and was worried about new code missing the url > > conversion too. > > You could try guarding it with LibraryLoader.isInitialized(). That has the > side-effect of potentially having the function return two different URLs, > depending on whether the library is available, but at least you'd be > consistently going through that unified code path... That was a concern of mine. I don't like the idea of the function returning different URLs based on the native initialization timing. While this does currently work very nicely, it could introduce unsuspected issues to users of getUrlFromIntent in the future. I prefer the current way that alters the url at a specific time.
On 2016/02/17 23:16:33, megjablon wrote: > https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... > File > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java > (right): > > https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:282: > private String getUrlFromIntent(Intent intent) { > On 2016/02/17 00:45:01, dfalcantara wrote: > > On 2016/02/17 00:17:44, megjablon wrote: > > > On 2016/02/16 23:36:03, Ian Wen wrote: > > > > Can we put the logic in this method to IntentHandler#getUrlFromIntent()? > > There > > > > are a series of url conversions in that function. And if in > > CustomTabActivity > > > we > > > > use a new getUrlFromIntent(), we might miss some conversions there. > > > > > > > > This way: > > > > 1. you can group all weblite url conversion in one place. > > > > 2. no need to change code in tabCreator. > > > > > > I initially tried this, but IntentHandler#getUrlFromIntent() is called many > > > times before native is initialized, and since we are checking for a Finch > > config > > > value, we need native to be initialized. Do you know if there a way to check > > if > > > native is initialized from IntentHandler? I like the idea of all the > > conversion > > > being done in one place and was worried about new code missing the url > > > conversion too. > > > > You could try guarding it with LibraryLoader.isInitialized(). That has the > > side-effect of potentially having the function return two different URLs, > > depending on whether the library is available, but at least you'd be > > consistently going through that unified code path... > > That was a concern of mine. I don't like the idea of the function returning > different URLs based on the native initialization timing. While this does > currently work very nicely, it could introduce unsuspected issues to users of > getUrlFromIntent in the future. I prefer the current way that alters the url at > a specific time. Granted that megjablon@'s concerns are valid. How about we keep two methods in IntentHandler: getUrlFromIntent(), and getUrlFromIntentWithNative(). In getUrlFromIntentWithNative(), it calls getUrlFromIntent() and then add conversions that requires native library. In CustomTabActivity, all the calls of getUrlFromIntent() happen after the native is loaded, meaning that we can safely convert to getUrlFromIntentWithNative(). I know AGSA is about to launch with custom tabs very soon, meaning that CustomTabActivity might be the major case you need to handle.
On 2016/02/18 00:34:23, Ian Wen wrote: > On 2016/02/17 23:16:33, megjablon wrote: > > > https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java > > (right): > > > > > https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:282: > > private String getUrlFromIntent(Intent intent) { > > On 2016/02/17 00:45:01, dfalcantara wrote: > > > On 2016/02/17 00:17:44, megjablon wrote: > > > > On 2016/02/16 23:36:03, Ian Wen wrote: > > > > > Can we put the logic in this method to IntentHandler#getUrlFromIntent()? > > > There > > > > > are a series of url conversions in that function. And if in > > > CustomTabActivity > > > > we > > > > > use a new getUrlFromIntent(), we might miss some conversions there. > > > > > > > > > > This way: > > > > > 1. you can group all weblite url conversion in one place. > > > > > 2. no need to change code in tabCreator. > > > > > > > > I initially tried this, but IntentHandler#getUrlFromIntent() is called > many > > > > times before native is initialized, and since we are checking for a Finch > > > config > > > > value, we need native to be initialized. Do you know if there a way to > check > > > if > > > > native is initialized from IntentHandler? I like the idea of all the > > > conversion > > > > being done in one place and was worried about new code missing the url > > > > conversion too. > > > > > > You could try guarding it with LibraryLoader.isInitialized(). That has the > > > side-effect of potentially having the function return two different URLs, > > > depending on whether the library is available, but at least you'd be > > > consistently going through that unified code path... > > > > That was a concern of mine. I don't like the idea of the function returning > > different URLs based on the native initialization timing. While this does > > currently work very nicely, it could introduce unsuspected issues to users of > > getUrlFromIntent in the future. I prefer the current way that alters the url > at > > a specific time. > > Granted that megjablon@'s concerns are valid. > > How about we keep two methods in IntentHandler: getUrlFromIntent(), and > getUrlFromIntentWithNative(). In getUrlFromIntentWithNative(), it calls > getUrlFromIntent() and then add conversions that requires native library. In > CustomTabActivity, all the calls of getUrlFromIntent() happen after the native > is loaded, meaning that we can safely convert to getUrlFromIntentWithNative(). > > I know AGSA is about to launch with custom tabs very soon, meaning that > CustomTabActivity might be the major case you need to handle. @bengr We wouldn't be able to verify the client package using CustomTabsConnection#getClientPackageNameForSession, but this would be a much cleaner way of modifying the url. We could at least verify that it's a first party app using IntentHandler#isIntentChromeOrFirstParty. We could also use this in combination with IntentHandler#determineExternalIntentSource as long as all versions of AGSA that have weblite also add the string extra Browser.EXTRA_APPLICATION_ID. WDYT?
On 2016/02/18 01:20:31, megjablon wrote: > On 2016/02/18 00:34:23, Ian Wen wrote: > > On 2016/02/17 23:16:33, megjablon wrote: > > > > > > https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java > > > (right): > > > > > > > > > https://chromiumcodereview.appspot.com/1688603004/diff/120001/chrome/android/... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:282: > > > private String getUrlFromIntent(Intent intent) { > > > On 2016/02/17 00:45:01, dfalcantara wrote: > > > > On 2016/02/17 00:17:44, megjablon wrote: > > > > > On 2016/02/16 23:36:03, Ian Wen wrote: > > > > > > Can we put the logic in this method to > IntentHandler#getUrlFromIntent()? > > > > There > > > > > > are a series of url conversions in that function. And if in > > > > CustomTabActivity > > > > > we > > > > > > use a new getUrlFromIntent(), we might miss some conversions there. > > > > > > > > > > > > This way: > > > > > > 1. you can group all weblite url conversion in one place. > > > > > > 2. no need to change code in tabCreator. > > > > > > > > > > I initially tried this, but IntentHandler#getUrlFromIntent() is called > > many > > > > > times before native is initialized, and since we are checking for a > Finch > > > > config > > > > > value, we need native to be initialized. Do you know if there a way to > > check > > > > if > > > > > native is initialized from IntentHandler? I like the idea of all the > > > > conversion > > > > > being done in one place and was worried about new code missing the url > > > > > conversion too. > > > > > > > > You could try guarding it with LibraryLoader.isInitialized(). That has > the > > > > side-effect of potentially having the function return two different URLs, > > > > depending on whether the library is available, but at least you'd be > > > > consistently going through that unified code path... > > > > > > That was a concern of mine. I don't like the idea of the function returning > > > different URLs based on the native initialization timing. While this does > > > currently work very nicely, it could introduce unsuspected issues to users > of > > > getUrlFromIntent in the future. I prefer the current way that alters the url > > at > > > a specific time. > > > > Granted that megjablon@'s concerns are valid. > > > > How about we keep two methods in IntentHandler: getUrlFromIntent(), and > > getUrlFromIntentWithNative(). In getUrlFromIntentWithNative(), it calls > > getUrlFromIntent() and then add conversions that requires native library. In > > CustomTabActivity, all the calls of getUrlFromIntent() happen after the native > > is loaded, meaning that we can safely convert to getUrlFromIntentWithNative(). > > > > I know AGSA is about to launch with custom tabs very soon, meaning that > > CustomTabActivity might be the major case you need to handle. > > @bengr > We wouldn't be able to verify the client package using > CustomTabsConnection#getClientPackageNameForSession, but this would be a much > cleaner way of modifying the url. We could at least verify that it's a first > party app using IntentHandler#isIntentChromeOrFirstParty. We could also use this > in combination with IntentHandler#determineExternalIntentSource as long as all > versions of AGSA that have weblite also add the string extra > Browser.EXTRA_APPLICATION_ID. WDYT? AGSA will use CustomTabActivity most of the time but occasionally will use vanilla intents. In the former case, it sounds like we are guaranteed to have native before trying to convert a URL, so using ...WithNative() seems like a great fit. For the latter case, however, native isn't guaranteed to be loaded, but we need to rewrite URLs nonetheless. Do we have a solution for this case? We need to do this only for AGSA, so verifying that the intent is from a first party (with and without custom tabs) plus checking the advertised package name seems ok, so long as AGSA currently adds Browser.EXTRA_APPLICATION_ID. I don't want to require code changes in AGSA. If AGSA doesn't currently add the EXTRA_APPLICATION_ID, we need another solution.
Yusuf: Do you know if we're guaranteed that GSA Intents will properly set the application ID in the Intent?
On 2016/02/18 01:37:57, dfalcantara wrote: > Yusuf: Do you know if we're guaranteed that GSA Intents will properly set the > application ID in the Intent? It is guaranteed as in they are adding it to all right now.
On 2016/02/18 19:41:33, Yusuf wrote: > On 2016/02/18 01:37:57, dfalcantara wrote: > > Yusuf: Do you know if we're guaranteed that GSA Intents will properly set the > > application ID in the Intent? > > It is guaranteed as in they are adding it to all right now. That won't work for us because we need to support earlier versions of GSA sending Weblite intents. Since this is for an experiment, can we move forward with the current design and revisit it if need be when we do a full launch?
Patchset #6 (id:200001) has been deleted
Patchset #7 (id:240001) has been deleted
On 2016/02/18 19:49:24, megjablon wrote: > On 2016/02/18 19:41:33, Yusuf wrote: > > On 2016/02/18 01:37:57, dfalcantara wrote: > > > Yusuf: Do you know if we're guaranteed that GSA Intents will properly set > the > > > application ID in the Intent? > > > > It is guaranteed as in they are adding it to all right now. > > That won't work for us because we need to support earlier versions of GSA > sending Weblite intents. Since this is for an experiment, can we move forward > with the current design and revisit it if need be when we do a full launch? Yusuf, I've implemented the changes we talked about. Could you please take a look? Thanks!
On 2016/02/20 01:28:15, megjablon wrote: > On 2016/02/18 19:49:24, megjablon wrote: > > On 2016/02/18 19:41:33, Yusuf wrote: > > > On 2016/02/18 01:37:57, dfalcantara wrote: > > > > Yusuf: Do you know if we're guaranteed that GSA Intents will properly set > > the > > > > application ID in the Intent? > > > > > > It is guaranteed as in they are adding it to all right now. > > > > That won't work for us because we need to support earlier versions of GSA > > sending Weblite intents. Since this is for an experiment, can we move forward > > with the current design and revisit it if need be when we do a full launch? > > Yusuf, I've implemented the changes we talked about. Could you please take a > look? Thanks! I'll wait for Yusuf to approve before taking a final look.
yusufo@chromium.org changed reviewers: + yusufo@chromium.org
https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:135: url = CustomTabsConnection.getInstance(sActiveContentHandler.getApplication()) instead of doing it here and adding getApplication, we can do it while overriding loadUrlAndTrackFromTimestamp inside the activity instance. you can get and recreate a loadurlparams if necessary from there. https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:289: url = CustomTabsConnection.getInstance(getApplication()).overrideUrlIfNecessary(url, customTabsConnection is local a few lines above. Rebase? Then it will fit in one line https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:305: url = CustomTabsConnection.getInstance(getApplication()).overrideUrlIfNecessary(url, customTabsConnection is created as local afew lines below https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabContentHandler.java (right): https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabContentHandler.java:37: Application getApplication(); this doesn't really belong here. this is an interface used for repeated intents coming to the same tab. https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:106: protected static boolean sTestIntentsEnabled; the connection is already a singleton. No need to have this static. https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:149: public static void setTestIntentsEnabled(boolean enabled) { do we ever set this? can't find it in the test change.
https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:135: url = CustomTabsConnection.getInstance(sActiveContentHandler.getApplication()) On 2016/02/23 23:26:00, Yusuf wrote: > instead of doing it here and adding getApplication, we can do it while > overriding loadUrlAndTrackFromTimestamp inside the activity instance. you can > get and recreate a loadurlparams if necessary from there. Done. https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:289: url = CustomTabsConnection.getInstance(getApplication()).overrideUrlIfNecessary(url, On 2016/02/23 23:25:59, Yusuf wrote: > customTabsConnection is local a few lines above. Rebase? Then it will fit in one > line Done. https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:305: url = CustomTabsConnection.getInstance(getApplication()).overrideUrlIfNecessary(url, On 2016/02/23 23:25:59, Yusuf wrote: > customTabsConnection is created as local afew lines below Done. https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabContentHandler.java (right): https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabContentHandler.java:37: Application getApplication(); On 2016/02/23 23:26:00, Yusuf wrote: > this doesn't really belong here. this is an interface used for repeated intents > coming to the same tab. Done. https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:106: protected static boolean sTestIntentsEnabled; On 2016/02/23 23:26:00, Yusuf wrote: > the connection is already a singleton. No need to have this static. Done. https://codereview.chromium.org/1688603004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:149: public static void setTestIntentsEnabled(boolean enabled) { On 2016/02/23 23:26:00, Yusuf wrote: > do we ever set this? can't find it in the test change. Moved internal. Ended up being in internal test.
mostly nits around rebase issues you will get and unnecessary imports. customtabs/ lgtm https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:211: CustomTabsConnection customTabsConnection = if you rebase, you will see that this is already there with a different name. https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:250: params.setUrl(CustomTabsConnection.getInstance(getApplication()) use the local instance? https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:287: mainTab.setAppAssociatedWith(customTabsConnection.getClientPackageNameForSession(mSession)); rebasing will remove some of these, fyi https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:295: CustomTabsConnection customTabsConnection = rebasing will remove some of these. https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabContentHandler.java (right): https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabContentHandler.java:10: import android.os.SystemClock; not needed https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:23: import android.support.customtabs.CustomTabsCallback; not needed
general lgtm % yusufo's nits https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:368: * @return The url to be used. Maybe overriden. nit: May be
Patchset #9 (id:300001) has been deleted
https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:211: CustomTabsConnection customTabsConnection = On 2016/02/24 00:42:10, Yusuf wrote: > if you rebase, you will see that this is already there with a different name. Ah now I see what you're saying. Done. https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:250: params.setUrl(CustomTabsConnection.getInstance(getApplication()) On 2016/02/24 00:42:10, Yusuf wrote: > use the local instance? Done. https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:287: mainTab.setAppAssociatedWith(customTabsConnection.getClientPackageNameForSession(mSession)); On 2016/02/24 00:42:10, Yusuf wrote: > rebasing will remove some of these, fyi Done. https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:295: CustomTabsConnection customTabsConnection = On 2016/02/24 00:42:10, Yusuf wrote: > rebasing will remove some of these. Done. https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabContentHandler.java (right): https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabContentHandler.java:10: import android.os.SystemClock; On 2016/02/24 00:42:10, Yusuf wrote: > not needed Done. https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:23: import android.support.customtabs.CustomTabsCallback; On 2016/02/24 00:42:10, Yusuf wrote: > not needed Done. https://codereview.chromium.org/1688603004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:368: * @return The url to be used. Maybe overriden. On 2016/02/24 00:50:02, dfalcantara wrote: > nit: May be Done.
Mostly good. https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:249: params.setUrl(connection.overrideUrlIfNecessary(params.getUrl(), mSession)); Can connection be null? https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:363: * Returns the url that should be used for the custom tab. Say more about what "used" means. Is this the URL to which the custom tab should first navigate? https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:366: * @param session The Binder object identifying a session. I find it a little odd that you're documenting a parameter that isn't used, but am fine with you leaving it this way. https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:287: Uri uri = Uri.parse(url); Should you swallow the NullPointerException thrown if url is null? See http://developer.android.com/reference/android/net/Uri.html#parse(java.lang.S... https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:290: String liteUrl = uri.getQueryParameter(WEBLITE_QUERY_PARAM); Should we also check that liteUrl is a well-formed URL? Does getScheme() effectively do this for you? https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:290: String liteUrl = uri.getQueryParameter(WEBLITE_QUERY_PARAM); Should we also check that liteUrl is a well-formed URL? Does getScheme() effectively do this for you? https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java:237: // using preview mode, then use the URL in the lite_url parameter if its scheme is HTTP. preview -> weblite. Names should be consistent throughput.
https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:249: params.setUrl(connection.overrideUrlIfNecessary(params.getUrl(), mSession)); On 2016/02/24 18:21:28, bengr wrote: > Can connection be null? Nope. If there isn't a CustomTabsConnection then getInstance creates one. https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:363: * Returns the url that should be used for the custom tab. On 2016/02/24 18:21:28, bengr wrote: > Say more about what "used" means. Is this the URL to which the custom tab should > first navigate? Done. https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:366: * @param session The Binder object identifying a session. On 2016/02/24 18:21:28, bengr wrote: > I find it a little odd that you're documenting a parameter that isn't used, but > am fine with you leaving it this way. Done. https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:287: Uri uri = Uri.parse(url); On 2016/02/24 18:21:28, bengr wrote: > Should you swallow the NullPointerException thrown if url is null? See > http://developer.android.com/reference/android/net/Uri.html#parse(java.lang.S... We check that the url is not null before calling maybeExtractWebliteUrl, but I added a null check so this will return null if the url is null. https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:290: String liteUrl = uri.getQueryParameter(WEBLITE_QUERY_PARAM); On 2016/02/24 18:21:29, bengr wrote: > Should we also check that liteUrl is a well-formed URL? Does getScheme() > effectively do this for you? Nope, the Uri class does not do validation. Added a check. https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java (right): https://chromiumcodereview.appspot.com/1688603004/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java:237: // using preview mode, then use the URL in the lite_url parameter if its scheme is HTTP. On 2016/02/24 18:21:29, bengr wrote: > preview -> weblite. Names should be consistent throughput. Done.
lgtm, but consider renaming and breaking up that function. https://codereview.chromium.org/1688603004/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/1688603004/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:282: * lite_url param is HTTP, returns the url contained in the lite_url param. Otherwise returns nit: url -> URL https://codereview.chromium.org/1688603004/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:288: public String maybeExtractWebliteUrl(String url) { Consider breaking this up into two functions as listed below. At the very least, please rename as maybeRewriteWebliteUrl(). String extractUrlFromWebliteQueryParams(String url) { if (!TextUtils.equals(uri.getHost(), WEBLITE_HOSTNAME)) return null; return Uri.parse(url).getQueryparameter(WEBLITE_QUERY_PARAM); } String maybeRewriteWebliteUrl(String url) { if (url == null || !areLoFiPreviewsEnabled() || !isDataReductionProxyEnabled()) { return url; } String rewritten = extractUrlFromWebliteQueryParams(url); if (rewritten == null || !TextUtils.equals("http", Uri.parse(rewritten).getScheme()) { return url; } return rewritten; }
Patchset #13 (id:400001) has been deleted
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, dfalcantara@chromium.org, bengr@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1688603004/#ps420001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688603004/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688603004/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, bengr@chromium.org, dfalcantara@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1688603004/#ps440001 (title: "bug fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688603004/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688603004/440001
Message was sent while issue was closed.
Committed patchset #14 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== AGSA-initiated weblite intents should be rewritten if Chrome can use weblite When Chrome receives a navigation intent for a page on googleweblight.com, Chrome should instead navigate to the url specified in the lite_url query parameter if all of the following are true: 1) The URL specifies a lite_url parameter 2) Data-Saver is enabled in Chrome 3) the LoFi weblite experiment is enabled 4) the scheme of the url in the lite_url param is HTTP BUG=584085 ========== to ========== AGSA-initiated weblite intents should be rewritten if Chrome can use weblite When Chrome receives a navigation intent for a page on googleweblight.com, Chrome should instead navigate to the url specified in the lite_url query parameter if all of the following are true: 1) The URL specifies a lite_url parameter 2) Data-Saver is enabled in Chrome 3) the LoFi weblite experiment is enabled 4) the scheme of the url in the lite_url param is HTTP BUG=584085 Committed: https://crrev.com/30814b95ba3406d644debb4391cfe1b289d29aa4 Cr-Commit-Position: refs/heads/master@{#378414} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/30814b95ba3406d644debb4391cfe1b289d29aa4 Cr-Commit-Position: refs/heads/master@{#378414}
Message was sent while issue was closed.
jbudorick@chromium.org changed reviewers: + jbudorick@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java (right): https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java:597: mTab = createActivityTab(asyncParams); Was adding this line intentional or the product of a bad rebase? It wasn't part of the diff in patchset 12 but was in patchset 13 (and in the version that landed). It seems to be causing four DocumentModeTests to fail: C 241.505s Main [ FAILED ] org.chromium.chrome.browser.document.DocumentModeTest#testLastTabIdUpdates (UNKNOWN) C 241.505s Main [ FAILED ] org.chromium.chrome.browser.document.DocumentModeTest#testTargetBlank (UNKNOWN) C 241.505s Main [ FAILED ] org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpen (UNKNOWN) C 241.506s Main [ FAILED ] org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpenWithOpenerSuppressed (UNKNOWN) If it wasn't intentional, I can remove it in https://codereview.chromium.org/1748853002/
Message was sent while issue was closed.
https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java (right): https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java:597: mTab = createActivityTab(asyncParams); On 2016/03/01 17:59:06, jbudorick wrote: > Was adding this line intentional or the product of a bad rebase? It wasn't part > of the diff in patchset 12 but was in patchset 13 (and in the version that > landed). > > It seems to be causing four DocumentModeTests to fail: > > C 241.505s Main [ FAILED ] > org.chromium.chrome.browser.document.DocumentModeTest#testLastTabIdUpdates > (UNKNOWN) > C 241.505s Main [ FAILED ] > org.chromium.chrome.browser.document.DocumentModeTest#testTargetBlank (UNKNOWN) > C 241.505s Main [ FAILED ] > org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpen (UNKNOWN) > C 241.506s Main [ FAILED ] > org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpenWithOpenerSuppressed > (UNKNOWN) > > If it wasn't intentional, I can remove it in > https://codereview.chromium.org/1748853002/ It's definitely a screwed up rebase; you can see the tab created above. This patch is broken in document mode now, because the tab is created before the URL is munged.
Message was sent while issue was closed.
On 2016/03/01 18:08:48, dfalcantara wrote: > https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java > (right): > > https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java:597: > mTab = createActivityTab(asyncParams); > On 2016/03/01 17:59:06, jbudorick wrote: > > Was adding this line intentional or the product of a bad rebase? It wasn't > part > > of the diff in patchset 12 but was in patchset 13 (and in the version that > > landed). > > > > It seems to be causing four DocumentModeTests to fail: > > > > C 241.505s Main [ FAILED ] > > org.chromium.chrome.browser.document.DocumentModeTest#testLastTabIdUpdates > > (UNKNOWN) > > C 241.505s Main [ FAILED ] > > org.chromium.chrome.browser.document.DocumentModeTest#testTargetBlank > (UNKNOWN) > > C 241.505s Main [ FAILED ] > > org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpen (UNKNOWN) > > C 241.506s Main [ FAILED ] > > > org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpenWithOpenerSuppressed > > (UNKNOWN) > > > > If it wasn't intentional, I can remove it in > > https://codereview.chromium.org/1748853002/ > > It's definitely a screwed up rebase; you can see the tab created above. This > patch is broken in document mode now, because the tab is created before the URL > is munged. Fixing in https://codereview.chromium.org/1748853002/, then.
Message was sent while issue was closed.
On 2016/03/01 18:09:47, jbudorick wrote: > On 2016/03/01 18:08:48, dfalcantara wrote: > > > https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java > > (right): > > > > > https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java:597: > > mTab = createActivityTab(asyncParams); > > On 2016/03/01 17:59:06, jbudorick wrote: > > > Was adding this line intentional or the product of a bad rebase? It wasn't > > part > > > of the diff in patchset 12 but was in patchset 13 (and in the version that > > > landed). > > > > > > It seems to be causing four DocumentModeTests to fail: > > > > > > C 241.505s Main [ FAILED ] > > > org.chromium.chrome.browser.document.DocumentModeTest#testLastTabIdUpdates > > > (UNKNOWN) > > > C 241.505s Main [ FAILED ] > > > org.chromium.chrome.browser.document.DocumentModeTest#testTargetBlank > > (UNKNOWN) > > > C 241.505s Main [ FAILED ] > > > org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpen > (UNKNOWN) > > > C 241.506s Main [ FAILED ] > > > > > > org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpenWithOpenerSuppressed > > > (UNKNOWN) > > > > > > If it wasn't intentional, I can remove it in > > > https://codereview.chromium.org/1748853002/ > > > > It's definitely a screwed up rebase; you can see the tab created above. This > > patch is broken in document mode now, because the tab is created before the > URL > > is munged. > > Fixing in https://codereview.chromium.org/1748853002/, then. Thanks for fixing this! Sorry about the bug. I am planning on merging this into M50 since this fixes external Weblite intents for the previews experiment. Can https://codereview.chromium.org/1748853002 be merged to?
Message was sent while issue was closed.
On 2016/03/02 01:08:11, megjablon wrote: > On 2016/03/01 18:09:47, jbudorick wrote: > > On 2016/03/01 18:08:48, dfalcantara wrote: > > > > > > https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java > > > (right): > > > > > > > > > https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java:597: > > > mTab = createActivityTab(asyncParams); > > > On 2016/03/01 17:59:06, jbudorick wrote: > > > > Was adding this line intentional or the product of a bad rebase? It wasn't > > > part > > > > of the diff in patchset 12 but was in patchset 13 (and in the version that > > > > landed). > > > > > > > > It seems to be causing four DocumentModeTests to fail: > > > > > > > > C 241.505s Main [ FAILED ] > > > > org.chromium.chrome.browser.document.DocumentModeTest#testLastTabIdUpdates > > > > (UNKNOWN) > > > > C 241.505s Main [ FAILED ] > > > > org.chromium.chrome.browser.document.DocumentModeTest#testTargetBlank > > > (UNKNOWN) > > > > C 241.505s Main [ FAILED ] > > > > org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpen > > (UNKNOWN) > > > > C 241.506s Main [ FAILED ] > > > > > > > > > > org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpenWithOpenerSuppressed > > > > (UNKNOWN) > > > > > > > > If it wasn't intentional, I can remove it in > > > > https://codereview.chromium.org/1748853002/ > > > > > > It's definitely a screwed up rebase; you can see the tab created above. > This > > > patch is broken in document mode now, because the tab is created before the > > URL > > > is munged. > > > > Fixing in https://codereview.chromium.org/1748853002/, then. > > Thanks for fixing this! Sorry about the bug. I am planning on merging this into > M50 since this fixes external Weblite intents for the previews experiment. Can > https://codereview.chromium.org/1748853002 be merged to? if this and https://codereview.chromium.org/1746933002 are going back to M50, then yes, it can. (otherwise it still can but it'll be a nontrivial cherry-pick)
Message was sent while issue was closed.
On 2016/03/02 01:30:32, jbudorick wrote: > On 2016/03/02 01:08:11, megjablon wrote: > > On 2016/03/01 18:09:47, jbudorick wrote: > > > On 2016/03/01 18:08:48, dfalcantara wrote: > > > > > > > > > > https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... > > > > File > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java:597: > > > > mTab = createActivityTab(asyncParams); > > > > On 2016/03/01 17:59:06, jbudorick wrote: > > > > > Was adding this line intentional or the product of a bad rebase? It > wasn't > > > > part > > > > > of the diff in patchset 12 but was in patchset 13 (and in the version > that > > > > > landed). > > > > > > > > > > It seems to be causing four DocumentModeTests to fail: > > > > > > > > > > C 241.505s Main [ FAILED ] > > > > > > org.chromium.chrome.browser.document.DocumentModeTest#testLastTabIdUpdates > > > > > (UNKNOWN) > > > > > C 241.505s Main [ FAILED ] > > > > > org.chromium.chrome.browser.document.DocumentModeTest#testTargetBlank > > > > (UNKNOWN) > > > > > C 241.505s Main [ FAILED ] > > > > > org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpen > > > (UNKNOWN) > > > > > C 241.506s Main [ FAILED ] > > > > > > > > > > > > > > > org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpenWithOpenerSuppressed > > > > > (UNKNOWN) > > > > > > > > > > If it wasn't intentional, I can remove it in > > > > > https://codereview.chromium.org/1748853002/ > > > > > > > > It's definitely a screwed up rebase; you can see the tab created above. > > This > > > > patch is broken in document mode now, because the tab is created before > the > > > URL > > > > is munged. > > > > > > Fixing in https://codereview.chromium.org/1748853002/, then. > > > > Thanks for fixing this! Sorry about the bug. I am planning on merging this > into > > M50 since this fixes external Weblite intents for the previews experiment. Can > > https://codereview.chromium.org/1748853002 be merged to? > > if this and https://codereview.chromium.org/1746933002 are going back to M50, > then yes, it can. (otherwise it still can but it'll be a nontrivial cherry-pick) Sorry, I'm not sure I understand. Are you suggesting merging all 3 cls individually or merging them all into a single patch set?
Message was sent while issue was closed.
On 2016/03/02 08:44:12, megjablon wrote: > On 2016/03/02 01:30:32, jbudorick wrote: > > On 2016/03/02 01:08:11, megjablon wrote: > > > On 2016/03/01 18:09:47, jbudorick wrote: > > > > On 2016/03/01 18:08:48, dfalcantara wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... > > > > > File > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java:597: > > > > > mTab = createActivityTab(asyncParams); > > > > > On 2016/03/01 17:59:06, jbudorick wrote: > > > > > > Was adding this line intentional or the product of a bad rebase? It > > wasn't > > > > > part > > > > > > of the diff in patchset 12 but was in patchset 13 (and in the version > > that > > > > > > landed). > > > > > > > > > > > > It seems to be causing four DocumentModeTests to fail: > > > > > > > > > > > > C 241.505s Main [ FAILED ] > > > > > > > > org.chromium.chrome.browser.document.DocumentModeTest#testLastTabIdUpdates > > > > > > (UNKNOWN) > > > > > > C 241.505s Main [ FAILED ] > > > > > > org.chromium.chrome.browser.document.DocumentModeTest#testTargetBlank > > > > > (UNKNOWN) > > > > > > C 241.505s Main [ FAILED ] > > > > > > org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpen > > > > (UNKNOWN) > > > > > > C 241.506s Main [ FAILED ] > > > > > > > > > > > > > > > > > > > > > org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpenWithOpenerSuppressed > > > > > > (UNKNOWN) > > > > > > > > > > > > If it wasn't intentional, I can remove it in > > > > > > https://codereview.chromium.org/1748853002/ > > > > > > > > > > It's definitely a screwed up rebase; you can see the tab created above. > > > This > > > > > patch is broken in document mode now, because the tab is created before > > the > > > > URL > > > > > is munged. > > > > > > > > Fixing in https://codereview.chromium.org/1748853002/, then. > > > > > > Thanks for fixing this! Sorry about the bug. I am planning on merging this > > into > > > M50 since this fixes external Weblite intents for the previews experiment. > Can > > > https://codereview.chromium.org/1748853002 be merged to? > > > > if this and https://codereview.chromium.org/1746933002 are going back to M50, > > then yes, it can. (otherwise it still can but it'll be a nontrivial > cherry-pick) > > Sorry, I'm not sure I understand. Are you suggesting merging all 3 cls > individually or merging them all into a single patch set? A friendly ping :)
Message was sent while issue was closed.
On 2016/03/07 23:33:40, megjablon wrote: > On 2016/03/02 08:44:12, megjablon wrote: > > On 2016/03/02 01:30:32, jbudorick wrote: > > > On 2016/03/02 01:08:11, megjablon wrote: > > > > On 2016/03/01 18:09:47, jbudorick wrote: > > > > > On 2016/03/01 18:08:48, dfalcantara wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... > > > > > > File > > > > > > > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1688603004/diff/440001/chrome/android/java/sr... > > > > > > > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java:597: > > > > > > mTab = createActivityTab(asyncParams); > > > > > > On 2016/03/01 17:59:06, jbudorick wrote: > > > > > > > Was adding this line intentional or the product of a bad rebase? It > > > wasn't > > > > > > part > > > > > > > of the diff in patchset 12 but was in patchset 13 (and in the > version > > > that > > > > > > > landed). > > > > > > > > > > > > > > It seems to be causing four DocumentModeTests to fail: > > > > > > > > > > > > > > C 241.505s Main [ FAILED ] > > > > > > > > > > org.chromium.chrome.browser.document.DocumentModeTest#testLastTabIdUpdates > > > > > > > (UNKNOWN) > > > > > > > C 241.505s Main [ FAILED ] > > > > > > > > org.chromium.chrome.browser.document.DocumentModeTest#testTargetBlank > > > > > > (UNKNOWN) > > > > > > > C 241.505s Main [ FAILED ] > > > > > > > org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpen > > > > > (UNKNOWN) > > > > > > > C 241.506s Main [ FAILED ] > > > > > > > > > > > > > > > > > > > > > > > > > > > > org.chromium.chrome.browser.document.DocumentModeTest#testWindowOpenWithOpenerSuppressed > > > > > > > (UNKNOWN) > > > > > > > > > > > > > > If it wasn't intentional, I can remove it in > > > > > > > https://codereview.chromium.org/1748853002/ > > > > > > > > > > > > It's definitely a screwed up rebase; you can see the tab created > above. > > > > This > > > > > > patch is broken in document mode now, because the tab is created > before > > > the > > > > > URL > > > > > > is munged. > > > > > > > > > > Fixing in https://codereview.chromium.org/1748853002/, then. > > > > > > > > Thanks for fixing this! Sorry about the bug. I am planning on merging this > > > into > > > > M50 since this fixes external Weblite intents for the previews experiment. > > Can > > > > https://codereview.chromium.org/1748853002 be merged to? > > > > > > if this and https://codereview.chromium.org/1746933002 are going back to > M50, > > > then yes, it can. (otherwise it still can but it'll be a nontrivial > > cherry-pick) > > > > Sorry, I'm not sure I understand. Are you suggesting merging all 3 cls > > individually or merging them all into a single patch set? > > A friendly ping :) Individually. I'll also need to merge https://codereview.chromium.org/1761383002/ back to M50 once it lands. |
