Tweaks to copy/paste of omnibox on windows:

Tweaks to copy/paste of omnibox on windows:
. Makes copy prefix the text with http if the user hasn't edited the
  text, and the scheme is http but the text doesn't include http.
. Makes copying use the url as the text if the user hasn't edited the
  text and it's http/https.
. Makes control-insert/shift-insert use our copy/paste.

BUG=41639
41489 41493
TEST=make sure copy/paste from the omnibox work sanely. Seriously
     though, see the three bugs for specifics.

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

[Peter Kasting, 2010-04-21 22:52:04]

http://codereview.chromium.org/1761002/diff/1/2
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/1761002/diff/1/2#newcode1170
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1170: void
AutocompleteEditViewWin::OnCopy() {
The modified logic in here isn't right.  We need to re-add the scheme even when
the user is editing.  But we shouldn't re-add the scheme if the user hasn't
selected at least the full hostname, or if the current hostname (or scheme)
doesn't match the permanent URL.

http://codereview.chromium.org/1761002/diff/1/2#newcode1862
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1862: if ((key != 'V')
|| (GetKeyState(VK_SHIFT) >= 0)) {
Why did you make this change?  Its sole effect is to make ctrl-shift-insert
paste, which isn't native behavior.

http://codereview.chromium.org/1761002/diff/1/2#newcode1926
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1926: case VK_INSERT:
Why not just append "case VK_INSERT:" below "case 'C':" above?  That would get
alt-ctrl-insert correct ("do nothing"), which this doesn't.

[sky, 2010-04-22 04:28:47]

I believe I've addressed all your concerns. I'm keeping this as one patch given
I can't land anything until you revert your revert.

[Peter Kasting, 2010-04-22 20:03:40]

LGTM with one substantive comment, and: can this be unittested?  It's subtle and
edgy and thus definitely good test fodder.

Were you planning to add code to Mac/Linux for this too?

http://codereview.chromium.org/1761002/diff/5001/6001
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/1761002/diff/5001/6001#newcode185
chrome/browser/autocomplete/autocomplete_edit.cc:185: if (GetURLForText(*text,
url)) {
Nit: You call GetURLForText() on both branches, you could hoist it.

http://codereview.chromium.org/1761002/diff/5001/6001#newcode194
chrome/browser/autocomplete/autocomplete_edit.cc:194: // host and the user
hasn't edited the host.
Nit: "selected the host" -> "selected the entire host", "edited the host" ->
"edited the host or manually removed the scheme".  (Your code gets this last bit
right, but it's subtle and thus worth calling out in the comment.)

http://codereview.chromium.org/1761002/diff/5001/6001#newcode195
chrome/browser/autocomplete/autocomplete_edit.cc:195: if (GetURLForText(*text,
url) && url->SchemeIs(chrome::kHttpScheme)) {
There's a subtle problem here: you're returning |url| based on the entire text
string rather than just the selected part.

It is probably OK to solve this via "if (GetURLForText(text->substr(0,
sel_length), url) && ...".  In that case, the comment above "the text parses"
should be "the selection parses".

http://codereview.chromium.org/1761002/diff/5001/6002
File chrome/browser/autocomplete/autocomplete_edit.h (right):

http://codereview.chromium.org/1761002/diff/5001/6002#newcode154
chrome/browser/autocomplete/autocomplete_edit.h:154: // to true and |url| set to
the url to write.
Nit: You might want to add a clause motivating the need for this, e.g. "Invoked
to adjust the text (e.g. by adding "http://" to the front) before writing..."

[sky, 2010-04-26 19:36:00]

I've added a test that exercises the new method directly. I considered writing a
browsertest and invoking copy directly, but that would require a number of
changes to the various EditView implementations. Let me know what you think.

I'll do mac/linux next.

http://codereview.chromium.org/1761002/diff/5001/6001
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/1761002/diff/5001/6001#newcode195
chrome/browser/autocomplete/autocomplete_edit.cc:195: if (GetURLForText(*text,
url) && url->SchemeIs(chrome::kHttpScheme)) {
On 2010/04/22 20:03:41, Peter Kasting wrote:
> There's a subtle problem here: you're returning |url| based on the entire text
> string rather than just the selected part.
> 
> It is probably OK to solve this via "if (GetURLForText(text->substr(0,
> sel_length), url) && ...".  In that case, the comment above "the text parses"
> should be "the selection parses".

text is just the selected text. I've updated the description to indicate this.

[Peter Kasting, 2010-04-26 20:10:27]

LGTM

http://codereview.chromium.org/1761002/diff/14001/15001
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/1761002/diff/14001/15001#newcode194
chrome/browser/autocomplete/autocomplete_edit.cc:194: // entire host and the
user hasn't edited the host, or manually removed the
Nit: This would be clearer if you moved the comma:
"entire host, and the user hasn't edited the host or manually removed the"

http://codereview.chromium.org/1761002/diff/14001/15003
File chrome/browser/autocomplete/autocomplete_edit_unittest.cc (right):

http://codereview.chromium.org/1761002/diff/14001/15003#newcode113
chrome/browser/autocomplete/autocomplete_edit_unittest.cc:113: for (size_t i =
0; i < ARRAYSIZE_UNSAFE(input); ++i) {
Nit: Do we have to use the UNSAFE variant?  Will this work with "arraysize()",
since the struct isn't anonymous?

[sky, 2010-04-26 21:24:01]

http://codereview.chromium.org/1761002/diff/14001/15003
File chrome/browser/autocomplete/autocomplete_edit_unittest.cc (right):

http://codereview.chromium.org/1761002/diff/14001/15003#newcode113
chrome/browser/autocomplete/autocomplete_edit_unittest.cc:113: for (size_t i =
0; i < ARRAYSIZE_UNSAFE(input); ++i) {
On 2010/04/26 20:10:27, Peter Kasting wrote:
> Nit: Do we have to use the UNSAFE variant?  Will this work with "arraysize()",
> since the struct isn't anonymous?

gcc doesn't like unsafe here.

[Evan Stade, 2010-04-26 23:52:33]

http://codereview.chromium.org/1761002/diff/18002/23001
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/1761002/diff/18002/23001#newcode186
chrome/browser/autocomplete/autocomplete_edit.cc:186: if
(!user_input_in_progress() && is_all_selected) {
comment here about no unescaping? otherwise this code seems subtle.