Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1753763002:
Tests for notification preferences intents fired by the Android system. (Closed)
|
Created:
4 years, 9 months ago by Michael van Ouwerkerk Modified:
4 years, 9 months ago CC:
chromium-reviews, Peter Wen, Peter Beverloo, mlamouri+watch-notifications_chromium.org, newt (away) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTests for notification preferences intents fired by the Android system.
BUG=461885, 589416
Committed: https://crrev.com/aa94a83c4e3aa516cadd2c4517232245a9d9d662
Cr-Commit-Position: refs/heads/master@{#380606}
Patch Set 1 #Patch Set 2 : Cleanups. #Patch Set 3 : Delete TODO now that it's done. #
Total comments: 10
Patch Set 4 : Address peter's comments. #
Total comments: 4
Patch Set 5 : Cleanups. #Patch Set 6 : Rebase. #
Total comments: 4
Patch Set 7 : Address tedchoc's comments. #
Total comments: 2
Patch Set 8 : Fix setClassName call. #
Messages
Total messages: 52 (23 generated)
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753763002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753763002/1
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753763002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753763002/20001
mvanouwerkerk@chromium.org changed reviewers: + peter@chromium.org, tedchoc@chromium.org
Peter and Ted, could you take a look please?
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753763002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753763002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yay tests! Perhaps reference the bug covering the recent crasher as well? https://codereview.chromium.org/1753763002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java (right): https://codereview.chromium.org/1753763002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:227: // instrumentation tests. Please adopt Glenn's approach: https://codereview.chromium.org/1745623002/ That'd allow you to revert most of the changes to this method. https://codereview.chromium.org/1753763002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java (right): https://codereview.chromium.org/1753763002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java:31: * the code under test. under the test -> the code exercised by this test? https://codereview.chromium.org/1753763002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java:63: * screen. Perhaps clarify that the difference is made by the presence (or absence) of the tag? https://codereview.chromium.org/1753763002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java:76: .setFlags(Intent.FLAG_ACTIVITY_NEW_TASK); We probably want this to be identical to the intent Android fires? (1) Could we use setClassName(String packageName, String className) instead? (2) Do we need FLAG_ACTIVITY_NEW_TASK? (dito in testLaunchNotificationPreferencesForWebsite) https://codereview.chromium.org/1753763002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java:131: activity, context.getResources().getString(R.string.prefs_site_settings)); Is there benefit in checking the title as well as the SingleWebsitePreferences fragment?
Description was changed from ========== Tests for notification preferences intents fired by the Android system. BUG=461885 ========== to ========== Tests for notification preferences intents fired by the Android system. BUG=461885 ==========
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753763002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753763002/60001
Description was changed from ========== Tests for notification preferences intents fired by the Android system. BUG=461885 ========== to ========== Tests for notification preferences intents fired by the Android system. BUG=461885,589416 ==========
Thanks for the quick review Peter. https://codereview.chromium.org/1753763002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java (right): https://codereview.chromium.org/1753763002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:227: // instrumentation tests. On 2016/03/01 18:50:59, Peter Beverloo wrote: > Please adopt Glenn's approach: > https://codereview.chromium.org/1745623002/ > > That'd allow you to revert most of the changes to this method. How is that patch related to the strict mode violation for reading from disk? It looks like it addresses an SELinux violation for writing to disk. https://codereview.chromium.org/1753763002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java (right): https://codereview.chromium.org/1753763002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java:31: * the code under test. On 2016/03/01 18:50:59, Peter Beverloo wrote: > under the test -> the code exercised by this test? Done. https://codereview.chromium.org/1753763002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java:63: * screen. On 2016/03/01 18:50:59, Peter Beverloo wrote: > Perhaps clarify that the difference is made by the presence (or absence) of the > tag? Done. https://codereview.chromium.org/1753763002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java:76: .setFlags(Intent.FLAG_ACTIVITY_NEW_TASK); On 2016/03/01 18:50:59, Peter Beverloo wrote: > We probably want this to be identical to the intent Android fires? > > (1) Could we use setClassName(String packageName, String className) instead? > (2) Do we need FLAG_ACTIVITY_NEW_TASK? > > (dito in testLaunchNotificationPreferencesForWebsite) (1) Sure, done. I don't think it makes much of a difference. Why did you ask for this? (2) ContextImpl.startActivity throws an AndroidRuntimeException without the flag: "Calling startActivity() from outside of an Activity context requires the FLAG_ACTIVITY_NEW_TASK flag. Is this really what you want?". So I think we need it ;-) https://codereview.chromium.org/1753763002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java:131: activity, context.getResources().getString(R.string.prefs_site_settings)); On 2016/03/01 18:50:59, Peter Beverloo wrote: > Is there benefit in checking the title as well as the SingleWebsitePreferences > fragment? Not much, the point is probably made well enough the moment we've got the expected fragment. I've removed the title checks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tedchoc@chromium.org changed reviewers: + yfriedman@chromium.org
+yfriedman for strictmode thoughts https://codereview.chromium.org/1753763002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java (right): https://codereview.chromium.org/1753763002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:234: ChromeBrowserInitializer.getInstance(context).handleSynchronousStartup(); I'm a bit confused here... LibraryLoader...ensureInitialized is already called from: handleSynchronousStartup -> handlePostNativeStartup -> startChromeBrowserProcessesSync Why do you need to add the lines above? Is it actually needed in practice or is it only needed to avoid strict mode violations? If the latter, than I think we should add the exception in the ChromeBrowserInitializer as every caller would need to add this workaround. I don't know why it was needed in the CustomTabsConnection either: https://bugs.chromium.org/p/chromium/issues/detail?id=574532
wnwen@chromium.org changed reviewers: + wnwen@chromium.org
https://codereview.chromium.org/1753763002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java (right): https://codereview.chromium.org/1753763002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:234: ChromeBrowserInitializer.getInstance(context).handleSynchronousStartup(); On 2016/03/08 21:21:05, Ted C wrote: > I'm a bit confused here... > > LibraryLoader...ensureInitialized is already called from: > > handleSynchronousStartup -> handlePostNativeStartup -> > startChromeBrowserProcessesSync > > Why do you need to add the lines above? Is it actually needed in practice or is > it only needed to avoid strict mode violations? If the latter, than I think we > should add the exception in the ChromeBrowserInitializer as every caller would > need to add this workaround. > > I don't know why it was needed in the CustomTabsConnection either: > https://bugs.chromium.org/p/chromium/issues/detail?id=574532 The extra call are not needed. Only the StrictMode.ThreadPolicy whitelisting code is necessary to wrap around the handleSynchronousStartup call. The extra code is also not necessary in CustomTabsConnection. It makes sense to add the whitelisting code in handleSynchronousStartup, as that is sure to be on the UI thread, and if the caller is explicitly calling for a synchronous startup, it's likely they already expect a frozen UI, although it's good to make sure that each synchronous startup call is absolutely necessary.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753763002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753763002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753763002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753763002/100001
Thanks, please take another look. https://codereview.chromium.org/1753763002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java (right): https://codereview.chromium.org/1753763002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:234: ChromeBrowserInitializer.getInstance(context).handleSynchronousStartup(); On 2016/03/08 21:21:05, Ted C wrote: > I'm a bit confused here... > > LibraryLoader...ensureInitialized is already called from: > > handleSynchronousStartup -> handlePostNativeStartup -> > startChromeBrowserProcessesSync > > Why do you need to add the lines above? Is it actually needed in practice or is > it only needed to avoid strict mode violations? If the latter, than I think we > should add the exception in the ChromeBrowserInitializer as every caller would > need to add this workaround. > > I don't know why it was needed in the CustomTabsConnection either: > https://bugs.chromium.org/p/chromium/issues/detail?id=574532 Done. https://codereview.chromium.org/1753763002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:234: ChromeBrowserInitializer.getInstance(context).handleSynchronousStartup(); On 2016/03/08 22:03:49, Peter Wen wrote: > On 2016/03/08 21:21:05, Ted C wrote: > > I'm a bit confused here... > > > > LibraryLoader...ensureInitialized is already called from: > > > > handleSynchronousStartup -> handlePostNativeStartup -> > > startChromeBrowserProcessesSync > > > > Why do you need to add the lines above? Is it actually needed in practice or > is > > it only needed to avoid strict mode violations? If the latter, than I think > we > > should add the exception in the ChromeBrowserInitializer as every caller would > > need to add this workaround. > > > > I don't know why it was needed in the CustomTabsConnection either: > > https://bugs.chromium.org/p/chromium/issues/detail?id=574532 > > The extra call are not needed. > > Only the StrictMode.ThreadPolicy whitelisting code is necessary to wrap around > the handleSynchronousStartup call. The extra code is also not necessary in > CustomTabsConnection. > > It makes sense to add the whitelisting code in handleSynchronousStartup, as that > is sure to be on the UI thread, and if the caller is explicitly calling for a > synchronous startup, it's likely they already expect a frozen UI, although it's > good to make sure that each synchronous startup call is absolutely necessary. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/1753763002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java (right): https://codereview.chromium.org/1753763002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:121: StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads(); I think these should be in startChromeBrowserProcessesSync instead of here. https://codereview.chromium.org/1753763002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java (right): https://codereview.chromium.org/1753763002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java:62: .setClassName("org.chromium.chrome", "com.google.android.apps.chrome.Main") Instead of hardcoding these, I would try: org.chromium.chrome = getTargetContext().getApplicationContext().getPackageName() com.google.android.apps.chrome.Main = ChromeLauncherActivity.class.getName() Main is just an alias to that, so I think that should get what you want. And org.chromium.chrome I think wouldn't allow us to run tests again non-local builds.
Thanks Ted. Please take another look. https://codereview.chromium.org/1753763002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java (right): https://codereview.chromium.org/1753763002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:121: StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads(); On 2016/03/09 17:49:32, Ted C wrote: > I think these should be in startChromeBrowserProcessesSync instead of here. Done. https://codereview.chromium.org/1753763002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java (right): https://codereview.chromium.org/1753763002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java:62: .setClassName("org.chromium.chrome", "com.google.android.apps.chrome.Main") On 2016/03/09 17:49:32, Ted C wrote: > Instead of hardcoding these, I would try: > > org.chromium.chrome = > getTargetContext().getApplicationContext().getPackageName() > > com.google.android.apps.chrome.Main = ChromeLauncherActivity.class.getName() > > Main is just an alias to that, so I think that should get what you want. And > org.chromium.chrome I think wouldn't allow us to run tests again non-local > builds. Heh. I had that. Peter asked for this change, and I didn't want to make a fuss of it: https://codereview.chromium.org/1753763002/diff/40001/chrome/android/javatest... Back as it was.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753763002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753763002/120001
lgtm w/ one last change https://codereview.chromium.org/1753763002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java (right): https://codereview.chromium.org/1753763002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java:95: .setClassName("org.chromium.chrome", "com.google.android.apps.chrome.Main") same comment as above.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm for the strictmode q - that whitelisting seems reasonable
https://codereview.chromium.org/1753763002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java (right): https://codereview.chromium.org/1753763002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerIntentTest.java:95: .setClassName("org.chromium.chrome", "com.google.android.apps.chrome.Main") On 2016/03/09 18:52:06, Ted C wrote: > same comment as above. Indeed. Thanks Ted.
Pinging Mr Beverloo as notifications owner.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753763002/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753763002/130001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mvanouwerkerk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/1753763002/#ps130001 (title: "Fix setClassName call.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753763002/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753763002/130001
Message was sent while issue was closed.
Description was changed from ========== Tests for notification preferences intents fired by the Android system. BUG=461885,589416 ========== to ========== Tests for notification preferences intents fired by the Android system. BUG=461885,589416 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== Tests for notification preferences intents fired by the Android system. BUG=461885,589416 ========== to ========== Tests for notification preferences intents fired by the Android system. BUG=461885,589416 Committed: https://crrev.com/aa94a83c4e3aa516cadd2c4517232245a9d9d662 Cr-Commit-Position: refs/heads/master@{#380606} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/aa94a83c4e3aa516cadd2c4517232245a9d9d662 Cr-Commit-Position: refs/heads/master@{#380606} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
