Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11418144:
[Search] Implementation of the invisible focus on Windows (Closed)
|
Created:
8 years ago by Mathieu Modified:
8 years ago CC:
chromium-reviews, tfarina, James Su, samarth Base URL:
http://git.chromium.org/chromium/src.git@samarthlatest Visibility:
Public. |
Description[Search] Implementation of the invisible focus on Windows
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171236
Patch Set 1 #Patch Set 2 : style fix #Patch Set 3 : Rebased to master instead of other change #
Total comments: 2
Patch Set 4 : addressed comment #Patch Set 5 : ws #Patch Set 6 : windows support #Patch Set 7 : ws #
Total comments: 14
Patch Set 8 : addressed comments #Patch Set 9 : windows typo #Patch Set 10 : Narrowed down to Windows only #Patch Set 11 : added comments #
Total comments: 22
Patch Set 12 : addressed comments #Patch Set 13 : ApplyCaretVisibility #Patch Set 14 : Caret height + comments #Patch Set 15 : Caret height + comments #
Total comments: 4
Patch Set 16 : Comments #Patch Set 17 : rebasing #Patch Set 18 : copied comment from other patch #
Total comments: 8
Patch Set 19 : Comments, mouse-click, SetFocus #
Total comments: 7
Patch Set 20 : mouse click #Patch Set 21 : rebase #Patch Set 22 : is_caret_visible #Patch Set 23 : Clean #Messages
Total messages: 29 (0 generated)
Hi Peter, This is the implementation of the Invisible focus, to go in conjunction with https://chromiumcodereview.appspot.com/11369137/ (which implements the API with the renderer). I decided to base it on HEAD so that this patch is self-contained and easier to review. In the event that the change quoted above is submitted, I will rebase. Nevertheless, I think this is ready for your review. Along with the other change, it gives a clear picture of what we want to implement. Thanks!
https://codereview.chromium.org/11418144/diff/4001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/11418144/diff/4001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:961: SK_ColorBLACK : textfield_->background_color()); Is BLACK always correct? If not, we may need to save the old value of CursorColor() or re-use whatever logic textfield_ uses to decide which color to use normally.
Thanks. https://codereview.chromium.org/11418144/diff/4001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/11418144/diff/4001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:961: SK_ColorBLACK : textfield_->background_color()); On 2012/11/28 17:43:54, samarth wrote: > Is BLACK always correct? If not, we may need to save the old value of > CursorColor() or re-use whatever logic textfield_ uses to decide which color to > use normally. Thanks for the comment. I've changed the logic so that it remembers the color. In most cases it will be black, but there could be a case where SetCursorColor was called from another class and changed to another color.
On 2012/11/28 21:03:05, Mathieu Perreault wrote: > Thanks. > > https://codereview.chromium.org/11418144/diff/4001/chrome/browser/ui/views/om... > File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): > > https://codereview.chromium.org/11418144/diff/4001/chrome/browser/ui/views/om... > chrome/browser/ui/views/omnibox/omnibox_view_views.cc:961: SK_ColorBLACK : > textfield_->background_color()); > On 2012/11/28 17:43:54, samarth wrote: > > Is BLACK always correct? If not, we may need to save the old value of > > CursorColor() or re-use whatever logic textfield_ uses to decide which color > to > > use normally. > > Thanks for the comment. I've changed the logic so that it remembers the color. > In most cases it will be black, but there could be a case where SetCursorColor > was called from another class and changed to another color. Added Windows support.
https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:586: DLOG(0) << "SetInvisibleFocus"; Don't check this in. https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:620: // regardless of its focus state, we reset the focus to visible. This is an odd place to put this comment. Clicks on the omnibox don't reach here immediately/directly (unless some IME work changed that), this is instead called when we're going to change something. If you want focus visibility reset when the user clicks, put it in the click handler. https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:469: caret_height_(0), Probably unnecessary, see below. https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:766: ApplyFocusVisibility(); Rather than calling this unconditionally, we should probably only do it when the focus visibility status changes. One way to handle this would be to make the edit model setter call back to the view when the requested visibility actually differs. This can also allow us to get rid of SetInvisibleFocus() entirely if its current callers either are expected to do "view->SetFocus(); model->SetFocusVisible(false);", or if the callers are expected to only ask for invisible focus when the omnibox already has focus. https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2778: HideCaret(); Why are we hiding the caret unconditionally instead of in an else clause? https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2779: ::DestroyCaret(); Do we have to DestroyCaret() and later CreateCaret()? Why is just HideCaret()/ShowCaret() insufficient? Note that the omnibox will never dynamically change height.
Thanks for the comments Peter! https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:586: DLOG(0) << "SetInvisibleFocus"; On 2012/12/01 01:48:31, Peter Kasting wrote: > Don't check this in. Done. https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:620: // regardless of its focus state, we reset the focus to visible. On 2012/12/01 01:48:31, Peter Kasting wrote: > This is an odd place to put this comment. Clicks on the omnibox don't reach > here immediately/directly (unless some IME work changed that), this is instead > called when we're going to change something. If you want focus visibility reset > when the user clicks, put it in the click handler. The comment wasn't explicit enough, my bad. We also use this location in the call sequence to restore the visibility of the caret when a user enters something in the omnibox. I thought this would kill two birds with one stone. What do you think? I've modified the comment to account for both cases. https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:469: caret_height_(0), On 2012/12/01 01:48:31, Peter Kasting wrote: > Probably unnecessary, see below. Done. https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:766: ApplyFocusVisibility(); On 2012/12/01 01:48:31, Peter Kasting wrote: > Rather than calling this unconditionally, we should probably only do it when the > focus visibility status changes. > > One way to handle this would be to make the edit model setter call back to the > view when the requested visibility actually differs. This can also allow us to > get rid of SetInvisibleFocus() entirely if its current callers either are > expected to do "view->SetFocus(); model->SetFocusVisible(false);", or if the > callers are expected to only ask for invisible focus when the omnibox already > has focus. Good idea. Done. https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2778: HideCaret(); On 2012/12/01 01:48:31, Peter Kasting wrote: > Why are we hiding the caret unconditionally instead of in an else clause? Ah, the Windows API... If DestroyCaret is called at the moment when the caret is in the visible part of the blinking, it will stay a solid, non-blinking bar. Hiding it prior to destroying solves that. https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2779: ::DestroyCaret(); On 2012/12/01 01:48:31, Peter Kasting wrote: > Do we have to DestroyCaret() and later CreateCaret()? Why is just > HideCaret()/ShowCaret() insufficient? Note that the omnibox will never > dynamically change height. The Hide/Show Windows API is broken in this way: when the view is repainted, the caret is automatically Show()'ed again (More specifically, HideCaret is called by Windows in BeginPaint, and ShowCaret is called in EndPaint; this is to avoid showing the caret when a character is painted). Therefore, we cannot really trust Hide to stick for a long time (a dirty omnibox, such as in the case of a window resize, will make the caret show again). Destroying and Creating it again solves that.
I've now based this CL on https://chromiumcodereview.appspot.com/11369137/ since all the previous changes are now over there. This is purely the Windows implementation.
https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2779: ::DestroyCaret(); On 2012/12/03 02:29:04, Mathieu Perreault wrote: > On 2012/12/01 01:48:31, Peter Kasting wrote: > > Do we have to DestroyCaret() and later CreateCaret()? Why is just > > HideCaret()/ShowCaret() insufficient? Note that the omnibox will never > > dynamically change height. > > The Hide/Show Windows API is broken in this way: when the view is repainted, the > caret is automatically Show()'ed again (More specifically, HideCaret is called > by Windows in BeginPaint, and ShowCaret is called in EndPaint; this is to avoid > showing the caret when a character is painted). Therefore, we cannot really > trust Hide to stick for a long time (a dirty omnibox, such as in the case of a > window resize, will make the caret show again). Destroying and Creating it again > solves that. Interesting. You should add comments about this. We're already hooking Begin/EndPaint (see OnPaint()). Can we just insert a HideCaret() call (if it should be currently hidden) after the DefWindowProc() call in that function? The advantage to this is that we won't need |caret_height_| and we won't have to worry about anything weird where the caret style or color or something is for some crazy reason different than what we're specifying here.
Thanks Peter! https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2779: ::DestroyCaret(); On 2012/12/03 23:03:32, Peter Kasting wrote: > On 2012/12/03 02:29:04, Mathieu Perreault wrote: > > On 2012/12/01 01:48:31, Peter Kasting wrote: > > > Do we have to DestroyCaret() and later CreateCaret()? Why is just > > > HideCaret()/ShowCaret() insufficient? Note that the omnibox will never > > > dynamically change height. > > > > The Hide/Show Windows API is broken in this way: when the view is repainted, > the > > caret is automatically Show()'ed again (More specifically, HideCaret is called > > by Windows in BeginPaint, and ShowCaret is called in EndPaint; this is to > avoid > > showing the caret when a character is painted). Therefore, we cannot really > > trust Hide to stick for a long time (a dirty omnibox, such as in the case of a > > window resize, will make the caret show again). Destroying and Creating it > again > > solves that. > > Interesting. You should add comments about this. > > We're already hooking Begin/EndPaint (see OnPaint()). Can we just insert a > HideCaret() call (if it should be currently hidden) after the DefWindowProc() > call in that function? > > The advantage to this is that we won't need |caret_height_| and we won't have to > worry about anything weird where the caret style or color or something is for > some crazy reason different than what we're specifying here. I've now added comments where I felt appropriate. Should have done it before, sorry. As for your suggestion, this was indeed my first approach and this one is based off it (ApplyFocusVisibility is called at the end of OnPaint, after the DefWindowProc). I've moved to the Destroy/Create approach because Hiding and Showing wasn't consistent, to the point where Philippe and I were almost pulling our hair out. According to the Windows API documentation, Hide and Show are cumulative -- calling Hide five times will need five Show calls. In our experience, it wasn't the case and was very inconsistent (we several scenarios, including gating the Hide and Show calls to make sure they were only called to cancel each other out). I believe that Destroy/Create is our only hope at this moment. As for different Caret shapes or color, the APIs do not exist on Windows (I wish changing the caret color existed!). The only call that can modify the shape of the caret is CreateCaret, and the only place it's called in Chromium is here http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/base/win/ime_input.cc&e... And the shape is not altered.
On 2012/12/03 23:03:32, Peter Kasting wrote: > https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... > File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): > > https://codereview.chromium.org/11418144/diff/8002/chrome/browser/ui/views/om... > chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2779: ::DestroyCaret(); > On 2012/12/03 02:29:04, Mathieu Perreault wrote: > > On 2012/12/01 01:48:31, Peter Kasting wrote: > > > Do we have to DestroyCaret() and later CreateCaret()? Why is just > > > HideCaret()/ShowCaret() insufficient? Note that the omnibox will never > > > dynamically change height. > > > > The Hide/Show Windows API is broken in this way: when the view is repainted, > the > > caret is automatically Show()'ed again (More specifically, HideCaret is called > > by Windows in BeginPaint, and ShowCaret is called in EndPaint; this is to > avoid > > showing the caret when a character is painted). Therefore, we cannot really > > trust Hide to stick for a long time (a dirty omnibox, such as in the case of a > > window resize, will make the caret show again). Destroying and Creating it > again > > solves that. > > Interesting. You should add comments about this. > > We're already hooking Begin/EndPaint (see OnPaint()). Can we just insert a > HideCaret() call (if it should be currently hidden) after the DefWindowProc() > call in that function? > > The advantage to this is that we won't need |caret_height_| and we won't have to > worry about anything weird where the caret style or color or something is for > some crazy reason different than what we're specifying here. Jumping in here, as I experimented on this with Mathieu quite a bit... A HideCaret() call in EndPaint() was actually the first thing we tried and it did not work. In fact, if you don't condition HideCaret() on anything I would expect the caret to remain hidden all the time, but doing so did not have any impact. We tried WinDbg to see exactly when the caret was made visible again but (maybe our windows-fu is too weak?) we were not really able to figure out where this was going on. If we figure out where this happens, I suspect than simply hooking into the right place would be a ugly hack in itself. Also it's worth mentioning that the documentation regarding the caret is, let's say, lacking. I agree that the |caret_height_| approach is not perfect. If you know of anybody who would be able to help us untangle that mess, I would be happy to spend some more time on it.
https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:763: model()->SetFocusVisibility(true); See question in the other CL about this particular call. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:767: if (!chrome::search::IsInstantExtendedAPIEnabled(parent_view_->profile())) I don't think it's appropriate to check this here. The omnibox view shouldn't second-guess the model on whether it's appropriate to do this. The instant controller, if anywhere, seems like a better place for this check. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:769: // We hide the caret just before destroying it, since destroying a caret that I still don't see why the hide/destroy part is done unconditionally as opposed to being done iff focus is not visible. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:775: // was called before. Nit: And add "While we do catch and handle these paint events (see OnPaint()), it doesn't seem to be enough to simply call HideCaret() while handling them. ..." and then a brief explanation of why/what problems you have. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:781: int caret_height = rect.Height() - 3; I suspect this calculation is wrong to hardcode "client height - 3". You probably want font_.GetBaseline() - font_y_adjustment_, but test how that looks with very large system default fonts (which should be shown with the tops and bottoms clipped off in the omnibox). https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:871: model()->SetFocusVisibility(true); Nit: Add comments about this. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1923: ApplyFocusVisibility(); Do you actually need to do this on every paint call? I thought since you destroyed the caret that you'd be fine until the visibility changed?
Thanks Peter, good comments. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:763: model()->SetFocusVisibility(true); On 2012/12/04 02:18:30, Peter Kasting wrote: > See question in the other CL about this particular call. Acknowledged. Will wait for the outcome of that comment. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:767: if (!chrome::search::IsInstantExtendedAPIEnabled(parent_view_->profile())) On 2012/12/04 02:18:30, Peter Kasting wrote: > I don't think it's appropriate to check this here. The omnibox view shouldn't > second-guess the model on whether it's appropriate to do this. The instant > controller, if anywhere, seems like a better place for this check. I was trying to minimize the manipulation of the Caret for users that don't have instant extended enabled. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:769: // We hide the caret just before destroying it, since destroying a caret that On 2012/12/04 02:18:30, Peter Kasting wrote: > I still don't see why the hide/destroy part is done unconditionally as opposed > to being done iff focus is not visible. Moved the DestroyCaret() call in an else, but that's the most I can do. Not hiding the caret before REcreating it causes issues (a solid caret). https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:775: // was called before. On 2012/12/04 02:18:30, Peter Kasting wrote: > Nit: And add "While we do catch and handle these paint events (see OnPaint()), > it doesn't seem to be enough to simply call HideCaret() while handling them. > ..." and then a brief explanation of why/what problems you have. Done. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:781: int caret_height = rect.Height() - 3; On 2012/12/04 02:18:30, Peter Kasting wrote: > I suspect this calculation is wrong to hardcode "client height - 3". You > probably want font_.GetBaseline() - font_y_adjustment_, but test how that looks > with very large system default fonts (which should be shown with the tops and > bottoms clipped off in the omnibox). The 3 points seem odd to me too, I agree. I looked into it, and referring to line 2459 or so and below, we can see that the scheme_rect, used to draw a slash in the scheme, is created with a height declared as the following: font_top + font_.GetBaseline() + kAdditionalSpaceOutsideFont In my test scenario, font_top = client_rect.top (0) + font_y_adjustment_ (1) = 1 font_.GetBaseline() = 16 kAdditionalSpaceOutsideFont = 3, set unconditionally in the previous lines as SkIntToScalar(2) * 1.5f for a total of 1 + 16 + 3 = 20. My previous approach, which gives the correct caret height, is: rect.Height() - 3 = 20 So we see the constant "3" points in existing code. I like the rect.Height() approach because it is in relation to the height of the omnibox, and not of font adjustments. I've tried making the text bigger on this Windows machine, but could only find the option to "Make text and other items larger or smaller", which is a DPI scaling and in which case the points measurements above still hold. If you know of a way to test unreasonably large fonts on windows, or someone who could help, I'd be interested in knowing. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:871: model()->SetFocusVisibility(true); On 2012/12/04 02:18:30, Peter Kasting wrote: > Nit: Add comments about this. Done. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1923: ApplyFocusVisibility(); On 2012/12/04 02:18:30, Peter Kasting wrote: > Do you actually need to do this on every paint call? I thought since you > destroyed the caret that you'd be fine until the visibility changed? I've tested this, and it is not so. This call needs to stay, or the caret shows up on a window resize. I guess we can see Destroy and Create as Hide/Show calls that actually work.
https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:767: if (!chrome::search::IsInstantExtendedAPIEnabled(parent_view_->profile())) On 2012/12/04 17:00:14, Mathieu Perreault wrote: > On 2012/12/04 02:18:30, Peter Kasting wrote: > > I don't think it's appropriate to check this here. The omnibox view shouldn't > > second-guess the model on whether it's appropriate to do this. The instant > > controller, if anywhere, seems like a better place for this check. > > I was trying to minimize the manipulation of the Caret for users that don't have > instant extended enabled. If they don't have instant extended enabled, this should never be called, should it? I just think this is a layering violation. If we need this functionality for some other feature someday, we're not going to want to reach into the platform-specific implementations to modify some safety checks. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:769: // We hide the caret just before destroying it, since destroying a caret that On 2012/12/04 17:00:14, Mathieu Perreault wrote: > On 2012/12/04 02:18:30, Peter Kasting wrote: > > I still don't see why the hide/destroy part is done unconditionally as opposed > > to being done iff focus is not visible. > > Moved the DestroyCaret() call in an else, but that's the most I can do. Not > hiding the caret before REcreating it causes issues (a solid caret). Then don't put any of it in an else, and add comments about this. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:781: int caret_height = rect.Height() - 3; On 2012/12/04 17:00:14, Mathieu Perreault wrote: > On 2012/12/04 02:18:30, Peter Kasting wrote: > > I suspect this calculation is wrong to hardcode "client height - 3". You > > probably want font_.GetBaseline() - font_y_adjustment_, but test how that > looks > > with very large system default fonts (which should be shown with the tops and > > bottoms clipped off in the omnibox). > > The 3 points seem odd to me too, I agree. I looked into it, and referring to > line 2459 or so and below, we can see that the scheme_rect, used to draw a slash > in the scheme, is created with a height declared as the following: > > font_top + font_.GetBaseline() + kAdditionalSpaceOutsideFont You don't want to refer to that rect. It's not guaranteed to have any relation to the cursor height. My directions before weren't right either, though, because for some reason I was thinking the cursor should extend down to the baseline. It doesn't, it takes the full font height. So I think what you want here is either just font_.GetHeight() or maybe std::min(font_.GetHeight(), client_rect_height). Testing instructions below. > If you know of a way to test unreasonably large fonts on windows, or someone who > could help, I'd be interested in knowing. Control Panel -> Appearance and Personalization -> Change window glass colors -> Advanced appearance settings... -> change the Message Box font size to something larger or smaller. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1923: ApplyFocusVisibility(); On 2012/12/04 17:00:14, Mathieu Perreault wrote: > On 2012/12/04 02:18:30, Peter Kasting wrote: > > Do you actually need to do this on every paint call? I thought since you > > destroyed the caret that you'd be fine until the visibility changed? > > I've tested this, and it is not so. This call needs to stay, or the caret shows > up on a window resize. I guess we can see Destroy and Create as Hide/Show calls > that actually work. Then you need to add commentary about this too. The theme here is that your code should contain comments about every single piece of information you found throughout your testing and implementation process. Someone later isn't going to have this knowledge otherwise and will be mystified.
Sorry, the diffs got borked. Read my comments below. Thanks! https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:767: if (!chrome::search::IsInstantExtendedAPIEnabled(parent_view_->profile())) On 2012/12/04 21:24:43, Peter Kasting wrote: > On 2012/12/04 17:00:14, Mathieu Perreault wrote: > > On 2012/12/04 02:18:30, Peter Kasting wrote: > > > I don't think it's appropriate to check this here. The omnibox view > shouldn't > > > second-guess the model on whether it's appropriate to do this. The instant > > > controller, if anywhere, seems like a better place for this check. > > > > I was trying to minimize the manipulation of the Caret for users that don't > have > > instant extended enabled. > > If they don't have instant extended enabled, this should never be called, should > it? > > I just think this is a layering violation. If we need this functionality for > some other feature someday, we're not going to want to reach into the > platform-specific implementations to modify some safety checks. Well there is an ApplyCaretVisibility() call in OnPaint, so it will get called. You're right that it is a violation, though. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:769: // We hide the caret just before destroying it, since destroying a caret that On 2012/12/04 21:24:43, Peter Kasting wrote: > On 2012/12/04 17:00:14, Mathieu Perreault wrote: > > On 2012/12/04 02:18:30, Peter Kasting wrote: > > > I still don't see why the hide/destroy part is done unconditionally as > opposed > > > to being done iff focus is not visible. > > > > Moved the DestroyCaret() call in an else, but that's the most I can do. Not > > hiding the caret before REcreating it causes issues (a solid caret). > > Then don't put any of it in an else, and add comments about this. Done. https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:781: int caret_height = rect.Height() - 3; On 2012/12/04 21:24:43, Peter Kasting wrote: > On 2012/12/04 17:00:14, Mathieu Perreault wrote: > > On 2012/12/04 02:18:30, Peter Kasting wrote: > > > I suspect this calculation is wrong to hardcode "client height - 3". You > > > probably want font_.GetBaseline() - font_y_adjustment_, but test how that > > looks > > > with very large system default fonts (which should be shown with the tops > and > > > bottoms clipped off in the omnibox). > > > > The 3 points seem odd to me too, I agree. I looked into it, and referring to > > line 2459 or so and below, we can see that the scheme_rect, used to draw a > slash > > in the scheme, is created with a height declared as the following: > > > > font_top + font_.GetBaseline() + kAdditionalSpaceOutsideFont > > You don't want to refer to that rect. It's not guaranteed to have any relation > to the cursor height. > > My directions before weren't right either, though, because for some reason I was > thinking the cursor should extend down to the baseline. It doesn't, it takes > the full font height. So I think what you want here is either just > font_.GetHeight() or maybe std::min(font_.GetHeight(), client_rect_height). > Testing instructions below. > > > If you know of a way to test unreasonably large fonts on windows, or someone > who > > could help, I'd be interested in knowing. > > Control Panel -> Appearance and Personalization -> Change window glass colors -> > Advanced appearance settings... -> change the Message Box font size to something > larger or smaller. Wow, it was indeed font_.GetHeight()! I tested with the biggest font size I could, and this works. Thanks! https://codereview.chromium.org/11418144/diff/7003/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1923: ApplyFocusVisibility(); On 2012/12/04 21:24:43, Peter Kasting wrote: > On 2012/12/04 17:00:14, Mathieu Perreault wrote: > > On 2012/12/04 02:18:30, Peter Kasting wrote: > > > Do you actually need to do this on every paint call? I thought since you > > > destroyed the caret that you'd be fine until the visibility changed? > > > > I've tested this, and it is not so. This call needs to stay, or the caret > shows > > up on a window resize. I guess we can see Destroy and Create as Hide/Show > calls > > that actually work. > > Then you need to add commentary about this too. > > The theme here is that your code should contain comments about every single > piece of information you found throughout your testing and implementation > process. Someone later isn't going to have this knowledge otherwise and will be > mystified. Got it. Makes sense, thanks.
https://codereview.chromium.org/11418144/diff/15001/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11418144/diff/15001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:770: // vertical bar. Nit: This second sentence might be clearer as: "We even hide and destroy the caret if we're going to create it again below. If the caret was already visible on entry to this function, the CreateCaret() call (which first destroys the old caret) might leave a solid vertical bar for the same reason as above. Unconditionally hiding prevents this. The caret could be visible on entry to this function if the underlying edit control had re-created it automatically (see comments in OnPaint())." Please correct me if I got the precise reasoning here incorrect. https://codereview.chromium.org/11418144/diff/15001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1930: // show when it is supposed to be hidden (i.e. it is re-created by the OS). Nit: If you could be clearer in this second sentence when this happens, it'd be good, e.g. "This is because the underlying edit control will automatically re-create the caret when it receives a size/move event." This also suggests that maybe instead of putting this in OnPaint() we could put it in some other more specific handler...
Thanks for the quick turnaround! https://codereview.chromium.org/11418144/diff/15001/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11418144/diff/15001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:770: // vertical bar. On 2012/12/04 22:45:35, Peter Kasting wrote: > Nit: This second sentence might be clearer as: > > "We even hide and destroy the caret if we're going to create it again below. If > the caret was already visible on entry to this function, the CreateCaret() call > (which first destroys the old caret) might leave a solid vertical bar for the > same reason as above. Unconditionally hiding prevents this. The caret could be > visible on entry to this function if the underlying edit control had re-created > it automatically (see comments in OnPaint())." > > Please correct me if I got the precise reasoning here incorrect. Done. It's precisely right and well worded. https://codereview.chromium.org/11418144/diff/15001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1930: // show when it is supposed to be hidden (i.e. it is re-created by the OS). On 2012/12/04 22:45:35, Peter Kasting wrote: > Nit: If you could be clearer in this second sentence when this happens, it'd be > good, e.g. > > "This is because the underlying edit control will automatically re-create the > caret when it receives a size/move event." > > This also suggests that maybe instead of putting this in OnPaint() we could put > it in some other more specific handler... Done. I modified a little bit to say "paint events", of which resize is an example. The move event is not a good example because Windows does not repaint the window in this case (it probably just moves it in graphical memory). Generally, if the textfield view is dirty, it will get repainted.
Rebased on top of Samarth's change. Copied some more of his comments, and moved SetCaretVisibility(true) from OnBeforePossibleChange to OnLButtonDown.
https://codereview.chromium.org/11418144/diff/22001/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11418144/diff/22001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:764: // will short-circuit, preventing us from reaching You did verify this short-circuiting occurs on Windows, right? I just want to make sure we actually need this. https://codereview.chromium.org/11418144/diff/22001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:768: model()->SetCaretVisibility(true); Nit: This should be next to the comment. https://codereview.chromium.org/11418144/diff/22001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1726: model()->SetCaretVisibility(true); A couple things. (1) Left clicks aren't enough, you need right clicks too. (2) You also need to simulate the "gaining focus" behavior so we modify the selection properly. I think you may be able to achieve both of these by removing the call here and instead modifying OnMouseActivate() to: (1) Check "if ((!model()->has_focus() || !model()->is_caret_visible()) && ..." (2) Place this call inside that conditional, next to the setting of |gaining_focus_|. You should test this to verify that it works. You should probably try to rig up a test environment like follows: * Every n seconds, we ask the omnibox to make its focus invisible * Then, test with the omnibox empty; containing text where the caret was previously at the beginning/middle/end; and containing text with a partial and a complete selection * Test left/right/middle clicks, drags, double and triple clicks https://codereview.chromium.org/11418144/diff/22001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1942: // receives a paint event, e.g. a window resize event. A resize event and a paint event aren't the same thing -- maybe you mean "when it receives certain events that trigger repaints, e.g. window resize events"?
Thanks, have a look. https://codereview.chromium.org/11418144/diff/22001/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11418144/diff/22001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:764: // will short-circuit, preventing us from reaching On 2012/12/05 02:28:50, Peter Kasting wrote: > You did verify this short-circuiting occurs on Windows, right? I just want to > make sure we actually need this. Modified the comment to better reflect that in certain cases (such as when a new tab is created, OnSetFocus is not called. https://codereview.chromium.org/11418144/diff/22001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:768: model()->SetCaretVisibility(true); On 2012/12/05 02:28:50, Peter Kasting wrote: > Nit: This should be next to the comment. Done. https://codereview.chromium.org/11418144/diff/22001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1726: model()->SetCaretVisibility(true); On 2012/12/05 02:28:50, Peter Kasting wrote: > A couple things. > > (1) Left clicks aren't enough, you need right clicks too. > (2) You also need to simulate the "gaining focus" behavior so we modify the > selection properly. > > I think you may be able to achieve both of these by removing the call here and > instead modifying OnMouseActivate() to: > (1) Check "if ((!model()->has_focus() || !model()->is_caret_visible()) && ..." > (2) Place this call inside that conditional, next to the setting of > |gaining_focus_|. > > You should test this to verify that it works. You should probably try to rig up > a test environment like follows: > > * Every n seconds, we ask the omnibox to make its focus invisible > * Then, test with the omnibox empty; containing text where the caret was > previously at the beginning/middle/end; and containing text with a partial and a > complete selection > * Test left/right/middle clicks, drags, double and triple clicks Your proposal was almost correct; I need to check if it already has focus (invisible focus) and the proposed "if" clause would have not done the trick. Let me know what you think of this implementation. As far as testing, I have tested this with left click and right click, triple and double clicks. Dragging something on the omnibox does not activate this event. I can't reproduce a middle click on this RDP setup (I'm at home at the moment). I have not implemented the "invisible focus every n seconds" test because we should never have text in the omnibox and have invisible focus, and would like to worry about these cases in another effort if possible. If there is an essential case I missed or if you disagree, let me know. https://codereview.chromium.org/11418144/diff/22001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1942: // receives a paint event, e.g. a window resize event. On 2012/12/05 02:28:50, Peter Kasting wrote: > A resize event and a paint event aren't the same thing -- maybe you mean "when > it receives certain events that trigger repaints, e.g. window resize events"? Done.
https://codereview.chromium.org/11418144/diff/21002/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11418144/diff/21002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:766: // when a new tab is created and the previous tab had invisible focus. Given that this apparently does short-circuit, I think the comment in the Views impl is actually somewhat clearer here, despite not having an example (we don't need one). https://codereview.chromium.org/11418144/diff/21002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:767: model()->SetCaretVisibility(true); The Views impl calls this before setting focus. I don't care which order you guys use, but be consistent. It's much easier to maintain things later if each platform is as close to the other platforms' implementations as possible in the small details like this. https://codereview.chromium.org/11418144/diff/21002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1776: if (model()->has_focus()) { I don't understand why you did things this way instead of the way I suggested. I was very intentional about wanting you to trigger the gaining_focus_ manipulation because that in turn is what triggers the click-to-select-all etc. behavior. I don't see advantage in going this route, it's just monotonically less correct. Even if you don't have the ability to test the with-text case it's much better to write code that's likely correct instead of code that's certainly not correct. But frankly, no, I don't want you to worry about those cases later, I'd like things to be correct from the start. I also don't understand your "the proposed if clause would not have done the trick" comment. The proposed if clause explicitly checked for invisible focus. You need to explain in more detail if something about it wasn't right.
https://codereview.chromium.org/11418144/diff/21002/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11418144/diff/21002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:766: // when a new tab is created and the previous tab had invisible focus. On 2012/12/05 04:10:57, Peter Kasting wrote: > Given that this apparently does short-circuit, I think the comment in the Views > impl is actually somewhat clearer here, despite not having an example (we don't > need one). Done. https://codereview.chromium.org/11418144/diff/21002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:767: model()->SetCaretVisibility(true); On 2012/12/05 04:10:57, Peter Kasting wrote: > The Views impl calls this before setting focus. I don't care which order you > guys use, but be consistent. It's much easier to maintain things later if each > platform is as close to the other platforms' implementations as possible in the > small details like this. This is definitely the right order for Windows, since we can't act on a caret if we don't have focus. ChromeOS doesn't differentiate. I will leave this part as is. https://codereview.chromium.org/11418144/diff/21002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1776: if (model()->has_focus()) { On 2012/12/05 04:10:57, Peter Kasting wrote: > I don't understand why you did things this way instead of the way I suggested. > I was very intentional about wanting you to trigger the gaining_focus_ > manipulation because that in turn is what triggers the click-to-select-all etc. > behavior. I don't see advantage in going this route, it's just monotonically > less correct. Even if you don't have the ability to test the with-text case > it's much better to write code that's likely correct instead of code that's > certainly not correct. But frankly, no, I don't want you to worry about those > cases later, I'd like things to be correct from the start. > > I also don't understand your "the proposed if clause would not have done the > trick" comment. The proposed if clause explicitly checked for invisible focus. > You need to explain in more detail if something about it wasn't right. Discussed offline. I had misunderstood the original proposal.
LGTM. If you can test the with-text cases, even if it's after you land this, that'd be nice. I'd rather know this works right than have it come back to bite us down the road. https://chromiumcodereview.appspot.com/11418144/diff/21002/chrome/browser/ui/... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://chromiumcodereview.appspot.com/11418144/diff/21002/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:767: model()->SetCaretVisibility(true); On 2012/12/05 04:28:35, Mathieu Perreault wrote: > On 2012/12/05 04:10:57, Peter Kasting wrote: > > The Views impl calls this before setting focus. I don't care which order you > > guys use, but be consistent. It's much easier to maintain things later if > each > > platform is as close to the other platforms' implementations as possible in > the > > small details like this. > > This is definitely the right order for Windows, since we can't act on a caret if > we don't have focus. ChromeOS doesn't differentiate. I will leave this part as > is. K. See if you can get the other CL to change to match, then. I wonder if we should have a comment here noting what you just noted...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/11418144/26003
Failed to apply patch for chrome/browser/ui/views/omnibox/omnibox_view_win.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/ui/views/omnibox/omnibox_view_win.cc
Hunk #1 FAILED at 760.
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/omnibox/omnibox_view_win.cc.rej
Patch: chrome/browser/ui/views/omnibox/omnibox_view_win.cc
Index: chrome/browser/ui/views/omnibox/omnibox_view_win.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
b/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
index
cdfce5fc6b9f1a7eb3e29d7aae290b38491a33b1..3d06e5da2a37886bdb21d9f81db292a044394444
100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
@@ -760,11 +760,41 @@ void OmniboxViewWin::UpdatePopup() {
void OmniboxViewWin::SetFocus() {
::SetFocus(m_hWnd);
-}
-
-void OmniboxViewWin::ApplyFocusVisibility() {
- // TODO(mathp): implement for Windows.
- NOTIMPLEMENTED();
+ // Restore caret visibility if focused explicitly. We need to do this here
+ // because if we already have invisible focus, the ::SetFocus call above will
+ // short-circuit, preventing us from reaching OmniboxEditModel::OnSetFocus(),
+ // which handles restoring visibility when we didn't previously have focus.
+ model()->SetCaretVisibility(true);
+}
+
+void OmniboxViewWin::ApplyCaretVisibility() {
+ // We hide the caret just before destroying it, since destroying a caret that
+ // is in the "solid" phase of its blinking will leave a solid vertical bar.
+ // We even hide and destroy the caret if we're going to create it again
below.
+ // If the caret was already visible on entry to this function, the
+ // CreateCaret() call (which first destroys the old caret) might leave a
solid
+ // vertical bar for the same reason as above. Unconditionally hiding
prevents
+ // this. The caret could be visible on entry to this function if the
+ // underlying edit control had re-created it automatically (see comments in
+ // OnPaint()).
+ HideCaret();
+ // We use DestroyCaret()/CreateCaret() instead of simply HideCaret()/
+ // ShowCaret() because HideCaret() is not sticky across paint events, e.g. a
+ // window resize will effectively restore caret visibility, regardless of
+ // whether HideCaret() was called before. While we do catch and handle these
+ // paint events (see OnPaint()), it doesn't seem to be enough to simply call
+ // HideCaret() while handling them because of the unpredictability of this
+ // Windows API. According to the documentation, it should be a cumulative
call
+ // e.g. 5 hide calls should be balanced by 5 show calls. We have not found
+ // this to be true, which may be explained by the fact that this API is
called
+ // internally in Windows, as well.
+ ::DestroyCaret();
+ if (model()->is_focus_visible()) {
+ ::CreateCaret(m_hWnd, (HBITMAP) NULL, 1, font_.GetHeight());
+ // According to the Windows API documentation, a newly created caret needs
+ // ShowCaret to be visible.
+ ShowCaret();
+ }
}
void OmniboxViewWin::SetDropHighlightPosition(int position) {
@@ -1741,7 +1771,7 @@ LRESULT OmniboxViewWin::OnMouseActivate(HWND window,
// reached before OnXButtonDown(), preventing us from detecting this properly
// there. Also in those cases, we need to already know in OnSetFocus() that
// we should not restore the saved selection.
- if (!model()->has_focus() &&
+ if ((!model()->has_focus() || !model()->is_caret_visible()) &&
((mouse_message == WM_LBUTTONDOWN || mouse_message == WM_RBUTTONDOWN)) &&
(result == MA_ACTIVATE)) {
if (gaining_focus_) {
@@ -1751,6 +1781,11 @@ LRESULT OmniboxViewWin::OnMouseActivate(HWND window,
return result;
}
gaining_focus_.reset(new ScopedFreeze(this, GetTextObjectModel()));
+
+ // Explicitely set focus visibility in the case of clicking on the omnibox,
+ // which will remove invisible focus if present.
+ model()->SetCaretVisibility(true);
+
// NOTE: Despite |mouse_message| being WM_XBUTTONDOWN here, we're not
// guaranteed to call OnXButtonDown() later! Specifically, if this is the
// second click of a double click, we'll reach here but later call
@@ -1899,6 +1934,12 @@ void OmniboxViewWin::OnPaint(HDC bogus_hdc) {
rect.left, rect.top, SRCCOPY);
memory_dc.SelectBitmap(old_bitmap);
edit_hwnd = old_edit_hwnd;
+
+ // This needs to be called regardless of the current state of the caret, even
+ // if reaffirming a current state (hidden or shown). This is because the
+ // underlying edit control will automatically re-create the caret when it
+ // receives certain events that trigger repaints, e.g. window resize events.
+ ApplyCaretVisibility();
}
void OmniboxViewWin::OnPaste() {
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/11418144/19002
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/11418144/31001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/11418144/28005
Message was sent while issue was closed.
Change committed as 171236 |
