Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
51933002:
[rAC, OSX] Autofill popup should be shown on 2nd click. (Closed)
|
Created:
7 years, 1 month ago by groby-ooo-7-16 Modified:
7 years, 1 month ago CC:
chromium-reviews, benquan, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[rAC, OSX] Autofill popup should be shown on 2nd click.
Desired behavior: Clicking on a field or giving it focus via tab should
result in the validation error message coming up, if any.
Clicking on an already focused field should toggle autofill suggestions
with validation messages.
Leaving an input field with either a validation message or an autofill
suggestion should hide both.
The tricky part: distinguishing mouse clicks that would give focus, and
mouse clicks that are on a focused field. Since mouseDown: messages in
OSX are only sent to fields that already have firstResponder status,
this requires a bit of trickery - this CL exploits the fact that for an
NSTextField, the first click goes to the NSTextField, and subsequent
ones go to the field editor.
Bonus trickery: Rapid clicking on an NSTextField prevents the field
editor from taking over until the user has calmed down, so the code
filters only the first click after becomeFirstResponder.
R=rsesek@chromium.org
BUG=308156
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232970
Patch Set 1 #
Total comments: 25
Patch Set 2 : Review fixes. #Patch Set 3 : More review fixes. #Patch Set 4 : Missing fixes. #
Total comments: 6
Patch Set 5 : More review fixes. #
Total comments: 2
Patch Set 6 : Review fix. #Patch Set 7 : Fix test helper. #Messages
Total messages: 25 (0 generated)
+shess for his knowledge of NSTextField and their editors. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:257: NSLog(@"FieldEditor mouseDown"); Remove https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:269: - (BOOL)becomeFirstResponder { Remove if it just calls super. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_input_field.h (right): https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_input_field.h:24: - (void)onMouseDown:(NSControl<AutofillInputField>*)sender; Comment. Not sure this name offers enough differentiation from -[NSResponder mouseDown:]. Should it?
rsesek: Addressed comments shess: PTAL https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:257: NSLog(@"FieldEditor mouseDown"); On 2013/10/30 14:44:31, rsesek wrote: > Remove Done. (I really should write a presubmit filter for that :) https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:269: - (BOOL)becomeFirstResponder { On 2013/10/30 14:44:31, rsesek wrote: > Remove if it just calls super. Done. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_input_field.h (right): https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_input_field.h:24: - (void)onMouseDown:(NSControl<AutofillInputField>*)sender; On 2013/10/30 14:44:31, rsesek wrote: > Comment. Not sure this name offers enough differentiation from -[NSResponder > mouseDown:]. Should it? Done. I don't think it should be much different, since it does reflect a mouseDown: up. I suppose I could go for the longer "receivedMouseDown:", but I don't think that adds clarity.
From the CL description, I'm not entirely clear I understand why the trickery is necessary. Seems like the one state is "when coming first responder" and the other state is "When someone clicks on the field editor" (having a field editor implies first responder). But what I can't remember without experimenting is whether -[NSTextField mouseDown:] forwards to the field editor's -mouseDown: after making it first responder? Note that if you're worried about multi-clicks, just check the -clickCount. Given that, you might be able to get away with an base::AutoReset BOOL on the order of inMouseDown to prevent forwarding of the editor's message, rather than needing a state machine. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:264: [[inputField delegate] onMouseDown:inputField]; IMHO, this feels like -onMouseDown: is inputField notifying of a -mouseDown: to itself. I think it should be -onEditorMouseDown: or -onFieldEditorMouseDown: or something like that. Actually, to match field-editor style, you'd have the field editor delegate something like: - (void)onMouseDown:(NSText*)sender; and the field itself would forward that message to something like: - (void)control:(NSControl*)control onMouseDown:(NSText*)editor; Reflecting off the field has the advantage that the field can suppress the handling-first-click directly, rather than having to expose that as part of the interface. [Remember, this is Objective-C. If your method names are not forcing you past the 80-column limit, you're doing it wrong.] https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:298: [[AutofillDialogFieldEditor alloc] initWithFrame:NSZeroRect]); -init should suffice for this. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:301: return fieldEditor_.get(); Is this actually worthwhile to cache? Hmm, I see the omnibox field editor is cached, I guess so. I suspect that it's not worth caching. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (right): https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:266: // Currently, only AutofillTextField generates onMouseDown: messages. Note that only NSTextField subclasses have field editors at all. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_textfield.mm (right): https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_textfield.mm:54: // editor instead. This is not quite true. Clicks to the field's border area will not go to the field editor, even though this area is not visually distinct.
I need to distinguish between "first click on field iff it made the field firstResponder" and "subsequent clicks on field". I.e. these three cases: * Click on field to make it firstResponder: Treat as first click * Tab to field to make it firstResponder, then click: Treat as subsequent click * Click on field to make it firstResponder, then click again: Treat as subsequent click. As for multi-clicks, I'm not worried about double-clicks, but about what I'd term "rage clicks" - the user repeatedly spamming the mouse button. I do get a separate mouseDown: for each of those. And, as you pointed out, clicks on the border area. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:264: [[inputField delegate] onMouseDown:inputField]; On 2013/10/30 17:57:23, shess wrote: > IMHO, this feels like -onMouseDown: is inputField notifying of a -mouseDown: to > itself. I think it should be -onEditorMouseDown: or -onFieldEditorMouseDown: or > something like that. > > Actually, to match field-editor style, you'd have the field editor delegate > something like: > - (void)onMouseDown:(NSText*)sender; > and the field itself would forward that message to something like: > - (void)control:(NSControl*)control onMouseDown:(NSText*)editor; > > Reflecting off the field has the advantage that the field can suppress the > handling-first-click directly, rather than having to expose that as part of the > interface. > > [Remember, this is Objective-C. If your method names are not forcing you past > the 80-column limit, you're doing it wrong.] To make sure I understand: You'd want me to call [inputField onMouseDown:] here, and then handle first click in the AutofillTextField? I've considered it, but the treatment of the first click is technically behavior enforced by the dialog, not the control. At the very least, I need to find a better name for the field delegate's method then, since I'd like it to reflect that it can swallow a mouseDown... onMouseDown:gaveFirstResponder: maybe? https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:301: return fieldEditor_.get(); If you don't cache it, your fieldEditor becomes completely stateless. IIRC, _any_ time a control uses an editor, it gets it via this method. Controls never cache it themselves. Furthermore, NSTextView is rather heavy, and this gets called quite often. (10's of time per second, for an otherwise idle dialog) On 2013/10/30 17:57:23, shess wrote: > Is this actually worthwhile to cache? Hmm, I see the omnibox field editor is > cached, I guess so. I suspect that it's not worth caching. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (right): https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:266: // Currently, only AutofillTextField generates onMouseDown: messages. On 2013/10/30 17:57:23, shess wrote: > Note that only NSTextField subclasses have field editors at all. Yes - other controls would need to generate that message from the control, as opposed to the editor. I've put that comment here to clarify that I don't expect other controls to invoke this delegate method. Yet. I've got to take what I have right now to the UX peeps to see what we do with PopUp treatment in general, but I don't want to rule out that they might opt in to some of this, too. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_textfield.mm (right): https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_textfield.mm:54: // editor instead. On 2013/10/30 17:57:23, shess wrote: > This is not quite true. Clicks to the field's border area will not go to the > field editor, even though this area is not visually distinct. Ah. Thankfully, this doesn't affect the code (border clicks also give focus, and also should toggle the different bubbles) but I updated the code accordingly.
PTAL https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:264: [[inputField delegate] onMouseDown:inputField]; On 2013/10/30 19:13:36, groby wrote: > On 2013/10/30 17:57:23, shess wrote: > > IMHO, this feels like -onMouseDown: is inputField notifying of a -mouseDown: > to > > itself. I think it should be -onEditorMouseDown: or -onFieldEditorMouseDown: > or > > something like that. > > > > Actually, to match field-editor style, you'd have the field editor delegate > > something like: > > - (void)onMouseDown:(NSText*)sender; > > and the field itself would forward that message to something like: > > - (void)control:(NSControl*)control onMouseDown:(NSText*)editor; > > > > Reflecting off the field has the advantage that the field can suppress the > > handling-first-click directly, rather than having to expose that as part of > the > > interface. > > > > [Remember, this is Objective-C. If your method names are not forcing you past > > the 80-column limit, you're doing it wrong.] > > To make sure I understand: You'd want me to call [inputField onMouseDown:] here, > and then handle first click in the AutofillTextField? I've considered it, but > the treatment of the first click is technically behavior enforced by the dialog, > not the control. > > At the very least, I need to find a better name for the field delegate's method > then, since I'd like it to reflect that it can swallow a mouseDown... > onMouseDown:gaveFirstResponder: maybe? OK, uploaded an implementation - LMKWYT. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:298: [[AutofillDialogFieldEditor alloc] initWithFrame:NSZeroRect]); On 2013/10/30 17:57:23, shess wrote: > -init should suffice for this. Hm. NSTextView does not sport an -init initializer. Given that initWithFrame: is explicitly flagged as creating container, layoutManager and textStorage, I'm not sure calling -init is the right thing.
Ping?
On 2013/10/30 19:13:36, groby wrote: > I need to distinguish between "first click on field iff it made the field > firstResponder" and "subsequent clicks on field". I.e. these three cases: > > * Click on field to make it firstResponder: Treat as first click > > * Tab to field to make it firstResponder, then click: Treat as subsequent click > > * Click on field to make it firstResponder, then click again: Treat as > subsequent click. I'm still not sure I'm following why this is all special, though. In the first case, the click comes to the field itself. In the second and third cases the click should come to the field editor. It is possible that the implementation of the field's -mouseDown: forwards the click to the field editor to position things, but that could be handled without affecting -becomeFirstResponder. AFAICT, though, what you actually have is: * Tab to field to make it firstResponder (setting shouldFilterClick_), then click: (set handlingFirstClick_) Don't forward the click at all. > As for multi-clicks, I'm not worried about double-clicks, but about what I'd > term "rage clicks" - the user repeatedly spamming the mouse button. I do get a > separate mouseDown: for each of those. And, as you pointed out, clicks on the > border area. If it's too long for Cocoa to see it as a multi-click, then it's another click. I don't think those need any handling at all, in general. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:264: [[inputField delegate] onMouseDown:inputField]; On 2013/10/30 19:13:36, groby wrote: > On 2013/10/30 17:57:23, shess wrote: > > IMHO, this feels like -onMouseDown: is inputField notifying of a -mouseDown: > to > > itself. I think it should be -onEditorMouseDown: or -onFieldEditorMouseDown: > or > > something like that. > > > > Actually, to match field-editor style, you'd have the field editor delegate > > something like: > > - (void)onMouseDown:(NSText*)sender; > > and the field itself would forward that message to something like: > > - (void)control:(NSControl*)control onMouseDown:(NSText*)editor; > > > > Reflecting off the field has the advantage that the field can suppress the > > handling-first-click directly, rather than having to expose that as part of > the > > interface. > > > > [Remember, this is Objective-C. If your method names are not forcing you past > > the 80-column limit, you're doing it wrong.] > > To make sure I understand: You'd want me to call [inputField onMouseDown:] here, > and then handle first click in the AutofillTextField? I've considered it, but > the treatment of the first click is technically behavior enforced by the dialog, > not the control. > > At the very least, I need to find a better name for the field delegate's method > then, since I'd like it to reflect that it can swallow a mouseDown... > onMouseDown:gaveFirstResponder: maybe? The control combination should implement the sense of "This is the first click" or "This is the subsequent click" or "Whatever other click it is" on the control, and the dialog should implement "What does the first click do", etc. That way the dialog doesn't have to have excessively deep knowledge of the control's implementation details. To the extent possible, of course. Sometimes it just isn't feasible. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:295: - (id)windowWillReturnFieldEditor:(NSWindow*)window toObject:(id)client { You should only return this field editor for clients of the appropriate class. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:298: [[AutofillDialogFieldEditor alloc] initWithFrame:NSZeroRect]); On 2013/10/31 00:02:43, groby wrote: > On 2013/10/30 17:57:23, shess wrote: > > -init should suffice for this. > > Hm. NSTextView does not sport an -init initializer. Given that initWithFrame: is > explicitly flagged as creating container, layoutManager and textStorage, I'm not > sure calling -init is the right thing. In general, if you find yourself making up a parameter to pass to -initWithWhatever:, then you probably should call -init and it will do the right thing. The Omnibox field editor is created with -init, it works fine. I expect that if you debugged it, -[NSView init] calls [self initWithFrame:NSZeroRect]. Regardless of what it does do, though, I have no doubt that their version of unit tests test whether -init does something sensible. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:301: return fieldEditor_.get(); On 2013/10/30 19:13:36, groby wrote: > If you don't cache it, your fieldEditor becomes completely stateless. IIRC, > _any_ time a control uses an editor, it gets it via this method. Controls never > cache it themselves. > > Furthermore, NSTextView is rather heavy, and this gets called quite often. (10's > of time per second, for an otherwise idle dialog) > > On 2013/10/30 17:57:23, shess wrote: > > Is this actually worthwhile to cache? Hmm, I see the omnibox field editor is > > cached, I guess so. I suspect that it's not worth caching. Really? I wouldn't expect that. I'd expect the field to keep using the existing editor so long as it's still editing. If it kept changing the editor, things wouldn't work. Hmm. Now that I think about it, I think the field editor might be used for other things, like layout, in which case it would make sense to get called tons of times. And I see that the other case I can think of caches it, so ... https://codereview.chromium.org/51933002/diff/170001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/autofill_textfield.h (right): https://codereview.chromium.org/51933002/diff/170001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/autofill_textfield.h:13: @protocol AutofillTextFieldDelegate I don't think this is right. AutofillTextFieldDelegate is the delegate of an AutofillTextField (which you already have as AutofillInputDelegate). If you wanted to use a specific protocol, it would be AutofillTextFieldEditorDelegate (or I guess AutofillDialogFieldEditorDelegate?). IMHO, I think there's no need to generalize, unless you're really certain you're going to share the field editor across different classes relatively soon. Then you could just say that AutofillTextField objects get a AutofillTextFieldEditor editor, and that editor can assume that the delegate isKindOf AutofillTextField. Then you just strict-cast the delegate to the expected class and call -[AutofillTextField onEditorMouseDown:]. Does that make sense? https://codereview.chromium.org/51933002/diff/170001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/autofill_textfield.mm (right): https://codereview.chromium.org/51933002/diff/170001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/autofill_textfield.mm:54: [delegate_ onMouseDown: self]; Yes, this is what I meant. The field is handling this internal information directly, rather than exposing it.
It's not special at all - I was just outlining the three cases since they're the different ways control flows. (I.e. what you said in the rest of that comment) As for the multi-clicking: That's the reason I need to flag the click that led to becomeFirstResponder. Otherwise, it'd always be the first one to NSTextField, the rest to the editor - which would make life so much easier. Again, nothing special about it, just enumerating the constraints that led to the code as-is. PTYAL :) https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:264: [[inputField delegate] onMouseDown:inputField]; On 2013/11/01 00:37:07, shess wrote: > On 2013/10/30 19:13:36, groby wrote: > > On 2013/10/30 17:57:23, shess wrote: > > > IMHO, this feels like -onMouseDown: is inputField notifying of a -mouseDown: > > to > > > itself. I think it should be -onEditorMouseDown: or > -onFieldEditorMouseDown: > > or > > > something like that. > > > > > > Actually, to match field-editor style, you'd have the field editor delegate > > > something like: > > > - (void)onMouseDown:(NSText*)sender; > > > and the field itself would forward that message to something like: > > > - (void)control:(NSControl*)control onMouseDown:(NSText*)editor; > > > > > > Reflecting off the field has the advantage that the field can suppress the > > > handling-first-click directly, rather than having to expose that as part of > > the > > > interface. > > > > > > [Remember, this is Objective-C. If your method names are not forcing you > past > > > the 80-column limit, you're doing it wrong.] > > > > To make sure I understand: You'd want me to call [inputField onMouseDown:] > here, > > and then handle first click in the AutofillTextField? I've considered it, but > > the treatment of the first click is technically behavior enforced by the > dialog, > > not the control. > > > > At the very least, I need to find a better name for the field delegate's > method > > then, since I'd like it to reflect that it can swallow a mouseDown... > > onMouseDown:gaveFirstResponder: maybe? > > The control combination should implement the sense of "This is the first click" > or "This is the subsequent click" or "Whatever other click it is" on the > control, and the dialog should implement "What does the first click do", etc. > That way the dialog doesn't have to have excessively deep knowledge of the > control's implementation details. > > To the extent possible, of course. Sometimes it just isn't feasible. That's what the current implementation does. It just swallows the first click silently. I suppose I could send it up to the dialog with a flag, but all I'd do then is ignore it in the dialog. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:298: [[AutofillDialogFieldEditor alloc] initWithFrame:NSZeroRect]); On 2013/11/01 00:37:07, shess wrote: > On 2013/10/31 00:02:43, groby wrote: > > On 2013/10/30 17:57:23, shess wrote: > > > -init should suffice for this. > > > > Hm. NSTextView does not sport an -init initializer. Given that initWithFrame: > is > > explicitly flagged as creating container, layoutManager and textStorage, I'm > not > > sure calling -init is the right thing. > > In general, if you find yourself making up a parameter to pass to > -initWithWhatever:, then you probably should call -init and it will do the right > thing. The Omnibox field editor is created with -init, it works fine. I expect > that if you debugged it, -[NSView init] calls [self initWithFrame:NSZeroRect]. > Regardless of what it does do, though, I have no doubt that their version of > unit tests test whether -init does something sensible. Done. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:301: return fieldEditor_.get(); On 2013/11/01 00:37:07, shess wrote: > On 2013/10/30 19:13:36, groby wrote: > > If you don't cache it, your fieldEditor becomes completely stateless. IIRC, > > _any_ time a control uses an editor, it gets it via this method. Controls > never > > cache it themselves. > > > > Furthermore, NSTextView is rather heavy, and this gets called quite often. > (10's > > of time per second, for an otherwise idle dialog) > > > > On 2013/10/30 17:57:23, shess wrote: > > > Is this actually worthwhile to cache? Hmm, I see the omnibox field editor > is > > > cached, I guess so. I suspect that it's not worth caching. > > Really? I wouldn't expect that. I'd expect the field to keep using the > existing editor so long as it's still editing. If it kept changing the editor, > things wouldn't work. > > Hmm. Now that I think about it, I think the field editor might be used for > other things, like layout, in which case it would make sense to get called tons > of times. And I see that the other case I can think of caches it, so ... I thought the same thing, but it turns out you really _must_ keep it around. See e.g. this discussion on CocoaDev: http://cocoadev.com/CreatingCustomFieldEditor This _used_ to be part of the docs: (see http://www.filibeto.org/unix/macos/lib/dev/documentation/Cocoa/Reference/Appl... - NSWindow docs from 2007) This method may be called multiple times while a control is first responder. Therefore, you must return the same field editor object for the control while the control is being edited That is still true - I've added logging code to verify - but the docs don't mention it any more :( https://codereview.chromium.org/51933002/diff/170001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/autofill_textfield.h (right): https://codereview.chromium.org/51933002/diff/170001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/autofill_textfield.h:13: @protocol AutofillTextFieldDelegate On 2013/11/01 00:37:07, shess wrote: > I don't think this is right. AutofillTextFieldDelegate is the delegate of an > AutofillTextField (which you already have as AutofillInputDelegate). If you > wanted to use a specific protocol, it would be AutofillTextFieldEditorDelegate > (or I guess AutofillDialogFieldEditorDelegate?). > > IMHO, I think there's no need to generalize, unless you're really certain you're > going to share the field editor across different classes relatively soon. Then > you could just say that AutofillTextField objects get a AutofillTextFieldEditor > editor, and that editor can assume that the delegate isKindOf AutofillTextField. > Then you just strict-cast the delegate to the expected class and call > -[AutofillTextField onEditorMouseDown:]. > > Does that make sense? Not planning to share, no. Kind of annoying to expose a callback API on the class, but you're probably right - it's not worth the additional (over)engineering. I guess the "proper" thing would be to call it AutofillTextFieldEditorDelegate, have it implement this protocol and the editordelegate protocal, factor out the editor into a separate class... Yeah, no. Moving it over. https://codereview.chromium.org/51933002/diff/170001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/autofill_textfield.mm (right): https://codereview.chromium.org/51933002/diff/170001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/autofill_textfield.mm:54: [delegate_ onMouseDown: self]; On 2013/11/01 00:37:07, shess wrote: > Yes, this is what I meant. The field is handling this internal information > directly, rather than exposing it. Done.
LGTM. Thanks for bearing with me. https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/51933002/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:264: [[inputField delegate] onMouseDown:inputField]; On 2013/11/01 03:48:44, groby wrote: > On 2013/11/01 00:37:07, shess wrote: > > On 2013/10/30 19:13:36, groby wrote: > > > On 2013/10/30 17:57:23, shess wrote: > > > > IMHO, this feels like -onMouseDown: is inputField notifying of a > -mouseDown: > > > to > > > > itself. I think it should be -onEditorMouseDown: or > > -onFieldEditorMouseDown: > > > or > > > > something like that. > > > > > > > > Actually, to match field-editor style, you'd have the field editor > delegate > > > > something like: > > > > - (void)onMouseDown:(NSText*)sender; > > > > and the field itself would forward that message to something like: > > > > - (void)control:(NSControl*)control onMouseDown:(NSText*)editor; > > > > > > > > Reflecting off the field has the advantage that the field can suppress the > > > > handling-first-click directly, rather than having to expose that as part > of > > > the > > > > interface. > > > > > > > > [Remember, this is Objective-C. If your method names are not forcing you > > past > > > > the 80-column limit, you're doing it wrong.] > > > > > > To make sure I understand: You'd want me to call [inputField onMouseDown:] > > here, > > > and then handle first click in the AutofillTextField? I've considered it, > but > > > the treatment of the first click is technically behavior enforced by the > > dialog, > > > not the control. > > > > > > At the very least, I need to find a better name for the field delegate's > > method > > > then, since I'd like it to reflect that it can swallow a mouseDown... > > > onMouseDown:gaveFirstResponder: maybe? > > > > The control combination should implement the sense of "This is the first > click" > > or "This is the subsequent click" or "Whatever other click it is" on the > > control, and the dialog should implement "What does the first click do", etc. > > That way the dialog doesn't have to have excessively deep knowledge of the > > control's implementation details. > > > > To the extent possible, of course. Sometimes it just isn't feasible. > > That's what the current implementation does. It just swallows the first click > silently. I suppose I could send it up to the dialog with a flag, but all I'd do > then is ignore it in the dialog. Yeah, was just reviewing prior responses to comments, and didn't have the will to re-edit WRT changes in later patches. Living hypertext! https://codereview.chromium.org/51933002/diff/170001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/autofill_textfield.h (right): https://codereview.chromium.org/51933002/diff/170001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/autofill_textfield.h:13: @protocol AutofillTextFieldDelegate On 2013/11/01 03:48:44, groby wrote: > On 2013/11/01 00:37:07, shess wrote: > > I don't think this is right. AutofillTextFieldDelegate is the delegate of an > > AutofillTextField (which you already have as AutofillInputDelegate). If you > > wanted to use a specific protocol, it would be AutofillTextFieldEditorDelegate > > (or I guess AutofillDialogFieldEditorDelegate?). > > > > IMHO, I think there's no need to generalize, unless you're really certain > you're > > going to share the field editor across different classes relatively soon. > Then > > you could just say that AutofillTextField objects get a > AutofillTextFieldEditor > > editor, and that editor can assume that the delegate isKindOf > AutofillTextField. > > Then you just strict-cast the delegate to the expected class and call > > -[AutofillTextField onEditorMouseDown:]. > > > > Does that make sense? > > Not planning to share, no. Kind of annoying to expose a callback API on the > class, but you're probably right - it's not worth the additional > (over)engineering. I guess the "proper" thing would be to call it > AutofillTextFieldEditorDelegate, have it implement this protocol and the > editordelegate protocal, factor out the editor into a separate class... Yeah, > no. Moving it over. I love writing delegate chains with all of the protocol support just like AppKit, but sometimes it's just so much architecting to no particular end. Someday I want to just dig in and refactor stuff so that custom fields can directly control their field editor, rather than having it bounce through the window. Twenty years ago the shared field editor made a lot of sense, but invariably when you're overriding it, you're adding specialized actions which don't make sense to share outside of your specific class. https://codereview.chromium.org/51933002/diff/280001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/51933002/diff/280001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:296: [[AutofillDialogFieldEditor alloc] init]); Looks like this can run on one line, now.
Thanks for reviewing it! https://codereview.chromium.org/51933002/diff/170001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/autofill_textfield.h (right): https://codereview.chromium.org/51933002/diff/170001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/autofill_textfield.h:13: @protocol AutofillTextFieldDelegate On 2013/11/02 00:26:22, shess wrote: > On 2013/11/01 03:48:44, groby wrote: > > On 2013/11/01 00:37:07, shess wrote: > > > I don't think this is right. AutofillTextFieldDelegate is the delegate of > an > > > AutofillTextField (which you already have as AutofillInputDelegate). If you > > > wanted to use a specific protocol, it would be > AutofillTextFieldEditorDelegate > > > (or I guess AutofillDialogFieldEditorDelegate?). > > > > > > IMHO, I think there's no need to generalize, unless you're really certain > > you're > > > going to share the field editor across different classes relatively soon. > > Then > > > you could just say that AutofillTextField objects get a > > AutofillTextFieldEditor > > > editor, and that editor can assume that the delegate isKindOf > > AutofillTextField. > > > Then you just strict-cast the delegate to the expected class and call > > > -[AutofillTextField onEditorMouseDown:]. > > > > > > Does that make sense? > > > > Not planning to share, no. Kind of annoying to expose a callback API on the > > class, but you're probably right - it's not worth the additional > > (over)engineering. I guess the "proper" thing would be to call it > > AutofillTextFieldEditorDelegate, have it implement this protocol and the > > editordelegate protocal, factor out the editor into a separate class... Yeah, > > no. Moving it over. > > I love writing delegate chains with all of the protocol support just like > AppKit, but sometimes it's just so much architecting to no particular end. > Someday I want to just dig in and refactor stuff so that custom fields can > directly control their field editor, rather than having it bounce through the > window. Twenty years ago the shared field editor made a lot of sense, but > invariably when you're overriding it, you're adding specialized actions which > don't make sense to share outside of your specific class. You _almost_ can. NSCell has -fieldEditorForView: The only reason I didn't use that is that I'd have to have a separate text editor for each and every textfield. In which case, they're all equivalent to super-heavy-weight NSTextViews anyways. I suppose I could do a global & a +sharedInstance call - but that's essentially a leak. The only decent way to handle I can think of is listening to NSApplicationWillTerminateNotification. And that sounds like a bit of overkill. If there's a better way, LMK... https://codereview.chromium.org/51933002/diff/280001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/51933002/diff/280001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:296: [[AutofillDialogFieldEditor alloc] init]); On 2013/11/02 00:26:22, shess wrote: > Looks like this can run on one line, now. And so it is. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/51933002/330001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/51933002/330001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/51933002/330001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/51933002/690001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/51933002/690001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/51933002/690001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/51933002/690001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/51933002/690001
Message was sent while issue was closed.
Change committed as 232970 |
