Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2188623007:
Fix DCHECK when opening two password bubbles one after another. (Closed)
|
Created:
4 years, 4 months ago by vasilii Modified:
4 years, 4 months ago Reviewers:
vabr (Chromium) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix DCHECK when opening two password bubbles one after another.
The test reproduced this by bringing the save bubble after the autosignin. Changing the state forced the bubble to close. However, it fades out and closes later. The second attempt to close was ignored and DCHECK fired.
BUG=626759
Committed: https://crrev.com/f79d8967f4c6bc91dfb93ffd4f7972a2e39fd05e
Cr-Commit-Position: refs/heads/master@{#408438}
Patch Set 1 #
Total comments: 3
Patch Set 2 : overload #
Messages
Total messages: 21 (10 generated)
The CQ bit was checked by vasilii@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...
vasilii@chromium.org changed reviewers: + vabr@chromium.org
Hi Vaclav, please review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LGTM with a question. Cheers, Vaclav https://codereview.chromium.org/2188623007/diff/1/chrome/browser/ui/cocoa/pas... File chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm (right): https://codereview.chromium.org/2188623007/diff/1/chrome/browser/ui/cocoa/pas... chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm:109: bubble_->Close(true); If this is the only callsite, then why don't you hard-code the argument value?
https://codereview.chromium.org/2188623007/diff/1/chrome/browser/ui/cocoa/pas... File chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm (right): https://codereview.chromium.org/2188623007/diff/1/chrome/browser/ui/cocoa/pas... chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm:109: bubble_->Close(true); On 2016/07/28 16:59:12, vabr (Chromium) wrote: > If this is the only callsite, then why don't you hard-code the argument value? It's not. There is also ManagePasswordsDecoration::HideBubble.
https://codereview.chromium.org/2188623007/diff/1/chrome/browser/ui/cocoa/pas... File chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm (right): https://codereview.chromium.org/2188623007/diff/1/chrome/browser/ui/cocoa/pas... chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm:109: bubble_->Close(true); On 2016/07/28 17:11:37, vasilii wrote: > On 2016/07/28 16:59:12, vabr (Chromium) wrote: > > If this is the only callsite, then why don't you hard-code the argument value? > > It's not. There is also ManagePasswordsDecoration::HideBubble. But that one contains a Close call without arguments. How does that work?
On 2016/07/28 17:14:17, vabr (Chromium) wrote: > https://codereview.chromium.org/2188623007/diff/1/chrome/browser/ui/cocoa/pas... > File chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm (right): > > https://codereview.chromium.org/2188623007/diff/1/chrome/browser/ui/cocoa/pas... > chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm:109: > bubble_->Close(true); > On 2016/07/28 17:11:37, vasilii wrote: > > On 2016/07/28 16:59:12, vabr (Chromium) wrote: > > > If this is the only callsite, then why don't you hard-code the argument > value? > > > > It's not. There is also ManagePasswordsDecoration::HideBubble. > > But that one contains a Close call without arguments. How does that work? default argument.
On 2016/07/28 17:15:40, vasilii wrote: > On 2016/07/28 17:14:17, vabr (Chromium) wrote: > > > https://codereview.chromium.org/2188623007/diff/1/chrome/browser/ui/cocoa/pas... > > File chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm (right): > > > > > https://codereview.chromium.org/2188623007/diff/1/chrome/browser/ui/cocoa/pas... > > chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm:109: > > bubble_->Close(true); > > On 2016/07/28 17:11:37, vasilii wrote: > > > On 2016/07/28 16:59:12, vabr (Chromium) wrote: > > > > If this is the only callsite, then why don't you hard-code the argument > > value? > > > > > > It's not. There is also ManagePasswordsDecoration::HideBubble. > > > > But that one contains a Close call without arguments. How does that work? > > default argument. Ah, I see. I'm not sure if this is best for readability. The style guide mentions a single reason to use default arguments over overloaded functions, implying to use default arguments if they "can improve the readability of their function declarations". Compared to declaring two versions of Close I don't see a gain in readability, quite the opposite, give that it's easy to overlook the default value. So my optional suggestion is to overload Close and let Close() call Close(false). If you disagree, I'm fine with default argument, because it is rather subjective and not very important. Cheers, Vaclav
The CQ bit was checked by vasilii@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...
On 2016/07/28 17:23:18, vabr (Chromium) wrote: > On 2016/07/28 17:15:40, vasilii wrote: > > On 2016/07/28 17:14:17, vabr (Chromium) wrote: > > > > > > https://codereview.chromium.org/2188623007/diff/1/chrome/browser/ui/cocoa/pas... > > > File chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm (right): > > > > > > > > > https://codereview.chromium.org/2188623007/diff/1/chrome/browser/ui/cocoa/pas... > > > chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm:109: > > > bubble_->Close(true); > > > On 2016/07/28 17:11:37, vasilii wrote: > > > > On 2016/07/28 16:59:12, vabr (Chromium) wrote: > > > > > If this is the only callsite, then why don't you hard-code the argument > > > value? > > > > > > > > It's not. There is also ManagePasswordsDecoration::HideBubble. > > > > > > But that one contains a Close call without arguments. How does that work? > > > > default argument. > > Ah, I see. I'm not sure if this is best for readability. The style guide > mentions a single reason to use default arguments over overloaded functions, > implying to use default arguments if they "can improve the readability of their > function declarations". Compared to declaring two versions of Close I don't see > a gain in readability, quite the opposite, give that it's easy to overlook the > default value. > > So my optional suggestion is to overload Close and let Close() call > Close(false). If you disagree, I'm fine with default argument, because it is > rather subjective and not very important. > > Cheers, > Vaclav Done.
Thanks, LGTM.
The CQ bit was unchecked by vasilii@chromium.org
The CQ bit was checked by vasilii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix DCHECK when opening two password bubbles one after another. The test reproduced this by bringing the save bubble after the autosignin. Changing the state forced the bubble to close. However, it fades out and closes later. The second attempt to close was ignored and DCHECK fired. BUG=626759 ========== to ========== Fix DCHECK when opening two password bubbles one after another. The test reproduced this by bringing the save bubble after the autosignin. Changing the state forced the bubble to close. However, it fades out and closes later. The second attempt to close was ignored and DCHECK fired. BUG=626759 Committed: https://crrev.com/f79d8967f4c6bc91dfb93ffd4f7972a2e39fd05e Cr-Commit-Position: refs/heads/master@{#408438} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f79d8967f4c6bc91dfb93ffd4f7972a2e39fd05e Cr-Commit-Position: refs/heads/master@{#408438} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
