[rAC, OSX] Fix Omnibox font issue

chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm

Index: chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
index 1777998887526adcc8cceccf1b949b828f4d40ce..9b53c5565bf1989d95485928c2d5aa04410e2ea6 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
@@ -401,6 +401,13 @@ void OmniboxViewMac::SetTextAndSelectedRange(const base::string16& display_text,
 }
 
 void OmniboxViewMac::EmphasizeURLComponents() {
+  // Obtain the text before any editing - GetText will update the field's value
+  // from textStorage, and if EmphasizeURLComponents is called from within the
+  //  editor's -resignFirstResponder, |field_| will have a text value that is
+  // stripped of all attributes. (Since the editor is about to die and has
+  // already synced the field to its value).
+  base::string16 text = GetText();

[Scott Hess - ex-Googler, 2014/03/06 02:42:24]

I cannot understand how the comment applies at all.  |text| itself will be
stripped of all attributes, so getting it up here shouldn't do anything at all
WRT the attributes.  What it sounds like is that you want the else{} rather than
the if(){} code path in this case.

AFAICT, the problem is that -currentEditor returns non-nil, but editor has
released first responder, so the -beginEditing/-endEditing stuff isn't
appropriate.  If so, would a better check be if (editor == [[editor window]
firstResponder])?

[groby-ooo-7-16, 2014/03/06 04:07:51]

Unfortunately, it still is the firstResponder, except having released it.  This
is called from within resignFirstResponder, which is called from [window
makeFirstResponder]. And window can't update its firstResponder before it has
gotten the return value from resignFirstResponder. As far as I can tell, there's
no straightforward way to distinguish this state.

The issue with GetText is that it calls [field_ stringValue] - which gets the
editors current attributedString, and updated [field_ attributedStringValue] in
the process. And the previous code has _just_ stripped all attributes off.

So now field_ has a "bare" string. Storage gets updated to be the full string,
but that doesn't matter, becomes we're called from the tail end of
[AutocompleteTextFieldEditor resignFirstResponder], with [NSTextView
resignFirstResponder] already finished. And so there will be no more syncing
from the editors proper value to the fields attributedStringValue.

You could also fix this by just calling [field_ attributedStringValue] after
-endEditing, forcing a sync - but that would break the IME use case which
necessitated this code in the first place.

(I believe the root cause for the IME issue is that OnDidChange is triggered
from interpetKeys, which isn't the proper place to handle this - but I don't
have a proper fix for that yet, and so I need to work around it here)

[Scott Hess - ex-Googler, 2014/03/06 06:30:32]

Still nack.  base::string16 cannot have Cocoa attributes on it.  I don't mean
it's unlikely, there is no there, there, GetText() doesn't have the scope to
return attributed text.  Either SetText() or ApplyTextAttributes() is what adds
the attributes to an unattributed string, so fetching it earlier shouldn't make
a difference.  I suspect that what's happening is that something in the if
(editor) case is causing a problem because the field editor is being torn town. 
Perhaps it is re-syncing the bare string, but then modifying the |storage|
string which is never re-synced.

I'm going to hate myself if there's an obvious thing sitting right in front of
me, here.  I just can't see how the comment relates.  The attributes of the text
before this call shouldn't matter, it's the attributes _after_ this call which
matter.

I would look to see if one of these cases could catch the case:
 - ![storage length]
 - [editor delegate] != field_
 - ![editor window]
and then modify it to do the else{} in that case (so that it modifies the field
value not the editor value).

[groby-ooo-7-16, 2014/03/06 19:07:58]

It doesn't. It's a side effect. Sequence of events, with indentation for
callstack levels

-[AutocompleteTextFieldEditor -resignFirstResponder] is called
   It calls [super resignFirstResponder]
       That calls a private method to sync field values between editor and
NSTextField
   Now ACTFE calls OnFocusLost
       That calls EmphasizeURLComponents
           Reset the storages text attributes
           Call GetText in preparation for ApplyTextAttributes
               GetText calls [field_ stringValue]
                   stringValue syncs the editor and the text field, both string
and attributedString 
                   (since resignFirstResponder hasn't completed, the field has
still an editor)
                   As result, field's attributed string has gotten the editor's
string, which just got stripped
           EmphasizeURLComponents applies attributes, updating storage only
    ACTFE's resignFirstResponder is now done
AppKit removes the editor and sets a new first responder, _without_ updating
field's values from the editor.

At that point, NSTextField has an attributedStringValue without any attributes,
and the editor is gone.
Yes, that's exactly what's happening. And the re-syncing is caused by GetText().
I've got the stack traces to prove it ;)
Hm. How would you word the comment then, in light of what I explained above. I
_thought_ the comment made that somewhat clear, but obviously it's misleading.
None of these do. The editor is still valid - AppKit tears it down _after_
resignFirstResponder.
I've looked for all sorts of conditions, but short of setting a flag in ACTFE's
resignFirstResponder and querying it here, there's no way to do that. (That I
can find)

[Scott Hess - ex-Googler, 2014/03/06 19:28:12]

This implies that calling [field_ stringValue] after -endEditing would re-sync
the right thing.

[Brevity of comment because I wanted to suggest that before joining the group
leaving for lunch.  I'll comment on the comment commenting when I'm back.]

[Scott Hess - ex-Googler, 2014/03/06 20:54:28]

OK, I'm reading old CLs trying to wrap my head around everything.  AFAICT, this
exists to do our annotations in-place over possibly-marked text (which only
exists in the editor, not the control).  So we cannot do anything which would
push in that direction.

The core problem is that [field_ stringValue] pulls the current data from editor
to control, and at the point where ApplyTextAttributes() is called, the
attributes have just been cleared.  The editor is then attributed, but nobody
ever pulls the data over to the field again.

So I think that calling GetText() up top merely retains the pre-existing
attributes, but it still throws away the attributes in ApplyTextAttributes().  I
can't really think of a case where a user event could make a change that won't
have already been synced, but it still seems subtly incorrect because our code
could make such a change.

I think if calling -stringValue after -endEditing works, that would probably
make the most sense.  Then that call needs a local comment, but it's pretty
straightforward about making sure the editor data is sync'ed to the control
(perhaps with a kicker that -resignFirstResponder is a case which doesn't
otherwise guarantee this, so that nobody decides it's unnecessary).

[Every time I try to phrase a comment for the existing code, I end up thinking
that just making the case not happen would be better.]

[groby-ooo-7-16, 2014/03/07 01:47:08]

Based on my reading of old CL's, we cannot call
setAttributedStringValue/setStringValue because it messed up IME events in
progress. (I'm not sure why -stringValue still works, but it seems to)
Yep.
This corner case _only_ happens during resignFirstResponder. Any key events have
already invoked EmphasizeURL and set the proper attributes for user input.
This code is still equivalent to the code we had before - we just get the
stringValue a bit earlier. If there was any editing case that lost attributes,
we'd already have seen it.
I can try that. I'm worried about IME interactions, but that will always be a
worry.
I think I have an inkling of how to do it, but I don't have the time to
investigate. If I understand Cocoa's text handling correctly, we don't want to
intercept interpretKeys for the OnChanged event - because those happen for every
key stroke, and thus in the middle of IME compositing. Instead, we only care if
a glyph is added, which would be insertText. That would, theoretically, obviate
the need to even account for IME issues, so all this would be is
SetText(GetText()).

+
   NSTextView* editor = (NSTextView*)[field_ currentEditor];
   // If the autocomplete text field is in editing mode, then we can just change
   // its attributes through its editor. Otherwise, we simply reset its content.
@@ -412,11 +419,11 @@ void OmniboxViewMac::EmphasizeURLComponents() {
     // overlay the appropriate Omnibox attributes.
     [storage setAttributes:[NSDictionary dictionary]
                      range:NSMakeRange(0, [storage length])];
-    ApplyTextAttributes(GetText(), storage);
+    ApplyTextAttributes(text, storage);
 
     [storage endEditing];
   } else {
-    SetText(GetText());
+    SetText(text);
   }
 }