New issue
Advanced search Search tips

Issue 818057 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Chrome should get detachable base state from the hammerd on startup

Project Member Reported by tbarzic@chromium.org, Mar 2 2018

Issue description

Chrome uses hammerd to keep track of the detachable base state, and to detect when it has to notify users the base has changed, and when the base requires firmware update.

On start-up Chrome should be able to properly initialize its state - Chrome should not lose information about paired bases after restart.
Also, Chrome should be able to handle the case when hammerd sends base pairing signal before Chrome initializes the hammerd dbus client. 
 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 7 2018

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

commit be20db5ab2108cfd77d84c6f9b68d6845cca3d30
Author: Toni Barzic <tbarzic@google.com>
Date: Wed Mar 07 03:02:42 2018

Use upstart to kick off hammerd when detachable base handler is started

The goal is to get hammerd to run pairing challenge, and send the
appropriate dbus pairing signals (indicating whether the base was
paired and authenticated, or whether it requires a firmware update),
in order to initialize Chrome's state.

Note: given that the hammerd job is short lived - it is usually run
when a detachable base is detached, and exists once pairing has been
completed - it cannot easily expose a dbus API to retrieve this state
when Chrome needs it (which might be a good alternative).

BUG=796342,  796300 ,  818057 

Change-Id: I40e71318f6d35d96aaf295033964864e3dc74f88
Reviewed-on: https://chromium-review.googlesource.com/941724
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541301}
[modify] https://crrev.com/be20db5ab2108cfd77d84c6f9b68d6845cca3d30/ash/detachable_base/DEPS
[modify] https://crrev.com/be20db5ab2108cfd77d84c6f9b68d6845cca3d30/ash/detachable_base/detachable_base_handler.cc
[modify] https://crrev.com/be20db5ab2108cfd77d84c6f9b68d6845cca3d30/ash/detachable_base/detachable_base_handler_unittest.cc
[modify] https://crrev.com/be20db5ab2108cfd77d84c6f9b68d6845cca3d30/chromeos/dbus/fake_upstart_client.cc
[modify] https://crrev.com/be20db5ab2108cfd77d84c6f9b68d6845cca3d30/chromeos/dbus/fake_upstart_client.h
[modify] https://crrev.com/be20db5ab2108cfd77d84c6f9b68d6845cca3d30/chromeos/dbus/upstart_client.cc
[modify] https://crrev.com/be20db5ab2108cfd77d84c6f9b68d6845cca3d30/chromeos/dbus/upstart_client.h

Project Member

Comment 2 by bugdroid1@chromium.org, Mar 14 2018

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

commit 9deedbdf6450d04927496513807d9b69e0283bd5
Author: Toni Barzic <tbarzic@chromium.org>
Date: Wed Mar 14 22:13:55 2018

[dbus] Move upstart client to from browser to common clients

ash/detachable_base_handler/ now depends on upstart client to
(re)start detachable base pairing status checks on start up, so the
client should be available to ash.

BUG= 818057 
TEST=No crash when starting Chrome with --enable-features=Mash

Change-Id: Id2e89b9c0f2fca0e850a05ac20d9d14c616e7923
Reviewed-on: https://chromium-review.googlesource.com/963297
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543216}
[modify] https://crrev.com/9deedbdf6450d04927496513807d9b69e0283bd5/chromeos/dbus/dbus_clients_browser.cc
[modify] https://crrev.com/9deedbdf6450d04927496513807d9b69e0283bd5/chromeos/dbus/dbus_clients_browser.h
[modify] https://crrev.com/9deedbdf6450d04927496513807d9b69e0283bd5/chromeos/dbus/dbus_clients_common.cc
[modify] https://crrev.com/9deedbdf6450d04927496513807d9b69e0283bd5/chromeos/dbus/dbus_clients_common.h
[modify] https://crrev.com/9deedbdf6450d04927496513807d9b69e0283bd5/chromeos/dbus/dbus_thread_manager.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/31c7222e482f4d51fba33013ba574a28050511a7

commit 31c7222e482f4d51fba33013ba574a28050511a7
Author: Toni Barzic <tbarzic@chromium.org>
Date: Wed Mar 28 03:34:49 2018

login: Add EmitAshInitialized dbus method

Adds EmitAshInitialized method to session manager interface.
The method emits ash-initialized upstart signal on behalf of
the browser. It is expected to be called when the ash shell is
initialized.

The signal will be used to run jobs that depend on ash shell - for
example to run hammerd. The hammerd job should run when ash is
initialized to ensure ash receives dbus signals emitted by hammerd
that match the current hammer state - this will ensure that ash
displays notifications matching the current hammer state, if needed.

BUG= chromium:818057 
TEST=(once Chrome and hammerd changes have landed) While on login
     screen, attach a detachable base different than the last one
     used by the user focused on the login screen. Run "restart ui".
     Upon Chrome restart, a warning that keyboard is different than
     the last one used by the current user should be displayed.

Change-Id: Ib42def82310c72e37c2476f39c8e121f8d4d09f4
Reviewed-on: https://chromium-review.googlesource.com/981604
Commit-Ready: Toni Barzic <tbarzic@chromium.org>
Tested-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/31c7222e482f4d51fba33013ba574a28050511a7/login_manager/session_manager_impl.cc
[modify] https://crrev.com/31c7222e482f4d51fba33013ba574a28050511a7/login_manager/dbus_bindings/org.chromium.SessionManagerInterface.xml
[modify] https://crrev.com/31c7222e482f4d51fba33013ba574a28050511a7/login_manager/session_manager_impl_unittest.cc
[modify] https://crrev.com/31c7222e482f4d51fba33013ba574a28050511a7/login_manager/SessionManager.conf
[modify] https://crrev.com/31c7222e482f4d51fba33013ba574a28050511a7/login_manager/session_manager_impl.h

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/system_api/+/3290a8e598f2bd2aecac0fbb25fc410b04d6b3c4

