Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1814693003:
Added support to blimp to be usable as a default browser (Closed)
|
Created:
4 years, 9 months ago by shaktisahu Modified:
4 years, 8 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded support to blimp to be usable as a default browser
Currently blimp cannot be launched from other applications i.e. blimp doesn't
show up as one of the browser options when user taps on an URL. This CL enables
blimp to be a default browser which can handle http:// and https:// URL schemes.
Intent is handled at the beginning of the application and also on any subsequent
onNewIntent call.
BUG=594660
Committed: https://crrev.com/4aadad4d568976931a377f567f25b93458a2530c
Cr-Commit-Position: refs/heads/master@{#384455}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Removed menu button functionality to a separate CL #
Total comments: 12
Patch Set 3 : Handled onNewIntent for new intents #
Messages
Total messages: 26 (12 generated)
shaktisahu@chromium.org changed reviewers: + dtrainor@chromium.org, nyquist@chromium.org
Description was changed from ========== Added menu button to toolbar and enabled blimp as default browser BUG=594660 ========== to ========== Added menu button to toolbar and enabled blimp as default browser BUG=594660 ==========
shaktisahu@chromium.org changed reviewers: + khushalsagar@chromium.org
Can you split this into 2 separate patches? One for the default browser and the other for the toolbar change. https://codereview.chromium.org/1814693003/diff/1/blimp/client/app/android/An... File blimp/client/app/android/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/1814693003/diff/1/blimp/client/app/android/An... blimp/client/app/android/AndroidManifest.xml.jinja2:30: <category android:name="android.intent.category.NOTIFICATION_PREFERENCES" /> Since we don't post any notifications right now, is this necessary? https://codereview.chromium.org/1814693003/diff/1/blimp/client/app/android/An... blimp/client/app/android/AndroidManifest.xml.jinja2:38: <data android:scheme="http" /> We are not handling any of these intents in the activity to process the URL and start a navigation. I would suggest add a subset of the filters in this patch, and add the intent handling logic for them to the BlimpRendererActivity. Take a look at ChromeLauncherActivity and IntentHandler.
On 2016/03/25 18:59:30, Khushal wrote: > Can you split this into 2 separate patches? One for the default browser and the > other for the toolbar change. > > https://codereview.chromium.org/1814693003/diff/1/blimp/client/app/android/An... > File blimp/client/app/android/AndroidManifest.xml.jinja2 (right): > > https://codereview.chromium.org/1814693003/diff/1/blimp/client/app/android/An... > blimp/client/app/android/AndroidManifest.xml.jinja2:30: <category > android:name="android.intent.category.NOTIFICATION_PREFERENCES" /> > Since we don't post any notifications right now, is this necessary? > > https://codereview.chromium.org/1814693003/diff/1/blimp/client/app/android/An... > blimp/client/app/android/AndroidManifest.xml.jinja2:38: <data > android:scheme="http" /> > We are not handling any of these intents in the activity to process the URL and > start a navigation. I would suggest add a subset of the filters in this patch, > and add the intent handling logic for them to the BlimpRendererActivity. > Take a look at ChromeLauncherActivity and IntentHandler. Moved the menu button stuff to another CL : https://codereview.chromium.org/1841653005
https://codereview.chromium.org/1814693003/diff/1/blimp/client/app/android/An... File blimp/client/app/android/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/1814693003/diff/1/blimp/client/app/android/An... blimp/client/app/android/AndroidManifest.xml.jinja2:30: <category android:name="android.intent.category.NOTIFICATION_PREFERENCES" /> On 2016/03/25 18:59:30, Khushal wrote: > Since we don't post any notifications right now, is this necessary? Done. https://codereview.chromium.org/1814693003/diff/1/blimp/client/app/android/An... blimp/client/app/android/AndroidManifest.xml.jinja2:38: <data android:scheme="http" /> On 2016/03/25 18:59:30, Khushal wrote: > We are not handling any of these intents in the activity to process the URL and > start a navigation. I would suggest add a subset of the filters in this patch, > and add the intent handling logic for them to the BlimpRendererActivity. > Take a look at ChromeLauncherActivity and IntentHandler. Done.
https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:148: String url = getUrlFromIntent(getIntent()); Is this the only place we need to handle the intent? What if the activity was already on the stack when the intent was sent? Also, please put this in a separate method so loading the url is distinct from setting up the features, which only happens after the native library is loaded. https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:157: String url = intent.getDataString(); Check if the intent is null. Android can do that. https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:161: return url; Do we need to check if the url is empty at this point?
The CL description seems a bit off. The first line should be the one-liner, then an empty line, and then a more thorough description. The first line currently sounds a bit like you did something to my mobile device... Maybe something like this as the first line: Added support for making blimp the default browser. Also, I don't see anything related to the menu button in the CL, even though the CL description mentions it. Maybe you could follow these guidelines for the rest of the CL description: - What currently happens? (blimp can't be default browser) - Why is that bad? - How does this CL make it better? Also maybe write a short paragraph on when and how we handle intents? https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:149: if (url != null) { Nit: How about flipping this to == instead of != for readability? https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:158: if (url != null) { Nit: if (url == null) return null; return url.trim();
Description was changed from ========== Added menu button to toolbar and enabled blimp as default browser BUG=594660 ========== to ========== Currently blimp cannot be launched by the user from other applications i.e. blimp doesn't show up as one of the browser options when user taps on an URL. This CL enables blimp to be a default browser. BUG=594660 ==========
Description was changed from ========== Currently blimp cannot be launched by the user from other applications i.e. blimp doesn't show up as one of the browser options when user taps on an URL. This CL enables blimp to be a default browser. BUG=594660 ========== to ========== Added support to blimp to be usable as a default browser Currently blimp cannot be launched from other applications i.e. blimp doesn't show up as one of the browser options when user taps on an URL. This CL enables blimp to be a default browser. BUG=594660 ==========
Description was changed from ========== Added support to blimp to be usable as a default browser Currently blimp cannot be launched from other applications i.e. blimp doesn't show up as one of the browser options when user taps on an URL. This CL enables blimp to be a default browser. BUG=594660 ========== to ========== Added support to blimp to be usable as a default browser Currently blimp cannot be launched from other applications i.e. blimp doesn't show up as one of the browser options when user taps on an URL. This CL enables blimp to be a default browser which can handle http:// and https:// url schemes. Intent is handled at the beginning of the application and also on any subsequent onNewIntent call. BUG=594660 ==========
https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:148: String url = getUrlFromIntent(getIntent()); On 2016/03/30 17:46:02, Khushal wrote: > Is this the only place we need to handle the intent? What if the activity was > already on the stack when the intent was sent? > > Also, please put this in a separate method so loading the url is distinct from > setting up the features, which only happens after the native library is loaded. Done. https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:148: String url = getUrlFromIntent(getIntent()); On 2016/03/30 17:46:02, Khushal wrote: > Is this the only place we need to handle the intent? What if the activity was > already on the stack when the intent was sent? > > Also, please put this in a separate method so loading the url is distinct from > setting up the features, which only happens after the native library is loaded. Good catch! Added the same code also to onNewIntent callback. https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:149: if (url != null) { On 2016/03/30 17:58:13, nyquist wrote: > Nit: How about flipping this to == instead of != for readability? Done. https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:149: if (url != null) { On 2016/03/30 17:58:13, nyquist wrote: > Nit: How about flipping this to == instead of != for readability? Done. https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:157: String url = intent.getDataString(); On 2016/03/30 17:46:02, Khushal wrote: > Check if the intent is null. Android can do that. Done. https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:158: if (url != null) { On 2016/03/30 17:58:13, nyquist wrote: > Nit: > if (url == null) return null; > return url.trim(); Done. https://codereview.chromium.org/1814693003/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:161: return url; On 2016/03/30 17:46:02, Khushal wrote: > Do we need to check if the url is empty at this point? Done.
lgtm. Thanks!
The CQ bit was checked by shaktisahu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814693003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814693003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Added support to blimp to be usable as a default browser Currently blimp cannot be launched from other applications i.e. blimp doesn't show up as one of the browser options when user taps on an URL. This CL enables blimp to be a default browser which can handle http:// and https:// url schemes. Intent is handled at the beginning of the application and also on any subsequent onNewIntent call. BUG=594660 ========== to ========== Added support to blimp to be usable as a default browser Currently blimp cannot be launched from other applications i.e. blimp doesn't show up as one of the browser options when user taps on an URL. This CL enables blimp to be a default browser which can handle http:// and https:// URL schemes. Intent is handled at the beginning of the application and also on any subsequent onNewIntent call. BUG=594660 ==========
The CQ bit was checked by nyquist@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814693003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814693003/40001
Message was sent while issue was closed.
Description was changed from ========== Added support to blimp to be usable as a default browser Currently blimp cannot be launched from other applications i.e. blimp doesn't show up as one of the browser options when user taps on an URL. This CL enables blimp to be a default browser which can handle http:// and https:// URL schemes. Intent is handled at the beginning of the application and also on any subsequent onNewIntent call. BUG=594660 ========== to ========== Added support to blimp to be usable as a default browser Currently blimp cannot be launched from other applications i.e. blimp doesn't show up as one of the browser options when user taps on an URL. This CL enables blimp to be a default browser which can handle http:// and https:// URL schemes. Intent is handled at the beginning of the application and also on any subsequent onNewIntent call. BUG=594660 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Added support to blimp to be usable as a default browser Currently blimp cannot be launched from other applications i.e. blimp doesn't show up as one of the browser options when user taps on an URL. This CL enables blimp to be a default browser which can handle http:// and https:// URL schemes. Intent is handled at the beginning of the application and also on any subsequent onNewIntent call. BUG=594660 ========== to ========== Added support to blimp to be usable as a default browser Currently blimp cannot be launched from other applications i.e. blimp doesn't show up as one of the browser options when user taps on an URL. This CL enables blimp to be a default browser which can handle http:// and https:// URL schemes. Intent is handled at the beginning of the application and also on any subsequent onNewIntent call. BUG=594660 Committed: https://crrev.com/4aadad4d568976931a377f567f25b93458a2530c Cr-Commit-Position: refs/heads/master@{#384455} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4aadad4d568976931a377f567f25b93458a2530c Cr-Commit-Position: refs/heads/master@{#384455} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
