Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2755003003:
Add Interstitial for user consent (Closed)
DescriptionAdd Interstitial for user consent
This change adds an interstitial to notify to the user that this URL
will be sent to Googles servers anonymously. The
PhysicalWebShareActivity start a dialogue because of the application
state listener that controls its componenet. For this reason, we send
the chromeactivity an ID which will launch an activity to show the
interstitial. This is done in the same way that the printshareactivity
does it.
BUG=685856
Review-Url: https://codereview.chromium.org/2755003003
Cr-Commit-Position: refs/heads/master@{#458522}
Committed: https://chromium.googlesource.com/chromium/src/+/d7323d358bb4c28778915daef88f72407e078bd0
Patch Set 1 #
Total comments: 20
Patch Set 2 : Personal Nits 1 #Patch Set 3 : Conleys Nits 1 #Patch Set 4 : Removing Comment Change from Service #
Total comments: 18
Patch Set 5 : Matts Nits 1 #
Total comments: 10
Patch Set 6 : JavaDoc Problems #Patch Set 7 : Teds Nits 1 #
Total comments: 8
Patch Set 8 : Teds Nits 2 #Messages
Total messages: 23 (7 generated)
Description was changed from ========== Add Interstitial for user consent This change adds an interstitial to notify to the user that this URL will be sent to Googles servers anonymously. The PhysicalWebShareActivity start a dialogue because of the application state listener that controls its componenet. For this reason, we send the chromeactivity an ID which will launch an activity to show the interstitial. This is done in the same way that the printshareactivity does it. BUG=685856 ========== to ========== Add Interstitial for user consent This change adds an interstitial to notify to the user that this URL will be sent to Googles servers anonymously. The PhysicalWebShareActivity start a dialogue because of the application state listener that controls its componenet. For this reason, we send the chromeactivity an ID which will launch an activity to show the interstitial. This is done in the same way that the printshareactivity does it. BUG=685856 ==========
iankc@google.com changed reviewers: + cco3@chromium.org, mattreynolds@chromium.org
https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:660: android:enabled="true" This line isn't needed. https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:154: private native byte[] nativeEncodeUrl(String url); This doesn't belong in this change, right? https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:22: final String url = triggeringActivity.getActivityTab().getUrl(); Why is this final? https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:25: boolean optedIn = I'd move reading and writing to this value to PhysicalWeb.java https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:54: && !PhysicalWeb.sharingIsEnabled(); ? https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java (right): https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:17: * A simple activity that allows Chrome to start the physical web sharing service. Update. https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:38: ContextUtils.getAppSharedPreferences() I'd move reading and writing to this value to PhysicalWeb.java https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:54: .setIcon(android.R.drawable.ic_dialog_alert) It seems a little harsh, is this what we want? Are there other options? https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:55: .setCancelable(false) Wouldn't it make more sense to get rid of the negative button and just setOnCancelListener? https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:2620: This service needs to anonymously send this website address to Google servers for verification full stop.
Good thing you looked over this one. https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:660: android:enabled="true" On 2017/03/16 22:54:32, cco3 wrote: > This line isn't needed. Done. https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:154: private native byte[] nativeEncodeUrl(String url); On 2017/03/16 22:54:32, cco3 wrote: > This doesn't belong in this change, right? Done. https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:22: final String url = triggeringActivity.getActivityTab().getUrl(); On 2017/03/16 22:54:32, cco3 wrote: > Why is this final? At one point I was using it in an attempt to open a dialog from this activity. It didn't workout and I forgot to revert it. Done. https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:25: boolean optedIn = On 2017/03/16 22:54:32, cco3 wrote: > I'd move reading and writing to this value to PhysicalWeb.java Done. https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:54: && !PhysicalWeb.sharingIsEnabled(); On 2017/03/16 22:54:32, cco3 wrote: > ? That was for testing... https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java (right): https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:17: * A simple activity that allows Chrome to start the physical web sharing service. On 2017/03/16 22:54:33, cco3 wrote: > Update. Done. https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:38: ContextUtils.getAppSharedPreferences() On 2017/03/16 22:54:33, cco3 wrote: > I'd move reading and writing to this value to PhysicalWeb.java Done. https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:54: .setIcon(android.R.drawable.ic_dialog_alert) On 2017/03/16 22:54:33, cco3 wrote: > It seems a little harsh, is this what we want? Are there other options? Yep. I could use the ic_dialog_info. http://docs.since2006.com/android/2.1-drawables.php https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:55: .setCancelable(false) On 2017/03/16 22:54:32, cco3 wrote: > Wouldn't it make more sense to get rid of the negative button and just > setOnCancelListener? Acknowledged. https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2755003003/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:2620: This service needs to anonymously send this website address to Google servers for verification On 2017/03/16 22:54:33, cco3 wrote: > full stop. Done.
https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/res... File chrome/android/java/res/values/ids.xml (right): https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/res... chrome/android/java/res/values/ids.xml:61: <item type="id" name="physical_web_sharing_id" /> Let's move this to a "Menu items IDs" section (would Menu items IDs in Settings be correct?) https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1843: intent.putExtra(PhysicalWebShareEntryActivity.SHARING_ENTRY_URL, currentTab.getUrl()); tab.getUrl() can sometimes return an empty string. Will that cause any problems for the share activity? https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:30: public static final String PHYSICAL_WEB_SHARING_PREFERENCE = "physical_web_sharing"; I think this can be private now. https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:71: return sharedPrefs.getBoolean(PhysicalWeb.PHYSICAL_WEB_SHARING_PREFERENCE, false); You can remove "PhysicalWeb." from the constant name now that this logic has moved to the PhysicalWeb class. (+1 more below) https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:24: triggeringActivity.onMenuOrKeyboardAction(R.id.physical_web_sharing_id, true); Please add a comment explaining that this shows the interstitial. https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java (right): https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:55: private void startBroadcastService(String url) { This is duplicated in PhysicalWebShareActivity. It would be nice to find a way to share it, or at least the part that creates the Intent. Maybe add it as a static method in PhysicalWebBroadcastService? https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2619: <message name="IDS_PHYSICAL_WEB_SHARE_ENTRY_MESSAGE" desc="."> Description https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2622: <message name="IDS_PHYSICAL_WEB_SHARE_ENTRY_CONTINUE" desc="Label on button on the alert dialogue for the user to consent to use our service."> IDS_CONTINUE_BUTTON already exists, let's use that https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2625: <message name="IDS_PHYSICAL_WEB_SHARE_ENTRY_CANCEL" desc="Label on button to alert dialogue for the user to not use our service."> IDS_CANCEL
https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/res... File chrome/android/java/res/values/ids.xml (right): https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/res... chrome/android/java/res/values/ids.xml:61: <item type="id" name="physical_web_sharing_id" /> On 2017/03/17 00:52:39, mattreynolds wrote: > Let's move this to a "Menu items IDs" section (would Menu items IDs in Settings > be correct?) Makes the most sense. https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1843: intent.putExtra(PhysicalWebShareEntryActivity.SHARING_ENTRY_URL, currentTab.getUrl()); On 2017/03/17 00:52:39, mattreynolds wrote: > tab.getUrl() can sometimes return an empty string. Will that cause any problems > for the share activity? No it would be able to handle this case. It would just give a toast that says "invalid url" it if were empty, or it would log and error if it were null. I'm not sure what cases this would happen in so I'm not sure what the users expected behavior would be. https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:30: public static final String PHYSICAL_WEB_SHARING_PREFERENCE = "physical_web_sharing"; On 2017/03/17 00:52:39, mattreynolds wrote: > I think this can be private now. Done. https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:71: return sharedPrefs.getBoolean(PhysicalWeb.PHYSICAL_WEB_SHARING_PREFERENCE, false); On 2017/03/17 00:52:39, mattreynolds wrote: > You can remove "PhysicalWeb." from the constant name now that this logic has > moved to the PhysicalWeb class. (+1 more below) Done. https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:24: triggeringActivity.onMenuOrKeyboardAction(R.id.physical_web_sharing_id, true); On 2017/03/17 00:52:39, mattreynolds wrote: > Please add a comment explaining that this shows the interstitial. Done. https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java (right): https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:55: private void startBroadcastService(String url) { On 2017/03/17 00:52:39, mattreynolds wrote: > This is duplicated in PhysicalWebShareActivity. It would be nice to find a way > to share it, or at least the part that creates the Intent. Maybe add it as a > static method in PhysicalWebBroadcastService? Done. https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2619: <message name="IDS_PHYSICAL_WEB_SHARE_ENTRY_MESSAGE" desc="."> On 2017/03/17 00:52:40, mattreynolds wrote: > Description Done. https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2622: <message name="IDS_PHYSICAL_WEB_SHARE_ENTRY_CONTINUE" desc="Label on button on the alert dialogue for the user to consent to use our service."> On 2017/03/17 00:52:40, mattreynolds wrote: > IDS_CONTINUE_BUTTON already exists, let's use that Done. https://codereview.chromium.org/2755003003/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2625: <message name="IDS_PHYSICAL_WEB_SHARE_ENTRY_CANCEL" desc="Label on button to alert dialogue for the user to not use our service."> On 2017/03/17 00:52:39, mattreynolds wrote: > IDS_CANCEL Done.
lgtm
iankc@google.com changed reviewers: + tedchoc@chromium.org
lgtm
Hey Ted, Here is a change to add an interstitial dialogue to the Physical Web Sharing Feature to allow the user to give consent to send the URL to Physical Web Servers that Google owns. Because of the way that the PhysicalWebSharingActivity is set up with a state listener, we had to redirect the flow to go through the ChromeActivity to activate the PhysicalWebShareEntryActivity. This is similar to how the PrintShareActivity does it as well.
https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1842: Intent intent = new Intent(this, PhysicalWebShareEntryActivity.class); I saw the commit message, but I'm not sure I follow why this can't be in PhysicalWebShareActivity? You have access to triggeringActivity, so you should be able to do triggeringActivity.getActivityTab().getUrl() and have access to the same. https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:73: public static void startBroadcastService(String url) { non-private apis should have javadoc as a general rule https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java (right): https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:31: new AlertDialog.Builder(this) if you're only showing a dialog, why not put this in the share activity? You'd probably need to change the style to @android:style/Theme.Translucent.NoTitleBar since NoDisplay will crash if you don't finish in onCreate. But then you could conditionally do this in there without the extra activity hop.
Hey Ted, I agree that having the Share Activity create the interstitial would be better. However, there are problems with that because we made the PhysicalWebShareActivity tied to the application state, which changes when we try to start and alert dialog. For this reason we redirect flow to the ChromeActivity to launch another activity. https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1842: Intent intent = new Intent(this, PhysicalWebShareEntryActivity.class); On 2017/03/20 21:05:59, Ted C wrote: > I saw the commit message, but I'm not sure I follow why this can't be in > PhysicalWebShareActivity? > > You have access to triggeringActivity, so you should be able to do > triggeringActivity.getActivityTab().getUrl() and have access to the same. The problem isn't gaining access to the URL. The problem is that the existence of the PhysicalWebShareActivity is dependent on the activity state, as seen in the OptionalShareTargetsManager.java. So when the PhysicalWebShareActivity goes into a different state, caused by launching an alert dialog, then the application will crash. More simply: alert dialog tries to go up, activity state changes, PhysicalWebShareActivity disabled and destroyed, application crashes because alert dialog has lost its launching activity. So the solution is to tell the ChromeActivity to launch a new activity (PhysicalWebShareENTRYActivity) that has a transparent layout to display the alert dialog. https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:73: public static void startBroadcastService(String url) { On 2017/03/20 21:05:59, Ted C wrote: > non-private apis should have javadoc as a general rule Done. https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java (right): https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:31: new AlertDialog.Builder(this) On 2017/03/20 21:05:59, Ted C wrote: > if you're only showing a dialog, why not put this in the share activity? > > You'd probably need to change the style to > @android:style/Theme.Translucent.NoTitleBar since NoDisplay will crash if you > don't finish in onCreate. But then you could conditionally do this in there > without the extra activity hop. I think this comment changes with knowledge of the State Change problem.
https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1842: Intent intent = new Intent(this, PhysicalWebShareEntryActivity.class); On 2017/03/20 21:32:22, iankc wrote: > On 2017/03/20 21:05:59, Ted C wrote: > > I saw the commit message, but I'm not sure I follow why this can't be in > > PhysicalWebShareActivity? > > > > You have access to triggeringActivity, so you should be able to do > > triggeringActivity.getActivityTab().getUrl() and have access to the same. > > The problem isn't gaining access to the URL. > The problem is that the existence of the PhysicalWebShareActivity is dependent > on the activity state, as seen in the OptionalShareTargetsManager.java. So when > the PhysicalWebShareActivity goes into a different state, caused by launching an > alert dialog, then the application will crash. More simply: alert dialog tries > to go up, activity state changes, PhysicalWebShareActivity disabled and > destroyed, application crashes because alert dialog has lost its launching > activity. > So the solution is to tell the ChromeActivity to launch a new activity > (PhysicalWebShareENTRYActivity) that has a transparent layout to display the > alert dialog. I guess I'm surprised as the manager only unregisters when they get something other than PAUSE. You shouldn't be getting STOP for the underlying activity when showing a dialog as it is still visible. If you add logs in onActivityStateChange in the manager, what are you seeing as the sequence for your activity types? https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:23: triggeringActivity.onMenuOrKeyboardAction(R.id.physical_web_sharing_id, true); what happens here if you were to do triggeringActivity.startActivity(your intent) here? Does Android freak out? If you add this, does it make it any happier? https://developer.android.com/reference/android/content/Intent.html#FLAG_ACTI...
Here is the summary of the log https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1842: Intent intent = new Intent(this, PhysicalWebShareEntryActivity.class); On 2017/03/21 00:01:22, Ted C wrote: > On 2017/03/20 21:32:22, iankc wrote: > > On 2017/03/20 21:05:59, Ted C wrote: > > > I saw the commit message, but I'm not sure I follow why this can't be in > > > PhysicalWebShareActivity? > > > > > > You have access to triggeringActivity, so you should be able to do > > > triggeringActivity.getActivityTab().getUrl() and have access to the same. > > > > The problem isn't gaining access to the URL. > > The problem is that the existence of the PhysicalWebShareActivity is dependent > > on the activity state, as seen in the OptionalShareTargetsManager.java. So > when > > the PhysicalWebShareActivity goes into a different state, caused by launching > an > > alert dialog, then the application will crash. More simply: alert dialog tries > > to go up, activity state changes, PhysicalWebShareActivity disabled and > > destroyed, application crashes because alert dialog has lost its launching > > activity. > > So the solution is to tell the ChromeActivity to launch a new activity > > (PhysicalWebShareENTRYActivity) that has a transparent layout to display the > > alert dialog. > > I guess I'm surprised as the manager only unregisters when they get something > other than PAUSE. You shouldn't be getting STOP for the underlying activity > when showing a dialog as it is still visible. > > If you add logs in onActivityStateChange in the manager, what are you seeing > as the sequence for your activity types? When 0. Click Share on menu. 1. Log: Components Enabled. 2. Click PhysicalWebLogo on Sharesheet. 3. Log: PhysicalWebShareActivity Started. 3.5 Here is where it starts to fire the dialog. 4. Log: Disabling components. 5. WindowManager Error: android.view.WindowLeaked: Activity org.chromium.chrome.browser.physicalweb.PhysicalWebShareActivity has leaked window com.android.internal.policy.PhoneWindow$DecorView{2ed1c93 V.E...... R......D 0,0-1280,829} that was originally added here https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2755003003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:23: triggeringActivity.onMenuOrKeyboardAction(R.id.physical_web_sharing_id, true); On 2017/03/21 00:01:22, Ted C wrote: > what happens here if you were to do triggeringActivity.startActivity(your > intent) here? Does Android freak out? > > If you add this, does it make it any happier? > https://developer.android.com/reference/android/content/Intent.html#FLAG_ACTI... This works without the Flag.
lgtm with last few comments https://codereview.chromium.org/2755003003/diff/120001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2755003003/diff/120001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:662: android:theme="@style/AlertDialogTheme" If you use @style/Theme.AppCompat.Dialog, do you need to create the alert dialog at all internally or does it display as a dialog automatically? This style applies to alert dialogs themselves and not to an activity. https://codereview.chromium.org/2755003003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2755003003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:84: * Static so it can be called from multiple places in Chrome and reduce code This comment seems to describe all static functions, so I would drop this. https://codereview.chromium.org/2755003003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java (right): https://codereview.chromium.org/2755003003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:29: final String url = extras.getString(SHARING_ENTRY_URL); if the url is null, you should also probably finish. https://codereview.chromium.org/2755003003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:31: new AlertDialog.Builder(this) ah with the activity style that I mentioned, I think you'd not get the buttons that you want, so I don't think that recommendation was good. This is probably what you need to do. You'll want to use a different activity theme though. I think you want @android:style/Theme.Translucent.NoTitleBar Also, here is where you should use R.style.AlertDialogTheme, it should be the second param to the builder constructor.
The CQ bit was checked by iankc@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from cco3@chromium.org, mattreynolds@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2755003003/#ps140001 (title: "Teds Nits 2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the help Ted! https://codereview.chromium.org/2755003003/diff/120001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2755003003/diff/120001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:662: android:theme="@style/AlertDialogTheme" On 2017/03/21 16:05:12, Ted C wrote: > If you use @style/Theme.AppCompat.Dialog, do you need to create the alert dialog > at all internally or does it display as a dialog automatically? > > This style applies to alert dialogs themselves and not to an activity. Done. https://codereview.chromium.org/2755003003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2755003003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:84: * Static so it can be called from multiple places in Chrome and reduce code On 2017/03/21 16:05:12, Ted C wrote: > This comment seems to describe all static functions, so I would drop this. Done. https://codereview.chromium.org/2755003003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java (right): https://codereview.chromium.org/2755003003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:29: final String url = extras.getString(SHARING_ENTRY_URL); On 2017/03/21 16:05:13, Ted C wrote: > if the url is null, you should also probably finish. Done. https://codereview.chromium.org/2755003003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareEntryActivity.java:31: new AlertDialog.Builder(this) On 2017/03/21 16:05:12, Ted C wrote: > ah with the activity style that I mentioned, I think you'd not get the buttons > that you want, so I don't think that recommendation was good. This is probably > what you need to do. > > You'll want to use a different activity theme though. I think you want > @android:style/Theme.Translucent.NoTitleBar > > Also, here is where you should use R.style.AlertDialogTheme, it should be the > second param to the builder constructor. Done.
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490121742772420,
"parent_rev": "1c0802c3c4c03b43efea23b82d7e624f1d8c7b57", "commit_rev":
"d7323d358bb4c28778915daef88f72407e078bd0"}
Message was sent while issue was closed.
Description was changed from ========== Add Interstitial for user consent This change adds an interstitial to notify to the user that this URL will be sent to Googles servers anonymously. The PhysicalWebShareActivity start a dialogue because of the application state listener that controls its componenet. For this reason, we send the chromeactivity an ID which will launch an activity to show the interstitial. This is done in the same way that the printshareactivity does it. BUG=685856 ========== to ========== Add Interstitial for user consent This change adds an interstitial to notify to the user that this URL will be sent to Googles servers anonymously. The PhysicalWebShareActivity start a dialogue because of the application state listener that controls its componenet. For this reason, we send the chromeactivity an ID which will launch an activity to show the interstitial. This is done in the same way that the printshareactivity does it. BUG=685856 Review-Url: https://codereview.chromium.org/2755003003 Cr-Commit-Position: refs/heads/master@{#458522} Committed: https://chromium.googlesource.com/chromium/src/+/d7323d358bb4c28778915daef88f... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/d7323d358bb4c28778915daef88f... |
