Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
360183002:
Set lockscreen settings in the locksettings db. (Closed)
|
Created:
6 years, 5 months ago by navabi Modified:
6 years, 5 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionSet lockscreen settings in the locksettings db.
BUG=389671
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281108
Patch Set 1 #Patch Set 2 : Used --auto-bots arg to git cl upload. #
Total comments: 13
Patch Set 3 : Addressed review comments. #
Total comments: 4
Patch Set 4 : Addressed reviews. #
Messages
Total messages: 26 (0 generated)
I've locally tested and verified that adding this will make it so that hitting the power button to turn the screen off and then again to turn it back on will not enable the lockscreen, as it currently does.
How often are we hitting the power buttons on devices? https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... File build/android/pylib/device_settings.py (right): https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:84: for locksetting in locksettings: nit: These four lines could be collapsed into: for table, key, value in locksettings: https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:91: print 'db: %s' % db What's up with the prints? Are these left over from debugging? https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:95: assert len(columns), 'must have at least one column' These asserts seems somewhat excessive given that everything checked is hard-coded in this function. https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:100: def escape(s): What's up with this? None of the items in |columns| or |values| contain single quotes. Are you anticipating them changing in the future? https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:114: output_msg = device.RunShellCommand('\'sqlite3 %s "%s"\'' % (db, cmd)) RunShellCommand should quote the command for you, so I don't think you need the single quotes here. https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:116: raise Exception('Failed to set system setting.\n%s' % ''.join(output_msg)) If RunShellCommand raises an error, output_msg isn't going to be set...
PTAL. I'm no longer sure that the power button being touched is the only way we get the lock screen. The last time I went to the lab (yesterday) all 3 devices were on the lock screen. https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... File build/android/pylib/device_settings.py (right): https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:84: for locksetting in locksettings: On 2014/07/01 15:10:55, jbudorick wrote: > nit: These four lines could be collapsed into: > > for table, key, value in locksettings: Done. https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:91: print 'db: %s' % db On 2014/07/01 15:10:55, jbudorick wrote: > What's up with the prints? Are these left over from debugging? Done. Yes. They are. Good catch. https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:95: assert len(columns), 'must have at least one column' On 2014/07/01 15:10:55, jbudorick wrote: > These asserts seems somewhat excessive given that everything checked is > hard-coded in this function. Removed the asserts. These were copied over from another provision script where this was a separate function. You are right that we don't need them here. https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:100: def escape(s): On 2014/07/01 15:10:55, jbudorick wrote: > What's up with this? None of the items in |columns| or |values| contain single > quotes. Are you anticipating them changing in the future? Removed this as well. See previous comment. https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:114: output_msg = device.RunShellCommand('\'sqlite3 %s "%s"\'' % (db, cmd)) On 2014/07/01 15:10:55, jbudorick wrote: > RunShellCommand should quote the command for you, so I don't think you need the > single quotes here. I don't think it adds the single quotes correctly. I added the single quotes after it did not work without them. Here is what the error is (i.e. output_msg) without the single quotes: ['Error: no such column: lockscreen.password_type_alternate'] I then looked at the RunShellCommand in build_internal/scripts/slave/android/device_utils.py and noticed that implementation put a single quote around the command. https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:116: raise Exception('Failed to set system setting.\n%s' % ''.join(output_msg)) On 2014/07/01 15:10:55, jbudorick wrote: > If RunShellCommand raises an error, output_msg isn't going to be set... Done.
On 2014/07/01 22:44:44, navabi wrote: > PTAL. > > I'm no longer sure that the power button being touched is the only way we get > the lock screen. The last time I went to the lab (yesterday) all 3 devices were > on the lock screen. > Can't it happen if the device crashes and reboots? > https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... > File build/android/pylib/device_settings.py (right): > > https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... > build/android/pylib/device_settings.py:84: for locksetting in locksettings: > On 2014/07/01 15:10:55, jbudorick wrote: > > nit: These four lines could be collapsed into: > > > > for table, key, value in locksettings: > > Done. > > https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... > build/android/pylib/device_settings.py:91: print 'db: %s' % db > On 2014/07/01 15:10:55, jbudorick wrote: > > What's up with the prints? Are these left over from debugging? > > Done. Yes. They are. Good catch. > > https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... > build/android/pylib/device_settings.py:95: assert len(columns), 'must have at > least one column' > On 2014/07/01 15:10:55, jbudorick wrote: > > These asserts seems somewhat excessive given that everything checked is > > hard-coded in this function. > > Removed the asserts. These were copied over from another provision script where > this was a separate function. You are right that we don't need them here. > > https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... > build/android/pylib/device_settings.py:100: def escape(s): > On 2014/07/01 15:10:55, jbudorick wrote: > > What's up with this? None of the items in |columns| or |values| contain single > > quotes. Are you anticipating them changing in the future? > > Removed this as well. See previous comment. > > https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... > build/android/pylib/device_settings.py:114: output_msg = > device.RunShellCommand('\'sqlite3 %s "%s"\'' % (db, cmd)) > On 2014/07/01 15:10:55, jbudorick wrote: > > RunShellCommand should quote the command for you, so I don't think you need > the > > single quotes here. > > I don't think it adds the single quotes correctly. I added the single quotes > after it did not work without them. Here is what the error is (i.e. output_msg) > without the single quotes: > > ['Error: no such column: lockscreen.password_type_alternate'] > > I then looked at the RunShellCommand in > build_internal/scripts/slave/android/device_utils.py and noticed that > implementation put a single quote around the command. > > https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... > build/android/pylib/device_settings.py:116: raise Exception('Failed to set > system setting.\n%s' % ''.join(output_msg)) > On 2014/07/01 15:10:55, jbudorick wrote: > > If RunShellCommand raises an error, output_msg isn't going to be set... > > Done.
On 2014/07/01 22:47:08, Yaron wrote: > On 2014/07/01 22:44:44, navabi wrote: > > PTAL. > > > > I'm no longer sure that the power button being touched is the only way we get > > the lock screen. The last time I went to the lab (yesterday) all 3 devices > were > > on the lock screen. > > > Can't it happen if the device crashes and reboots? > I'm not sure. It does not happen after a regular reboot. I have not watched a phone crash and reboot. Anyway, it is happening enough that I think we need this fix.
happy with this when John is https://codereview.chromium.org/360183002/diff/40001/build/android/pylib/devi... File build/android/pylib/device_settings.py (right): https://codereview.chromium.org/360183002/diff/40001/build/android/pylib/devi... build/android/pylib/device_settings.py:10: PASSWORD_QUALITY_UNSPECIFIED = 0 why not just make this a string literal and avoid the |str| calls below?
lgtm w/ nit https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... File build/android/pylib/device_settings.py (right): https://codereview.chromium.org/360183002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:114: output_msg = device.RunShellCommand('\'sqlite3 %s "%s"\'' % (db, cmd)) On 2014/07/01 22:44:43, navabi wrote: > On 2014/07/01 15:10:55, jbudorick wrote: > > RunShellCommand should quote the command for you, so I don't think you need > the > > single quotes here. > > I don't think it adds the single quotes correctly. I added the single quotes > after it did not work without them. Here is what the error is (i.e. output_msg) > without the single quotes: > > ['Error: no such column: lockscreen.password_type_alternate'] > > I then looked at the RunShellCommand in > build_internal/scripts/slave/android/device_utils.py and noticed that > implementation put a single quote around the command. In that case, this is fine with me. I'll have to look at it when I switch the implementation of RunShellCommand away from the AndroidCommands version. https://codereview.chromium.org/360183002/diff/40001/build/android/pylib/devi... File build/android/pylib/device_settings.py (right): https://codereview.chromium.org/360183002/diff/40001/build/android/pylib/devi... build/android/pylib/device_settings.py:95: 'columns': ', '.join([column for column in columns]), nit: this can just be 'columns': ', '.join(columns)
PTAL. > In that case, this is fine with me. I'll have to look at it when I switch the > implementation of RunShellCommand away from the AndroidCommands version. Thanks John. When you do, please talk to me, or put me on the review and I will test the problem I ran into. Namely, that if you replace the RunShellCommand in build_internal/scripts/slave/android/provision_devices.py (which calls it's own RunShellCommand defined in device_utils.py in that directory), with the RunCommand implementation we are using in android_tools.py, the command is messed up because of single quote(s). https://codereview.chromium.org/360183002/diff/40001/build/android/pylib/devi... File build/android/pylib/device_settings.py (right): https://codereview.chromium.org/360183002/diff/40001/build/android/pylib/devi... build/android/pylib/device_settings.py:10: PASSWORD_QUALITY_UNSPECIFIED = 0 On 2014/07/02 00:33:34, Yaron wrote: > why not just make this a string literal and avoid the |str| calls below? Done. https://codereview.chromium.org/360183002/diff/40001/build/android/pylib/devi... build/android/pylib/device_settings.py:95: 'columns': ', '.join([column for column in columns]), On 2014/07/02 01:01:29, jbudorick wrote: > nit: this can just be > > 'columns': ', '.join(columns) Done.
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/360183002/60001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2014/07/02 05:35:52, I haz the power (commit-bot) wrote: > No LGTM from a valid reviewer yet. Only full committers are accepted. > Even if an LGTM may have been provided, it was from a non-committer or > a provisional committer, _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. Sigh. Maybe updates to committers take a few days to propagate.
lgtm
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/360183002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/360183002/60001
Message was sent while issue was closed.
Change committed as 281108 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
