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

Issue 767756 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Do not show "setup needed" notification until ConnectTetheringRequest sent

Project Member Reported by khorimoto@chromium.org, Sep 22 2017

Issue description

Currently, we show the "setup needed" notification as soon as we start trying to contact the device we're connecting to.

However, if there is an error connecting to the device (e.g., Bluetooth fails), then we confuse the user by telling the user to continue setup on the phone side, even though the setup UI is never shown.

Instead, we should wait until we successfully send a ConnectTetheringRequest before showing that UI.
 
Status: Started (was: Available)
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 10 2017

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

commit ef61658957c5b0a8d0f2ceb007230d4cfa01675d
Author: Kyle Horimoto <khorimoto@google.com>
Date: Tue Oct 10 23:06:42 2017

[CrOS Tether] Only show "setup required" notification when needed.

Previously, we would display this notification as soon as a connection
attempt to a device was underway. However, it is possible for a
connection attempt to fail; in those cases, the notification was
confusing because it told the users to perform an action on their phones
even though the phones would never display anything in this case.

This CL delays showing this notification until the phone has responded
successfully, ensuring the user will not see the notification when it is
not actionable.

Bug:  767756 , 672263
Change-Id: I11eb5c326d45606bb27b9dda2b8b69ad6b031efb
Reviewed-on: https://chromium-review.googlesource.com/710194
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507816}
[modify] https://crrev.com/ef61658957c5b0a8d0f2ceb007230d4cfa01675d/chromeos/components/tether/tether_connector_impl.cc
[modify] https://crrev.com/ef61658957c5b0a8d0f2ceb007230d4cfa01675d/chromeos/components/tether/tether_connector_impl_unittest.cc

Status: Fixed (was: Started)
Status: Started (was: Fixed)
This fix was actually insufficient. Instead, I should have shown the notification once the message had been sent, not once a response had been received. Re-opening.
Project Member

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

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

commit c5d007e48576ac32eccd29b50969816823c29a1f
Author: Kyle Horimoto <khorimoto@google.com>
Date: Fri Oct 20 23:52:32 2017

[CrOS Tether] Show "setup required" notification after request is sent.

Previously, we would show the notification after a reply was received.
This was incorrect, since the setup is required BEFORE the response is
sent.

Bug:  767756 , 672263
Change-Id: I9de2fa09ab9c113e814febdaf4085751760b0d02
Reviewed-on: https://chromium-review.googlesource.com/731680
Reviewed-by: Jeremy Klein <jlklein@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510617}
[modify] https://crrev.com/c5d007e48576ac32eccd29b50969816823c29a1f/chromeos/components/tether/connect_tethering_operation.cc
[modify] https://crrev.com/c5d007e48576ac32eccd29b50969816823c29a1f/chromeos/components/tether/connect_tethering_operation.h
[modify] https://crrev.com/c5d007e48576ac32eccd29b50969816823c29a1f/chromeos/components/tether/connect_tethering_operation_unittest.cc
[modify] https://crrev.com/c5d007e48576ac32eccd29b50969816823c29a1f/chromeos/components/tether/tether_connector_impl.cc
[modify] https://crrev.com/c5d007e48576ac32eccd29b50969816823c29a1f/chromeos/components/tether/tether_connector_impl.h
[modify] https://crrev.com/c5d007e48576ac32eccd29b50969816823c29a1f/chromeos/components/tether/tether_connector_impl_unittest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 23 2017

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

commit e21133e9d9947d039666c3d8b7f2f6cb2609b75d
Author: Kyle Horimoto <khorimoto@google.com>
Date: Mon Oct 23 17:23:36 2017

[CrOS Tether] Show "setup required" notification after request is sent.

Previously, we would show the notification after a reply was received.
This was incorrect, since the setup is required BEFORE the response is
sent.

TBR=khorimoto@google.com

(cherry picked from commit c5d007e48576ac32eccd29b50969816823c29a1f)

Bug:  767756 , 672263
Change-Id: I9de2fa09ab9c113e814febdaf4085751760b0d02
Reviewed-on: https://chromium-review.googlesource.com/731680
Reviewed-by: Jeremy Klein <jlklein@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#510617}
Reviewed-on: https://chromium-review.googlesource.com/733803
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#156}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/e21133e9d9947d039666c3d8b7f2f6cb2609b75d/chromeos/components/tether/connect_tethering_operation.cc
[modify] https://crrev.com/e21133e9d9947d039666c3d8b7f2f6cb2609b75d/chromeos/components/tether/connect_tethering_operation.h
[modify] https://crrev.com/e21133e9d9947d039666c3d8b7f2f6cb2609b75d/chromeos/components/tether/connect_tethering_operation_unittest.cc
[modify] https://crrev.com/e21133e9d9947d039666c3d8b7f2f6cb2609b75d/chromeos/components/tether/tether_connector_impl.cc
[modify] https://crrev.com/e21133e9d9947d039666c3d8b7f2f6cb2609b75d/chromeos/components/tether/tether_connector_impl.h
[modify] https://crrev.com/e21133e9d9947d039666c3d8b7f2f6cb2609b75d/chromeos/components/tether/tether_connector_impl_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment