Fix DCHECK, memory leak, and refactor PasswordStore to use CancelableRequest

When looking at this bug, I found at least the following issues:

1) PasswordManagerHandler was not canceling requests if new requests were made (by double-clicking the "Manage Saved Passwords..." button with a slow database - simulated with long Sleep).
2) PasswordStore starts its handle numbering at 0 meaning that the very first request is non-true and could hit DCHECK(s).
3) PasswordManagerHandler doesn't free the results.

When talking with the DOMUI TL (jhawkins) and an author of PasswordStore (stuartmorgan), I learned that it was modeled after HistoryService, which had since been refactored into a reusable suite of classes in content/browser/cancelable_request.h

So the CL will include fixes for the 3 issues, as well as a refactor to use the shared code in cancelable_requst.h

BUG=71466
TEST=chrome://settings/personal, then click "Manage Saved Passwords".  Put
PlatformThread::Sleep(10000), then try double-clicking the button to see no error.

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

[tim (not reviewing), 2011-03-16 00:09:57]

Is there an overview / could one be provided as to why this is necessary?  It's
not clear to me from the bug, sorry if I'm missing something.

[Sheridan Rawlins, 2011-03-16 00:20:24]

On 2011/03/16 00:09:57, timsteele wrote:
> Is there an overview / could one be provided as to why this is necessary? 
It's
> not clear to me from the bug, sorry if I'm missing something.

I updated the bug with my findings.  Please have a look.

Thanks,

-SCR

[Sheridan Rawlins, 2011-03-16 21:58:12]

Ok, after wrestling a broken python_26 dir on my windows, and our shared mac
which was out of resources, I was able to manually test on both platforms.  That
plus the passing trybot unittest results, says this is ready for review.

