Omnibox: Change |inline_autocomplete_offset| to |inline_autocompletion|

chrome/browser/ui/omnibox/omnibox_edit_model.h

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_UI_OMNIBOX_OMNIBOX_EDIT_MODEL_H_
 #define CHROME_BROWSER_UI_OMNIBOX_OMNIBOX_EDIT_MODEL_H_
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 252 matching lines @@
   //   |text| is either the new temporary text from the user manually selecting
   //     a different match, or the inline autocomplete text.  We distinguish by
   //     checking if |destination_for_temporary_text_change| is NULL.
   //   |destination_for_temporary_text_change| is NULL (if temporary text should
   //     not change) or the pre-change destination URL (if temporary text should
   //     change) so we can save it off to restore later.
   //   |keyword| is the keyword to show a hint for if |is_keyword_hint| is true,
   //     or the currently selected keyword if |is_keyword_hint| is false (see
   //     comments on keyword_ and is_keyword_hint_).
   void OnPopupDataChanged(
-      const string16& text,
+      const string16 text,

[Peter Kasting, 2013/07/16 18:09:05]

Removing the '&' here seems like a mistake

[Mark P, 2013/07/16 18:53:23]

This is critical.  If the & is there, it doesn't work.  It took me two hours to
debug this and figure out the & is the cause.

We pass match.inline_autocompletion into this function as |text|.  This function
resets match, which means it clears match.inline_autocompletion.  Hence,
everywhere later in the function that refers to |text| sees an empty string.

[Peter Kasting, 2013/07/16 18:59:55]

Hmm.  In that case perhaps the affected caller(s) should copy the string before
calling the function, with a comment about why they're doing it.

I mean, there need to be comments regardless, but it's more efficient and
perhaps more clear to do this explicitly at the caller side in the affected
places rather than implicitly in the function call itself.

[Mark P, 2013/07/16 19:47:12]

Are you sure you want me to do this?  It requires putting the ugly "make a copy"
code and comment at the location of three separate callers.

Wouldn't it be easier to add a comment here?
// We cannot take |text| or |keyword| as references because callers tends to get
these values from the current match and OnPopupDataChanged() resets the current
match.  If OnPopupDataChanged() took them as references, it would lose knowledge
of their original values.

       GURL* destination_for_temporary_text_change,
       const string16& keyword,
       bool is_keyword_hint);
 
   // Called by the OmniboxView after something changes, with details about what
   // state changes occured.  Updates internal state, updates the popup if
   // necessary, and returns true if any significant changes occurred.  Note that
   // |text_differs| may be set even if |old_text| == |new_text|, e.g. if we've
   // just committed an IME composition.
   //
@@ skipping 227 matching lines @@
   // an exact keyword match.  If this is true then keyword mode will be
   // triggered automatically if the input is "<keyword> <search string>".  We
   // allow this when CreatedKeywordSearchByInsertingSpaceInMiddle() is true.
   // This has no effect if we're already in keyword mode.
   bool allow_exact_keyword_match_;
 
   DISALLOW_COPY_AND_ASSIGN(OmniboxEditModel);
 };
 
 #endif  // CHROME_BROWSER_UI_OMNIBOX_OMNIBOX_EDIT_MODEL_H_