Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
328031:
Refactor paste and go out of AutocompleteEdit. (Closed)
|
Created:
11 years, 1 month ago by tfarina (gmail-do not use) Modified:
9 years, 7 months ago Reviewers:
Peter Kasting CC:
chromium-reviews_googlegroups.com, ben+cc_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionRefactor paste and go out of AutocompleteEdit.
Changes:
- Added a new class called SearchVersusNavigateClassifier to classify some given text in a query or an URL.
- The profile class owns the object SearchVersusNavigateClassifier, also a getter function was added.
- Cleanup was made in AutocompleteEdit.
BUG=21317
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30473
Patch Set 1 #
Total comments: 11
Patch Set 2 : '' #
Total comments: 4
Patch Set 3 : '' #
Total comments: 15
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 28
Patch Set 6 : '' #
Total comments: 16
Patch Set 7 : '' #
Total comments: 19
Patch Set 8 : '' #
Total comments: 9
Patch Set 9 : '' #
Total comments: 4
Patch Set 10 : style fixes #
Messages
Total messages: 28 (0 generated)
Peter this is my idea, but I think you'll have comments about that.
http://codereview.chromium.org/328031/diff/1/3 File chrome/browser/url_verifier.cc (right): http://codereview.chromium.org/328031/diff/1/3#newcode12 Line 12: bool UrlVerifier::IsValidUrl(const std::wstring& text, Profile* profile) { Technically we're not really saying whether |text| is a valid URL, instead we're asking whether we should search or navigate in reaction to it. I'd change the name to indicate this. http://codereview.chromium.org/328031/diff/1/3#newcode13 Line 13: AutocompleteController controller(profile); You don't want to dynamically create the controller. You want the code relating to |synchronous_controller| in autocomplete_edit.cc. http://codereview.chromium.org/328031/diff/1/3#newcode24 Line 24: if (url.is_valid()) This is the wrong check. This URL is nearly always going to be valid. What you want is the condition under which AutocompleteEdit::is_paste_and_search() would return true.
http://codereview.chromium.org/328031/diff/1/3 File chrome/browser/url_verifier.cc (right): http://codereview.chromium.org/328031/diff/1/3#newcode12 Line 12: bool UrlVerifier::IsValidUrl(const std::wstring& text, Profile* profile) { On 2009/10/26 21:29:53, Peter Kasting wrote: > Technically we're not really saying whether |text| is a valid URL, instead we're > asking whether we should search or navigate in reaction to it. I'd change the > name to indicate this. Maybe either CanNavigateToUrl or CanGoToUrl? http://codereview.chromium.org/328031/diff/1/3#newcode13 Line 13: AutocompleteController controller(profile); On 2009/10/26 21:29:53, Peter Kasting wrote: > You don't want to dynamically create the controller. You want the code relating > to |synchronous_controller| in autocomplete_edit.cc. I don't know what you meant here. Perphas, passing the |synchronous_controller| by parameter? http://codereview.chromium.org/328031/diff/1/3#newcode24 Line 24: if (url.is_valid()) On 2009/10/26 21:29:53, Peter Kasting wrote: > This is the wrong check. This URL is nearly always going to be valid. What you > want is the condition under which AutocompleteEdit::is_paste_and_search() would > return true. OK, I will change that check.
http://codereview.chromium.org/328031/diff/1/3 File chrome/browser/url_verifier.cc (right): http://codereview.chromium.org/328031/diff/1/3#newcode12 Line 12: bool UrlVerifier::IsValidUrl(const std::wstring& text, Profile* profile) { On 2009/10/26 21:45:18, tfarina wrote: > Maybe either CanNavigateToUrl or CanGoToUrl? How about class SearchVersusNavigateClassifier { static bool IsSearch(...); }; http://codereview.chromium.org/328031/diff/1/3#newcode13 Line 13: AutocompleteController controller(profile); On 2009/10/26 21:45:18, tfarina wrote: > On 2009/10/26 21:29:53, Peter Kasting wrote: > > You don't want to dynamically create the controller. You want the code > relating > > to |synchronous_controller| in autocomplete_edit.cc. > I don't know what you meant here. Perphas, passing the |synchronous_controller| > by parameter? No, that obviously wouldn't work because the whole point is to be called by people who aren't the AutocompleteEdit. I mean that you want to take that code. The creation, usage, cleanup, etc. You're refactoring it from AutocompleteEdit, so refactor it.
http://codereview.chromium.org/328031/diff/1/3 File chrome/browser/url_verifier.cc (right): http://codereview.chromium.org/328031/diff/1/3#newcode12 Line 12: bool UrlVerifier::IsValidUrl(const std::wstring& text, Profile* profile) { On 2009/10/26 21:48:53, Peter Kasting wrote: > On 2009/10/26 21:45:18, tfarina wrote: > > Maybe either CanNavigateToUrl or CanGoToUrl? > > How about > > class SearchVersusNavigateClassifier { > static bool IsSearch(...); > }; Changed. http://codereview.chromium.org/328031/diff/1/3#newcode13 Line 13: AutocompleteController controller(profile); On 2009/10/26 21:48:53, Peter Kasting wrote: > No, that obviously wouldn't work because the whole point is to be called by > people who aren't the AutocompleteEdit. > > I mean that you want to take that code. The creation, usage, cleanup, etc. > You're refactoring it from AutocompleteEdit, so refactor it. Since I'm refactoring it, maybe the stuff about paste_and_go should goes to this class? (paste_and_go_url_, transition_, alterante_nav_url_) and add getters for these members? Are you thinking in moving these stuff to this class, or doesn't make sense? http://codereview.chromium.org/328031/diff/1/3#newcode24 Line 24: if (url.is_valid()) On 2009/10/26 21:45:18, tfarina wrote: > On 2009/10/26 21:29:53, Peter Kasting wrote: > > This is the wrong check. This URL is nearly always going to be valid. What > you > > want is the condition under which AutocompleteEdit::is_paste_and_search() > would > > return true. > OK, I will change that check. > Changed.
On 2009/10/26 22:47:22, tfarina wrote: > Since I'm refactoring it, maybe the stuff about paste_and_go should goes to this > class? (paste_and_go_url_, transition_, alterante_nav_url_) and add getters for > these members? I suggest just making them all outparams to the main function, which could then be named Classify: void Classify(cont std::wstring& input, bool* is_search, GURL* destination_url, PageTransition::Type* transition, GURL* alternate_nav_url); http://codereview.chromium.org/328031/diff/4002/6004 File chrome/browser/search_versus_navigate_classifier.cc (right): http://codereview.chromium.org/328031/diff/4002/6004#newcode12 Line 12: static bool is_synchronous_controller_loaded = false; Nit: No need for the bool, we can key off whether the pointer is NULL. http://codereview.chromium.org/328031/diff/4002/6004#newcode17 Line 17: if (!is_synchronous_controller_loaded) { Hmm, this route we never destroy the controller. Maybe this object should be owned by the Profile, which would make the API a little more obvious too (profile->GetSearchVersusNavigateClassifier()->IsSearch("foo")).
Peter the changes are: - No more the static method, just a normal function IsSearch. - Added Classify main function. - Added a getter for our SearchVersusNavigateClassifier to Profile class. http://codereview.chromium.org/328031/diff/4002/6004 File chrome/browser/search_versus_navigate_classifier.cc (right): http://codereview.chromium.org/328031/diff/4002/6004#newcode12 Line 12: static bool is_synchronous_controller_loaded = false; On 2009/10/26 23:28:18, Peter Kasting wrote: > Nit: No need for the bool, we can key off whether the pointer is NULL. Removed. http://codereview.chromium.org/328031/diff/4002/6004#newcode17 Line 17: if (!is_synchronous_controller_loaded) { On 2009/10/26 23:28:18, Peter Kasting wrote: > Hmm, this route we never destroy the controller. Maybe this object should be > owned by the Profile, which would make the API a little more obvious too > (profile->GetSearchVersusNavigateClassifier()->IsSearch("foo")). To fit in this way, I changed the class. No more the static method. Added the Classify method, that now calls the IsSearch function.
Besides my comments below you're missing two major areas: * Converting the code that currently uses a private controller to use this (e.g. AutocompleteEdit) * Profile modifications are nowhere near complete -- you will need to modify multiple different testing profiles, the off-the-record profile, etc. Search for matches for the other accessors in the profile. http://codereview.chromium.org/328031/diff/8004/7005 File chrome/browser/search_versus_navigate_classifier.cc (right): http://codereview.chromium.org/328031/diff/8004/7005#newcode10 Line 10: static AutocompleteController* synchronous_controller = NULL; This should be a class member using scoped_ptr. http://codereview.chromium.org/328031/diff/8004/7005#newcode16 Line 16: // we'll set this before each call to the controller. No, we should create using the supplied profile, since this is a member of the profile. http://codereview.chromium.org/328031/diff/8004/7005#newcode40 Line 40: synchronous_controller->SetProfile(profile_); No need for this once we fix creation to use the profile. At that point we may be able to remove AutocompleteController::SetProfile() entirely. http://codereview.chromium.org/328031/diff/8004/7004 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/8004/7004#newcode20 Line 20: // Classifies the given |text| return wheter it is an search text or an Nit: This comment needs to be fleshed out; the outparams should be described, you should say whether callers can supply NULL, etc. http://codereview.chromium.org/328031/diff/8004/7004#newcode22 Line 22: void Classify(const std::wstring& text, bool* is_search, Nit: Please put one arg per line http://codereview.chromium.org/328031/diff/8004/7004#newcode28 Line 28: bool IsSearch(const std::wstring& text); We should just have Classify(), this function is superfluous. http://codereview.chromium.org/328031/diff/8004/7004#newcode31 Line 31: Profile* profile_; No need for this. http://codereview.chromium.org/328031/diff/8004/7004#newcode33 Line 33: GURL destination_url_; Why do we have these? They should only be needed as locals inside Classify().
Peter I will take care of all these issues tomorrow, now I have to go (sleep).
On Mon, Oct 26, 2009 at 11:03 PM, <pkasting@chromium.org> wrote: > Besides my comments below you're missing two major areas: > > * Converting the code that currently uses a private controller to use this > (e.g. > AutocompleteEdit) > The patchset 5 doesn't include these changes yet. > * Profile modifications are nowhere near complete -- you will need to modify > multiple different testing profiles, the off-the-record profile, etc. > Search > for matches for the other accessors in the profile. > I included changes for AutomationProfileImpl, OffTheRecordProfileImpl, Profile, ProfileImpl, TestingProfile in patchset 5. Sorry for the delay, I had problems with my internet. So is only missing the refactor of AutocompleteEdit, when you are fine with these changes, I will start in there. http://codereview.chromium.org/328031/diff/8004/7005 File chrome/browser/search_versus_navigate_classifier.cc (right): http://codereview.chromium.org/328031/diff/8004/7005#newcode10 Line 10: static AutocompleteController* synchronous_controller = NULL; On 2009/10/27 01:03:33, Peter Kasting wrote: > This should be a class member using scoped_ptr. Done. http://codereview.chromium.org/328031/diff/8004/7005#newcode16 Line 16: // we'll set this before each call to the controller. On 2009/10/27 01:03:33, Peter Kasting wrote: > No, we should create using the supplied profile, since this is a member of the > profile. Now I we are using the |profile|. http://codereview.chromium.org/328031/diff/8004/7005#newcode40 Line 40: synchronous_controller->SetProfile(profile_); On 2009/10/27 01:03:33, Peter Kasting wrote: > No need for this once we fix creation to use the profile. Removed, since we are using the |profile| to create the synchronous_controller in the constructor. > > At that point we may be able to remove AutocompleteController::SetProfile() > entirely. Once we finish this class, I will get rid of this latter. http://codereview.chromium.org/328031/diff/8004/7004 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/8004/7004#newcode22 Line 22: void Classify(const std::wstring& text, bool* is_search, On 2009/10/27 01:03:33, Peter Kasting wrote: > Nit: Please put one arg per line Done. http://codereview.chromium.org/328031/diff/8004/7004#newcode28 Line 28: bool IsSearch(const std::wstring& text); On 2009/10/27 01:03:33, Peter Kasting wrote: > We should just have Classify(), this function is superfluous. Removed in patchset 4. http://codereview.chromium.org/328031/diff/8004/7004#newcode31 Line 31: Profile* profile_; On 2009/10/27 01:03:33, Peter Kasting wrote: > No need for this. Removed. http://codereview.chromium.org/328031/diff/8004/7004#newcode33 Line 33: GURL destination_url_; On 2009/10/27 01:03:33, Peter Kasting wrote: > Why do we have these? They should only be needed as locals inside Classify(). Removed. Now they are locals inside Classify.
http://codereview.chromium.org/328031/diff/9001/10002 File chrome/browser/search_versus_navigate_classifier.cc (right): http://codereview.chromium.org/328031/diff/9001/10002#newcode7 Line 7: #include "chrome/browser/profile.h" Why do we need this #include? http://codereview.chromium.org/328031/diff/9001/10002#newcode10 Line 10: SearchVersusNavigateClassifier::SearchVersusNavigateClassifier(Profile *profile) Nit: * on type name, not variable name http://codereview.chromium.org/328031/diff/9001/10002#newcode13 Line 13: synchronous_controller_.reset(new AutocompleteController(profile)); You should just init this unconditionally in the constructor initializer list: : synchronous_controller_(new AutocompleteController(profile)) http://codereview.chromium.org/328031/diff/9001/10002#newcode26 Line 26: *is_search = false; You don't NULL-check |is_search| here even though the comments in the header didn't say it couldn't be NULL. Also, if |result| is empty, we can't grab the default_match(), we have to return before that. And you'll need to set the other outparams correctly for that case. http://codereview.chromium.org/328031/diff/9001/10002#newcode38 Line 38: *is_search = true; Your code doesn't set |is_search| if this condition fails. You want: // If this is a search, the page transition will be GENERATED rather than TYPED. *is_search = (match->transition != PageTransition::TYPED); http://codereview.chromium.org/328031/diff/9001/10001 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/9001/10001#newcode11 Line 11: #include "chrome/browser/autocomplete/autocomplete.h" Nit: Just forward-declare AutocompleteController in this header, and #include autocomplete.h in the .cc file. http://codereview.chromium.org/328031/diff/9001/10001#newcode21 Line 21: // Classifies the given |text| return wheter it's a search text or an url to Nit: Grammar. How about: // Given some string |text| that the user wants to use for navigation, determines whether to treat it as a search query or a URL, and returns the details of the resulting navigation. After |text|, all parameters are potentially-NULL outparams. http://codereview.chromium.org/328031/diff/9001/10001#newcode23 Line 23: // |is_search| - Returns true if it's to search for the given |text| Nit: Grammar. How about: "Set to true if this is to be treated as a query rather than a URL." http://codereview.chromium.org/328031/diff/9001/10001#newcode26 Line 26: // possible navigation. Nit: For clarity, I'd add "(i.e. when |text| is empty)" at the end. http://codereview.chromium.org/328031/diff/9001/10001#newcode29 Line 29: // the default match but intended something else. Nit: This is a little unclear, how about leaving it as: "The navigational URL in the case of an accidental search; see comments on AutocompleteResult::alternate_nav_url_ in autocomplete.h." http://codereview.chromium.org/328031/diff/9001/10001#newcode32 Line 32: // and transition. Nit: Are all callers going to want these? Seems like we should let any of them be NULL if not. http://codereview.chromium.org/328031/diff/9001/10001#newcode40 Line 40: scoped_ptr<AutocompleteController> synchronous_controller_; Nit: "controller_" is probably sufficient here
http://codereview.chromium.org/328031/diff/9001/10002 File chrome/browser/search_versus_navigate_classifier.cc (right): http://codereview.chromium.org/328031/diff/9001/10002#newcode26 Line 26: *is_search = false; On 2009/10/27 20:12:09, Peter Kasting wrote: > You don't NULL-check |is_search| here even though the comments in the header > didn't say it couldn't be NULL. > You mean: if (is_search) check? > Also, if |result| is empty, we can't grab the default_match(), we have to return > before that. And you'll need to set the other outparams correctly for that > case. Something like this? if (result.empty()) { *is_search = false; *destination_url = GURL(); *alternate_nav_url = GURL(); *transition = PageTransition::TYPED; return; } OK. But these requires that these outparams should not be NULL, no? If they can be null we have to check this before. http://codereview.chromium.org/328031/diff/9001/10001 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/9001/10001#newcode32 Line 32: // and transition. On 2009/10/27 20:12:09, Peter Kasting wrote: > Nit: Are all callers going to want these? Seems like we should let any of them > be NULL if not. If I'm not wrong, LocationBarView::OnAutocompleteAccept need these outparams. At least the destination_url and the alternate_nav_url.
http://codereview.chromium.org/328031/diff/9001/10002 File chrome/browser/search_versus_navigate_classifier.cc (right): http://codereview.chromium.org/328031/diff/9001/10002#newcode26 Line 26: *is_search = false; On 2009/10/27 21:01:32, tfarina wrote: > You mean: if (is_search) check? Yes. > > Also, if |result| is empty, we can't grab the default_match(), we have to > return > > before that. And you'll need to set the other outparams correctly for that > > case. > > Something like this? > if (result.empty()) { > *is_search = false; > *destination_url = GURL(); > *alternate_nav_url = GURL(); > *transition = PageTransition::TYPED; > return; > } Yes. > OK. But these requires that these outparams should not be NULL, no? If they can > be null we have to check this before. You have to check each as you assign it. http://codereview.chromium.org/328031/diff/9001/10001 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/9001/10001#newcode32 Line 32: // and transition. On 2009/10/27 21:01:32, tfarina wrote: > On 2009/10/27 20:12:09, Peter Kasting wrote: > > Nit: Are all callers going to want these? Seems like we should let any of > them > > be NULL if not. > > If I'm not wrong, LocationBarView::OnAutocompleteAccept need these outparams. Yes, _some_ callers want them. That's not the same as _all_ callers.
Made the changes. http://codereview.chromium.org/328031/diff/9001/10002 File chrome/browser/search_versus_navigate_classifier.cc (right): http://codereview.chromium.org/328031/diff/9001/10002#newcode7 Line 7: #include "chrome/browser/profile.h" On 2009/10/27 20:12:09, Peter Kasting wrote: > Why do we need this #include? Removed, not needed. http://codereview.chromium.org/328031/diff/9001/10002#newcode10 Line 10: SearchVersusNavigateClassifier::SearchVersusNavigateClassifier(Profile *profile) On 2009/10/27 20:12:09, Peter Kasting wrote: > Nit: * on type name, not variable name Done. http://codereview.chromium.org/328031/diff/9001/10002#newcode13 Line 13: synchronous_controller_.reset(new AutocompleteController(profile)); On 2009/10/27 20:12:09, Peter Kasting wrote: > You should just init this unconditionally in the constructor initializer list: > > : synchronous_controller_(new AutocompleteController(profile)) Done. http://codereview.chromium.org/328031/diff/9001/10002#newcode26 Line 26: *is_search = false; On 2009/10/27 21:09:22, Peter Kasting wrote: > You have to check each as you assign it. Checking them before assign. http://codereview.chromium.org/328031/diff/9001/10002#newcode38 Line 38: *is_search = true; On 2009/10/27 20:12:09, Peter Kasting wrote: > Your code doesn't set |is_search| if this condition fails. You want: > > // If this is a search, the page transition will be GENERATED rather than TYPED. > *is_search = (match->transition != PageTransition::TYPED); Even if the transition assumes another value? Is this okay? Anyway, I changed to this statement. http://codereview.chromium.org/328031/diff/9001/10001 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/9001/10001#newcode11 Line 11: #include "chrome/browser/autocomplete/autocomplete.h" On 2009/10/27 20:12:09, Peter Kasting wrote: > Nit: Just forward-declare AutocompleteController in this header, and #include > autocomplete.h in the .cc file. Done. http://codereview.chromium.org/328031/diff/9001/10001#newcode21 Line 21: // Classifies the given |text| return wheter it's a search text or an url to On 2009/10/27 20:12:09, Peter Kasting wrote: > Nit: Grammar. How about: > > // Given some string |text| that the user wants to use for navigation, > determines whether to treat it as a search query or a URL, and returns the > details of the resulting navigation. After |text|, all parameters are > potentially-NULL outparams. Done. http://codereview.chromium.org/328031/diff/9001/10001#newcode23 Line 23: // |is_search| - Returns true if it's to search for the given |text| On 2009/10/27 20:12:09, Peter Kasting wrote: > Nit: Grammar. How about: "Set to true if this is to be treated as a query > rather than a URL." Done. http://codereview.chromium.org/328031/diff/9001/10001#newcode26 Line 26: // possible navigation. On 2009/10/27 20:12:09, Peter Kasting wrote: > Nit: For clarity, I'd add "(i.e. when |text| is empty)" at the end. Done. http://codereview.chromium.org/328031/diff/9001/10001#newcode29 Line 29: // the default match but intended something else. On 2009/10/27 20:12:09, Peter Kasting wrote: > Nit: This is a little unclear, how about leaving it as: "The navigational URL in > the case of an accidental search; see comments on > AutocompleteResult::alternate_nav_url_ in autocomplete.h." Leaved as in the way you wrote. http://codereview.chromium.org/328031/diff/9001/10001#newcode32 Line 32: // and transition. On 2009/10/27 21:09:22, Peter Kasting wrote: > On 2009/10/27 21:01:32, tfarina wrote: > > On 2009/10/27 20:12:09, Peter Kasting wrote: > > > Nit: Are all callers going to want these? Seems like we should let any of > > them > > > be NULL if not. > > > > If I'm not wrong, LocationBarView::OnAutocompleteAccept need these outparams. > > Yes, _some_ callers want them. That's not the same as _all_ callers. So we will allow them to be NULL. http://codereview.chromium.org/328031/diff/9001/10001#newcode40 Line 40: scoped_ptr<AutocompleteController> synchronous_controller_; On 2009/10/27 20:12:09, Peter Kasting wrote: > Nit: "controller_" is probably sufficient here Done.
You need to update the title and description of this patch. Also you should go ahead and add in the changes to the autocomplete code to use this in lieu of its existing code. http://codereview.chromium.org/328031/diff/12009/11004 File chrome/browser/search_versus_navigate_classifier.cc (right): http://codereview.chromium.org/328031/diff/12009/11004#newcode26 Line 26: Nit: No need for all the blank lines inside this block. http://codereview.chromium.org/328031/diff/12009/11004#newcode39 Line 39: // Set local state based on the default action for this input. Nit: This comment isn't meaningful in this context the way it was in the original code, I'd remove it. http://codereview.chromium.org/328031/diff/12009/11004#newcode43 Line 43: *destination_url = match->destination_url; You're not NULL-checking these args here. http://codereview.chromium.org/328031/diff/12009/11004#newcode49 Line 49: *is_search = (match->transition != PageTransition::TYPED); Nit: Set this before the other three since it's listed before the other three. http://codereview.chromium.org/328031/diff/12009/11003 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/12009/11003#newcode26 Line 26: // |is_search| - It's assigned to true if this is to be treated as a Nit: "It's assigned" -> "Set" for consistency with surrounding comment format http://codereview.chromium.org/328031/diff/12009/11003#newcode29 Line 29: // possible navigation when |text| is empty. Nit: Confusing; add "(i.e. " before "when" (and ")" before ".") http://codereview.chromium.org/328031/diff/12009/11003#newcode31 Line 31: // search. Nit: Too vague, please add "; see comments on AutocompleteResult::alternate_nav_url_ in autocomplete.h" before "." http://codereview.chromium.org/328031/diff/12009/11003#newcode37 Line 37: PageTransition::Type* transition); Nit: I'd reverse the order of |alternate_nav_url| and |transition| to be consistent with the comments above, the code implementation, and how useful these are, respectively, to callers.
Changes made Peter, also the title and description updated too. Note: Maybe you would have suggestions about the description. http://codereview.chromium.org/328031/diff/12009/11004 File chrome/browser/search_versus_navigate_classifier.cc (right): http://codereview.chromium.org/328031/diff/12009/11004#newcode26 Line 26: On 2009/10/27 23:23:03, Peter Kasting wrote: > Nit: No need for all the blank lines inside this block. Done. http://codereview.chromium.org/328031/diff/12009/11004#newcode39 Line 39: // Set local state based on the default action for this input. On 2009/10/27 23:23:03, Peter Kasting wrote: > Nit: This comment isn't meaningful in this context the way it was in the > original code, I'd remove it. Removed. http://codereview.chromium.org/328031/diff/12009/11004#newcode43 Line 43: *destination_url = match->destination_url; On 2009/10/27 23:23:03, Peter Kasting wrote: > You're not NULL-checking these args here. Checking them. http://codereview.chromium.org/328031/diff/12009/11004#newcode49 Line 49: *is_search = (match->transition != PageTransition::TYPED); On 2009/10/27 23:23:03, Peter Kasting wrote: > Nit: Set this before the other three since it's listed before the other three. Moved up. http://codereview.chromium.org/328031/diff/12009/11003 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/12009/11003#newcode26 Line 26: // |is_search| - It's assigned to true if this is to be treated as a On 2009/10/27 23:23:03, Peter Kasting wrote: > Nit: "It's assigned" -> "Set" for consistency with surrounding comment format Done. http://codereview.chromium.org/328031/diff/12009/11003#newcode29 Line 29: // possible navigation when |text| is empty. On 2009/10/27 23:23:03, Peter Kasting wrote: > Nit: Confusing; add "(i.e. " before "when" (and ")" before ".") Done. http://codereview.chromium.org/328031/diff/12009/11003#newcode31 Line 31: // search. On 2009/10/27 23:23:03, Peter Kasting wrote: > Nit: Too vague, please add "; see comments on > AutocompleteResult::alternate_nav_url_ in autocomplete.h" before "." Done. http://codereview.chromium.org/328031/diff/12009/11003#newcode37 Line 37: PageTransition::Type* transition); On 2009/10/27 23:23:03, Peter Kasting wrote: > Nit: I'd reverse the order of |alternate_nav_url| and |transition| to be > consistent with the comments above, the code implementation, and how useful > these are, respectively, to callers. Done.
Getting close. The issue description is fine. I don't think you tested this, though, based on how the existing patch has both a crash and an infinite recursion instance in it. Remember to test carefully. http://codereview.chromium.org/328031/diff/9010/9017 File chrome/browser/autocomplete/autocomplete_edit.cc (right): http://codereview.chromium.org/328031/diff/9010/9017#newcode33 Line 33: // won't have thread-safety problems. This comment should disappear now that the variables it describes have gone. http://codereview.chromium.org/328031/diff/9010/9017#newcode56 Line 56: AutocompleteEditModel::~AutocompleteEditModel() { Nit: This can be turned into a "{}" in the header http://codereview.chromium.org/328031/diff/9010/9017#newcode201 Line 201: return !is_to_search; You should be returning the original condition. This function returns true if we can search _or_ navigate. That means you can also get rid of |is_to_search| and pass NULL in its place. http://codereview.chromium.org/328031/diff/9010/9017#newcode583 Line 583: GURL AutocompleteEditModel::URLsForDefaultMatch( Nit: You can just eliminate this as a separate function and roll it into its lone caller. Note that that will mean converting the ?: in GetURLForCurrentText() into a conditional. http://codereview.chromium.org/328031/diff/9010/9017#newcode590 Line 590: &destination_url, transition, alternate_nav_url); This call loses the GetDesiredTLD() info the old code had. Looks like you'll need to add that as a parameter to Classify() after |text| so you can pass it to the autocomplete controller. It also loses the setting of |is_history_what_you_typed_match|, so you'll probably need to add that as an outparam. http://codereview.chromium.org/328031/diff/9010/9017#newcode593 Line 593: alternate_nav_url); Why did you add this call? It wasn't in the old code, and worse, causes infinite recursion. http://codereview.chromium.org/328031/diff/9010/9012 File chrome/browser/search_versus_navigate_classifier.cc (right): http://codereview.chromium.org/328031/diff/9010/9012#newcode40 Line 40: *is_search = (match->transition != PageTransition::TYPED); Need to NULL-check this too.
Peter I think you could take another look again. This time I built the chrome and made some tests, like typing "www.google.com", "http://www.google.com", "google", copying it, and then right clicking on omnibox to see the result. Worked fine here. http://codereview.chromium.org/328031/diff/9010/9017 File chrome/browser/autocomplete/autocomplete_edit.cc (right): http://codereview.chromium.org/328031/diff/9010/9017#newcode33 Line 33: // won't have thread-safety problems. On 2009/10/28 01:37:38, Peter Kasting wrote: > This comment should disappear now that the variables it describes have gone. Removed. http://codereview.chromium.org/328031/diff/9010/9017#newcode56 Line 56: AutocompleteEditModel::~AutocompleteEditModel() { On 2009/10/28 01:37:38, Peter Kasting wrote: > Nit: This can be turned into a "{}" in the header Moved to header. http://codereview.chromium.org/328031/diff/9010/9017#newcode201 Line 201: return !is_to_search; On 2009/10/28 01:37:38, Peter Kasting wrote: > You should be returning the original condition. This function returns true if > we can search _or_ navigate. > > That means you can also get rid of |is_to_search| and pass NULL in its place. OK. Returning the original condition. http://codereview.chromium.org/328031/diff/9010/9017#newcode583 Line 583: GURL AutocompleteEditModel::URLsForDefaultMatch( On 2009/10/28 01:37:38, Peter Kasting wrote: > Nit: You can just eliminate this as a separate function and roll it into its > lone caller. Note that that will mean converting the ?: in > GetURLForCurrentText() into a conditional. Removed URlsForDefaultMatch function and made a conditional if-else on GetURLForCurrentText(). http://codereview.chromium.org/328031/diff/9010/9017#newcode590 Line 590: &destination_url, transition, alternate_nav_url); On 2009/10/28 01:37:38, Peter Kasting wrote: > This call loses the GetDesiredTLD() info the old code had. Looks like you'll > need to add that as a parameter to Classify() after |text| so you can pass it to > the autocomplete controller. Added |desired_tld| parameter after |text| and passed it to |controller_| > It also loses the setting of |is_history_what_you_typed_match|, so you'll > probably need to add that as an outparam. Also added |is_history_what_you_typed_match| after |transition|, and checking before assign. http://codereview.chromium.org/328031/diff/9010/9017#newcode593 Line 593: alternate_nav_url); On 2009/10/28 01:37:38, Peter Kasting wrote: > Why did you add this call? It wasn't in the old code, and worse, causes > infinite recursion. Sorry about that, the intend was to get the value of |is_history_what_you_typed_match|. http://codereview.chromium.org/328031/diff/9010/9012 File chrome/browser/search_versus_navigate_classifier.cc (right): http://codereview.chromium.org/328031/diff/9010/9012#newcode40 Line 40: *is_search = (match->transition != PageTransition::TYPED); On 2009/10/28 01:37:38, Peter Kasting wrote: > Need to NULL-check this too. Done. http://codereview.chromium.org/328031/diff/9010/9011 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/9010/9011#newcode13 Line 13: class AutocompleteController; Just declaring a forward declaration was giving an error when compiling browser project; for some reason compiling just the .cc file compiles fine.
http://codereview.chromium.org/328031/diff/9010/9011 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/9010/9011#newcode13 Line 13: class AutocompleteController; On 2009/10/28 20:06:32, tfarina wrote: > Just declaring a forward declaration was giving an error when compiling browser > project; for some reason compiling just the .cc file compiles fine. What was the error? http://codereview.chromium.org/328031/diff/11029/11036 File chrome/browser/autocomplete/autocomplete_edit.cc (right): http://codereview.chromium.org/328031/diff/11029/11036#newcode575 Line 575: } else { Nit: No "else" after "return". http://codereview.chromium.org/328031/diff/11029/11037 File chrome/browser/autocomplete/autocomplete_edit.h (right): http://codereview.chromium.org/328031/diff/11029/11037#newcode114 Line 114: void SetProfile(Profile* profile); How come you didn't remove this anymore in this patch? http://codereview.chromium.org/328031/diff/11029/11031 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/11029/11031#newcode32 Line 32: // |is_history_what_you_typed_match| - Set to true when this match is the Nit: "this" -> "the default". To make the lines line up, I'd just start the comment on the line after this, indented even with all the other comments.
http://codereview.chromium.org/328031/diff/9010/9011 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/9010/9011#newcode13 Line 13: class AutocompleteController; On 2009/10/28 20:15:58, Peter Kasting wrote: > On 2009/10/28 20:06:32, tfarina wrote: > > Just declaring a forward declaration was giving an error when compiling > browser > > project; for some reason compiling just the .cc file compiles fine. > > What was the error? I don't remember exactly what it said, but it was something about the destructor not defined. http://codereview.chromium.org/328031/diff/11029/11037 File chrome/browser/autocomplete/autocomplete_edit.h (right): http://codereview.chromium.org/328031/diff/11029/11037#newcode114 Line 114: void SetProfile(Profile* profile); On 2009/10/28 20:15:58, Peter Kasting wrote: > How come you didn't remove this anymore in this patch? Oh, I contacted you on the IRC talking about that issue, but you didn't answer me. In the IRC I mentioned that LocationBarView has a call to location_entry_->model()->SetProfile(profile); location_bar_view.cc:333 Removing this call from LocationBarView makes sense? http://codereview.chromium.org/328031/diff/11029/11031 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/11029/11031#newcode32 Line 32: // |is_history_what_you_typed_match| - Set to true when this match is the On 2009/10/28 20:15:58, Peter Kasting wrote: > Nit: "this" -> "the default". > > To make the lines line up, I'd just start the comment on the line after this, > indented even with all the other comments. Like this: // |is_history_what_you_typed_match| // - Set to true when the default match is the // "what you typed" match from the history.
http://codereview.chromium.org/328031/diff/9010/9011 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/9010/9011#newcode13 Line 13: class AutocompleteController; On 2009/10/28 20:33:31, tfarina wrote: > On 2009/10/28 20:15:58, Peter Kasting wrote: > > On 2009/10/28 20:06:32, tfarina wrote: > > > Just declaring a forward declaration was giving an error when compiling > > browser > > > project; for some reason compiling just the .cc file compiles fine. > > > > What was the error? > I don't remember exactly what it said, but it was something about the destructor > not defined. I'd like to see the actual error. Seems like we should be able to get away with this. http://codereview.chromium.org/328031/diff/11029/11037 File chrome/browser/autocomplete/autocomplete_edit.h (right): http://codereview.chromium.org/328031/diff/11029/11037#newcode114 Line 114: void SetProfile(Profile* profile); On 2009/10/28 20:33:31, tfarina wrote: > On 2009/10/28 20:15:58, Peter Kasting wrote: > > How come you didn't remove this anymore in this patch? > > Oh, I contacted you on the IRC talking about that issue, but you didn't answer > me. > In the IRC I mentioned that LocationBarView has a call to > location_entry_->model()->SetProfile(profile); > location_bar_view.cc:333 > > Removing this call from LocationBarView makes sense? Ah. Technically, it would be safe since this is a remnant from when we made profiles per-tab. But it's better not to remove that piecemeal. OK, leave this in. http://codereview.chromium.org/328031/diff/11029/11031 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/11029/11031#newcode32 Line 32: // |is_history_what_you_typed_match| - Set to true when this match is the On 2009/10/28 20:33:31, tfarina wrote: > On 2009/10/28 20:15:58, Peter Kasting wrote: > > To make the lines line up, I'd just start the comment on the line after this, > > indented even with all the other comments. > > Like this: > // |is_history_what_you_typed_match| > // - Set to true when the default match is the > // "what you typed" match from the history. > Yes.
http://codereview.chromium.org/328031/diff/9010/9011 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/9010/9011#newcode13 Line 13: class AutocompleteController; On 2009/10/28 20:39:40, Peter Kasting wrote: > On 2009/10/28 20:33:31, tfarina wrote: > > On 2009/10/28 20:15:58, Peter Kasting wrote: > > > On 2009/10/28 20:06:32, tfarina wrote: > > > > Just declaring a forward declaration was giving an error when compiling > > > browser > > > > project; for some reason compiling just the .cc file compiles fine. > > > > > > What was the error? > > I don't remember exactly what it said, but it was something about the > destructor > > not defined. > > I'd like to see the actual error. Seems like we should be able to get away with > this. http://pastebin.com/d6bdb6bec
On 2009/10/28 21:10:28, tfarina wrote: > http://pastebin.com/d6bdb6bec This is being triggered by the inline destructor. We can fix in two ways: (1) Move the empty destructor body from the header to the .cc file. (2) Go ahead and #include autocomplete.h. I think fix (1) is better.
On 2009/10/28 21:21:29, Peter Kasting wrote: > This is being triggered by the inline destructor. > > We can fix in two ways: > (1) Move the empty destructor body from the header to the .cc file. > (2) Go ahead and #include autocomplete.h. > > I think fix (1) is better. Using the approach 1, because it is better. Thanks for that Peter. http://codereview.chromium.org/328031/diff/11029/11036 File chrome/browser/autocomplete/autocomplete_edit.cc (right): http://codereview.chromium.org/328031/diff/11029/11036#newcode575 Line 575: } else { On 2009/10/28 20:15:58, Peter Kasting wrote: > Nit: No "else" after "return". No more else. http://codereview.chromium.org/328031/diff/11029/11031 File chrome/browser/search_versus_navigate_classifier.h (right): http://codereview.chromium.org/328031/diff/11029/11031#newcode32 Line 32: // |is_history_what_you_typed_match| - Set to true when this match is the On 2009/10/28 20:39:40, Peter Kasting wrote: > On 2009/10/28 20:33:31, tfarina wrote: > > On 2009/10/28 20:15:58, Peter Kasting wrote: > > > To make the lines line up, I'd just start the comment on the line after > this, > > > indented even with all the other comments. > > > > Like this: > > // |is_history_what_you_typed_match| > > // - Set to true when the default match is the > > // "what you typed" match from the history. > > > > Yes. Done.
LGTM http://codereview.chromium.org/328031/diff/12050/12057 File chrome/browser/autocomplete/autocomplete_edit.cc (right): http://codereview.chromium.org/328031/diff/12050/12057#newcode575 Line 575: } Nit: I'd put a newline after this http://codereview.chromium.org/328031/diff/12050/12057#newcode578 Line 578: UserTextFromDisplayText(view_->GetText()), GetDesiredTLD(), NULL, Nit: Subsequent portions of a line are indented 4 spaces, not 2.
http://codereview.chromium.org/328031/diff/12050/12057 File chrome/browser/autocomplete/autocomplete_edit.cc (right): http://codereview.chromium.org/328031/diff/12050/12057#newcode575 Line 575: } On 2009/10/28 21:41:16, Peter Kasting wrote: > Nit: I'd put a newline after this Done. http://codereview.chromium.org/328031/diff/12050/12057#newcode578 Line 578: UserTextFromDisplayText(view_->GetText()), GetDesiredTLD(), NULL, On 2009/10/28 21:41:16, Peter Kasting wrote: > Nit: Subsequent portions of a line are indented 4 spaces, not 2. Done.
Landed in r30473. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
