Fix for: Autofill should not ping the server again for the same form...

Fix for: Autofill should not ping the server again for the same form
BUG=67039
TEST=unit-tested. Any network sniffer could be used to test that there no subsequent calls to the web for the same form.

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

[dhollowa, 2011-01-25 23:33:02]

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_download.cc (right):

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.cc:38: number_of_forms_to_cache_(16),
Let's pull this |16| out into a named constant within the .cc file. 
kFormCacheSize say?

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.cc:81: return false;
nit: style guide says this should be wrapped in braces due to the conditional
that spans two lines.

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.cc:240: size_t total_size =
forms_in_query.size();
My read of this is that if I passed in { "a", "b", "c" }, then I'd get back
"ab,c,".  And |total_size| would be 1 too big.  A small unit test for this
method in isolation would be helpful.

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_download.h (right):

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.h:120: void
CacheQueryRequest(std::vector<std::string> const& forms_in_query,
|const| in front of argument type please.  And elsewhere.

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.h:148:
std::list<std::pair<std::string, std::string> > cached_forms_;
Add a typedef for the cache type.  It'll make iterators more readable /
manageable.

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.h:149: size_t
number_of_forms_to_cache_;
This seems a bit verbose.  How about |form_cache_size_|.  Similarly with
getter/setter.

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_download_unittest.cc (right):

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download_unittest.cc:282: // Modify form
structures to miss the cache.
I'm curious why this is was added to the existing test.

[GeorgeY, 2011-01-25 23:51:29]

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_download.cc (right):

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.cc:38: number_of_forms_to_cache_(16),
On 2011/01/25 23:33:02, dhollowa wrote:
> Let's pull this |16| out into a named constant within the .cc file. 
> kFormCacheSize say?

Done.

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.cc:81: return false;
On 2011/01/25 23:33:02, dhollowa wrote:
> nit: style guide says this should be wrapped in braces due to the conditional
> that spans two lines.

Done.

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.cc:240: size_t total_size =
forms_in_query.size();
On 2011/01/25 23:33:02, dhollowa wrote:
> My read of this is that if I passed in { "a", "b", "c" }, then I'd get back
> "ab,c,".  And |total_size| would be 1 too big.  A small unit test for this
> method in isolation would be helpful.

nope "a,b,c\0" so everything is right. I do not think any unit tests needed for
*this* function. (evermore it is indirectly tested in every other test)

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_download.h (right):

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.h:120: void
CacheQueryRequest(std::vector<std::string> const& forms_in_query,
On 2011/01/25 23:33:02, dhollowa wrote:
> |const| in front of argument type please.  And elsewhere.

Done.

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.h:148:
std::list<std::pair<std::string, std::string> > cached_forms_;
On 2011/01/25 23:33:02, dhollowa wrote:
> Add a typedef for the cache type.  It'll make iterators more readable /
> manageable.

Sure, but as this used only in these two functions, explicit description
actually adds to readability.

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.h:149: size_t
number_of_forms_to_cache_;
On 2011/01/25 23:33:02, dhollowa wrote:
> This seems a bit verbose.  How about |form_cache_size_|.  Similarly with
> getter/setter.

Done.

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_download_unittest.cc (right):

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download_unittest.cc:282: // Modify form
structures to miss the cache.
On 2011/01/25 23:33:02, dhollowa wrote:
> I'm curious why this is was added to the existing test.

If we hit the cache, the request is always going to be successful, as we never
go over the wire, but the following test is specifically for failure due to the
requested server backoff.

[dhollowa, 2011-01-26 00:15:02]

LGTM.  With small spelling nit.  Thanks!

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_download.cc (right):

http://codereview.chromium.org/6366014/diff/8002/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.cc:240: size_t total_size =
forms_in_query.size();
Ok, I misread it.  Seems ok.

On 2011/01/25 23:51:30, GeorgeY wrote:
> On 2011/01/25 23:33:02, dhollowa wrote:
> > My read of this is that if I passed in { "a", "b", "c" }, then I'd get back
> > "ab,c,".  And |total_size| would be 1 too big.  A small unit test for this
> > method in isolation would be helpful.
> 
> nope "a,b,c\0" so everything is right. I do not think any unit tests needed
for
> *this* function. (evermore it is indirectly tested in every other test)

http://codereview.chromium.org/6366014/diff/16001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_download.h (right):

http://codereview.chromium.org/6366014/diff/16001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_download.h:120: // the query. |query_data| is
the successfull data returned over the wire.
nit: s/successfull/successful /

[GeorgeY, 2011-01-26 00:45:56]

http://codereview.chromium.org/6366014/diff/16001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_download.h (right):

http://codereview.chromium.org/6366014/diff/16001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_download.h:120: // the query. |query_data| is
the successfull data returned over the wire.
On 2011/01/26 00:15:02, dhollowa wrote:
> nit: s/successfull/successful /

Too much doubled letters :). Fixed.