[James Hawkins, 2011-03-16 22:39:34]

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector.h
File base/scoped_vector.h (right):

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector.h#newcode57
base/scoped_vector.h:57: void reset(const std::vector<T*>& vec) {
You're essentially adding operator= by another name. I'm concerned about
overloading reset(). A few options (each with pros/cons):
* Don't use DISALLOW_COPY_AND_ASSIGN, override operator=().
* Rename this to copy().

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector_unittest.cc
File base/scoped_vector_unittest.cc (right):

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:8: using std::vector;
Never use 'using std::'.

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:12: class ConDecLogger {
This class name is unreadable. The class also needs documentation.

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:29: TEST(ScopedVectorTest, ScopedVector) {
s/ScopedVector/<something more descriptive>/

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:53: // TODO write more tests.
Remove the TODO. It's implicit.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_form_manager.cc (left):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_form_manager.cc:42:
CancelLoginsQuery();
How does the PasswordStore know to not notify this object of
OnPasswordStoreRequestDone() after this object has been deleted?

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:55: class GetLoginsRequest
This should probably be defined in the implementation file.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:119: Handle Schedule(
Don't add code to headers.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_default.cc (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store_default.cc:76: 
Remove blank line.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_mac.cc (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store_mac.cc:891: &request->value);
You use |request->value| three times in this method: pull it out into a local
ref.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_x_unittest.cc (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store_x_unittest.cc:40: const
std::vector<webkit_glue::PasswordForm*>&));
You have a 'using webkit_glue::PasswordForm' right above this, so revert this
piece of the change.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/password_manager_handler.h (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/password_manager_handler.h:61: virtual
~ListPopulater() {}
Please keep implementations in the source file.

[stuartmorgan, 2011-03-16 23:25:20]

It would be good to loop in someone who is familiar with CancelableRequest to
review this as well; never having used it before it's hard for me to validate
that aspect of this patch.

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector.h
File base/scoped_vector.h (right):

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector.h#newcode60
base/scoped_vector.h:60: }
I don't feel comfortable being the reviewer for adding new functionality to a
base/ class I'm not involved in. I'd suggest splitting this off into a
completely different CL, but failing that this CL needs another reviewer.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_form_manager.cc:309: const
std::vector<webkit_glue::PasswordForm*>& result) {
What's the purpose of this change? This file has a using directive.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_form_manager.cc:319:
pending_login_query_ = PasswordStore::Handle(0);
0 feels like a magic number. Can we have a constructor for making an invalid
handle instead (assuming that's what this is doing)

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:35:
CancelableRequestConsumerBase* cancelable_consumer() {
Comment?

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:54: // Destructor cleans up
after canceled requests by deleting vector elements.
This doesn't seem like a description of the class...

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:61:
std::vector<webkit_glue::PasswordForm*> >(callback) {}
Fix indentation. Also, I believe having the empty constructor inline violates
our style (and may even be a clang error now).

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:120: BackendFunc func, //
Function to call on PasswordStore's thread.
I've never seen this style; please use the normal Chromium style to document
this method and its arguments.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:122:
scoped_refptr<GetLoginsRequest> request(new GetLoginsRequest(
Why is the implementation inline?

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:131: Handle Schedule(
Same concerns here.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:172: virtual void
ForwardLoginsResult(GetLoginsRequest* request);
Comment

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_mac.cc (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store_mac.cc:891: &request->value);
Since you use this several times, maybe declare an alias for it and use that?

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/password_manager_handler.cc:168:
store->CancelRequest(pending_login_query_);
Why the change from DCHECK to handling gracefully?

[brettw, 2011-03-17 21:40:34]

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:55: class GetLoginsRequest
Why do you do this at all instead of just having a typedef. see
browser/history/history_marshaling.h

[Sheridan Rawlins, 2011-03-18 15:45:48]

I wanted to get some responses out before tackling some of this, so I didn't
respond to every comment.  Please consider & respond to my replies and I'll
proceed with fixes.

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector.h
File base/scoped_vector.h (right):

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector.h#newcode57
base/scoped_vector.h:57: void reset(const std::vector<T*>& vec) {
On 2011/03/16 22:39:35, James Hawkins wrote:
> You're essentially adding operator= by another name. I'm concerned about
> overloading reset(). A few options (each with pros/cons):
> * Don't use DISALLOW_COPY_AND_ASSIGN, override operator=().
> * Rename this to copy().

Ok, discussing alternative as you know on chromium-dev

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector.h#newcode60
base/scoped_vector.h:60: }
On 2011/03/16 23:25:20, stuartmorgan wrote:
> I don't feel comfortable being the reviewer for adding new functionality to a
> base/ class I'm not involved in. I'd suggest splitting this off into a
> completely different CL, but failing that this CL needs another reviewer.

This may be moot if I take alternative.  o/w I'll add another reviewer.

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector_unittest.cc
File base/scoped_vector_unittest.cc (right):

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:8: using std::vector;
On 2011/03/16 22:39:35, James Hawkins wrote:
> Never use 'using std::'.

Done.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_form_manager.cc (left):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_form_manager.cc:42:
CancelLoginsQuery();
On 2011/03/16 22:39:35, James Hawkins wrote:
> How does the PasswordStore know to not notify this object of
> OnPasswordStoreRequestDone() after this object has been deleted?

Good question.  The cancelable_request recommends having a
CancelableRequestConsumer as a member of every consumer object.  Upon
destruction, that class cancels all of the pending requests managed by / pending
a response for it.  The framework has a thread-safe cancel flag and has several
checks for a request being canceled.  The one which answers your question is the
execution of the callback, which happens on the calling (UI) thread and here's a
code snippet:

class CancelableRequest : public CancelableRequestBase {
...
  void ExecuteCallback(const TupleType& param) {
    if (!canceled_.IsSet()) {
        ...
      // Execute the callback.
        ...
    }
  }

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_form_manager.cc:309: const
std::vector<webkit_glue::PasswordForm*>& result) {
On 2011/03/16 23:25:20, stuartmorgan wrote:
> What's the purpose of this change? This file has a using directive.

It was a global replace script that did it.  I'll revert and leave just the int
-> Handle change.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_form_manager.cc:319:
pending_login_query_ = PasswordStore::Handle(0);
On 2011/03/16 23:25:20, stuartmorgan wrote:
> 0 feels like a magic number. Can we have a constructor for making an invalid
> handle instead (assuming that's what this is doing)

Handle is actually just an int.  Would the default constructor be better a la
the following?

    pending_login_query_ = PasswordStore::Handle();

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:55: class GetLoginsRequest
On 2011/03/17 21:40:34, brettw wrote:
> Why do you do this at all instead of just having a typedef. see
> browser/history/history_marshaling.h

If I can change the type of |value| to a ScopedVector, then I can change this
_back_ to a typedef.  At the moment, as per the comment, the destructor is
needed to clean up after canceled requests.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:61:
std::vector<webkit_glue::PasswordForm*> >(callback) {}
On 2011/03/16 23:25:20, stuartmorgan wrote:
> Fix indentation. Also, I believe having the empty constructor inline violates
> our style (and may even be a clang error now).

Good point.  Again, if I change to typedef, this will become a non-issue.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:119: Handle Schedule(
On 2011/03/16 22:39:35, James Hawkins wrote:
> Don't add code to headers.

I referenced the HistoryService files when creating this.  If I can declare the
templated method and define it in the c++ class, I will.  If all the
BackendFunc's have the same signature, I can also do away with the
templatization.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:120: BackendFunc func, //
Function to call on PasswordStore's thread.
On 2011/03/16 23:25:20, stuartmorgan wrote:
> I've never seen this style; please use the normal Chromium style to document
> this method and its arguments.

I believe this also is referenced from HistoryService files.  I'll fix the style
in this file.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:122:
scoped_refptr<GetLoginsRequest> request(new GetLoginsRequest(
On 2011/03/16 23:25:20, stuartmorgan wrote:
> Why is the implementation inline?

Because of templating.  If this is defined in the implementation, then only the
implementation can use it.  I suppose that's fine as long as I ensure that its
private.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:131: Handle Schedule(
On 2011/03/16 23:25:20, stuartmorgan wrote:
> Same concerns here.

Ditto above.  Will look into this.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/password_manager_handler.cc:168:
store->CancelRequest(pending_login_query_);
On 2011/03/16 23:25:20, stuartmorgan wrote:
> Why the change from DCHECK to handling gracefully?

My thought was that Populate should just "make it so" rather than requiring the
client to need to cancel first and then call.  I can change the commenting in
header if necessary - do you agree with this change, with explanation?

[James Hawkins, 2011-03-20 01:17:47]

On 2011/03/18 15:45:48, Sheridan Rawlins wrote:
> I wanted to get some responses out before tackling some of this, so I didn't
> respond to every comment.  Please consider & respond to my replies and I'll
> proceed with fixes.
> 

I don't see a follow-up patch; can you upload it?

[Sheridan Rawlins, 2011-03-20 08:13:11]

Nico, can you eyeball the changes to add ScopedVector::insert(position, first,
last)? It makes it slightly easier to transfer ownership from a vector to a
ScopedVector via

  scoped_vector.insert(scoped_vector.end(), regular_vector.begin(),
regular_vector.end());

I looked at the idea of moving the ScopedVector over to the Request object and
using swap to transfer ownership in the consumers' callbacks.  I think, if still
desired, it would merit a second CL, where this CL can focus on the cancelling
and refactor of PasswordStore to use the CancelableRequest framework.

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector.h
File base/scoped_vector.h (right):

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector.h#newcode57
base/scoped_vector.h:57: void reset(const std::vector<T*>& vec) {
On 2011/03/18 15:45:48, Sheridan Rawlins wrote:
> Ok, discussing alternative as you know on chromium-dev

I'd like to tackle the idea of moving ScopedVector to the PasswordStoreRequest
object and transferring ownership with a swap, in a separate CL.

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector.h#newcode60
base/scoped_vector.h:60: }
On 2011/03/16 23:25:20, stuartmorgan wrote:
> I don't feel comfortable being the reviewer for adding new functionality to a
> base/ class I'm not involved in. I'd suggest splitting this off into a
> completely different CL, but failing that this CL needs another reviewer.

Nico seems to have some commits in base/scoped_vector.h I'll add him.

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector_unittest.cc
File base/scoped_vector_unittest.cc (right):

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:12: class ConDecLogger {
On 2011/03/16 22:39:35, James Hawkins wrote:
> This class name is unreadable. The class also needs documentation.

Done.

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:29: TEST(ScopedVectorTest, ScopedVector) {
On 2011/03/16 22:39:35, James Hawkins wrote:
> s/ScopedVector/<something more descriptive>/

Done.

http://codereview.chromium.org/6646051/diff/12001/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:53: // TODO write more tests.
On 2011/03/16 22:39:35, James Hawkins wrote:
> Remove the TODO. It's implicit.

Done.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:35:
CancelableRequestConsumerBase* cancelable_consumer() {
On 2011/03/16 23:25:20, stuartmorgan wrote:
> Comment?

Done.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:54: // Destructor cleans up
after canceled requests by deleting vector elements.
On 2011/03/16 23:25:20, stuartmorgan wrote:
> This doesn't seem like a description of the class...

Done.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:55: class GetLoginsRequest
On 2011/03/16 22:39:35, James Hawkins wrote:
> This should probably be defined in the implementation file.

Done.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:61:
std::vector<webkit_glue::PasswordForm*> >(callback) {}
On 2011/03/16 23:25:20, stuartmorgan wrote:
> Fix indentation. Also, I believe having the empty constructor inline violates
> our style (and may even be a clang error now).

Done.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:119: Handle Schedule(
On 2011/03/16 22:39:35, James Hawkins wrote:
> Don't add code to headers.

Done.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:122:
scoped_refptr<GetLoginsRequest> request(new GetLoginsRequest(
On 2011/03/18 15:45:48, Sheridan Rawlins wrote:
> On 2011/03/16 23:25:20, stuartmorgan wrote:
> > Why is the implementation inline?
> 
> Because of templating.  If this is defined in the implementation, then only
the
> implementation can use it.  I suppose that's fine as long as I ensure that its
> private.

Done.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:131: Handle Schedule(
On 2011/03/18 15:45:48, Sheridan Rawlins wrote:
> On 2011/03/16 23:25:20, stuartmorgan wrote:
> > Same concerns here.
> 
> Ditto above.  Will look into this.

Done.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:172: virtual void
ForwardLoginsResult(GetLoginsRequest* request);
On 2011/03/16 23:25:20, stuartmorgan wrote:
> Comment

Done.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_mac.cc (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store_mac.cc:891: &request->value);
On 2011/03/16 23:25:20, stuartmorgan wrote:
> Since you use this several times, maybe declare an alias for it and use that?

Done.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_x_unittest.cc (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store_x_unittest.cc:40: const
std::vector<webkit_glue::PasswordForm*>&));
On 2011/03/16 22:39:35, James Hawkins wrote:
> You have a 'using webkit_glue::PasswordForm' right above this, so revert this
> piece of the change.

Done.

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/password_manager_handler.h (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/password_manager_handler.h:61: virtual
~ListPopulater() {}
On 2011/03/16 22:39:35, James Hawkins wrote:
> Please keep implementations in the source file.

Done.

[James Hawkins, 2011-03-21 01:45:05]

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest.cc
File base/scoped_vector_unittest.cc (right):

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:12: // only really need one observer, but use the
standard ObserverList pattern.
Why?

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:16: public:
protected:
 virtual ~Observer() {}

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:44: // destoyed, and further wish to test
expectations against the state of that
s/destoyed/destroyed/

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:46: // pairing of the watcher which observes the
object, which notifies of
s/object, which/object then/ ?

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:75: virtual void OnLifeCycleDestroy
(LifeCycleObject* object) {
Remove space before opening parenthesis.

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:92: LifeCycleObject*
constructed_life_cycle_object_;
scoped_ptr

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:97: }  // namespace
Wrap the rest of the file in the anonymous namespace.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_form_manager.cc:319:
pending_login_query_ = PasswordStore::Handle(0);
You're ignoring the fact that PasswordStore::Handle is opaque and implying that
the value '0' means something.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.cc (right):

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.cc:66: void
PasswordStore::ForwardLoginsResult(GetLoginsRequest* request) {
Method ordering: ForwardLoginsResult() implementation should be  after
ScheduleTask() implementation.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.cc:111:
PasswordStore::GetLoginsRequest::GetLoginsRequest(GetLoginsCallback* callback)
The ordering of the methods in the source file should match the ordering in the
header.  These two methods should be placed above the PasswordStore constructor
implementation.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.h (left):

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:18: class Profile;
Nice catch.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:27: // This abstract base class
for consumers of PasswordStore calls that need
nit: This is quite a long sentence. Consider breaking it up for clarity.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:31: class PasswordStoreConsumer
{
Move this class to its own header.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:33: virtual
~PasswordStoreConsumer() {}
Make the destructor protected.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:63: // PasswordForm vector
elements are meant to be owned (i.e. deleted) by the
nit: You don't need to clarify this: "i.e. deleted".

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:64: // PasswordStoreConsumer. 
However, if the request is canceled after the
nit: Single space between sentences.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:112: // request can be tracked.
Implement the Consumer interface to be
Why have you made this ambiguous? Isn't it actually the PasswordStoreConsumer?

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:178: // Dispatches the result
to the PasswordStoreConsumer on the correct thread so
Which thread is the correct thread?

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/password_manager_handler.cc:156:
PasswordManagerHandler::ListPopulater::~ListPopulater() {
Source ordering: ~ListPopulater() implementation should go after the
ListPopulater() implementation.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/password_manager_handler.cc:161:
pending_login_query_(0) {
|pending_login_query_| should be a PasswordStore::Handle, and that should be
opaque, so we're going to need another way to initialize this.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/password_manager_handler.cc:171: if
(pending_login_query_)
Isn't PasswordStore::Handle opaque?  We're assuming an unset Handle is the value
0/false.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/password_manager_handler.h (right):

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/password_manager_handler.h:75: int
pending_login_query_;
s/int/PasswordStore::Handle/

[Sheridan Rawlins, 2011-03-21 05:20:17]

Fixed most comments, save moving PasswordStoreConsumer to its own file.  I will
need to think about where Handle should be defined, since it's currently in the
base class of PasswordStore, but needed by PasswordStoreConsumer's callback, as
well as by implementations wishing to honor the intended opacity of the Handle
type by using kInvalidHandle and handle_is_valid()...

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/6646051/diff/12001/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:61:
std::vector<webkit_glue::PasswordForm*> >(callback) {}
On 2011/03/16 23:25:20, stuartmorgan wrote:
> Fix indentation. Also, I believe having the empty constructor inline violates
> our style (and may even be a clang error now).

I noticed the following addition was merged in recently:
    virtual ~Observer() {}

Are empty destructors allowed? 
http://www.chromium.org/developers/coding-style#TOC-Inline-functions would seem
to indicate otherwise...

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest.cc
File base/scoped_vector_unittest.cc (right):

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:12: // only really need one observer, but use the
standard ObserverList pattern.
On 2011/03/21 01:45:06, James Hawkins wrote:
> Why?

I was exploring use of ObserverList and thought it made sense, but I suppose it
does seem like overkill.  I'll simplify with one observer_.

Done.

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:16: public:
On 2011/03/21 01:45:06, James Hawkins wrote:
> protected:
>  virtual ~Observer() {}

You know, I was going to do that, but I saw another interface which didn't have
a destructor...  Good lesson/confirmation - will always have virtual destructors
as private as possible.

Done.

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:44: // destoyed, and further wish to test
expectations against the state of that
On 2011/03/21 01:45:06, James Hawkins wrote:
> s/destoyed/destroyed/

Done.

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:46: // pairing of the watcher which observes the
object, which notifies of
On 2011/03/21 01:45:06, James Hawkins wrote:
> s/object, which/object then/ ?

Done.

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:75: virtual void OnLifeCycleDestroy
(LifeCycleObject* object) {
On 2011/03/21 01:45:06, James Hawkins wrote:
> Remove space before opening parenthesis.

Done.

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:92: LifeCycleObject*
constructed_life_cycle_object_;
On 2011/03/21 01:45:06, James Hawkins wrote:
> scoped_ptr

Done.

http://codereview.chromium.org/6646051/diff/14002/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:97: }  // namespace
On 2011/03/21 01:45:06, James Hawkins wrote:
> Wrap the rest of the file in the anonymous namespace.

Man, I really copy/pasted some mediocrity from scoped_ptr.h

Done.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_form_manager.cc:319:
pending_login_query_ = PasswordStore::Handle(0);
On 2011/03/21 01:45:06, James Hawkins wrote:
> You're ignoring the fact that PasswordStore::Handle is opaque and implying
that
> the value '0' means something.

Stuart said something similar before.  I think this solution is better - I
provided a handle_is_valid static method for checking and a kInvalidHandle
constant for initialization.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.cc (right):

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.cc:66: void
PasswordStore::ForwardLoginsResult(GetLoginsRequest* request) {
On 2011/03/21 01:45:06, James Hawkins wrote:
> Method ordering: ForwardLoginsResult() implementation should be  after
> ScheduleTask() implementation.

Done.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.cc:111:
PasswordStore::GetLoginsRequest::GetLoginsRequest(GetLoginsCallback* callback)
On 2011/03/21 01:45:06, James Hawkins wrote:
> The ordering of the methods in the source file should match the ordering in
the
> header.  These two methods should be placed above the PasswordStore
constructor
> implementation.

Done.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:27: // This abstract base class
for consumers of PasswordStore calls that need
On 2011/03/21 01:45:06, James Hawkins wrote:
> nit: This is quite a long sentence. Consider breaking it up for clarity.

Done.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:31: class PasswordStoreConsumer
{
On 2011/03/21 01:45:06, James Hawkins wrote:
> Move this class to its own header.

This is fairly tricky, due to PasswordStore providing PasswordStore::Handle.  As
I think about it, I'm wondering if the validity check should move into
CancelableRequestProvider::handle_is_valid (since the provider has the Handle
typedef anyway...  Could also fix its int next_handle_ to be Handle
next_handle_...

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:33: virtual
~PasswordStoreConsumer() {}
On 2011/03/21 01:45:06, James Hawkins wrote:
> Make the destructor protected.

Done.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:63: // PasswordForm vector
elements are meant to be owned (i.e. deleted) by the
On 2011/03/21 01:45:06, James Hawkins wrote:
> nit: You don't need to clarify this: "i.e. deleted".

Done.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:64: // PasswordStoreConsumer. 
However, if the request is canceled after the
On 2011/03/21 01:45:06, James Hawkins wrote:
> nit: Single space between sentences.

Done.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:112: // request can be tracked.
Implement the Consumer interface to be
On 2011/03/21 01:45:06, James Hawkins wrote:
> Why have you made this ambiguous? Isn't it actually the PasswordStoreConsumer?

Nice catch - that was a search/replace artifact - I had this as
PasswordStore::Consumer at one point, but ended up preferring
PasswordStoreConsumer, and this comment stuck.  No semantic changes were
intended.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:178: // Dispatches the result
to the PasswordStoreConsumer on the correct thread so
On 2011/03/21 01:45:06, James Hawkins wrote:
> Which thread is the correct thread?

The original caller's thread (UI, in our case).

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/password_manager_handler.cc:156:
PasswordManagerHandler::ListPopulater::~ListPopulater() {
On 2011/03/21 01:45:06, James Hawkins wrote:
> Source ordering: ~ListPopulater() implementation should go after the
> ListPopulater() implementation.

Done.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/password_manager_handler.cc:161:
pending_login_query_(0) {
On 2011/03/21 01:45:06, James Hawkins wrote:
> |pending_login_query_| should be a PasswordStore::Handle, and that should be
> opaque, so we're going to need another way to initialize this.

Done.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/password_manager_handler.cc:171: if
(pending_login_query_)
On 2011/03/21 01:45:06, James Hawkins wrote:
> Isn't PasswordStore::Handle opaque?  We're assuming an unset Handle is the
value
> 0/false.

Done.

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/password_manager_handler.h (right):

http://codereview.chromium.org/6646051/diff/14002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/password_manager_handler.h:75: int
pending_login_query_;
On 2011/03/21 01:45:06, James Hawkins wrote:
> s/int/PasswordStore::Handle/

Done.

[James Hawkins, 2011-03-21 17:53:11]

This is looking much better.

http://codereview.chromium.org/6646051/diff/20003/base/scoped_vector_unittest.cc
File base/scoped_vector_unittest.cc (right):

http://codereview.chromium.org/6646051/diff/20003/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:18: protected:
nit: Blank line between sections for readability.

http://codereview.chromium.org/6646051/diff/20003/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:25: }
nit: Blank line between methods.

http://codereview.chromium.org/6646051/diff/20003/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.cc (right):

http://codereview.chromium.org/6646051/diff/20003/chrome/browser/password_man...
chrome/browser/password_manager/password_store.cc:16: const
PasswordStore::Handle PasswordStore::kInvalidHandle = 0;
Isn't Handle defined by CancelableRequest?  It seems strange that we're making
assumptions about the validity of a handle in a derived class. This is fragile.

http://codereview.chromium.org/6646051/diff/20003/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/6646051/diff/20003/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:62: static const Handle
kInvalidHandle;
nit: Document this constant.

http://codereview.chromium.org/6646051/diff/20003/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:62: static const Handle
kInvalidHandle;
I'm not going to ask you to change this because it's extremely unlikely, but
consider the scenario where Handle changes to some object with state in the
future. You'd have to change all instances of kInvalidHandle to a Factory
method. If you create the factory method in the beginning, you won't have to
change the call sites for this case.

http://codereview.chromium.org/6646051/diff/20003/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:117: // request can be tracked.
Implement the Consumer interface to be
s/Consumer/PasswordStoreConsumer/

http://codereview.chromium.org/6646051/diff/20003/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:143: static bool
handle_is_valid(Handle handle) {
This is not an accessor method; as such, the method should be CamelCase and
defined in the implementation file.

[Sheridan Rawlins, 2011-03-21 18:36:41]

How's this - I moved Handle methods to CancelableRequestProvider (where Handle's
type is defined) and made factory + validating methods camel-case.

http://codereview.chromium.org/6646051/diff/20003/base/scoped_vector_unittest.cc
File base/scoped_vector_unittest.cc (right):

http://codereview.chromium.org/6646051/diff/20003/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:18: protected:
On 2011/03/21 17:53:11, James Hawkins wrote:
> nit: Blank line between sections for readability.

Done.

http://codereview.chromium.org/6646051/diff/20003/base/scoped_vector_unittest...
base/scoped_vector_unittest.cc:25: }
On 2011/03/21 17:53:11, James Hawkins wrote:
> nit: Blank line between methods.

Done.

http://codereview.chromium.org/6646051/diff/20003/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.cc (right):

http://codereview.chromium.org/6646051/diff/20003/chrome/browser/password_man...
chrome/browser/password_manager/password_store.cc:16: const
PasswordStore::Handle PasswordStore::kInvalidHandle = 0;
On 2011/03/21 17:53:11, James Hawkins wrote:
> Isn't Handle defined by CancelableRequest?  It seems strange that we're making
> assumptions about the validity of a handle in a derived class. This is
fragile.

Yep, was thinking the same thing, but too tired to implement last night.  I
moved the constants, validity check, and InvalidHandle factory method to
CancelableRequest.

http://codereview.chromium.org/6646051/diff/20003/chrome/browser/password_man...
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/6646051/diff/20003/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:62: static const Handle
kInvalidHandle;
On 2011/03/21 17:53:11, James Hawkins wrote:
> I'm not going to ask you to change this because it's extremely unlikely, but
> consider the scenario where Handle changes to some object with state in the
> future. You'd have to change all instances of kInvalidHandle to a Factory
> method. If you create the factory method in the beginning, you won't have to
> change the call sites for this case.

I created a factory method in CancelableRequestProvider.

Does the constant still need documentation if it's protected and there's a
factory method?

http://codereview.chromium.org/6646051/diff/20003/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:117: // request can be tracked.
Implement the Consumer interface to be
On 2011/03/21 17:53:11, James Hawkins wrote:
> s/Consumer/PasswordStoreConsumer/

Done.

http://codereview.chromium.org/6646051/diff/20003/chrome/browser/password_man...
chrome/browser/password_manager/password_store.h:143: static bool
handle_is_valid(Handle handle) {
On 2011/03/21 17:53:11, James Hawkins wrote:
> This is not an accessor method; as such, the method should be CamelCase and
> defined in the implementation file.

Done.

[Sheridan Rawlins, 2011-03-21 22:12:30]

Nico's on vacation - Brett would you mind reviewing the newly-added
ScopedVector::insert?

Jam, can you review the additions to CancelableRequestProvider to make the
"Handle" typedef act more opaque?  (i.e. InvalidHandle, HandleIsValid)?

[brettw, 2011-03-22 21:43:41]

I'm fine with the ScopedVector changes.

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
File content/browser/cancelable_request.cc (right):

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
content/browser/cancelable_request.cc:13: :
next_handle_(CancelableRequestProvider::kFirstHandle) {
Indent 2 more spaces.

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
File content/browser/cancelable_request.h (right):

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
content/browser/cancelable_request.h:134: static bool HandleIsValid(Handle
handle);
Why not just have calling code compare to InvalidHandle() themselves? This
function is potentially ambiguous since it could be construed to mean that there
is actually a request for the given handle, which isn't necessarily the case for
you.

[Sheridan Rawlins, 2011-03-22 22:04:08]

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
File content/browser/cancelable_request.cc (right):

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
content/browser/cancelable_request.cc:13: :
next_handle_(CancelableRequestProvider::kFirstHandle) {
On 2011/03/22 21:43:41, brettw wrote:
> Indent 2 more spaces.

Done.

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
File content/browser/cancelable_request.h (right):

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
content/browser/cancelable_request.h:134: static bool HandleIsValid(Handle
handle);
On 2011/03/22 21:43:41, brettw wrote:
> Why not just have calling code compare to InvalidHandle() themselves? This
> function is potentially ambiguous since it could be construed to mean that
there
> is actually a request for the given handle, which isn't necessarily the case
for
> you.

One could imagine Handles which are not equal to themselves being invalid.  That
said, in this case, only the 0 handle is invalid.  Can you recommend a better
name to be less ambiguous?

[brettw, 2011-03-22 22:11:08]

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
File content/browser/cancelable_request.h (right):

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
content/browser/cancelable_request.h:134: static bool HandleIsValid(Handle
handle);
I'm not sure what you're saying here.

[stuartmorgan, 2011-03-22 22:50:03]

LGTM

Good luck on not awakening PasswordStore's infamous Curse of the Reliability
Bot!

[James Hawkins, 2011-03-22 22:59:48]

http://codereview.chromium.org/6646051/diff/30001/content/browser/cancelable_...
File content/browser/cancelable_request.h (right):

http://codereview.chromium.org/6646051/diff/30001/content/browser/cancelable_...
content/browser/cancelable_request.h:137: static const Handle kFirstHandle;
Can these be private?

[Sheridan Rawlins, 2011-03-23 00:39:52]

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
File content/browser/cancelable_request.h (right):

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
content/browser/cancelable_request.h:134: static bool HandleIsValid(Handle
handle);
On 2011/03/22 22:11:08, brettw wrote:
> I'm not sure what you're saying here.

I'm saying maybe there are multiple Invalid Handles -1, 0, or maybe Handle
becomes something other than an int.  In that case, InvalidHandle was meant to
be a factory or singleton at the discretion of the implementor of
CancelableRequestProvider (not called NewInvalidHandle since it doesn't allocate
something in need of freeing).  HandleIsValid is meant to check the validity of
the handle and return true if the handle is valid.

I see your point, however, inasmuch as this doesn't check if there's a request
outstanding, only that the handle is useable...  Maybe HandleIsInitialized()
would be a better name?  I can change the name in a followup CL.

http://codereview.chromium.org/6646051/diff/30001/content/browser/cancelable_...
content/browser/cancelable_request.h:137: static const Handle kFirstHandle;
On 2011/03/22 22:59:49, James Hawkins wrote:
> Can these be private?

Done.

[brettw, 2011-03-23 01:49:00]

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
File content/browser/cancelable_request.h (right):

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
content/browser/cancelable_request.h:134: static bool HandleIsValid(Handle
handle);
On 2011/03/23 00:39:52, Sheridan Rawlins wrote:
> On 2011/03/22 22:11:08, brettw wrote:
> > I'm not sure what you're saying here.
> 
> I'm saying maybe there are multiple Invalid Handles -1, 0, or maybe Handle
> becomes something other than an int.  In that case, InvalidHandle was meant to
> be a factory or singleton at the discretion of the implementor of
> CancelableRequestProvider (not called NewInvalidHandle since it doesn't
allocate
> something in need of freeing).  HandleIsValid is meant to check the validity
of
> the handle and return true if the handle is valid.

Having complicated "multiple invalid handles" seems like a silly design and we
shouldn't design this based on the fact that we may want to change this
many-year-old code in that way.

I think 0 was a fine invalid handle, and I don't even see the point of having
constants or functions. The callers should just do
  if (!handle)
if they want to check handle validity. This is well-defined and obvious when
reading the calling code, and it's worked fine for all the other consumers of
this.

[Sheridan Rawlins, 2011-03-23 03:16:50]

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
File content/browser/cancelable_request.h (right):

http://codereview.chromium.org/6646051/diff/22030/content/browser/cancelable_...
content/browser/cancelable_request.h:134: static bool HandleIsValid(Handle
handle);
On 2011/03/23 01:49:00, brettw wrote:
> 
> Having complicated "multiple invalid handles" seems like a silly design and we
> shouldn't design this based on the fact that we may want to change this
> many-year-old code in that way.
> 
> I think 0 was a fine invalid handle, and I don't even see the point of having
> constants or functions. The callers should just do
>   if (!handle)
> if they want to check handle validity. This is well-defined and obvious when
> reading the calling code, and it's worked fine for all the other consumers of
> this.

I think you're describing Speculative Generality
(http://sourcemaking.com/refactoring/speculative-generality).

As an extension of your reasoning, and to help me understand the code, why do we
have/need the typedef int Handle?

I didn't realize this was many-year-old code.  I understood that PasswordStore
was modeled after HistoryService, which at the time wasn't factored into the
general cancelable_request framework.  FWIW, before I rewrote PasswordStore to
use CancelableRequestProvider, it had a bug, where the numbering for
PasswordStore started at 0 (not 1 like CancelableRequestProvider), and the first
client doing if (handle) could take the wrong action...

Because of the typedef for Handle, the start @ 0 bug I found in PasswordStore,
and the comments of my reviewers, I strove to make Handle more opaque.

So, what would LGTY?  Would ditching the methods/constants, and commenting the
initialize to 0 and boolean test for validity suffice?  Should we also ditch the
typedef and have all clients use int?

[Sheridan Rawlins, 2011-03-25 20:12:56]

FYI, http://codereview.chromium.org/6665029/ shows a recent review of some code
which has typedef int Handle and promotes opaque usage with handle_is_valid.

Can I get a LG on the code as-is (in this CL)?

[brettw, 2011-03-28 16:17:09]

I think I prefer the 0. Introducing the checking code makes the existing code
that uses 0 for the undefined handle a bit confusing. With some code using your
new opaque functions and some code using 0, is the code using 0 wrong? I don't
really want to have that ambiguity.

It is a bit weird that it's not documented anywhere that 0 is the special "I
failed" handle. Can you find a good place or places to add that in
cancelable_request so somebody in your shoes in the future won't get confused?

I don't think this is the same case as the TransportDIB. The TransportDIB's
handle is actually different for different platforms. On Windows it's a HANDLE,
on Mac it's a shared memory handle, and on Linux it's a SysV shared memory
handle. The types are not compatible and we don't want to hardcode assumptions
about platform behavior in every place.

[Sheridan Rawlins, 2011-03-28 17:32:03]

Filed http://crbug.com/77650 to address the opacity and making it used by all
clients.

Rolled back CancelableRequestProvider::Handle opacity methods and their use by
clients.

[brettw, 2011-03-28 20:27:24]

LGTM (I only checked the other stuff briefly)