New issue
Advanced search Search tips

Issue 741940 link

Starred by 2 users
Status: Archived
Owner:
tbarzic@chromium.org
Closed: Jul 2017
Cc:
r...@chromium.org
tbuck...@chromium.org
jdufault@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Stylus settings UI is not notified of preferred note taking app changes

Project Member Reported by tbarzic@chromium.org, Jul 13 2017 
In stylus device settings UI, the UI code is not notified of changes to preferred not taking app, and thus the UI is not properly updated when the preferred note taking app changes:
For example:
1. Open stylus settings in browser window A
2. Open stylus settings in browser window B, alongside A
3. Change preferred note taking app in window B
4. Value in window A remains the same
5. It's not immediately clear which stylus settings value are actually true

Comment 1 by tbarzic@chromium.org, Jul 13 2017

Cc: tbuck...@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 24 2017 

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

commit e4a5c06a33df72ea7960883381427ac6b67b2f1e
Author: Toni Barzic <tbarzic@google.com>
Date: Mon Jul 24 23:40:41 2017

Update stylus settings for lock screen note taking

Fixes a number of issues with stylus settings for note taking on lock
screen:
 *  Provide a NoteTakingHelper observer interface called when the
    preferred app changes (or when it's lock screen status changes)
     *  Settings UI can use this to update itself when the preferred
        app changes.
     *  Switch lock_screen_apps::AppManagerImpl to observe this event
        for preferred app changes (instead of observing note taking
        pref directly)
  *  Introduces kNoteTakingAppsAllowedOnLockScreen pref, that will be
    used by a user policy to whitelist apps available on the lock
    screen (to be added in dependent patch)
 *  Disable lock screen support for note taking apps in non-primary
    profiles (the profile that supports lock screen use case is set
    by lock_screen_apps::StateController during its initialization).
 *  Redo settings UI for enabling apps on the lock screen so its
    state (whether it's disabled, the policy indicator) does not
    depend on prefs directly, instead derive the state from the
    note taking app's NoteAppInfo (in particular lockScreenSupport
    property)

While here, did some cleanup in test code:
 *  Provided utility methods to NoteTakingHelper unit tests to
    reduce code duplication:
     *  Method to create/install lock screen enabled note taking app
     *  Methods for verifying preferred app and available apps info
 *  In stylus device page browser tests, made fake browser proxy
    smarter, so test don't have to "manually" trigger note taking app
    changes


Bug:  741940 
Bug: 741053
Bug: 746116
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I5e2ee138df620d3832a8ad0d1b4d0db285fba0da
Reviewed-on: https://chromium-review.googlesource.com/572842
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489142}
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/chromeos/lock_screen_apps/app_manager_impl.h
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/chromeos/lock_screen_apps/app_manager_impl_unittest.cc
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/chromeos/lock_screen_apps/note_taking_browsertest.cc
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/chromeos/lock_screen_apps/state_controller.cc
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/chromeos/note_taking_helper.cc
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/chromeos/note_taking_helper.h
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/chromeos/note_taking_helper_unittest.cc
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/chromeos/preferences.cc
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/extensions/api/settings_private/prefs_util.cc
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/resources/settings/device_page/compiled_resources2.gyp
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/resources/settings/device_page/device_page_browser_proxy.js
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/resources/settings/device_page/stylus.html
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/resources/settings/device_page/stylus.js
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/ui/webui/options/chromeos/options_stylus_handler.cc
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/ui/webui/options/chromeos/options_stylus_handler.h
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.cc
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.h
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/common/pref_names.cc
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/common/pref_names.h
[modify] https://crrev.com/e4a5c06a33df72ea7960883381427ac6b67b2f1e/chrome/test/data/webui/settings/device_page_tests.js
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 27 2017

Labels: merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7270d2d7667e871f0e5d80d5a54b0473eb45bace

commit 7270d2d7667e871f0e5d80d5a54b0473eb45bace
Author: Toni Barzic <tbarzic@google.com>
Date: Thu Jul 27 01:34:08 2017

Update stylus settings for lock screen note taking

Fixes a number of issues with stylus settings for note taking on lock
screen:
 *  Provide a NoteTakingHelper observer interface called when the
    preferred app changes (or when it's lock screen status changes)
     *  Settings UI can use this to update itself when the preferred
        app changes.
     *  Switch lock_screen_apps::AppManagerImpl to observe this event
        for preferred app changes (instead of observing note taking
        pref directly)
  *  Introduces kNoteTakingAppsAllowedOnLockScreen pref, that will be
    used by a user policy to whitelist apps available on the lock
    screen (to be added in dependent patch)
 *  Disable lock screen support for note taking apps in non-primary
    profiles (the profile that supports lock screen use case is set
    by lock_screen_apps::StateController during its initialization).
 *  Redo settings UI for enabling apps on the lock screen so its
    state (whether it's disabled, the policy indicator) does not
    depend on prefs directly, instead derive the state from the
    note taking app's NoteAppInfo (in particular lockScreenSupport
    property)

While here, did some cleanup in test code:
 *  Provided utility methods to NoteTakingHelper unit tests to
    reduce code duplication:
     *  Method to create/install lock screen enabled note taking app
     *  Methods for verifying preferred app and available apps info
 *  In stylus device page browser tests, made fake browser proxy
    smarter, so test don't have to "manually" trigger note taking app
    changes

TBR=tbarzic@google.com

(cherry picked from commit e4a5c06a33df72ea7960883381427ac6b67b2f1e)

Bug:  741940 
Bug: 741053
Bug: 746116
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I5e2ee138df620d3832a8ad0d1b4d0db285fba0da
Reviewed-on: https://chromium-review.googlesource.com/572842
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#489142}
Reviewed-on: https://chromium-review.googlesource.com/588341
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#72}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/chromeos/lock_screen_apps/app_manager_impl.h
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/chromeos/lock_screen_apps/app_manager_impl_unittest.cc
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/chromeos/lock_screen_apps/note_taking_browsertest.cc
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/chromeos/lock_screen_apps/state_controller.cc
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/chromeos/note_taking_helper.cc
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/chromeos/note_taking_helper.h
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/chromeos/note_taking_helper_unittest.cc
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/chromeos/preferences.cc
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/extensions/api/settings_private/prefs_util.cc
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/resources/settings/device_page/compiled_resources2.gyp
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/resources/settings/device_page/device_page_browser_proxy.js
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/resources/settings/device_page/stylus.html
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/resources/settings/device_page/stylus.js
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/ui/webui/options/chromeos/options_stylus_handler.cc
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/ui/webui/options/chromeos/options_stylus_handler.h
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.cc
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.h
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/common/pref_names.cc
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/common/pref_names.h
[modify] https://crrev.com/7270d2d7667e871f0e5d80d5a54b0473eb45bace/chrome/test/data/webui/settings/device_page_tests.js

Comment 4 by tbarzic@chromium.org, Jul 28 2017

Status: Fixed (was: Started)

Comment 5 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)
►

Sign in to add a comment