New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 767238 link

Starred by 1 user
Status: Assigned
Owner:
huayinz@chromium.org
Cc:
twelling...@chromium.org
tedc...@chromium.org
█ bauerb@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Fix local failure on HistoryActivityTest#testSupervisedUser

Project Member Reported by huayinz@chromium.org, Sep 20 2017 
The following exception happens when running HistoryActivityTest locally:

09-20 09:51:12.746 20575-20611/org.chromium.chrome I/TestRunner: ----- begin exception -----
09-20 09:51:12.746 20575-20611/org.chromium.chrome I/TestRunner: junit.framework.AssertionFailedError: Expected: <null> but was: Open in incognito tab
                                                                     at junit.framework.Assert.fail(Assert.java:50)
                                                                     at junit.framework.Assert.assertTrue(Assert.java:20)
                                                                     at junit.framework.Assert.assertNull(Assert.java:237)
                                                                     at junit.framework.Assert.assertNull(Assert.java:230)
                                                                     at org.chromium.chrome.browser.history.HistoryActivityTest.testSupervisedUser(HistoryActivityTest.java:375)
                                                                     at java.lang.reflect.Method.invoke(Native Method)
                                                                     at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:220)
                                                                     at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:205)
                                                                     at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
                                                                     at junit.framework.TestCase.runBare(TestCase.java:134)
                                                                     at junit.framework.TestResult$1.protect(TestResult.java:115)
                                                                     at junit.framework.TestResult.runProtected(TestResult.java:133)
                                                                     at junit.framework.TestResult.run(TestResult.java:118)
                                                                     at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:110)
                                                                     at junit.framework.TestCase.run(TestCase.java:124)
                                                                     at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:198)
                                                                     at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:183)
                                                                     at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:560)
                                                                     at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1932)
09-20 09:51:12.746 20575-20611/org.chromium.chrome I/TestRunner: ----- end exception -----

Attached logs that show the signed-in account becomes a non-child account after SignInManager#signIn()
logcat
8.1 KB View Download

Comment 1 by huayinz@chromium.org, Sep 21 2017

Cc: bauerb@chromium.org tedc...@chromium.org
After discussing with bauerb@, instead of fixing the test, we should make HistoryManager listen to PrefService change notification for setting delete button and incognito mode because child account is set asynchronously after sign in (and enterprise policy is independent from sign in that can change at any point at runtime).

Comment 2 by twelling...@chromium.org, Oct 3 2017 

There is a native pref_observer.h that we may be able to hook into. Or, if kAllowDeletingBrowserHistory is saved to SharedPreferences, there is an Android observer interface for SharedPreferences that we can utilize.

Comment 3 by bauerb@chromium.org, Oct 4 2017 

PrefObserver is an implementation detail of the PrefService and can't be used to observe the PrefService itself -- what you want is PrefChangeRegistrar. kAllowDeletingBrowserHistory is stored in PrefService and not in SharedPreferences, as PrefService with its enterprise policy etc. parts is what might cause the value to change at runtime.

The biggest issue is that PrefChangeRegistrar (and all other PrefService methods for that matter) use pref keys as strings, which is different from the current Java API we use where every. single. preference. has its own method. I think it's good overall to move away from that (which would for example also make it a lot easier to check for an arbitrary preference whether it can be changed by the user, whether we should show the enterprise icon next to it, etc.), but it will require us to have the pref keys available in Java in some form (see  issue 647266 ). It's also a fairly big change in terms of API surface, so maybe we can start with exposing a Java version of PrefChangeRegistrar and later moving PrefServiceBridge over to be basically a more straightforward bridge to PrefService.

Comment 4 by huayinz@chromium.org, Oct 9 2017 

Hm... I feel like we can also simply add a PrefChangeRegistrar to BrowsingHistoryBridge without exposing a java version PrefChangeRegistrar. But of course this is not at all scalable... I'm also curious if we expose a Java version of PrefChangeRegistrar, how should we handle reset/destroy the native PrefChangeRegistrar, something like overriding finalize()?
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 20 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d4234912f585297e603162729de4dea0482867f0

commit d4234912f585297e603162729de4dea0482867f0
Author: Becky Zhou <huayinz@chromium.org>
Date: Fri Oct 20 19:46:26 2017

[Android] Expose PrefChangeRegistrar to Java

+ Add PrefChangeRegistrar to History
+ Fix HistoryActivityTest#testSupervisedUser to have supervised user id
  setup correctly

Bug: 767238
Change-Id: Ifa86b2f1979a9033adaf1bb8d74d59c98f2256df
Reviewed-on: https://chromium-review.googlesource.com/714416
Reviewed-by: John Budorick <jbudorick@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Becky Zhou <huayinz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510527}
[modify] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/BUILD.gn
[modify] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/android/BUILD.gn
[modify] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java
[modify] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java
[modify] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java
[modify] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManagerToolbar.java
[add] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefChangeRegistrar.java
[modify] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
[modify] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
[modify] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/android/java_sources.gni
[modify] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/android/javatests/src/org/chromium/chrome/browser/history/HistoryActivityTest.java
[modify] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/browser/BUILD.gn
[add] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/browser/android/preferences/pref_change_registrar_android.cc
[add] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/browser/android/preferences/pref_change_registrar_android.h
[modify] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/browser/android/preferences/pref_service_bridge.cc
[modify] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/browser/android/preferences/pref_service_bridge.h
[add] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/browser/android/preferences/prefs.h
[add] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/browser/android/preferences/prefs_unittest.cc
[modify] https://crrev.com/d4234912f585297e603162729de4dea0482867f0/chrome/test/BUILD.gn
►

Sign in to add a comment