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

Issue 807577 link

Starred by 0 users
Status: Fixed
Owner:
█ vabr@chromium.org
hobby only
Closed: Mar 2018
Cc:
melandory@chromium.org
isherman@chromium.org
jdoerrie@chromium.org
fhorschig@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Task



Sign in to add a comment

PasswordEntryEditor.java's PASSWORD_ENTRY_ACTION_CANCELLED is unused

Project Member Reported by vabr@chromium.org, Jan 31 2018 
It seems that PasswordEntryEditor does not log the cancelling action, whatever that might mean.

Should we add that logging, or remove the references to that value in Java and mark it obsolete in UMA (if possible for a single value)?

Adding melandory@ and jdoerrie@ in case they have historical context and fhorschig@ who has also been recently active in that code area.

Comment 1 by vabr@chromium.org, Feb 2 2018

Cc: isherman@chromium.org
I dug up the details: the cancelled action was dropped in https://chromium-review.googlesource.com/c/chromium/src/+/666624 together with dropping the UI which emitted that value in the histogram.

@melandory, could you please confirm that the new UI is not supposed to emit the "cancelled" value?

@isherman, do you know if there is a way to mark a single value of a histogram enum (from enums.xml) as obsolete while keeping the rest?

Thanks!

Comment 2 by fhorschig@chromium.org, Feb 2 2018 

Is there something we could (or should) interpret in the new UI as "cancelled"?

To me it looks like "just viewing" might be a valid use case and returning without action wouldn't necessarily mean the same.
Therefore, marking PASSWORD_ENTRY_ACTION_CANCELLED as obsolete - i.e. by prefixing the label with "OBSOLETE_" - would make sense here.

Comment 3 by isherman@chromium.org, Feb 2 2018 

@vabr, I usually just rename the enum entry to something like "Cancelled (obsolete)" or "Cancelled (deprecated)", and make sure to similarly mark the enum slot reserved in the corresponding C++ code somehow.

Comment 4 by vabr@chromium.org, Mar 14 2018

Owner: vabr@chromium.org
Status: Started (was: Available)
Thanks, fhorschig@ and isherman@ for the helpful answers!

I agree with #2 and will implement the advice from #3.

Comment 5 by vabr@chromium.org, Mar 14 2018

Blocking: 817292

Comment 6 by vabr@chromium.org, Mar 14 2018

Blocking: -817292
Project Member

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

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

commit e5feb7e85061b88e8771ad95441713ac6d4401a4
Author: Vaclav Brozek <vabr@chromium.org>
Date: Thu Mar 15 21:32:16 2018

Mark PASSWORD_ENTRY_ACTION_CANCELLED as obsolete

In PasswordEntryEditor, PASSWORD_ENTRY_ACTION_CANCELLED represented
the action of cancelling viewing of a password. This action no longer
maps to the current possible UI flows and its representation is
therefore marked as obsolete in this CL.

For more info see the discussion on the associated bug.

Bug:  807577 
Change-Id: I79dc75a4d3a14cf1df7f46d1a5a2b26b664e5a0f
Reviewed-on: https://chromium-review.googlesource.com/962453
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543503}
[modify] https://crrev.com/e5feb7e85061b88e8771ad95441713ac6d4401a4/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java
[modify] https://crrev.com/e5feb7e85061b88e8771ad95441713ac6d4401a4/tools/metrics/histograms/enums.xml

Comment 8 by vabr@chromium.org, Mar 15 2018

Status: Fixed (was: Started)
►

Sign in to add a comment