Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(92)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2165333002: Revert of Redirect users without sync passphrase to passwords.google.com (Closed)

Created:
4 years, 5 months ago by Ted C
Modified:
4 years, 5 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Revert of Redirect users without sync passphrase to passwords.google.com (patchset #50 id:980001 of https://codereview.chromium.org/2092723002/ ) Reason for revert: The tests are checking their expectations on the wrong thread. https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/35372/steps/chrome_public_test_apk/logs/stdio I 703.521s run_tests_on_device(0ca41989032f9f23) java.lang.AssertionError I 703.521s run_tests_on_device(0ca41989032f9f23) at org.chromium.base.ThreadUtils.assertOnUiThread(ThreadUtils.java:192) I 703.521s run_tests_on_device(0ca41989032f9f23) at org.chromium.chrome.browser.sync.ProfileSyncService.get(ProfileSyncService.java:104) I 703.521s run_tests_on_device(0ca41989032f9f23) at org.chromium.chrome.browser.preferences.PasswordViewingTypeTest.testUserRedirectSyncSettings(PasswordViewingTypeTest.java:133) I 703.521s run_tests_on_device(0ca41989032f9f23) at java.lang.reflect.Method.invokeNative(Native Method) I 703.521s run_tests_on_device(0ca41989032f9f23) at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) I 703.521s run_tests_on_device(0ca41989032f9f23) at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) I 703.521s run_tests_on_device(0ca41989032f9f23) at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:129) I 703.521s run_tests_on_device(0ca41989032f9f23) at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) I 703.521s run_tests_on_device(0ca41989032f9f23) at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) I 703.521s run_tests_on_device(0ca41989032f9f23) at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) I 703.521s run_tests_on_device(0ca41989032f9f23) at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) Original issue's description: > Redirect syncing users without sync passphrase to passwords.google.com > > This CL redirects syncing users without a sync passphrase to https://passwords.google.com instead of natively displaying passwords. The changes created in this CL are hidden behind the ViewPasswords feature. > > BUG=623488 > > Committed: https://crrev.com/13085d446efc3b8cd2c26338738c04a7516953a8 > Cr-Commit-Position: refs/heads/master@{#406823} TBR=vabr@chromium.org,bauerb@chromium.org,dozsa@google.com # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=623488 Committed: https://crrev.com/0073a6f1f50b7347daa0b6b968f06c9bcf13e38c Cr-Commit-Position: refs/heads/master@{#406885}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -218 lines) Patch
M chrome/android/java/res/xml/main_preferences.xml View 1 chunk +3 lines, -1 line 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java View 4 chunks +7 lines, -48 lines 0 comments Download
M chrome/android/java/strings/android_chrome_strings.grd View 1 chunk +0 lines, -6 lines 0 comments Download
M chrome/android/java_sources.gni View 1 chunk +0 lines, -1 line 0 comments Download
D chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java View 1 chunk +0 lines, -160 lines 0 comments Download
M chrome/browser/android/chrome_feature_list.cc View 2 chunks +0 lines, -2 lines 0 comments Download

Messages

Total messages: 10 (3 generated)
Ted C
Created Revert of Redirect users without sync passphrase to passwords.google.com
4 years, 5 months ago (2016-07-21 17:23:43 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165333002/1
4 years, 5 months ago (2016-07-21 17:24:11 UTC) #3
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 5 months ago (2016-07-21 17:25:00 UTC) #5
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/0073a6f1f50b7347daa0b6b968f06c9bcf13e38c Cr-Commit-Position: refs/heads/master@{#406885}
4 years, 5 months ago (2016-07-21 17:26:46 UTC) #7
vabr (Chromium)
On 2016/07/21 17:26:46, commit-bot: I haz the power wrote: > Patchset 1 (id:??) landed as ...
4 years, 5 months ago (2016-07-22 07:49:09 UTC) #8
Bernhard Bauer
On 2016/07/22 07:49:09, vabr (Chromium) wrote: > On 2016/07/21 17:26:46, commit-bot: I haz the power ...
4 years, 5 months ago (2016-07-22 09:53:33 UTC) #9
Yaron
4 years, 5 months ago (2016-07-22 17:10:03 UTC) #10
Message was sent while issue was closed. 
On 2016/07/22 09:53:33, Bernhard Bauer wrote:
> On 2016/07/22 07:49:09, vabr (Chromium) wrote:
> > On 2016/07/21 17:26:46, commit-bot: I haz the power wrote:
> > > Patchset 1 (id:??) landed as
> > > https://crrev.com/0073a6f1f50b7347daa0b6b968f06c9bcf13e38c
> > > Cr-Commit-Position: refs/heads/master@{#406885}
> > 
> > Hi Paula,
> > 
> > Apparently, ProfileSyncService should be accessed only on UI thread, so you
> need
> > to wrap lines 133 and 134 of the test in ThreadUtils.runOnUiThreadBlocking
> like
> > you do above.
> > 
> > To create a re-land of the CL, please follow this pattern:
> > 
> > 1. Upload a new CL with the first patch set equal to
> > https://codereview.chromium.org/2092723002/#ps980001
> >   -> This can be done by git new-branch reland; git cl patch 2092723002; git
> cl
> > upload
> >   -> Please add "Reland" to the upload message, which should otherwise
contain
> a
> > copy of the original one.
> >   -> Also please add CQ_INCLUDE_TRYBOTS=linux_android_dbg_ng
> > 2. Run specifically the linux_android_dbg_ng trybot on the first patch. It
> > should fail. That would give us reassurance that passing that trybot before
> > landing will prevent us from breaking Android Test (dbg) again.
> >   -> I'm not sure why linux_android_dbg_ng is not part of the default trybot
> set
> > (probably too heavy on resources). The release version of the bot
surprisingly
> > did not hit the thread checking:
> >
>
https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/2...
> 
> Actually, I'm more surprised some bot hit this check at all:
> ThreadUtils.assertOnUiThread() is implemented with a Java assert, and those
are
> a) not functional in ART, which is used from Android L on, and b) need to be
> explicitly enabled when running in Dalvik.

The truth is that the vast majority of our bots are still testing on KitKat
> 
> In true Google fashion, the next-generation Android toolchain (Jack) will
> support asserts (in a similar manner to how DCHECKs work in C++ right now:
> functional in Debug builds, stripped out of Release builds), but we don't
> support building with Jack yet. 
> 
> > 3. Fix the threading issue using ThreadUtils.runOnUiThreadBlocking, upload
> that
> > as a second patch set (name it "Fix" or similar).
> > 4. Run the trybots, ensure that the linux_android_dbg_ng is green, get the
CL
> > reviewed, land.
> > 
> > Thanks!
> > Vaclav

Powered by Google App Engine
This is Rietveld 408576698