OmniboxView: Attempt to fix a crash in GetSelectedText().

OmniboxView: Attempt to fix a crash in GetSelectedText().

BUG=101938
TEST=none

R=pkasting@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107620

[Peter Kasting, 2011-10-27 18:17:53]

LGTM with one addition

http://codereview.chromium.org/8417004/diff/1/chrome/browser/ui/views/omnibox...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/8417004/diff/1/chrome/browser/ui/views/omnibox...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:615:
GetWindowText(WriteInto(&str, len), len);
Can you also put this line under the condition:

  if (len > 1)
    GetWindowText(WriteInto(&str, len), len);

GetWindowText() seems to handle NULL safely but I'd rather just be consistent.

http://codereview.chromium.org/8417004/diff/1/chrome/browser/ui/views/omnibox...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2113: return string16();
Nit: Might want to add an EOL comment: "// GetSelText() will crash if we pass it
NULL."

[James Hawkins, 2011-10-27 18:26:41]

http://codereview.chromium.org/8417004/diff/1/chrome/browser/ui/views/omnibox...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/8417004/diff/1/chrome/browser/ui/views/omnibox...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:615:
GetWindowText(WriteInto(&str, len), len);
On 2011/10/27 18:17:53, Peter Kasting wrote:
> Can you also put this line under the condition:
> 
>   if (len > 1)
>     GetWindowText(WriteInto(&str, len), len);
> 
> GetWindowText() seems to handle NULL safely but I'd rather just be consistent.

Done. I structured it similar to the other change piece so it reads more of
"Nothing to do in this case" than "GetWindowText() won't crash here, but we're
being safe."  That cool?

http://codereview.chromium.org/8417004/diff/1/chrome/browser/ui/views/omnibox...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2113: return string16();
On 2011/10/27 18:17:53, Peter Kasting wrote:
> Nit: Might want to add an EOL comment: "// GetSelText() will crash if we pass
it
> NULL."

Done.

[Peter Kasting, 2011-10-27 18:38:54]

On 2011/10/27 18:26:41, James Hawkins wrote:
> On 2011/10/27 18:17:53, Peter Kasting wrote:
> > Can you also put this line under the condition:
> > 
> >   if (len > 1)
> >     GetWindowText(WriteInto(&str, len), len);
> > 
> > GetWindowText() seems to handle NULL safely but I'd rather just be
consistent.
> 
> Done. I structured it similar to the other change piece so it reads more of
> "Nothing to do in this case" than "GetWindowText() won't crash here, but we're
> being safe."  That cool?

Sure, whatever.  The other way is shorter but I don't really care much.