Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
96753004:
Ensure zero suggest can handle XSSI-escaped output. (Closed)
|
Created:
7 years ago by Maria Modified:
7 years ago CC:
chromium-reviews, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnsure zero suggest can handle XSSI-escaped output.
Zero suggest request re-uses the same URL as regular suggest and
therefore will be requesting xssi=t by default. We should make sure the
output is then correctly parsed.
BUG=329382
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238360
Patch Set 1 #
Total comments: 1
Patch Set 2 : Update CL to share json parsing code #
Total comments: 9
Patch Set 3 : Comment fix #Patch Set 4 : Fixing the return value in failure case #
Total comments: 2
Patch Set 5 : added a unit test for invalid response case #
Messages
Total messages: 17 (0 generated)
This is the same change as Anuj made to search_provider in https://codereview.chromium.org/45863006/. Needs to be here as well since we are using the same base request URL.
lgtm https://codereview.chromium.org/96753004/diff/1/chrome/browser/autocomplete/z... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/96753004/diff/1/chrome/browser/autocomplete/z... chrome/browser/autocomplete/zero_suggest_provider.cc:137: for (size_t response_start_index = json_data.find("["), i = 0; I'm not sure why we try up to 5 times and why we ignore contiguous open brackets, but I guess this works. I'm not sure if it might be better to put this code in a separate function in SearchProvider to avoid duplicate code?
I changed the code to share JSON-parsing logic. PTAL.
lgtm (though I don't fully grok the original code, the change you made here I understand and is fine) https://codereview.chromium.org/96753004/diff/20001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/96753004/diff/20001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:413: // Attempts to parse JSON response received from the provider stripping XSSI nit: Attempts to parse -> Parses (the attempting is obvious from the second sentence) nit: comma after provider https://codereview.chromium.org/96753004/diff/20001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:414: // protection if needed. Returns the parsed data, if successful, NULL nit: no comma before "if successful"
https://codereview.chromium.org/96753004/diff/20001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/96753004/diff/20001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1044: return scoped_ptr<Value>(Value::CreateNullValue()); Are you depending on ParseSuggestResults to fail from the caller in this case (since I think data.get() will succeed)? Also, are you depending on RVO since I don't think scoped_ptr's can be copied? Or maybe I don't understand how scoped_ptr works (in Chrome), in which case feel free to ignore this comment. https://codereview.chromium.org/96753004/diff/20001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/96753004/diff/20001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:413: // Attempts to parse JSON response received from the provider stripping XSSI comma after "provider"?
https://codereview.chromium.org/96753004/diff/20001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/96753004/diff/20001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1044: return scoped_ptr<Value>(Value::CreateNullValue()); On 2013/12/02 23:04:52, H Fung wrote: > Are you depending on ParseSuggestResults to fail from the caller in this case > (since I think data.get() will succeed)? Also, are you depending on RVO since I > don't think scoped_ptr's can be copied? Or maybe I don't understand how > scoped_ptr works (in Chrome), in which case feel free to ignore this comment. I was actually expecting data.get() to return null in this case. I'll double check. As for the scoped_ptr return value, I was following the instructions here: http://www.chromium.org/developers/smart-pointer-guidelines, which I was somewhat surprised about as well. https://codereview.chromium.org/96753004/diff/20001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/96753004/diff/20001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:413: // Attempts to parse JSON response received from the provider stripping XSSI On 2013/12/02 23:04:52, H Fung wrote: > comma after "provider"? Done. https://codereview.chromium.org/96753004/diff/20001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:413: // Attempts to parse JSON response received from the provider stripping XSSI On 2013/12/02 22:32:19, Mark P wrote: > nit: Attempts to parse -> Parses > (the attempting is obvious from the second sentence) > > nit: comma after provider Done. https://codereview.chromium.org/96753004/diff/20001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:414: // protection if needed. Returns the parsed data, if successful, NULL On 2013/12/02 22:32:19, Mark P wrote: > nit: no comma before "if successful" Done.
On Mon, Dec 2, 2013 at 3:24 PM, <mariakhomenko@chromium.org> wrote: > > https://codereview.chromium.org/96753004/diff/20001/ > chrome/browser/autocomplete/search_provider.cc > File chrome/browser/autocomplete/search_provider.cc (right): > > https://codereview.chromium.org/96753004/diff/20001/ > chrome/browser/autocomplete/search_provider.cc#newcode1044 > chrome/browser/autocomplete/search_provider.cc:1044: return > scoped_ptr<Value>(Value::CreateNullValue()); > On 2013/12/02 23:04:52, H Fung wrote: > >> Are you depending on ParseSuggestResults to fail from the caller in >> > this case > >> (since I think data.get() will succeed)? Also, are you depending on >> > RVO since I > >> don't think scoped_ptr's can be copied? Or maybe I don't understand >> > how > >> scoped_ptr works (in Chrome), in which case feel free to ignore this >> > comment. > > I was actually expecting data.get() to return null in this case. I'll > double check. > > As for the scoped_ptr return value, I was following the instructions > here: http://www.chromium.org/developers/smart-pointer-guidelines, which > I was somewhat surprised about as well. You were right about the return value here -- I updated the return line so that it would return null in this case. I would appreciate if someone confirm that what I am doing with returning scoped_ptr object actually makes sense. > > > https://codereview.chromium.org/96753004/diff/20001/ > chrome/browser/autocomplete/search_provider.h > File chrome/browser/autocomplete/search_provider.h (right): > > https://codereview.chromium.org/96753004/diff/20001/ > chrome/browser/autocomplete/search_provider.h#newcode413 > chrome/browser/autocomplete/search_provider.h:413: // Attempts to parse > JSON response received from the provider stripping XSSI > On 2013/12/02 23:04:52, H Fung wrote: > >> comma after "provider"? >> > > Done. > > > https://codereview.chromium.org/96753004/diff/20001/ > chrome/browser/autocomplete/search_provider.h#newcode413 > chrome/browser/autocomplete/search_provider.h:413: // Attempts to parse > JSON response received from the provider stripping XSSI > On 2013/12/02 22:32:19, Mark P wrote: > >> nit: Attempts to parse -> Parses >> (the attempting is obvious from the second sentence) >> > > nit: comma after provider >> > > Done. > > > https://codereview.chromium.org/96753004/diff/20001/ > chrome/browser/autocomplete/search_provider.h#newcode414 > chrome/browser/autocomplete/search_provider.h:414: // protection if > needed. Returns the parsed data, if successful, NULL > On 2013/12/02 22:32:19, Mark P wrote: > >> nit: no comma before "if successful" >> > > Done. > > https://codereview.chromium.org/96753004/ > 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/96753004/diff/20001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/96753004/diff/20001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1044: return scoped_ptr<Value>(Value::CreateNullValue()); On 2013/12/02 23:24:29, Maria wrote: > On 2013/12/02 23:04:52, H Fung wrote: > > Are you depending on ParseSuggestResults to fail from the caller in this case > > (since I think data.get() will succeed)? Also, are you depending on RVO since > I > > don't think scoped_ptr's can be copied? Or maybe I don't understand how > > scoped_ptr works (in Chrome), in which case feel free to ignore this comment. > > I was actually expecting data.get() to return null in this case. I'll double > check. > > As for the scoped_ptr return value, I was following the instructions here: > http://www.chromium.org/developers/smart-pointer-guidelines, which I was > somewhat surprised about as well. Thanks, it seems like Chrome's scoped_ptr is copyable. https://codereview.chromium.org/96753004/diff/60001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/96753004/diff/60001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1040: if (error_code == 0) { No curly braces since I think that's the convention here?
If you're concern about anything, feel free to add Peter to this review. On Mon, Dec 2, 2013 at 3:43 PM, <hfung@chromium.org> wrote: > lgtm > > > > > > https://codereview.chromium.org/96753004/diff/20001/ > chrome/browser/autocomplete/search_provider.cc > File chrome/browser/autocomplete/search_provider.cc (right): > > https://codereview.chromium.org/96753004/diff/20001/ > chrome/browser/autocomplete/search_provider.cc#newcode1044 > chrome/browser/autocomplete/search_provider.cc:1044: return > scoped_ptr<Value>(Value::CreateNullValue()); > On 2013/12/02 23:24:29, Maria wrote: > >> On 2013/12/02 23:04:52, H Fung wrote: >> > Are you depending on ParseSuggestResults to fail from the caller in >> > this case > >> > (since I think data.get() will succeed)? Also, are you depending on >> > RVO since > >> I >> > don't think scoped_ptr's can be copied? Or maybe I don't understand >> > how > >> > scoped_ptr works (in Chrome), in which case feel free to ignore this >> > comment. > > I was actually expecting data.get() to return null in this case. I'll >> > double > >> check. >> > > As for the scoped_ptr return value, I was following the instructions >> > here: > >> http://www.chromium.org/developers/smart-pointer-guidelines, which I >> > was > >> somewhat surprised about as well. >> > > Thanks, it seems like Chrome's scoped_ptr is copyable. > > https://codereview.chromium.org/96753004/diff/60001/ > chrome/browser/autocomplete/search_provider.cc > File chrome/browser/autocomplete/search_provider.cc (right): > > https://codereview.chromium.org/96753004/diff/60001/ > chrome/browser/autocomplete/search_provider.cc#newcode1040 > chrome/browser/autocomplete/search_provider.cc:1040: if (error_code == > 0) { > No curly braces since I think that's the convention here? > > https://codereview.chromium.org/96753004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I added a unit test to cover the case where we get an invalid response. Going to submit it now. https://codereview.chromium.org/96753004/diff/60001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/96753004/diff/60001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1040: if (error_code == 0) { On 2013/12/02 23:43:45, H Fung wrote: > No curly braces since I think that's the convention here? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mariakhomenko@chromium.org/96753004/80001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mariakhomenko@chromium.org/96753004/80001
Message was sent while issue was closed.
Change committed as 238360
Message was sent while issue was closed.
Hi Maria, I just realized that the fix is in M33 and was broken in M32 (https://codereview.chromium.org/51603002), and our beta ZeroSuggest experiments are broken. Should I request a merge to M32 or do you want to?
Would you mind requesting and doing the merge? I do not have committer status, so I would have to ask someone to do it regardless since I don't have permissions to run drover. Thanks for catching this! And sorry for the trouble. Maria On Tue, Dec 17, 2013 at 4:59 PM, <hfung@chromium.org> wrote: > Hi Maria, > I just realized that the fix is in M33 and was broken in M32 > (https://codereview.chromium.org/51603002), and our beta ZeroSuggest > experiments > are broken. Should I request a merge to M32 or do you want to? > > https://codereview.chromium.org/96753004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
I'll request the merge and see what happens (though I'm not a committer either). On 2013/12/18 01:12:39, mariakhomenko_google.com wrote: > Would you mind requesting and doing the merge? I do not have committer > status, so I would have to ask someone to do it regardless since I don't > have permissions to run drover. > > Thanks for catching this! And sorry for the trouble. > > Maria > > > On Tue, Dec 17, 2013 at 4:59 PM, <mailto:hfung@chromium.org> wrote: > > > Hi Maria, > > I just realized that the fix is in M33 and was broken in M32 > > (https://codereview.chromium.org/51603002), and our beta ZeroSuggest > > experiments > > are broken. Should I request a merge to M32 or do you want to? > > > > https://codereview.chromium.org/96753004/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
