Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740823003:
[PasswordGeneration] Makes generated passwords copyable (Closed)
Description[PasswordGeneration] Makes password fields with revealed password copyable
If there is the confirmation field, but we filled the wrong field or hidden field, the user should be able to copy the generated password.
* Dragging passwords are not allowed at this point. PasswordGenerationAgent should listen for dragging events. Now it listens only for text changes. Let's see if dragging password is needed.
* Copying DHTML are not allowed. Passwords is just text.
BUG=632617
Review-Url: https://codereview.chromium.org/2740823003
Cr-Commit-Position: refs/heads/master@{#456719}
Committed: https://chromium.googlesource.com/chromium/src/+/2f5e4fd1fa6edfe86f3b01da639255a937c5cd20
Patch Set 1 #
Total comments: 4
Patch Set 2 : Moved IsInPasswordField to Editor.cpp #Patch Set 3 : Added a test #
Total comments: 4
Patch Set 4 : Changes in the test addressed to reviewer comments #
Messages
Total messages: 28 (19 generated)
Description was changed from ========== [PasswordGeneration] Makes password fields with revealed password copyable If there is the confirmation field, but we filled the wrong field or hidden field, the user should be able to copy the generated password. * Cutting passwords are not allowed. It is hardly a use case of password generation. * Dragging passwords are not allowed at this point. PasswordGenerationAgent should listen for dragging events. Now it listens only for text changes. Let's see dragging password is needed. * Copying DHTML are not allowed. Passwords is just text. BUG=632617 ========== to ========== [PasswordGeneration] Makes password fields with revealed password copyable If there is the confirmation field, but we filled the wrong field or hidden field, the user should be able to copy the generated password. * Cutting passwords are not allowed. It is hardly a use case of password generation. * Dragging passwords are not allowed at this point. PasswordGenerationAgent should listen for dragging events. Now it listens only for text changes. Let's see if dragging password is needed. * Copying DHTML are not allowed. Passwords is just text. BUG=632617 ==========
kolos@chromium.org changed reviewers: + dvadym@chromium.org
Description was changed from ========== [PasswordGeneration] Makes password fields with revealed password copyable If there is the confirmation field, but we filled the wrong field or hidden field, the user should be able to copy the generated password. * Cutting passwords are not allowed. It is hardly a use case of password generation. * Dragging passwords are not allowed at this point. PasswordGenerationAgent should listen for dragging events. Now it listens only for text changes. Let's see if dragging password is needed. * Copying DHTML are not allowed. Passwords is just text. BUG=632617 ========== to ========== [PasswordGeneration] Makes password fields with revealed password copyable If there is the confirmation field, but we filled the wrong field or hidden field, the user should be able to copy the generated password. * Dragging passwords are not allowed at this point. PasswordGenerationAgent should listen for dragging events. Now it listens only for text changes. Let's see if dragging password is needed. * Copying DHTML are not allowed. Passwords is just text. BUG=632617 ==========
kolos@chromium.org changed reviewers: + dominicc@chromium.org, yosin@chromium.org
Hi Dominic and Yoshifumi! Please review this CL. We decided to support only regular copying, no drag&drop and no DHTML stuff. Regards, Maxim
The CQ bit was checked by yosin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
- Please add a test for Editor::canCopy() to avoid regression in future. - Could you line wrap description in 80 characters to help people see "git log" in console? https://codereview.chromium.org/2740823003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2740823003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2084: bool isInPasswordFieldWithUnrevealedPassword(const Position& position) { Could you implement this function in Editor.cpp as file local function? At this time, we don't expect to use this function in other places. https://codereview.chromium.org/2740823003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2740823003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:328: !isInPasswordFieldWithUnrevealedPassword( Could you add test in "EditorTest.cpp" for Editor::canCopy() change?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for comments Yoshifumi. Please review changes. Regards, Maxim https://codereview.chromium.org/2740823003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2740823003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2084: bool isInPasswordFieldWithUnrevealedPassword(const Position& position) { On 2017/03/10 03:48:52, yosin_UTC9 wrote: > Could you implement this function in Editor.cpp as file local function? > At this time, we don't expect to use this function in other places. Done. https://codereview.chromium.org/2740823003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2740823003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:328: !isInPasswordFieldWithUnrevealedPassword( On 2017/03/10 03:48:52, yosin_UTC9 wrote: > Could you add test in "EditorTest.cpp" for Editor::canCopy() change? Done.
The CQ bit was checked by kolos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2740823003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditorTest.cpp (right): https://codereview.chromium.org/2740823003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditorTest.cpp:70: Node* node = document().body()->firstChild(); L70 to L71 should be HTMLInputElement& element = toHTMLInputElement(*document.getElementById("password")); Getting node from id is easier to reader rather than think structure, e.g. body->firstChild() https://codereview.chromium.org/2740823003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditorTest.cpp:74: String passwordValue = "secret"; nit: s/String passwordValue/const String kPasswordValue/
https://codereview.chromium.org/2740823003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditorTest.cpp (right): https://codereview.chromium.org/2740823003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditorTest.cpp:70: Node* node = document().body()->firstChild(); On 2017/03/13 07:35:41, yosin_UTC9 wrote: > L70 to L71 should be > > HTMLInputElement& element = > toHTMLInputElement(*document.getElementById("password")); > > Getting node from id is easier to reader rather than think structure, e.g. > body->firstChild() Done. https://codereview.chromium.org/2740823003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditorTest.cpp:74: String passwordValue = "secret"; On 2017/03/13 07:35:41, yosin_UTC9 wrote: > nit: s/String passwordValue/const String kPasswordValue/ Done.
lgtm
The CQ bit was checked by kolos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kolos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489507334919610,
"parent_rev": "051f03439e4cd21d7eaf0d776fcbf67a50e90f9f", "commit_rev":
"2f5e4fd1fa6edfe86f3b01da639255a937c5cd20"}
Message was sent while issue was closed.
Description was changed from ========== [PasswordGeneration] Makes password fields with revealed password copyable If there is the confirmation field, but we filled the wrong field or hidden field, the user should be able to copy the generated password. * Dragging passwords are not allowed at this point. PasswordGenerationAgent should listen for dragging events. Now it listens only for text changes. Let's see if dragging password is needed. * Copying DHTML are not allowed. Passwords is just text. BUG=632617 ========== to ========== [PasswordGeneration] Makes password fields with revealed password copyable If there is the confirmation field, but we filled the wrong field or hidden field, the user should be able to copy the generated password. * Dragging passwords are not allowed at this point. PasswordGenerationAgent should listen for dragging events. Now it listens only for text changes. Let's see if dragging password is needed. * Copying DHTML are not allowed. Passwords is just text. BUG=632617 Review-Url: https://codereview.chromium.org/2740823003 Cr-Commit-Position: refs/heads/master@{#456719} Committed: https://chromium.googlesource.com/chromium/src/+/2f5e4fd1fa6edfe86f3b01da6392... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/2f5e4fd1fa6edfe86f3b01da6392... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
