Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
904613006:
Autofill: First step toward CVC unmask prompt dialog on OSX. (Closed)
|
Created:
5 years, 10 months ago by bondd Modified:
5 years, 10 months ago CC:
chromium-reviews, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAutofill: First step toward CVC unmask prompt dialog on OSX.
Shows an empty dialog with only 'OK' and 'Cancel' buttons when the CVC
unmask prompt dialog is requested.
Based on https://crrev.com/13470023
BUG=448572
Committed: https://crrev.com/a27670eb86cb62e06a26c11903a619d014d110f4
Cr-Commit-Position: refs/heads/master@{#317959}
Patch Set 1 #Patch Set 2 : Remove about_flags.cc change to upload as a separate CL. #Patch Set 3 : Fix TODO. #
Total comments: 31
Patch Set 4 : Fix some groby@ comments. Before renaming classes. #Patch Set 5 : Rename classes. #Patch Set 6 : Get rid of GTMWidthBasedTweaker and card_unmask_prompt_view.cc. #Patch Set 7 : Add test. #
Total comments: 12
Patch Set 8 : Address groby@ comments for patch set 7. #
Total comments: 12
Patch Set 9 : Address groby@ comments for patch set 8. #
Total comments: 2
Patch Set 10 : Add CardUnmaskPromptViewTester and use it. #Patch Set 11 : Rename new files and classes to include word 'View'. #
Total comments: 6
Patch Set 12 : Address estade@ comments for patch set 11. #Patch Set 13 : Rebase. #Patch Set 14 : Rebase. #Patch Set 15 : Fix compile error in browser test. #
Total comments: 3
Patch Set 16 : Rename MockCardUnmaskDelegate -> TestCardUnmaskDelegate. #Patch Set 17 : Fix leak detected in browser test. #
Total comments: 6
Patch Set 18 : Remove view_tester files and don't return WeakPtrs. #Messages
Total messages: 44 (12 generated)
bondd@chromium.org changed reviewers: + estade@chromium.org, groby@chromium.org
bondd@chromium.org changed required reviewers: + groby@chromium.org
A few comments... Also, I can haz test, please? https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/card_unmask_prompt_view.cc (right): https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_view.cc:12: CardUnmaskPromptView::~CardUnmaskPromptView() { Why do these two (or the whole .cc file, really) exist? https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h (right): https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:49: <NSWindowDelegate> { That's odd formatting - git cl format? https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:52: autofill::CardUnmaskPromptCocoa* cardUnmaskPrompt_; // weak. I'd suggest calling it cardUnmaskPromptBridge_ since it bridges between the C++ and the ObjC side. https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:54: base::scoped_nsobject<GTMWidthBasedTweaker> buttonContainer_; Do you need a tweaker? https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:57: // Designated initializer. The WebContents cannot be NULL. |webContents| cannot... https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:59: cardUnmaskPrompt:(autofill::CardUnmaskPromptCocoa*)cardUnmaskPrompt; Align ":" https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:61: // Closes the sheet and ends the modal loop. This will also clean up the memory. Which memory? https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm (right): https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:19: const CGFloat kButtonGap = 6; nit: 6.0f - it's a float :) https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:59: // |this| belongs to |controller_|, so no self-destruction here. You can probably skip this comment https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:65: constrained_window_->CloseWebContentsModalDialog(); This seems odd - CloseWebContentsModalDialog should trigger OnConstrainedWindowClosed, which will call OnUnmaskDialogClosed anyways. Why double-call? https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:96: if (buttonContainer_.get()) Is that called repeatedly? Otherwise, why check |buttonContainer_|? https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:100: ui::kWindowSizeDeterminedLater]); NSZeroRect is fine. ui::kWindowSizeDeterminedLater is specifically for windows. https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:134: base::scoped_nsobject<GTMUILocalizerAndLayoutTweaker> layoutTweaker( -buildButtons already lays things out - I don't think you need to tweak it again.
On 2015/02/06 03:06:19, groby wrote: > A few comments... Also, I can haz test, please? > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/autofi... > File chrome/browser/ui/autofill/card_unmask_prompt_view.cc (right): > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/autofi... > chrome/browser/ui/autofill/card_unmask_prompt_view.cc:12: > CardUnmaskPromptView::~CardUnmaskPromptView() { > Why do these two (or the whole .cc file, really) exist? > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h (right): > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:49: > <NSWindowDelegate> { > That's odd formatting - git cl format? > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:52: > autofill::CardUnmaskPromptCocoa* cardUnmaskPrompt_; // weak. > I'd suggest calling it cardUnmaskPromptBridge_ since it bridges between the C++ > and the ObjC side. > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:54: > base::scoped_nsobject<GTMWidthBasedTweaker> buttonContainer_; > Do you need a tweaker? > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:57: // Designated > initializer. The WebContents cannot be NULL. > |webContents| cannot... > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:59: > cardUnmaskPrompt:(autofill::CardUnmaskPromptCocoa*)cardUnmaskPrompt; > Align ":" > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:61: // Closes the > sheet and ends the modal loop. This will also clean up the memory. > Which memory? > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm (right): > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:19: const CGFloat > kButtonGap = 6; > nit: 6.0f - it's a float :) > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:59: // |this| > belongs to |controller_|, so no self-destruction here. > You can probably skip this comment > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:65: > constrained_window_->CloseWebContentsModalDialog(); > This seems odd - CloseWebContentsModalDialog should trigger > OnConstrainedWindowClosed, which will call OnUnmaskDialogClosed anyways. Why > double-call? > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:96: if > (buttonContainer_.get()) > Is that called repeatedly? Otherwise, why check |buttonContainer_|? > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:100: > ui::kWindowSizeDeterminedLater]); > NSZeroRect is fine. ui::kWindowSizeDeterminedLater is specifically for windows. > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:134: > base::scoped_nsobject<GTMUILocalizerAndLayoutTweaker> layoutTweaker( > -buildButtons already lays things out - I don't think you need to tweak it > again. re: tests --- I've been holding off on writing these because a) the feature isn't launched, so we can't regress (yet) b) the UI is still in flux, and writing and rewriting tests would be a waste of time
I addressed most reviewer comments in patch set #4, then renamed classes in patch set #5. Renamed CardUnmaskPromptCocoa -> CardUnmaskPromptBridge as suggested. Then renamed CardUnmaskPromptWindowController -> CardUnmaskPromptCocoa. This follows the naming convention in c/b/ui/cocoa/autofill/password_generation_popup_view_*, which AFAICT was done recently and makes sense. I'd like to wait until a future CL to split CardUnmaskPromptCocoa into a separate file. This is how it was done in the CL I am loosely basing this on: https://crrev.com/13470023 https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/card_unmask_prompt_view.cc (right): https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_view.cc:12: CardUnmaskPromptView::~CardUnmaskPromptView() { On 2015/02/06 03:06:18, groby wrote: > Why do these two (or the whole .cc file, really) exist? @estade: Is it okay with you if I get rid of this .cc file and put empty definitions of these two in the .h file? https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h (right): https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:49: <NSWindowDelegate> { On 2015/02/06 03:06:18, groby wrote: > That's odd formatting - git cl format? Done. Applied 'git cl format' to entire CL. https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:52: autofill::CardUnmaskPromptCocoa* cardUnmaskPrompt_; // weak. On 2015/02/06 03:06:18, groby wrote: > I'd suggest calling it cardUnmaskPromptBridge_ since it bridges between the C++ > and the ObjC side. Done. https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:54: base::scoped_nsobject<GTMWidthBasedTweaker> buttonContainer_; On 2015/02/06 03:06:18, groby wrote: > Do you need a tweaker? This is the same way AutofillMainContainer does it: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... From what I can tell this will be used to tweak button sizes for l10n. If that's the case I'd like to leave it like this for now, instead of changing to a different container only to switch back later. If I misunderstand or you'd still like a different class than GTMWidthBasedTweaker used here, please let me know which class you'd prefer. Thanks! :) https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:57: // Designated initializer. The WebContents cannot be NULL. On 2015/02/06 03:06:18, groby wrote: > |webContents| cannot... Done. https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:59: cardUnmaskPrompt:(autofill::CardUnmaskPromptCocoa*)cardUnmaskPrompt; On 2015/02/06 03:06:18, groby wrote: > Align ":" Done. https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:61: // Closes the sheet and ends the modal loop. This will also clean up the memory. On 2015/02/06 03:06:18, groby wrote: > Which memory? Done. https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm (right): https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:19: const CGFloat kButtonGap = 6; On 2015/02/06 03:06:19, groby wrote: > nit: 6.0f - it's a float :) Done. https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:59: // |this| belongs to |controller_|, so no self-destruction here. On 2015/02/06 03:06:19, groby wrote: > You can probably skip this comment Done. https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:65: constrained_window_->CloseWebContentsModalDialog(); On 2015/02/06 03:06:19, groby wrote: > This seems odd - CloseWebContentsModalDialog should trigger > OnConstrainedWindowClosed, which will call OnUnmaskDialogClosed anyways. Why > double-call? Done. https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:96: if (buttonContainer_.get()) On 2015/02/06 03:06:19, groby wrote: > Is that called repeatedly? Otherwise, why check |buttonContainer_|? Done. https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:100: ui::kWindowSizeDeterminedLater]); On 2015/02/06 03:06:19, groby wrote: > NSZeroRect is fine. ui::kWindowSizeDeterminedLater is specifically for windows. Done. https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:134: base::scoped_nsobject<GTMUILocalizerAndLayoutTweaker> layoutTweaker( On 2015/02/06 03:06:19, groby wrote: > -buildButtons already lays things out - I don't think you need to tweak it > again. Done.
https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/card_unmask_prompt_view.cc (right): https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_view.cc:12: CardUnmaskPromptView::~CardUnmaskPromptView() { On 2015/02/09 19:21:00, bondd wrote: > On 2015/02/06 03:06:18, groby wrote: > > Why do these two (or the whole .cc file, really) exist? > > @estade: Is it okay with you if I get rid of this .cc file and put empty > definitions of these two in the .h file? I think presubmit checks might complain with something like "non-trivial class requires explicit out of line constructor/destructor" or some such. But if not, sure.
re: tests I'm fine with not testing functionality in excruciating detail - since, as you said, it's not stable yet - but I'd still like to see a browsertest just to pop up and hide the dialog. OSX reference counting is a tricky beast, and having that test on memory/memory.fyi flushes out a lot of issues.
https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h (right): https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:54: base::scoped_nsobject<GTMWidthBasedTweaker> buttonContainer_; On 2015/02/09 19:21:00, bondd wrote: > On 2015/02/06 03:06:18, groby wrote: > > Do you need a tweaker? > > This is the same way AutofillMainContainer does it: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > From what I can tell this will be used to tweak button sizes for l10n. If that's > the case I'd like to leave it like this for now, instead of changing to a > different container only to switch back later. If I misunderstand or you'd still > like a different class than GTMWidthBasedTweaker used here, please let me know > which class you'd prefer. Thanks! :) It is, but by now I consider it overkill for just two buttons :) I'll not scream bloody murder for leaving it in, though. (This stuff usually makes more sense for complex layouts, see: https://code.google.com/p/google-toolbox-for-mac/wiki/UILocalization) https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm (right): https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:115: [button setFrameOrigin:NSMakePoint(nextX, 0)]; That's what I meant when talking about the tweaker - you already manually position your buttons, so _technically_ the tweaker should do nothing. You should be able to replace it with an NSView and be fine.
Addressed open comments, except for lack of test. I'm off to write a simple test now. You can review this now if you like, or wait until the test arrives and do it all at once. Makes no difference to me. :) https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h (right): https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.h:54: base::scoped_nsobject<GTMWidthBasedTweaker> buttonContainer_; On 2015/02/09 20:25:02, groby wrote: > On 2015/02/09 19:21:00, bondd wrote: > > On 2015/02/06 03:06:18, groby wrote: > > > Do you need a tweaker? > > > > This is the same way AutofillMainContainer does it: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > From what I can tell this will be used to tweak button sizes for l10n. If > that's > > the case I'd like to leave it like this for now, instead of changing to a > > different container only to switch back later. If I misunderstand or you'd > still > > like a different class than GTMWidthBasedTweaker used here, please let me know > > which class you'd prefer. Thanks! :) > > It is, but by now I consider it overkill for just two buttons :) I'll not scream > bloody murder for leaving it in, though. > > (This stuff usually makes more sense for complex layouts, see: > https://code.google.com/p/google-toolbox-for-mac/wiki/UILocalization) Done. I understand what you mean now. Changed it to an NSView. https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm (right): https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa.mm:115: [button setFrameOrigin:NSMakePoint(nextX, 0)]; On 2015/02/09 20:25:02, groby wrote: > That's what I meant when talking about the tweaker - you already manually > position your buttons, so _technically_ the tweaker should do nothing. You > should be able to replace it with an NSView and be fine. Done.
On 2015/02/09 19:30:22, Evan Stade wrote: > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/autofi... > File chrome/browser/ui/autofill/card_unmask_prompt_view.cc (right): > > https://codereview.chromium.org/904613006/diff/40001/chrome/browser/ui/autofi... > chrome/browser/ui/autofill/card_unmask_prompt_view.cc:12: > CardUnmaskPromptView::~CardUnmaskPromptView() { > On 2015/02/09 19:21:00, bondd wrote: > > On 2015/02/06 03:06:18, groby wrote: > > > Why do these two (or the whole .cc file, really) exist? > > > > @estade: Is it okay with you if I get rid of this .cc file and put empty > > definitions of these two in the .h file? > > I think presubmit checks might complain with something like "non-trivial class > requires explicit out of line constructor/destructor" or some such. But if not, > sure. Done. Presubmit didn't complain.
Patchset #7 (id:120001) has been deleted
Added test, based on autofill_dialog_cococa_browsertest.mm. The addition of CancelForTesting() is from a previous revision of the file in https://crrev.com/13470023 (the old CL that this current CL is based on). Previous patch set #6 addresses other reviewer comments.
https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h (right): https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h:50: content::WebContents* webContents_; // weak. nit: No need to label as weak (if it weren't, it should be a scoped_ptr<>) nit2: *if* you add a comment, 2 spaces after ; and don't bother aligning. https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h:51: autofill::CardUnmaskPromptBridge* cardUnmaskPromptBridge_; // weak. You might want to add the comment that it owns |self| https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h:56: // Designated initializer. |webContents| cannot be NULL. nit: must not :) https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.mm (right): https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.mm:97: [buttonContainer_ setAutoresizingMask:(NSViewMinXMargin | NSViewMinYMargin)]; N.B: You only need autoresizingmask if you plan to resize this at a later point https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa_browsertest.mm (right): https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa_browsertest.mm:20: class TestCardUnmaskPromptController : public CardUnmaskPromptControllerImpl, Can this be shared with Views tests? https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa_browsertest.mm:85: scoped_refptr<content::MessageLoopRunner> runner_; If it must outlive the controller, it might be better to declare it before controller_ (deletion happens in reverse declaration order) I know that currently the test controller also holds a reference, but that's an implementation detail we shouldn't rely on.
https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h (right): https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h:50: content::WebContents* webContents_; // weak. On 2015/02/12 00:51:55, groby wrote: > nit: No need to label as weak (if it weren't, it should be a scoped_ptr<>) > nit2: *if* you add a comment, 2 spaces after ; and don't bother aligning. Done. https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h:51: autofill::CardUnmaskPromptBridge* cardUnmaskPromptBridge_; // weak. On 2015/02/12 00:51:55, groby wrote: > You might want to add the comment that it owns |self| Done. https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h:56: // Designated initializer. |webContents| cannot be NULL. On 2015/02/12 00:51:55, groby wrote: > nit: must not :) Done. https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.mm (right): https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.mm:97: [buttonContainer_ setAutoresizingMask:(NSViewMinXMargin | NSViewMinYMargin)]; On 2015/02/12 00:51:55, groby wrote: > N.B: You only need autoresizingmask if you plan to resize this at a later point Done. https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa_browsertest.mm (right): https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa_browsertest.mm:20: class TestCardUnmaskPromptController : public CardUnmaskPromptControllerImpl, On 2015/02/12 00:51:55, groby wrote: > Can this be shared with Views tests? Yes I think it can. I was planning to do that as a separate CL. Is that okay by you? https://codereview.chromium.org/904613006/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa_browsertest.mm:85: scoped_refptr<content::MessageLoopRunner> runner_; On 2015/02/12 00:51:56, groby wrote: > If it must outlive the controller, it might be better to declare it before > controller_ (deletion happens in reverse declaration order) > > I know that currently the test controller also holds a reference, but that's an > implementation detail we shouldn't rely on. Done. Thanks!
c/b/ui/cocoa LGTM w/nits https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h (right): https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h:51: // Owns |self|. Personal nit: I like empty lines in front of comments - but your choice. https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h:54: base::scoped_nsobject<NSView> buttonContainer_; No need to keep a reference to the container here - it's retained when it's added to the contentView. https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h:57: // Designated initializer. |webContents| must not be NULL. Can |bridge| be NULL? https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.mm (right): https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.mm:17: Kill a newline, save a whitespace;) (Here and at end). There's only one line in there, it's awfully empty around it. https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.mm:71: #pragma mark CardUnmaskPromptCocoa Only #pragma mark if it actually helps you editing, btw. And if you do, I'd #mark the CardUnmaskPromptBridge as well. https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa_browsertest.mm (right): https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa_browsertest.mm:46: DCHECK(runner_); If you want to DCHECK at all, I'd say the ctor is enough
New patchsets have been uploaded after l-g-t-m from groby@chromium.org
bondd@chromium.org changed required reviewers: + estade@chromium.org
estade@: I added you as a required reviewer. groby@ reviewed the c/b/ui/cocoa stuff. PTAL at the rest. Thanks. https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h (right): https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h:51: // Owns |self|. On 2015/02/13 01:53:40, groby wrote: > Personal nit: I like empty lines in front of comments - but your choice. Done. https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h:54: base::scoped_nsobject<NSView> buttonContainer_; On 2015/02/13 01:53:40, groby wrote: > No need to keep a reference to the container here - it's retained when it's > added to the contentView. Done. https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.h:57: // Designated initializer. |webContents| must not be NULL. On 2015/02/13 01:53:40, groby wrote: > Can |bridge| be NULL? Done. https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.mm (right): https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.mm:17: On 2015/02/13 01:53:40, groby wrote: > Kill a newline, save a whitespace;) (Here and at end). There's only one line in > there, it's awfully empty around it. Done. https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_bridge.mm:71: #pragma mark CardUnmaskPromptCocoa On 2015/02/13 01:53:40, groby wrote: > Only #pragma mark if it actually helps you editing, btw. And if you do, I'd > #mark the CardUnmaskPromptBridge as well. Done. Added #pragma CardUnmaskPromptBridge as well. https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa_browsertest.mm (right): https://codereview.chromium.org/904613006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_cocoa_browsertest.mm:46: DCHECK(runner_); On 2015/02/13 01:53:40, groby wrote: > If you want to DCHECK at all, I'd say the ctor is enough Done. Removed DCHECKs.
https://codereview.chromium.org/904613006/diff/180001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_view.h (right): https://codereview.chromium.org/904613006/diff/180001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view.h:25: virtual void CancelForTesting() = 0; instead of adding this here, create another interface for testing in the mold of AutofillDialogViewTester
Patchset #10 (id:200001) has been deleted
Patch set #10: Test via CardUnmaskPromptViewTester, per estade@ request. Patch set #11: Renamed: CardUnmaskPromptBridge -> CardUnmaskPromptViewBridge CardUnmaskPromptCocoa -> CardUnmaskPromptViewCocoa CardUnmaskPromptCocoaBrowserTest -> CardUnmaskPromptViewCocoaBrowserTest The renaming is the only change in patch set #11. I added 'View' to the class names to be less confusing and to be more consistent with PasswordGenerationPopupView*. IMO it was confusing to drop the View from the class names when it was present in the parent class name. If you'd like me to upload the rename as a separate CL just let me know. I figured since the files don't exist in master yet it makes sense to do the rename before the first commit, so there is less renaming in the file history. https://codereview.chromium.org/904613006/diff/180001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_view.h (right): https://codereview.chromium.org/904613006/diff/180001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view.h:25: virtual void CancelForTesting() = 0; On 2015/02/13 03:27:05, Evan Stade wrote: > instead of adding this here, create another interface for testing in the mold of > AutofillDialogViewTester Done.
https://codereview.chromium.org/904613006/diff/240001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_cocoa_browsertest.mm (right): https://codereview.chromium.org/904613006/diff/240001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_cocoa_browsertest.mm:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. what makes this test file cocoa specific? https://codereview.chromium.org/904613006/diff/240001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_cocoa_browsertest.mm:10: #import "chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h" is this used?
Patchset #12 (id:260001) has been deleted
https://codereview.chromium.org/904613006/diff/240001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_cocoa_browsertest.mm (right): https://codereview.chromium.org/904613006/diff/240001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_cocoa_browsertest.mm:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/02/17 20:44:09, Evan Stade wrote: > what makes this test file cocoa specific? Done. Renamed it to be platform independent. Updated chrome_tests.gypi so this test only compiles for OSX. I'd like to make it work for other platforms in a later CL. I looked at making it work for all platforms as part of this CL and it's not completely trivial. IMO better to do it separately. https://codereview.chromium.org/904613006/diff/240001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_cocoa_browsertest.mm:10: #import "chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h" On 2015/02/17 20:44:09, Evan Stade wrote: > is this used? Nope! Done. Probably left over from a previous patch set.
lgtm https://codereview.chromium.org/904613006/diff/240001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_cocoa_browsertest.mm (right): https://codereview.chromium.org/904613006/diff/240001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_cocoa_browsertest.mm:10: #import "chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h" On 2015/02/17 23:45:11, bondd wrote: > On 2015/02/17 20:44:09, Evan Stade wrote: > > is this used? > > Nope! Done. Probably left over from a previous patch set. agreed
https://codereview.chromium.org/904613006/diff/240001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_cocoa_browsertest.mm (right): https://codereview.chromium.org/904613006/diff/240001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_cocoa_browsertest.mm:10: #import "chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h" On 2015/02/18 00:39:38, Evan Stade wrote: > On 2015/02/17 23:45:11, bondd wrote: > > On 2015/02/17 20:44:09, Evan Stade wrote: > > > is this used? > > > > Nope! Done. Probably left over from a previous patch set. > > agreed oops, meant to say "agreed" to the other comment (about bringing up tests on other platforms)
New patchsets have been uploaded after l-g-t-m from estade@chromium.org
The CQ bit was checked by bondd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/904613006/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
estade@: PTAL at patch set 15. Fixed a compile error that showed up after rebasing. https://codereview.chromium.org/904613006/diff/340001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/904613006/diff/340001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:20: class MockCardUnmaskDelegate : public CardUnmaskDelegate { With updated code there were compile errors on OSX browser_tests. See http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_... CardUnmaskPromptController and CardUnmaskDelegate both have methods named OnUnmaskResponse, with different arguments. TestCardUnmaskPromptController was inheriting from both of them, causing a compile error because both base classes had a method with the same name. Rather than making that work I split the CardUnmaskDelegate inheritance out into MockCardUnmaskDelegate. IMO this is better, because in normal usage it doesn't make sense for a class to inherit from both CardUnmaskPromptController and CardUnmaskDelegate (the object would be notifying itself of what it's doing).
https://codereview.chromium.org/904613006/diff/340001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/904613006/diff/340001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:20: class MockCardUnmaskDelegate : public CardUnmaskDelegate { On 2015/02/19 00:17:20, bondd wrote: > With updated code there were compile errors on OSX browser_tests. > See > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_... > > CardUnmaskPromptController and CardUnmaskDelegate both have methods named > OnUnmaskResponse, with different arguments. TestCardUnmaskPromptController was > inheriting from both of them, causing a compile error because both base classes > had a method with the same name. > Rather than making that work I split the CardUnmaskDelegate inheritance out into > MockCardUnmaskDelegate. IMO this is better, because in normal usage it doesn't > make sense for a class to inherit from both CardUnmaskPromptController and > CardUnmaskDelegate (the object would be notifying itself of what it's doing). yes, agreed. But you shouldn't call this MockCardUnmaskDelegate because it's not a gmock. Either call it TestCardUnmaskDelegate or make it actually a mock (override methods via MOCK_METHOD)
Renamed MockCardUnmaskDelegate -> TestCardUnmaskDelegate as requested. https://codereview.chromium.org/904613006/diff/340001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/904613006/diff/340001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:20: class MockCardUnmaskDelegate : public CardUnmaskDelegate { On 2015/02/19 00:52:13, Evan Stade wrote: > On 2015/02/19 00:17:20, bondd wrote: > > With updated code there were compile errors on OSX browser_tests. > > See > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_... > > > > CardUnmaskPromptController and CardUnmaskDelegate both have methods named > > OnUnmaskResponse, with different arguments. TestCardUnmaskPromptController was > > inheriting from both of them, causing a compile error because both base > classes > > had a method with the same name. > > Rather than making that work I split the CardUnmaskDelegate inheritance out > into > > MockCardUnmaskDelegate. IMO this is better, because in normal usage it doesn't > > make sense for a class to inherit from both CardUnmaskPromptController and > > CardUnmaskDelegate (the object would be notifying itself of what it's doing). > > yes, agreed. But you shouldn't call this MockCardUnmaskDelegate because it's not > a gmock. Either call it TestCardUnmaskDelegate or make it actually a mock > (override methods via MOCK_METHOD) Done.
Already approved, but PTAL again. Fixes test-only leak detected by LeakTracker. This patch set no longer requires new class CardUnmaskPromptViewTester: it was added to provide CancelForTesting(), which is no longer used. I can now delete: card_unmask_prompt_view_tester.h card_unmask_prompt_view_tester_cocoa.h card_unmask_prompt_view_tester_cocoa.mm Let me know if you want me to leave those around, otherwise I will remove them from this CL. https://codereview.chromium.org/904613006/diff/380001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.h (right): https://codereview.chromium.org/904613006/diff/380001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.h:21: virtual ~CardUnmaskPromptControllerImpl(); This is subclassed by TestCardUnmaskPromptController so destructor should be virtual now. https://codereview.chromium.org/904613006/diff/380001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/904613006/diff/380001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:24: virtual ~TestCardUnmaskDelegate() {} scoped_ptr complains if destructor is non-virtual. https://codereview.chromium.org/904613006/diff/380001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:98: } Made these WeakPtrs to automatically detect if stale pointer is used. https://codereview.chromium.org/904613006/diff/380001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:104: scoped_ptr<TestCardUnmaskPromptController> controller_; Comment "The controller owns itself" was wrong; fixed controller_ ownership by making it a scoped_ptr.
I'd delete the viewstester files from this patchset, but save them for when you do need them. https://codereview.chromium.org/904613006/diff/380001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/904613006/diff/380001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:98: } On 2015/02/24 00:24:33, bondd wrote: > Made these WeakPtrs to automatically detect if stale pointer is used. not sure I follow, the lifetime of |controller_| and |delegate_| is bound to |this|. How could delegate() be stale? If delegate_ is null, what does delegate_->GetWeakPtr() give you?
Deleted view_tester files, and got rid of returning WeakPtrs. https://codereview.chromium.org/904613006/diff/380001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/904613006/diff/380001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:98: } On 2015/02/24 22:19:22, Evan Stade wrote: > On 2015/02/24 00:24:33, bondd wrote: > > Made these WeakPtrs to automatically detect if stale pointer is used. > > not sure I follow, the lifetime of |controller_| and |delegate_| is bound to > |this|. How could delegate() be stale? If delegate_ is null, what does > delegate_->GetWeakPtr() give you? Yep you're right. Fixed.
lgtm
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org Link to the patchset: https://codereview.chromium.org/904613006/#ps400001 (title: "Remove view_tester files and don't return WeakPtrs.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/904613006/400001
Message was sent while issue was closed.
Committed patchset #18 (id:400001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/a27670eb86cb62e06a26c11903a619d014d110f4 Cr-Commit-Position: refs/heads/master@{#317959} |
