Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
54203008:
Store xsrf token received with psuggest results. (Closed)
|
Created:
7 years, 1 month ago by Maria Modified:
7 years ago CC:
chromium-reviews, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionStore xsrf token received with psuggest results.
Stores xsrf token in AutocompleteMatch search_terms_args for later use
in case the user triggers deletion of a suggestion.
BUG=312477
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238422
Patch Set 1 #Patch Set 2 : Added test case #
Total comments: 2
Patch Set 3 : Implemented end-to-end deletion #Patch Set 4 : Update test to use consts #Patch Set 5 : Remove match from matches_ when deletion is requested #
Total comments: 11
Patch Set 6 : Addressed Anuj's comments #
Total comments: 61
Patch Set 7 : Addressing Peter's comments #Patch Set 8 : Added a missing comment #Patch Set 9 : Moved helper class to cc file #
Total comments: 41
Patch Set 10 : Addresses more of Peter's comments #Patch Set 11 : Remove virtual from OnDeletionComplete #
Total comments: 1
Patch Set 12 : More comments taken care of #
Total comments: 22
Patch Set 13 : Small fixes #Patch Set 14 : Update to the tests #
Total comments: 6
Patch Set 15 : Nit fixes #
Messages
Total messages: 35 (0 generated)
Honestly I would rather see a single change that both stores and uses this token. It's harder to gauge whether the implementation is correct when no code actually uses the value you're setting here. You'll note below I veto where you're storing this, but don't suggest an alternative. That's due to the above issue. We could record it on AutocompleteMatch as additional info, but maybe instead we should be recording an entire URL to ping to perform the deletion. Etc. https://codereview.chromium.org/54203008/diff/30001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/54203008/diff/30001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:92: const std::string& xsrf_token, Don't pass this in if no callers but SearchProvider itself care about it. Instead set it appropriately on the caller side. https://codereview.chromium.org/54203008/diff/30001/chrome/browser/search_eng... File chrome/browser/search_engines/template_url.h (right): https://codereview.chromium.org/54203008/diff/30001/chrome/browser/search_eng... chrome/browser/search_engines/template_url.h:112: std::string xsrf_token; This is the wrong place to store this. Since ReplaceSearchTerms() doesn't need this value, it shouldn't be in this struct.
How about this: I will rework this CL to add the functionality for DeleteMatch() in SearchProvider that will use the token to send a request to delete to the server. However, this change will not yet handle robust deletion or UI for psuggest. Also in terms of where xsrf_token is stored, I was thinking that one option would be to add a new field to prepopulated_engines_schema.json to store a URL for deletion and substitute the token into the placeholder on deletion. However, I am unsure whether we need to make this that generic -- I could just construct the URL to call in code. Thoughts? On Thu, Oct 31, 2013 at 4:54 PM, <pkasting@chromium.org> wrote: > Honestly I would rather see a single change that both stores and uses this > token. It's harder to gauge whether the implementation is correct when no > code > actually uses the value you're setting here. You'll note below I veto > where > you're storing this, but don't suggest an alternative. That's due to the > above > issue. We could record it on AutocompleteMatch as additional info, but > maybe > instead we should be recording an entire URL to ping to perform the > deletion. > Etc. > > > https://codereview.chromium.org/54203008/diff/30001/ > chrome/browser/autocomplete/search_provider.h > File chrome/browser/autocomplete/search_provider.h (right): > > https://codereview.chromium.org/54203008/diff/30001/ > chrome/browser/autocomplete/search_provider.h#newcode92 > chrome/browser/autocomplete/search_provider.h:92: const std::string& > xsrf_token, > Don't pass this in if no callers but SearchProvider itself care about > it. Instead set it appropriately on the caller side. > > https://codereview.chromium.org/54203008/diff/30001/ > chrome/browser/search_engines/template_url.h > File chrome/browser/search_engines/template_url.h (right): > > https://codereview.chromium.org/54203008/diff/30001/ > chrome/browser/search_engines/template_url.h#newcode112 > chrome/browser/search_engines/template_url.h:112: std::string > xsrf_token; > This is the wrong place to store this. Since ReplaceSearchTerms() > doesn't need this value, it shouldn't be in this struct. > > https://codereview.chromium.org/54203008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/11/01 00:19:51, mariakhomenko_google.com wrote: > How about this: I will rework this CL to add the functionality for > DeleteMatch() in SearchProvider that will use the token to send a request > to delete to the server. However, this change will not yet handle robust > deletion or UI for psuggest. SGTM; we can implement things piecemeal and only turn the feature on once all the bits are in. > Also in terms of where xsrf_token is stored, I was thinking that one option > would be to add a new field to prepopulated_engines_schema.json to store a > URL for deletion and substitute the token into the placeholder on deletion. > However, I am unsure whether we need to make this that generic -- I could > just construct the URL to call in code. Thoughts? Think from the perspective of, say, Microsoft wanting to implement suggestion deletion for Bing. What framework would they want? Maybe they'd want to be able to just parse all the necessary data (URLs and any params) out of the suggest response. Or maybe it would make sense for them to have a deletion URL stored in the prepopulate data, with some substitutable params (possibly not XSRF tokens, or at least not solely those) that came from the response. The implementation we design should preferably be extensible to other providers in a way they would be able to easily use. What we shouldn't do is e.g. hardcode a deletion URL or other information inside SearchProvider, that comes neither from the prepopulate data nor from the suggest response.
On Thu, Oct 31, 2013 at 5:24 PM, <pkasting@chromium.org> wrote: > On 2013/11/01 00:19:51, mariakhomenko_google.com wrote: > >> How about this: I will rework this CL to add the functionality for >> DeleteMatch() in SearchProvider that will use the token to send a request >> to delete to the server. However, this change will not yet handle robust >> deletion or UI for psuggest. >> > > SGTM; we can implement things piecemeal and only turn the feature on once > all > the bits are in. > > > Also in terms of where xsrf_token is stored, I was thinking that one >> option >> would be to add a new field to prepopulated_engines_schema.**json to >> store a >> URL for deletion and substitute the token into the placeholder on >> deletion. >> However, I am unsure whether we need to make this that generic -- I could >> just construct the URL to call in code. Thoughts? >> > > Think from the perspective of, say, Microsoft wanting to implement > suggestion > deletion for Bing. What framework would they want? Maybe they'd want to > be > able to just parse all the necessary data (URLs and any params) out of the > suggest response. Or maybe it would make sense for them to have a > deletion URL > stored in the prepopulate data, with some substitutable params (possibly > not > XSRF tokens, or at least not solely those) that came from the response. > The > implementation we design should preferably be extensible to other > providers in a > way they would be able to easily use. > > What we shouldn't do is e.g. hardcode a deletion URL or other information > inside > SearchProvider, that comes neither from the prepopulate data nor from the > suggest response. > My vote will be for more power to the server :) To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry, not sure what you mean by "more power to the server". My current plan is to add a new url type for deletions that other search engines could customize. Are you advocating towards sending the url in the response instead? On Thu, Oct 31, 2013 at 5:45 PM, Anuj Sharma <skanuj@google.com> wrote: > On Thu, Oct 31, 2013 at 5:24 PM, <pkasting@chromium.org> wrote: > >> On 2013/11/01 00:19:51, mariakhomenko_google.com wrote: >> >>> How about this: I will rework this CL to add the functionality for >>> DeleteMatch() in SearchProvider that will use the token to send a request >>> to delete to the server. However, this change will not yet handle robust >>> deletion or UI for psuggest. >>> >> >> SGTM; we can implement things piecemeal and only turn the feature on once >> all >> the bits are in. >> >> >> Also in terms of where xsrf_token is stored, I was thinking that one >>> option >>> would be to add a new field to prepopulated_engines_schema.json to >>> store a >>> URL for deletion and substitute the token into the placeholder on >>> deletion. >>> However, I am unsure whether we need to make this that generic -- I could >>> just construct the URL to call in code. Thoughts? >>> >> >> Think from the perspective of, say, Microsoft wanting to implement >> suggestion >> deletion for Bing. What framework would they want? Maybe they'd want to >> be >> able to just parse all the necessary data (URLs and any params) out of the >> suggest response. Or maybe it would make sense for them to have a >> deletion URL >> stored in the prepopulate data, with some substitutable params (possibly >> not >> XSRF tokens, or at least not solely those) that came from the response. >> The >> implementation we design should preferably be extensible to other >> providers in a >> way they would be able to easily use. >> >> What we shouldn't do is e.g. hardcode a deletion URL or other information >> inside >> SearchProvider, that comes neither from the prepopulate data nor from the >> suggest response. >> > > My vote will be for more power to the server :) > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Oct 31, 2013 at 5:50 PM, Maria Khomenko <mariakhomenko@google.com>wrote: > Sorry, not sure what you mean by "more power to the server". My current > plan is to add a new url type for deletions that other search engines could > customize. Are you advocating towards sending the url in the response > instead? > Yeah. Server changes go out under a week. And for all purposes, the deletion url is to be interpreted only by the server. There is no device local use for that url. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sending the URL down would make the code easy, but it seems inefficient to re-send the same URL a bunch of times in an autocomplete response with just the XSRF/query token changing every time. On Thu, Oct 31, 2013 at 5:53 PM, Anuj Sharma <skanuj@google.com> wrote: > On Thu, Oct 31, 2013 at 5:50 PM, Maria Khomenko <mariakhomenko@google.com>wrote: > >> Sorry, not sure what you mean by "more power to the server". My current >> plan is to add a new url type for deletions that other search engines could >> customize. Are you advocating towards sending the url in the response >> instead? >> > > Yeah. Server changes go out under a week. And for all purposes, the > deletion url is to be interpreted only by the server. There is no device > local use for that url. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Inefficiency in what terms? We use param names as descriptive as google:clientdata, google:suggesttype. The display strings array is just a long list of empty strings usually. google:suggesttype repeats "QUERY" 10 times, and that brings up the point that we fetch 10+ suggestions when we show just 4-5. Worst case, you can add google:delete_url_path as a separate response parameter along side google:suggesttype which is present only when there are personalized suggestions in response. Although letting other search engines do deletion is a non-goal right now - a bit like instant search. We are using google specific parameter keys in the opensearch json response. We are not changing that standard. On Fri, Nov 1, 2013 at 11:48 AM, Maria Khomenko <mariakhomenko@google.com>wrote: > Sending the URL down would make the code easy, but it seems inefficient to > re-send the same URL a bunch of times in an autocomplete response with just > the XSRF/query token changing every time. > > > On Thu, Oct 31, 2013 at 5:53 PM, Anuj Sharma <skanuj@google.com> wrote: > >> On Thu, Oct 31, 2013 at 5:50 PM, Maria Khomenko <mariakhomenko@google.com >> > wrote: >> >>> Sorry, not sure what you mean by "more power to the server". My current >>> plan is to add a new url type for deletions that other search engines could >>> customize. Are you advocating towards sending the url in the response >>> instead? >>> >> >> Yeah. Server changes go out under a week. And for all purposes, the >> deletion url is to be interpreted only by the server. There is no device >> local use for that url. >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ok, here are pros and cons of the two approaches as I see them: 1) Adding new URL to prepopulated engine schema: Pros: - Follows the pattern of how we make other search engine requests - Minimal data required to be sent from the server, no need to duplicate query names, client name, etc Cons: - More code necessary to implement - Changes in the URL would require more time to propagate to users - The URL is not useful without XSRF token that needs to be obtained from the server 2) Sending the full URL with the response in google:suggestdetail Pros: - Easy and fast to change deletion URL - Simple to implement - More flexible -- could possibly support different deletion URLs for different types Cons: - A lot of redundant data sent over the network (and yes, google:suggesttype also has this issue, but to a lesser degree) - Keeps more data in memory as well (though this is probably not a big deal given the amount of data) In terms of supporting 3rd party search engines I think both approaches are equally bad since the 3rd party engine would need to provide some sort of XSSI protection as well and hence would need to send something in autocomplete response that we don't support today as we are using our custom response types for this. In terms of sending the URL template in the response as a separate item -- that would only be an advantage over approach 1) if we expected the deletion URL to change a lot which I suspect won't happen. Otherwise we would need to write extra custom code to fill out URL parameters into this new template -- might as well just have it in prepopulated engines. I am hesitantly OK with implementing approach 2). *Peter*, can you please weigh in if you have strong opinions on going one way or another. Thanks, Maria On Fri, Nov 1, 2013 at 2:38 PM, Anuj Sharma <skanuj@google.com> wrote: > Inefficiency in what terms? > We use param names as descriptive as google:clientdata, > google:suggesttype. The display strings array is just a long list of empty > strings usually. > google:suggesttype repeats "QUERY" 10 times, and that brings up the point > that we fetch 10+ suggestions when we show just 4-5. > > Worst case, you can add google:delete_url_path as a separate response > parameter along side google:suggesttype which is present only when there > are personalized suggestions in response. > > Although letting other search engines do deletion is a non-goal right now > - a bit like instant search. We are using google specific parameter keys in > the opensearch json response. We are not changing that standard. > > > > On Fri, Nov 1, 2013 at 11:48 AM, Maria Khomenko <mariakhomenko@google.com>wrote: > >> Sending the URL down would make the code easy, but it seems inefficient >> to re-send the same URL a bunch of times in an autocomplete response with >> just the XSRF/query token changing every time. >> >> >> On Thu, Oct 31, 2013 at 5:53 PM, Anuj Sharma <skanuj@google.com> wrote: >> >>> On Thu, Oct 31, 2013 at 5:50 PM, Maria Khomenko < >>> mariakhomenko@google.com> wrote: >>> >>>> Sorry, not sure what you mean by "more power to the server". My current >>>> plan is to add a new url type for deletions that other search engines could >>>> customize. Are you advocating towards sending the url in the response >>>> instead? >>>> >>> >>> Yeah. Server changes go out under a week. And for all purposes, the >>> deletion url is to be interpreted only by the server. There is no device >>> local use for that url. >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Nov 1, 2013 at 3:34 PM, Maria Khomenko <mariakhomenko@google.com>wrote: > I am hesitantly OK with implementing approach 2). *Peter*, can you please > weigh in if you have strong opinions on going one way or another. > I don't feel strongly either way. I don't think the cons in number 2 are a very big deal, so I wouldn't veto it on those grounds. PK To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Note that the cons of 2 (response size) are reduced by the fact that the max number of personalized suggestions is 3. It will generally be more common to have fewer personalized suggestions. On Mon, Nov 4, 2013 at 2:50 PM, Peter Kasting <pkasting@chromium.org> wrote: > On Fri, Nov 1, 2013 at 3:34 PM, Maria Khomenko <mariakhomenko@google.com>wrote: > >> I am hesitantly OK with implementing approach 2). *Peter*, can you >> please weigh in if you have strong opinions on going one way or another. >> > > I don't feel strongly either way. I don't think the cons in number 2 are > a very big deal, so I wouldn't veto it on those grounds. > > PK > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I uploaded a new patch for this that's ready for review. Please take a look. On Mon, Nov 4, 2013 at 2:58 PM, Anuj Sharma <skanuj@google.com> wrote: > Note that the cons of 2 (response size) are reduced by the fact that the > max number of personalized suggestions is 3. It will generally be more > common to have fewer personalized suggestions. > > > On Mon, Nov 4, 2013 at 2:50 PM, Peter Kasting <pkasting@chromium.org>wrote: > >> On Fri, Nov 1, 2013 at 3:34 PM, Maria Khomenko <mariakhomenko@google.com>wrote: >> >>> I am hesitantly OK with implementing approach 2). *Peter*, can you >>> please weigh in if you have strong opinions on going one way or another. >>> >> >> I don't feel strongly either way. I don't think the cons in number 2 are >> a very big deal, so I wouldn't veto it on those grounds. >> >> PK >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Looking good. Some comments nonetheless. https://codereview.chromium.org/54203008/diff/250001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/54203008/diff/250001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1750: DCHECK(match.deletable); This deletes only the server provided suggestions. Please add a TODO for deleting search history suggestions. https://codereview.chromium.org/54203008/diff/250001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1755: deletion_handler_.get()->StartRequest(deletion_url, profile_, Use deletion_handler_->StartRequest. -> is pass-through for scoped_ptr. https://codereview.chromium.org/54203008/diff/250001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1764: UMA_HISTOGRAM_COUNTS("Omnibox.PsuggestDelete.Success", 1); Not sure about mentioning this event as "PsuggestDelete". A generic name will be better. https://codereview.chromium.org/54203008/diff/250001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1768: deletion_handler_.reset(NULL); Doesn't this create race? What if I try to delete multiple suggestions quickly? https://codereview.chromium.org/54203008/diff/250001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1832: match.deletable = true; Add a TODO about marking search-history suggestions as deletable too. https://codereview.chromium.org/54203008/diff/250001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/54203008/diff/250001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:373: virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE; Probably an empty line is needed - here between method and properties. - before private:
https://codereview.chromium.org/54203008/diff/250001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/54203008/diff/250001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1750: DCHECK(match.deletable); On 2013/11/21 17:27:52, Anuj wrote: > This deletes only the server provided suggestions. Please add a TODO for > deleting search history suggestions. Done. https://codereview.chromium.org/54203008/diff/250001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1755: deletion_handler_.get()->StartRequest(deletion_url, profile_, On 2013/11/21 17:27:52, Anuj wrote: > Use deletion_handler_->StartRequest. -> is pass-through for scoped_ptr. Done. https://codereview.chromium.org/54203008/diff/250001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1764: UMA_HISTOGRAM_COUNTS("Omnibox.PsuggestDelete.Success", 1); On 2013/11/21 17:27:52, Anuj wrote: > Not sure about mentioning this event as "PsuggestDelete". A generic name will be > better. Done. https://codereview.chromium.org/54203008/diff/250001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1768: deletion_handler_.reset(NULL); On 2013/11/21 17:27:52, Anuj wrote: > Doesn't this create race? What if I try to delete multiple suggestions quickly? I think you are right. Trying a new memory management scheme, let me know if you think that works. https://codereview.chromium.org/54203008/diff/250001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1832: match.deletable = true; On 2013/11/21 17:27:52, Anuj wrote: > Add a TODO about marking search-history suggestions as deletable too. I think when we implement it, we would have to do it for it to work. I think the todo to implement it is enough.
lgtm Looks good enough to me. I am sure Peter will weed out all the style, naming and comment concerns.
https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:327: SearchProvider::SuggestionDeletionHandler::SuggestionDeletionHandler() { Nit: Blank line above this https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:330: SearchProvider::SuggestionDeletionHandler::~SuggestionDeletionHandler() { Nit: Function definition order should match the class' declaration order https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:334: const std::string& deletion_url, Profile* profile, Nit: One arg per line https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:337: GURL url = GURL(deletion_url); Nit: GURL url(deletion_url); https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:338: if (url.is_valid()) { Perhaps we should check GURL validity in AddMatchToMap(). Then we could avoid marking matches deletable which had an invalid URL, which will be a win once the "deletable" state is exposed to users in the UI. At that point this could be a DCHECK. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:355: source->GetResponseCode() == 200; Nit: Parens around binary subexprs Optional: Inline into next statement (meaning is pretty clear) https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:359: Nit: One more blank line here https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1766: // Allocate on the heap, class will delete itself when the request is done. I think the SearchProvider should probably own this object (e.g. stick it on a ScopedVector). This way, during program shutdown, any still-in-progress fetches will be auto-canceled. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1775: void SearchProvider::OnDeletionComplete(bool success) { If we check URL validity in AddMatchToMap(), then I think this can only be called back after the fetch completes; at which point this function can be inlined into the deletion handler. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1777: UMA_HISTOGRAM_COUNTS("Omnibox.ServerSuggestDelete.Success", 1); Nit: Is it possible to use ?: to simplify this to a single UMA_HISTOGRAM_COUNTS() invocation? Or do we have a utility that scans the source code for explicit strings that follow this macro? (I seem to remember some such thing for some kind of stat collection, but I can't recall the details.) Also, shouldn't we be defining this in histograms.xml? https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1783: void SearchProvider::DeleteMatchFromMatches(const AutocompleteMatch& match) { The body of this function is akin to the identically-named function in history_provider.h. Perhaps we should define a function in AutocompleteProvider, e.g. FindMatchForDeletion(), that returns an iterator to the appropriate match (and NOTREACHEDs on failure). Then the caller can decide how to handle this. This would centralized the comparison portion of this function, so if we decide to change it at some point, all providers will be updated simultaneously. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:281: // Optional deletion url provided with psuggest results to delete a Nit: Don't use "psuggest" in code (not a well-understood phrase). How about: Optional deletion URL provided with suggestions. Fetching this URL should result in some reasonable deletion behavior on the server, e.g. deleting this term out of a user's server-side search history. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:366: class SuggestionDeletionHandler : public net::URLFetcherDelegate { Declare this class in the .cc file, since nothing else in this header needs it. Add a comment above the class describing briefly what it's for and how it's used. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:369: virtual void StartRequest(const std::string& deletion_url, Profile* profile, Nit: One arg per line Is this an override? If so, you should say OVERRIDE, and put a "// net::URLFetcherDelegate:" comment above it. (Put a blank line above said comment.) https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:371: private: Nit: Blank line above this https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:372: // The class takes care of deleting itself when it's done. Nit: This kind of comment should either be stated in the above-class class-level comment or not at all. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:374: virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE; Nit: Again, put a blank line and a parent-class comment above this as in the public section. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:375: scoped_ptr<net::URLFetcher> deletion_fetcher_; Nit: Blank line above this https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:403: // Callback for deletion request Nit: This comment is too vague, and doesn't have trailing punctuation. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:406: // Removes the deleted match from the list of matches. Nit: ...from |matches_|. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:620: // Used to store a deletion request url for psuggest autocomplete matches. Nit: Don't use "psuggest"; try "for server-provided suggestions". https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4047: AutocompleteMatch m = AutocompleteMatch( Nit: Always do "Class varname(...)" instead of "Class varname = Class(...)"; it's not only shorter, it's more efficient. (severla places) https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4049: 640, Nit: Is the value here important? If not use 0 or some other value that looks less particularly-important. (several places) https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4074: fetcher->SetResponseString(""); Nit: Is this line necessary at all? If it is, use std::string() (2 places) https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4076: ASSERT_TRUE(provider_->deletion_handler_.get() == NULL); What is |deletion_handler_|? I don't see it in your change. Also, it looks like this test and the next both check the same thing: there is no deletion handler. It would be nice to have tests that test that something obviously different happens in response to success versus failure -- if such a thing is even possible (it may not be if you follow my refactoring suggestions in the .cc file).
Thanks for the code review! PTAL. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:327: SearchProvider::SuggestionDeletionHandler::SuggestionDeletionHandler() { On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: Blank line above this Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:330: SearchProvider::SuggestionDeletionHandler::~SuggestionDeletionHandler() { On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: Function definition order should match the class' declaration order Done. Quick question: that doesn't seem currently true for search_provider.cc in general. I grouped the functions together by usage -- should I rearrange them to declaration order in .h file instead? https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:334: const std::string& deletion_url, Profile* profile, On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: One arg per line Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:337: GURL url = GURL(deletion_url); On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: GURL url(deletion_url); Done. Sorry, Java habits die hard. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:338: if (url.is_valid()) { On 2013/11/23 00:08:43, Peter Kasting wrote: > Perhaps we should check GURL validity in AddMatchToMap(). Then we could avoid > marking matches deletable which had an invalid URL, which will be a win once the > "deletable" state is exposed to users in the UI. > > At that point this could be a DCHECK. Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:355: source->GetResponseCode() == 200; On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: Parens around binary subexprs > > Optional: Inline into next statement (meaning is pretty clear) Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:359: On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: One more blank line here Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1766: // Allocate on the heap, class will delete itself when the request is done. On 2013/11/23 00:08:43, Peter Kasting wrote: > I think the SearchProvider should probably own this object (e.g. stick it on a > ScopedVector). This way, during program shutdown, any still-in-progress fetches > will be auto-canceled. Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1775: void SearchProvider::OnDeletionComplete(bool success) { On 2013/11/23 00:08:43, Peter Kasting wrote: > If we check URL validity in AddMatchToMap(), then I think this can only be > called back after the fetch completes; at which point this function can be > inlined into the deletion handler. True, but I thought this may be a more flexible approach. It's possible we may need to add to this function in the future to call something to notify the UI that the deletion was successful (since some of the UI mock variations have a confirmation message for deletion). It would be helpful if the function was run in the SearchProvider class and had access to search provider members. Does that sound reasonable? https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1777: UMA_HISTOGRAM_COUNTS("Omnibox.ServerSuggestDelete.Success", 1); On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: Is it possible to use ?: to simplify this to a single > UMA_HISTOGRAM_COUNTS() invocation? Or do we have a utility that scans the > source code for explicit strings that follow this macro? (I seem to remember > some such thing for some kind of stat collection, but I can't recall the > details.) > > Also, shouldn't we be defining this in histograms.xml? I read a bit about this and figured we should be using UserMetricsAction here instead. I am adding an update to tools/metrics/actions/chromeactions.txt for that and yes, it cannot use :? type logic because there's some processing done on the source to extract the action -- and the call along with the constant string need to appear on the same line for it to work correctly. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1783: void SearchProvider::DeleteMatchFromMatches(const AutocompleteMatch& match) { On 2013/11/23 00:08:43, Peter Kasting wrote: > The body of this function is akin to the identically-named function in > history_provider.h. > > Perhaps we should define a function in AutocompleteProvider, e.g. > FindMatchForDeletion(), that returns an iterator to the appropriate match (and > NOTREACHEDs on failure). Then the caller can decide how to handle this. This > would centralized the comparison portion of this function, so if we decide to > change it at some point, all providers will be updated simultaneously. It is a similar function, but it's not identical. In the other implementation they check the URL of the match is identical rather than the content. This doesn't work here because the URL gets modified with aqs= parameter and has some other subtle parameter differences even if it should be considered a match, so I am not sure it's possible to unify this code. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:281: // Optional deletion url provided with psuggest results to delete a On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: Don't use "psuggest" in code (not a well-understood phrase). How about: > > Optional deletion URL provided with suggestions. Fetching this URL should > result in some reasonable deletion behavior on the server, e.g. deleting this > term out of a user's server-side search history. Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:366: class SuggestionDeletionHandler : public net::URLFetcherDelegate { On 2013/11/23 00:08:43, Peter Kasting wrote: > Declare this class in the .cc file, since nothing else in this header needs it. > > Add a comment above the class describing briefly what it's for and how it's > used. Since I added a ScopedVector<SuggestionDeletionHandler> I need it in the header class now. Added a comment. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:369: virtual void StartRequest(const std::string& deletion_url, Profile* profile, On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: One arg per line > > Is this an override? If so, you should say OVERRIDE, and put a "// > net::URLFetcherDelegate:" comment above it. (Put a blank line above said > comment.) Done with args. this is not an override. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:371: private: On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: Blank line above this Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:372: // The class takes care of deleting itself when it's done. On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: This kind of comment should either be stated in the above-class class-level > comment or not at all. Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:374: virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE; On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: Again, put a blank line and a parent-class comment above this as in the > public section. Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:375: scoped_ptr<net::URLFetcher> deletion_fetcher_; On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: Blank line above this Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:403: // Callback for deletion request On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: This comment is too vague, and doesn't have trailing punctuation. Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:406: // Removes the deleted match from the list of matches. On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: ...from |matches_|. Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:620: // Used to store a deletion request url for psuggest autocomplete matches. On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: Don't use "psuggest"; try "for server-provided suggestions". Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4047: AutocompleteMatch m = AutocompleteMatch( On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: Always do "Class varname(...)" instead of "Class varname = Class(...)"; > it's not only shorter, it's more efficient. (severla places) Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4049: 640, On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: Is the value here important? If not use 0 or some other value that looks > less particularly-important. (several places) Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4074: fetcher->SetResponseString(""); On 2013/11/23 00:08:43, Peter Kasting wrote: > Nit: Is this line necessary at all? If it is, use std::string() (2 places) Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4076: ASSERT_TRUE(provider_->deletion_handler_.get() == NULL); On 2013/11/23 00:08:43, Peter Kasting wrote: > What is |deletion_handler_|? I don't see it in your change. > > Also, it looks like this test and the next both check the same thing: there is > no deletion handler. It would be nice to have tests that test that something > obviously different happens in response to success versus failure -- if such a > thing is even possible (it may not be if you follow my refactoring suggestions > in the .cc file). I would like to check whether my metric got recorded correctly in UMA, but I am not sure how to figure that out in a unit test. Any suggestions?
https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:330: SearchProvider::SuggestionDeletionHandler::~SuggestionDeletionHandler() { On 2013/11/26 02:36:27, Maria wrote: > On 2013/11/23 00:08:43, Peter Kasting wrote: > > Nit: Function definition order should match the class' declaration order > > Done. > > Quick question: that doesn't seem currently true for search_provider.cc in > general. I grouped the functions together by usage -- should I rearrange them to > declaration order in .h file instead? All class definitions in all files should always match the class declaration order. If an existing file doesn't conform to this, at least put your new functions in some sort-of-compliant spot, e.g. just below the definition of whatever the immediate prior function declaration was. Better yet, make the file compliant, either in this change or a separate one. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1775: void SearchProvider::OnDeletionComplete(bool success) { On 2013/11/26 02:36:27, Maria wrote: > On 2013/11/23 00:08:43, Peter Kasting wrote: > > If we check URL validity in AddMatchToMap(), then I think this can only be > > called back after the fetch completes; at which point this function can be > > inlined into the deletion handler. > > True, but I thought this may be a more flexible approach. It's possible we may > need to add to this function in the future to call something to notify the UI > that the deletion was successful (since some of the UI mock variations have a > confirmation message for deletion). It would be helpful if the function was run > in the SearchProvider class and had access to search provider members. Does that > sound reasonable? I would say, let's add that capability when it becomes necessary, unless you are already in the process of adding it in a followup patch. In general, my rule is, always make the code as simple as possible for the current capabilities, and explicitly ignore any design possibilities that aren't 100% guaranteed. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1783: void SearchProvider::DeleteMatchFromMatches(const AutocompleteMatch& match) { On 2013/11/26 02:36:27, Maria wrote: > On 2013/11/23 00:08:43, Peter Kasting wrote: > > The body of this function is akin to the identically-named function in > > history_provider.h. > > > > Perhaps we should define a function in AutocompleteProvider, e.g. > > FindMatchForDeletion(), that returns an iterator to the appropriate match (and > > NOTREACHEDs on failure). Then the caller can decide how to handle this. This > > would centralized the comparison portion of this function, so if we decide to > > change it at some point, all providers will be updated simultaneously. > > It is a similar function, but it's not identical. In the other implementation > they check the URL of the match is identical rather than the content. This > doesn't work here because the URL gets modified with aqs= parameter and has some > other subtle parameter differences even if it should be considered a match, so I > am not sure it's possible to unify this code. Hmm. That actually worries me. With things like entity suggestions, I think you can possibly have multiple matches whose |contents| and |type| match. So this deletion code may delete the wrong thing. I am not really sure how to deal with this. We may need to also check the additional recorded data on the matches or something. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:366: class SuggestionDeletionHandler : public net::URLFetcherDelegate { On 2013/11/26 02:36:27, Maria wrote: > Since I added a ScopedVector<SuggestionDeletionHandler> I need it in the header > class now. You can just forward-declare for that, you don't need the whole declaration. When you do, don't use a member class anyway; member classes are generally discouraged and the use of so many here in SearchProvider is probably a bad thing. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:369: virtual void StartRequest(const std::string& deletion_url, Profile* profile, On 2013/11/26 02:36:27, Maria wrote: > On 2013/11/23 00:08:43, Peter Kasting wrote: > > Nit: One arg per line > > > > Is this an override? If so, you should say OVERRIDE, and put a "// > > net::URLFetcherDelegate:" comment above it. (Put a blank line above said > > comment.) > > Done with args. this is not an override. Why is it virtual then? https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4076: ASSERT_TRUE(provider_->deletion_handler_.get() == NULL); On 2013/11/26 02:36:27, Maria wrote: > On 2013/11/23 00:08:43, Peter Kasting wrote: > > What is |deletion_handler_|? I don't see it in your change. > > > > Also, it looks like this test and the next both check the same thing: there is > > no deletion handler. It would be nice to have tests that test that something > > obviously different happens in response to success versus failure -- if such a > > thing is even possible (it may not be if you follow my refactoring suggestions > > in the .cc file). > > I would like to check whether my metric got recorded correctly in UMA, but I am > not sure how to figure that out in a unit test. Any suggestions? Nope. Sorry.
Made changes as you suggested. PTAL. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:330: SearchProvider::SuggestionDeletionHandler::~SuggestionDeletionHandler() { On 2013/11/26 02:46:22, Peter Kasting wrote: > On 2013/11/26 02:36:27, Maria wrote: > > On 2013/11/23 00:08:43, Peter Kasting wrote: > > > Nit: Function definition order should match the class' declaration order > > > > Done. > > > > Quick question: that doesn't seem currently true for search_provider.cc in > > general. I grouped the functions together by usage -- should I rearrange them > to > > declaration order in .h file instead? > > All class definitions in all files should always match the class declaration > order. > > If an existing file doesn't conform to this, at least put your new functions in > some sort-of-compliant spot, e.g. just below the definition of whatever the > immediate prior function declaration was. Better yet, make the file compliant, > either in this change or a separate one. Done. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1775: void SearchProvider::OnDeletionComplete(bool success) { On 2013/11/26 02:46:22, Peter Kasting wrote: > On 2013/11/26 02:36:27, Maria wrote: > > On 2013/11/23 00:08:43, Peter Kasting wrote: > > > If we check URL validity in AddMatchToMap(), then I think this can only be > > > called back after the fetch completes; at which point this function can be > > > inlined into the deletion handler. > > > > True, but I thought this may be a more flexible approach. It's possible we may > > need to add to this function in the future to call something to notify the UI > > that the deletion was successful (since some of the UI mock variations have a > > confirmation message for deletion). It would be helpful if the function was > run > > in the SearchProvider class and had access to search provider members. Does > that > > sound reasonable? > > I would say, let's add that capability when it becomes necessary, unless you are > already in the process of adding it in a followup patch. > > In general, my rule is, always make the code as simple as possible for the > current capabilities, and explicitly ignore any design possibilities that aren't > 100% guaranteed. Now this callback has another use which is to delete the deletion handler that finished (which we have to do from search_provider class since it owns the handlers). I think it makes sense to keep -- this is also a common pattern used by other url fetchers. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1783: void SearchProvider::DeleteMatchFromMatches(const AutocompleteMatch& match) { On 2013/11/26 02:46:22, Peter Kasting wrote: > On 2013/11/26 02:36:27, Maria wrote: > > On 2013/11/23 00:08:43, Peter Kasting wrote: > > > The body of this function is akin to the identically-named function in > > > history_provider.h. > > > > > > Perhaps we should define a function in AutocompleteProvider, e.g. > > > FindMatchForDeletion(), that returns an iterator to the appropriate match > (and > > > NOTREACHEDs on failure). Then the caller can decide how to handle this. > This > > > would centralized the comparison portion of this function, so if we decide > to > > > change it at some point, all providers will be updated simultaneously. > > > > It is a similar function, but it's not identical. In the other implementation > > they check the URL of the match is identical rather than the content. This > > doesn't work here because the URL gets modified with aqs= parameter and has > some > > other subtle parameter differences even if it should be considered a match, so > I > > am not sure it's possible to unify this code. > > Hmm. That actually worries me. With things like entity suggestions, I think > you can possibly have multiple matches whose |contents| and |type| match. So > this deletion code may delete the wrong thing. > > I am not really sure how to deal with this. We may need to also check the > additional recorded data on the matches or something. Right now the only deletable suggestions in search_provider come from psuggest. Since those are queries you typed, I don't think it can have multiple ones with the same content that are in fact different. Entity suggestions are not deletable. I will add a comment here for the future to note this assumption by the code. https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:366: class SuggestionDeletionHandler : public net::URLFetcherDelegate { On 2013/11/26 02:46:22, Peter Kasting wrote: > On 2013/11/26 02:36:27, Maria wrote: > > Since I added a ScopedVector<SuggestionDeletionHandler> I need it in the > header > > class now. > > You can just forward-declare for that, you don't need the whole declaration. > When you do, don't use a member class anyway; member classes are generally > discouraged and the use of so many here in SearchProvider is probably a bad > thing. Done.
https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/54203008/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4076: ASSERT_TRUE(provider_->deletion_handler_.get() == NULL); On 2013/11/26 02:46:22, Peter Kasting wrote: > On 2013/11/26 02:36:27, Maria wrote: > > On 2013/11/23 00:08:43, Peter Kasting wrote: > > > What is |deletion_handler_|? I don't see it in your change. > > > > > > Also, it looks like this test and the next both check the same thing: there > is > > > no deletion handler. It would be nice to have tests that test that > something > > > obviously different happens in response to success versus failure -- if such > a > > > thing is even possible (it may not be if you follow my refactoring > suggestions > > > in the .cc file). > > > > I would like to check whether my metric got recorded correctly in UMA, but I > am > > not sure how to figure that out in a unit test. Any suggestions? > > Nope. Sorry. OK, here's an idea. Subclass SearchProvider for this test and override OnDeletionComplete() so you can tell when it gets called back and with what arg. Then these tests can at least check that the callback is called with the correct arg when it's supposed to be. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:180: // suggestions out of web history. Nit: "suggestions out of web history" -> "personalized suggestions" ("web history" is sort of a Google product phrase) https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:187: void StartRequest( Given that our lone caller creates us and then immediately runs this function, why is this a separate function at all? It seems like the constructor should take these args and perform this functionality. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:190: const base::Callback<void(bool, SuggestionDeletionHandler *)>& Nit: No space before '*' This callback type should be a typedef. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:204: // SuggestionDeletionHandler --------------------------------- Nit: This goes above the declaration. Extend the "-"s out to column 79. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:506: SearchProvider::kDeletionUrlKey); Nit: Just inline this below https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:511: // Immediately updates the list of matches to show the match was deleted, Nit: updates -> update I'd put a blank line above this as well. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:776: NOTREACHED() << "Deletion handler should be in the vector of all handlers."; Nit: :Don't handle DCHECK failure" style rule means this should just be: DCHECK(it != deletion_handlers_.end()); deletion_handlers_.erase(it); https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:786: // re-examined if you are adding other deletable types. Nit: How about moving this above the conditional and changing to: Find the desired match to delete by checking the type and contents. We can't check the destination URL, because the autocomplete controller may have reformulated that. Note that while checking for matching contents works for personalized suggestions, if more match types gain deletion support, this algorithm may need to be re-examined. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:793: << "of matches"; Nit: I don't think the DCHECK here is of enough value to warrant the existence of |found|; I'd just remove all that https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:393: virtual void OnDeletionComplete(bool success, Remove "virtual" on these next two functions. (Note, though that if you follow my suggestion in the unittest, you'll actually need "virtual" on this function, unless instead of subclassing you use some sort of injection pattern.) https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:650: // A vector of all deletion handlers that are currently in progress. Nit: How about: Each deletion handler in this vector corresponds to an outstanding request that a server delete a personalized suggestion. Making this a ScopedVector causes us to auto-cancel all such requests on shutdown. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:651: ScopedVector<SuggestionDeletionHandler> deletion_handlers_; Nit: This should probably be a typedef. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3849: kNotApplicable, "", AutocompleteMatchType::NUM_TYPES}; Nit: "" -> std::string() Place '}' on the start of the next line. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3859: // Personalized query comes with a deletion url Nit: How about: A deletion URL on a personalized query should be reflected in the resulting AutocompleteMatch. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3873: // Personalized query, but no deletion url in response -- old server case Nit: How about: Personalized queries without deletion URLs shouldn't cause errors. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3886: // should be ignored. Is there any particular reason why we should ignore this? It seems like if a server sends us a deletion URL, we should just accept it. What problem are we trying to avoid? https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4078: AutocompleteMatch m( Tiny nit: |match| would be a better name than |m| (2 places) https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4079: provider_, Nit: For an arg list this short, I'd just wrap as: AutocompleteMatch match(provider_, 0, true, AutocompleteMatchType::SEARCH_SUGGEST); (2 places) https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4086: provider_->DeleteMatch(m); Seems like after this line, we should check that |deletion_handlers_| is non-empty, to verify that the fetch will actually result in some kind of processing. We should probably also check that |matches_| is cleared immediately. (2 places)
Hey, I tried your approach with SearchProvider subclassing, but unfortunately that's hard because SearchProvider has a private destructor in order to prevent that, it seems. I think the tests we have as-is are pretty reasonable even though we don't verify the success vs. failure scenario, so I would like to propose we just stick with them. At the very least they provide code coverage. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:180: // suggestions out of web history. On 2013/11/27 21:58:19, Peter Kasting wrote: > Nit: "suggestions out of web history" -> "personalized suggestions" ("web > history" is sort of a Google product phrase) Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:187: void StartRequest( On 2013/11/27 21:58:19, Peter Kasting wrote: > Given that our lone caller creates us and then immediately runs this function, > why is this a separate function at all? It seems like the constructor should > take these args and perform this functionality. I don't like putting work in constructor and generally the style guide agrees: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Doing_... https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:190: const base::Callback<void(bool, SuggestionDeletionHandler *)>& On 2013/11/27 21:58:19, Peter Kasting wrote: > Nit: No space before '*' > > This callback type should be a typedef. Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:204: // SuggestionDeletionHandler --------------------------------- On 2013/11/27 21:58:19, Peter Kasting wrote: > Nit: This goes above the declaration. Extend the "-"s out to column 79. Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:506: SearchProvider::kDeletionUrlKey); On 2013/11/27 21:58:19, Peter Kasting wrote: > Nit: Just inline this below Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:511: // Immediately updates the list of matches to show the match was deleted, On 2013/11/27 21:58:19, Peter Kasting wrote: > Nit: updates -> update > > I'd put a blank line above this as well. Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:776: NOTREACHED() << "Deletion handler should be in the vector of all handlers."; On 2013/11/27 21:58:19, Peter Kasting wrote: > Nit: :Don't handle DCHECK failure" style rule means this should just be: > > DCHECK(it != deletion_handlers_.end()); > deletion_handlers_.erase(it); Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:786: // re-examined if you are adding other deletable types. On 2013/11/27 21:58:19, Peter Kasting wrote: > Nit: How about moving this above the conditional and changing to: > > Find the desired match to delete by checking the type and contents. We can't > check the destination URL, because the autocomplete controller may have > reformulated that. Note that while checking for matching contents works for > personalized suggestions, if more match types gain deletion support, this > algorithm may need to be re-examined. Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:793: << "of matches"; On 2013/11/27 21:58:19, Peter Kasting wrote: > Nit: I don't think the DCHECK here is of enough value to warrant the existence > of |found|; I'd just remove all that Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:393: virtual void OnDeletionComplete(bool success, On 2013/11/27 21:58:19, Peter Kasting wrote: > Remove "virtual" on these next two functions. (Note, though that if you follow > my suggestion in the unittest, you'll actually need "virtual" on this function, > unless instead of subclassing you use some sort of injection pattern.) Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:650: // A vector of all deletion handlers that are currently in progress. On 2013/11/27 21:58:19, Peter Kasting wrote: > Nit: How about: > > Each deletion handler in this vector corresponds to an outstanding request that > a server delete a personalized suggestion. Making this a ScopedVector causes us > to auto-cancel all such requests on shutdown. Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:651: ScopedVector<SuggestionDeletionHandler> deletion_handlers_; On 2013/11/27 21:58:19, Peter Kasting wrote: > Nit: This should probably be a typedef. Not sure I understand the reason to typedef this when this is the only place where the type is used https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3849: kNotApplicable, "", AutocompleteMatchType::NUM_TYPES}; On 2013/11/27 21:58:19, Peter Kasting wrote: > Nit: "" -> std::string() > > Place '}' on the start of the next line. Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3859: // Personalized query comes with a deletion url On 2013/11/27 21:58:19, Peter Kasting wrote: > Nit: How about: > > A deletion URL on a personalized query should be reflected in the resulting > AutocompleteMatch. Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3873: // Personalized query, but no deletion url in response -- old server case On 2013/11/27 21:58:19, Peter Kasting wrote: > Nit: How about: > > Personalized queries without deletion URLs shouldn't cause errors. Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3886: // should be ignored. On 2013/11/27 21:58:19, Peter Kasting wrote: > Is there any particular reason why we should ignore this? It seems like if a > server sends us a deletion URL, we should just accept it. What problem are we > trying to avoid? During results parsing we don't bother looking at the last map and parsing its contents unless we are expecting some information to be there. So we first do a check for a relevant suggest type result before doing all the work. While we could always do it, currently 99% of the time that would be wasted work. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4078: AutocompleteMatch m( On 2013/11/27 21:58:19, Peter Kasting wrote: > Tiny nit: |match| would be a better name than |m| (2 places) Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4079: provider_, On 2013/11/27 21:58:19, Peter Kasting wrote: > Nit: For an arg list this short, I'd just wrap as: > > AutocompleteMatch match(provider_, 0, true, > AutocompleteMatchType::SEARCH_SUGGEST); > > (2 places) Done. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4086: provider_->DeleteMatch(m); On 2013/11/27 21:58:19, Peter Kasting wrote: > Seems like after this line, we should check that |deletion_handlers_| is > non-empty, to verify that the fetch will actually result in some kind of > processing. We should probably also check that |matches_| is cleared > immediately. (2 places) Done.
On 2013/11/28 00:17:02, Maria wrote: > Hey, I tried your approach with SearchProvider subclassing, but unfortunately > that's hard because SearchProvider has a private destructor in order to prevent > that, it seems. Just make the destructor protected. I always make all functions as private as possible for clarity reasons. It doesn't mean they can't be made more-public later if there's a good reason. I think there's value in testing that something actually happens differently as a result of a success versus failure server response. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:187: void StartRequest( On 2013/11/28 00:17:03, Maria wrote: > On 2013/11/27 21:58:19, Peter Kasting wrote: > > Given that our lone caller creates us and then immediately runs this function, > > why is this a separate function at all? It seems like the constructor should > > take these args and perform this functionality. > > I don't like putting work in constructor and generally the style guide agrees: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Doing_... This code can neither fail nor perform virtual method calls on |this|, so it's safe to perform at construction time. Please put it in the constructor, it's not only a simpler API, it's also safer and easier to reason about because we know the relevant variables can never be unset. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:651: ScopedVector<SuggestionDeletionHandler> deletion_handlers_; On 2013/11/28 00:17:03, Maria wrote: > On 2013/11/27 21:58:19, Peter Kasting wrote: > > Nit: This should probably be a typedef. > > Not sure I understand the reason to typedef this when this is the only place > where the type is used Because it's not the only place. OnDeletionComplete() uses this type. https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/54203008/diff/430001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3886: // should be ignored. On 2013/11/28 00:17:03, Maria wrote: > On 2013/11/27 21:58:19, Peter Kasting wrote: > > Is there any particular reason why we should ignore this? It seems like if a > > server sends us a deletion URL, we should just accept it. What problem are we > > trying to avoid? > > During results parsing we don't bother looking at the last map and parsing its > contents unless we are expecting some information to be there. So we first do a > check for a relevant suggest type result before doing all the work. While we > could always do it, currently 99% of the time that would be wasted work. I left a comment on that code. https://codereview.chromium.org/54203008/diff/470001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/54203008/diff/470001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1197: suggestion_details->GetDictionary(index, &suggestion_detail) && Nit: Instead of duplicating this GetDictionary() call, why don't we just do it unconditionally; this will simplify these conditionals to just "if ((type == ...) && suggestion_detail). This isn't costly, we already have the list sitting around. At that point, we really don't need the type check at all, unless we wanted some kind of sanity check, or unless we wanted different types to support different meanings of the same parameters (ugh!). We could just put it all in a block.
On Wed, Nov 27, 2013 at 4:44 PM, <pkasting@chromium.org> wrote: > On 2013/11/28 00:17:02, Maria wrote: > >> Hey, I tried your approach with SearchProvider subclassing, but >> unfortunately >> that's hard because SearchProvider has a private destructor in order to >> > prevent > >> that, it seems. >> > > Just make the destructor protected. > > I always make all functions as private as possible for clarity reasons. It > doesn't mean they can't be made more-public later if there's a good > reason. I > think there's value in testing that something actually happens differently > as a > result of a success versus failure server response. Done. > > > > https://codereview.chromium.org/54203008/diff/430001/ > chrome/browser/autocomplete/search_provider.cc > File chrome/browser/autocomplete/search_provider.cc (right): > > https://codereview.chromium.org/54203008/diff/430001/ > chrome/browser/autocomplete/search_provider.cc#newcode187 > chrome/browser/autocomplete/search_provider.cc:187: void StartRequest( > On 2013/11/28 00:17:03, Maria wrote: > >> On 2013/11/27 21:58:19, Peter Kasting wrote: >> > Given that our lone caller creates us and then immediately runs this >> > function, > >> > why is this a separate function at all? It seems like the >> > constructor should > >> > take these args and perform this functionality. >> > > I don't like putting work in constructor and generally the style guide >> > agrees: > > http://google-styleguide.googlecode.com/svn/trunk/ > cppguide.xml?showone=Doing_Work_in_Constructors#Doing_Work_in_Constructors > > This code can neither fail nor perform virtual method calls on |this|, > so it's safe to perform at construction time. Please put it in the > constructor, it's not only a simpler API, it's also safer and easier to > reason about because we know the relevant variables can never be unset. Done. Though IMO having a method was better. While my constructor doesn't do anything potentially harmful now, it's now easy for someone to add a call there doing one of the problematic things without realizing it's an issue. > > > https://codereview.chromium.org/54203008/diff/430001/ > chrome/browser/autocomplete/search_provider.h > File chrome/browser/autocomplete/search_provider.h (right): > > https://codereview.chromium.org/54203008/diff/430001/ > chrome/browser/autocomplete/search_provider.h#newcode651 > chrome/browser/autocomplete/search_provider.h:651: > ScopedVector<SuggestionDeletionHandler> deletion_handlers_; > On 2013/11/28 00:17:03, Maria wrote: > >> On 2013/11/27 21:58:19, Peter Kasting wrote: >> > Nit: This should probably be a typedef. >> > > Not sure I understand the reason to typedef this when this is the only >> > place > >> where the type is used >> > > Because it's not the only place. OnDeletionComplete() uses this type. Done. > > > https://codereview.chromium.org/54203008/diff/430001/ > chrome/browser/autocomplete/search_provider_unittest.cc > File chrome/browser/autocomplete/search_provider_unittest.cc (right): > > https://codereview.chromium.org/54203008/diff/430001/ > chrome/browser/autocomplete/search_provider_unittest.cc#newcode3886 > chrome/browser/autocomplete/search_provider_unittest.cc:3886: // should > be ignored. > On 2013/11/28 00:17:03, Maria wrote: > >> On 2013/11/27 21:58:19, Peter Kasting wrote: >> > Is there any particular reason why we should ignore this? It seems >> > like if a > >> > server sends us a deletion URL, we should just accept it. What >> > problem are we > >> > trying to avoid? >> > > During results parsing we don't bother looking at the last map and >> > parsing its > >> contents unless we are expecting some information to be there. So we >> > first do a > >> check for a relevant suggest type result before doing all the work. >> > While we > >> could always do it, currently 99% of the time that would be wasted >> > work. > > I left a comment on that code. > > https://codereview.chromium.org/54203008/diff/470001/ > chrome/browser/autocomplete/search_provider.cc > File chrome/browser/autocomplete/search_provider.cc (right): > > https://codereview.chromium.org/54203008/diff/470001/ > chrome/browser/autocomplete/search_provider.cc#newcode1197 > chrome/browser/autocomplete/search_provider.cc:1197: > suggestion_details->GetDictionary(index, &suggestion_detail) && > Nit: Instead of duplicating this GetDictionary() call, why don't we just > do it unconditionally; this will simplify these conditionals to just "if > ((type == ...) && suggestion_detail). This isn't costly, we already > have the list sitting around. > > At that point, we really don't need the type check at all, unless we > wanted some kind of sanity check, or unless we wanted different types to > support different meanings of the same parameters (ugh!). We could just > put it all in a block. > I changed the code to get deletion URL unconditionally. I don't want to do the same for entity code though because it's possible that those items could get sent for something that's not an entity request and that would trigger a change to the UI that may not be intentional. I don't want to accidentally introduce new unintended behaviour. > > https://codereview.chromium.org/54203008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1181: string16 disambiguating_query; Nit: Declare this in the most local scope possible https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:398: // Records in UMA whether the deletion request resulted in success. Nit: Maybe add "This is virtual so test code can override it to check that we correctly handle the request result." https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:73: FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, TestDeleteMatch_FakeProvider); Nit: This declaration seems useless? https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:79: Profile* profile) : SearchProvider(listener, profile), Nit: Wrap ":" and following to next line https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3792: }; Nit: Wrapping the } is fine, but in that case, also wrap after the opening "{". https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4119: "https://www.google.com/complete/deleteitem?q=foo"); Nit: If we combine these two tests into one, we can eliminate this re-construction of |match| and maybe a few of the other lines too.
https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1181: string16 disambiguating_query; On 2013/12/03 00:33:51, Peter Kasting wrote: > Nit: Declare this in the most local scope possible Done. https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:73: FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, TestDeleteMatch_FakeProvider); On 2013/12/03 00:33:51, Peter Kasting wrote: > Nit: This declaration seems useless? Done. https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:79: Profile* profile) : SearchProvider(listener, profile), On 2013/12/03 00:33:51, Peter Kasting wrote: > Nit: Wrap ":" and following to next line Done. https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3792: }; On 2013/12/03 00:33:51, Peter Kasting wrote: > Nit: Wrapping the } is fine, but in that case, also wrap after the opening "{". Done. https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4119: "https://www.google.com/complete/deleteitem?q=foo"); On 2013/12/03 00:33:51, Peter Kasting wrote: > Nit: If we combine these two tests into one, we can eliminate this > re-construction of |match| and maybe a few of the other lines too. We could, but I'd rather have two separate test cases. It seems cleaner that they don't depend on each other's passing and I don't believe it's a big deal to have repeated code in a unit test.
https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4119: "https://www.google.com/complete/deleteitem?q=foo"); On 2013/12/03 01:00:26, Maria wrote: > On 2013/12/03 00:33:51, Peter Kasting wrote: > > Nit: If we combine these two tests into one, we can eliminate this > > re-construction of |match| and maybe a few of the other lines too. > > We could, but I'd rather have two separate test cases. It seems cleaner that > they don't depend on each other's passing They shouldn't depend on each other's passing. Which makes me notice: Almost all these ASSERTs should be EXPECTs. Only use ASSERT for conditions where failing the condition and continuing would result in a subsequent crash. Once you do this, I believe the tests should be independent even if combined into one function. > and I don't believe it's a big deal to > have repeated code in a unit test. It's the same as repeated code in Chrome -- it's a maintenance burden if you need to modify the affected code, and risks not all tests getting changed appropriately. I don't think it's a huge deal, but I think it's strictly better to not duplicate than to duplicate.
On Mon, Dec 2, 2013 at 5:03 PM, <pkasting@chromium.org> wrote: > > https://codereview.chromium.org/54203008/diff/490001/ > chrome/browser/autocomplete/search_provider_unittest.cc > File chrome/browser/autocomplete/search_provider_unittest.cc (right): > > https://codereview.chromium.org/54203008/diff/490001/ > chrome/browser/autocomplete/search_provider_unittest.cc#newcode4119 > chrome/browser/autocomplete/search_provider_unittest.cc:4119: > "https://www.google.com/complete/deleteitem?q=foo"); > On 2013/12/03 01:00:26, Maria wrote: > >> On 2013/12/03 00:33:51, Peter Kasting wrote: >> > Nit: If we combine these two tests into one, we can eliminate this >> > re-construction of |match| and maybe a few of the other lines too. >> > > We could, but I'd rather have two separate test cases. It seems >> > cleaner that > >> they don't depend on each other's passing >> > > They shouldn't depend on each other's passing. Which makes me notice: > Almost all these ASSERTs should be EXPECTs. Only use ASSERT for > conditions where failing the condition and continuing would result in a > subsequent crash. Once you do this, I believe the tests should be > independent even if combined into one function. I can change ASSETs to EXPECTs, not a problem. > > > and I don't believe it's a big deal to >> have repeated code in a unit test. >> > > It's the same as repeated code in Chrome -- it's a maintenance burden if > you need to modify the affected code, and risks not all tests getting > changed appropriately. > > I don't think it's a huge deal, but I think it's strictly better to not > duplicate than to duplicate. > I don't think it makes sense in this case. It seems to me like there are two options here: a) We just paste the code from Failure case below the Success case code, minus the match declaration. In this case we've created a long test function, that's harder to read and that saves us on exactly two lines, but makes the testing function itself long and harder to understand. I don't think that makes a lot of sense, but I am willing to do it if you feel strongly about this change. b) We refactor some of the code into helper method parametrized by success criteria. In this case, we have less duplication, but anyone trying to understand test cases now has to jump around the code to figure out what's actually being tested and how. I think that it's important for tests to be simple and clear because it helps developers new to the code to understand the expectations of the code by the people who wrote it. And I wouldn't want to obfuscate meaning behind a helper function. In this particular case, I don't think it's a large burden to update two places. > > https://codereview.chromium.org/54203008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4094: TEST_F(SearchProviderTest, TestDeleteMatch_HasDeletionUrlSuccess) { I think TestDeleteMatch_Success is sufficient. HasDeletionUrlSuccess is confusing to me. https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4101: provider_->DeleteMatch(match); An empty line before this to create separation between setup and test part of the code. https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4107: ASSERT_TRUE(fetcher); Omit this? It doesn't seem particularly useful. https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4108: fetcher->set_response_code(200); Move the fetcher set up code before the call to DeleteMatch. https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4119: "https://www.google.com/complete/deleteitem?q=foo"); I have been told that the assert/expects should not be put in helper methods, as they affect readability of tests. I think the only part which really merits avoiding duplication is creating and adding the match to provider_.matches_.
On 2013/12/03 01:18:09, mariakhomenko_google.com wrote: > a) We just paste the code from Failure case below the Success case code, > minus the match declaration. I was thinking this: TEST_F(SearchProviderTest, TestDeleteMatch_HasDeletionUrlSuccess) { AutocompleteMatch match(provider_, 0, true, AutocompleteMatchType::SEARCH_SUGGEST); match.RecordAdditionalInfo( SearchProvider::kDeletionUrlKey, "https://www.google.com/complete/deleteitem?q=foo"); // Test a successful deletion request. provider_->matches_.push_back(match); provider_->DeleteMatch(match); EXPECT_FALSE(provider_->deletion_handlers_.empty()); EXPECT_TRUE(provider_->matches_.empty()); net::TestURLFetcher* fetcher = test_factory_.GetFetcherByID( SearchProvider::kDeletionURLFetcherID); ASSERT_TRUE(fetcher); fetcher->set_response_code(200); fetcher->delegate()->OnURLFetchComplete(fetcher); EXPECT_TRUE(provider_->deletion_handlers_.empty()); EXPECT_TRUE(provider_->is_success()); // Test a failing deletion request. provider_->matches_.push_back(match); provider_->DeleteMatch(match); EXPECT_FALSE(provider_->deletion_handlers_.empty()); net::TestURLFetcher* fetcher = test_factory_.GetFetcherByID( SearchProvider::kDeletionURLFetcherID); fetcher->set_response_code(500); fetcher->delegate()->OnURLFetchComplete(fetcher); EXPECT_TRUE(provider_->deletion_handlers_.empty()); EXPECT_FALSE(provider_->is_success()); } It's a total of 10 physical lines saved. Not a ton, but not nothing. I don't feel violently, I just think this is not only shorter, it's actually _easier_ to read than the existing version. The idea to use a helper function actually isn't bad, though. If the helper takes both the response code and the expected "success" result, then it seems perhaps even more readable. I don't think it obfuscates anything. Who reads code without the ability to jump to a called function? as long as you don't have to keep hopping back and forth, it's not a problem.
IMHO, this is worse, but I combined the two test cases together. I don't think lines saved is a good metric to use for the unit test clarity. And I think having separate function to test separate cases makes more sense. But in the overall scheme of things, this is probably minor... :) https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4101: provider_->DeleteMatch(match); On 2013/12/03 01:25:47, Anuj wrote: > An empty line before this to create separation between setup and test part of > the code. Done. https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4107: ASSERT_TRUE(fetcher); On 2013/12/03 01:25:47, Anuj wrote: > Omit this? It doesn't seem particularly useful. All the other tests in this class do it. It's useful because if the tested code unexpected didn't create a fetcher, we will catch it right here. https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4108: fetcher->set_response_code(200); On 2013/12/03 01:25:47, Anuj wrote: > Move the fetcher set up code before the call to DeleteMatch. That's not possible because GetFetcherByID relies on the fetcher getting created within DeleteMatch() call.
Still LGTM https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:398: // Records in UMA whether the deletion request resulted in success. On 2013/12/03 00:33:51, Peter Kasting wrote: > Nit: Maybe add "This is virtual so test code can override it to check that we > correctly handle the request result." Did you decide this wasn't worth doing? https://codereview.chromium.org/54203008/diff/530001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/54203008/diff/530001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1197: suggestion_detail->GetString("q", &suggest_query_params); Nit: Either add a blank line above this, or remove the one just above. https://codereview.chromium.org/54203008/diff/530001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/54203008/diff/530001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4100: // Test a successful deletion request Nit: Comments should end in periods. (2 places) https://codereview.chromium.org/54203008/diff/530001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4118: EXPECT_TRUE(provider_->matches_.empty()); Nit: Not necessary (above test already tests this)
https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/54203008/diff/490001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:398: // Records in UMA whether the deletion request resulted in success. On 2013/12/03 01:47:50, Peter Kasting wrote: > On 2013/12/03 00:33:51, Peter Kasting wrote: > > Nit: Maybe add "This is virtual so test code can override it to check that we > > correctly handle the request result." > > Did you decide this wasn't worth doing? Done. https://codereview.chromium.org/54203008/diff/530001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/54203008/diff/530001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1197: suggestion_detail->GetString("q", &suggest_query_params); On 2013/12/03 01:47:50, Peter Kasting wrote: > Nit: Either add a blank line above this, or remove the one just above. Done. https://codereview.chromium.org/54203008/diff/530001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/54203008/diff/530001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4100: // Test a successful deletion request On 2013/12/03 01:47:50, Peter Kasting wrote: > Nit: Comments should end in periods. (2 places) Done. https://codereview.chromium.org/54203008/diff/530001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:4118: EXPECT_TRUE(provider_->matches_.empty()); On 2013/12/03 01:47:50, Peter Kasting wrote: > Nit: Not necessary (above test already tests this) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mariakhomenko@chromium.org/54203008/55...
Message was sent while issue was closed.
Change committed as 238422 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