commit 3290a8e598f2bd2aecac0fbb25fc410b04d6b3c4
Author: Toni Barzic <tbarzic@chromium.org>
Date: Wed Mar 28 20:34:36 2018

Add EmitAshInitialized method to dbus constants

BUG= chromium:818057 
TEST=None

Change-Id: I848d5c4cd995bc318d7d0fc6ecdfe26c9602e8c3
Reviewed-on: https://chromium-review.googlesource.com/982792
Commit-Ready: Toni Barzic <tbarzic@chromium.org>
Tested-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/3290a8e598f2bd2aecac0fbb25fc410b04d6b3c4/dbus/login_manager/dbus-constants.h

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/eb37ded853b2baa80837b8847c1e3570a3dc32c9

commit eb37ded853b2baa80837b8847c1e3570a3dc32c9
Author: Toni Barzic <tbarzic@chromium.org>
Date: Wed Mar 28 20:34:24 2018

hammerd: Run hammerd on ash-initialized

Starts hammerd job when ash-initialized signal is emitted by
upstart - the goal is to ensure that hammerd signals that match the
current hammer state are dispatched while ash shell is listening to
them, and can react to them.

BUG= chromium:818057 
TEST=While on login screen, attach a hammer that does not match the
     last hammer used by the user focused on the login screen. Run
     restart ui,  and verify that a warning about keyboard changing
     from the last used is shown.

Change-Id: Idea99b4b6f1ea9063275ee6ebf88931c2613ec75
Reviewed-on: https://chromium-review.googlesource.com/982253
Commit-Ready: Toni Barzic <tbarzic@chromium.org>
Tested-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Chih-Yu Huang <akahuang@chromium.org>

[modify] https://crrev.com/eb37ded853b2baa80837b8847c1e3570a3dc32c9/hammerd/init/hammerd.conf

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 30 2018

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

commit 85c9e080d7f1b39af2ba216f604b99d67d4566a1
Author: Toni Barzic <tbarzic@chromium.org>
Date: Fri Mar 30 02:46:01 2018

Emit ash-initialized upstart signal on ash shell init

Adds EmitAshInitialized method to session_manager_client - the
method requests from session manager to emit "ash-initialized"
upstart signal. The signal can be used to run upstart jobs that
should be run when ash is initialized - for example, to run hammerd
task, which will ensure that detachable base pairing signals are
emitted after ash starts observing dbus signals emitted by hammerd.
DetachableBaseHandler relies on hammerd signals to determine when,
whether a detachable base notification should be shown to the user, so
these signals should be re-emitted whenever ash is (re)started.

Makes ash::Shell::Init call the new session manager method to actually
emit the upstart signal once it's done initializing.

BUG= 818057 

Change-Id: If901e125f8f3f8563c22a0633644a0c6ff143f7d
Reviewed-on: https://chromium-review.googlesource.com/981330
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547082}
[modify] https://crrev.com/85c9e080d7f1b39af2ba216f604b99d67d4566a1/ash/shell.cc
[modify] https://crrev.com/85c9e080d7f1b39af2ba216f604b99d67d4566a1/chromeos/dbus/fake_session_manager_client.cc
[modify] https://crrev.com/85c9e080d7f1b39af2ba216f604b99d67d4566a1/chromeos/dbus/fake_session_manager_client.h
[modify] https://crrev.com/85c9e080d7f1b39af2ba216f604b99d67d4566a1/chromeos/dbus/session_manager_client.cc
[modify] https://crrev.com/85c9e080d7f1b39af2ba216f604b99d67d4566a1/chromeos/dbus/session_manager_client.h

Project Member

Comment 7 by bugdroid1@chromium.org, Mar 30 2018

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

commit 0ac12f94fa238d03f27b41c57d2c5769b7848e70
Author: Toni Barzic <tbarzic@chromium.org>
Date: Fri Mar 30 18:42:11 2018

Remove UpstartClient::StartHammerd

This is not needed anymore. Running hammerd service on session start
is being implemented in a way that does not require Chrome directly
starting hammerd using Upstart interface.

Instead, session_manager will emit "ash-initialized" signal when ash
shell is initialized, and hammerd will be configured to run on that
signal, similar to how it's configured to run when hammer device is
added.

BUG= 818057 

Change-Id: I3c6172fa1df1e1e0c80f7ecd57a6c56b0d98164a
Reviewed-on: https://chromium-review.googlesource.com/986915
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547219}
[modify] https://crrev.com/0ac12f94fa238d03f27b41c57d2c5769b7848e70/ash/detachable_base/DEPS
[modify] https://crrev.com/0ac12f94fa238d03f27b41c57d2c5769b7848e70/ash/detachable_base/detachable_base_handler.cc
[modify] https://crrev.com/0ac12f94fa238d03f27b41c57d2c5769b7848e70/ash/detachable_base/detachable_base_handler_unittest.cc
[modify] https://crrev.com/0ac12f94fa238d03f27b41c57d2c5769b7848e70/chromeos/dbus/fake_upstart_client.cc
[modify] https://crrev.com/0ac12f94fa238d03f27b41c57d2c5769b7848e70/chromeos/dbus/fake_upstart_client.h
[modify] https://crrev.com/0ac12f94fa238d03f27b41c57d2c5769b7848e70/chromeos/dbus/upstart_client.cc
[modify] https://crrev.com/0ac12f94fa238d03f27b41c57d2c5769b7848e70/chromeos/dbus/upstart_client.h

Status: Fixed (was: Started)
Labels: -M-66 M-67

Sign in to add a comment