Reduce visibility of methods in AutofillManager and AutofillDownload.

Reduce visibility of methods in AutofillManager and AutofillDownload.

Also remove some obsolete code and simplify some of the code's expectations.

BUG=none
TEST=none


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

[dhollowa, 2011-10-26 15:58:32]

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

http://codereview.chromium.org/8351027/diff/6001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.cc:51: DCHECK(observer_);
Should have a DCHECK(profile_) here if you're eliminating them below.

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

http://codereview.chromium.org/8351027/diff/6001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.h:130: Profile* const profile_;  //
WEAK
It would be helpful to add a comment that profile_ may not be NULL.  I like the
const pointer thing... but maybe a note as well to indicate intent: ie. can only
be set in ctor.

http://codereview.chromium.org/8351027/diff/6001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.h:132:
AutofillDownloadManager::Observer* const observer_;  // WEAK
It would be helpful to add a comment that observer_ may not be NULL.

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

http://codereview.chromium.org/8351027/diff/6001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download_unittest.cc:115: enum TYPE_OF_RESPONSE
{
Please change this to ResponseType (or something).  ALL CAPs hurts.

http://codereview.chromium.org/8351027/diff/6001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/8351027/diff/6001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_manager.h:97: const
ViewHostMsg_FrameNavigate_Params& params);
OVERRIDE with these virtual methods.

[Ilya Sherman, 2011-10-26 23:06:32]

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

http://codereview.chromium.org/8351027/diff/6001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.cc:51: DCHECK(observer_);
On 2011/10/26 15:58:33, dhollowa wrote:
> Should have a DCHECK(profile_) here if you're eliminating them below.

|profile_| is used on line 52, so the DCHECK would be redundant.

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

http://codereview.chromium.org/8351027/diff/6001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.h:130: Profile* const profile_;  //
WEAK
On 2011/10/26 15:58:33, dhollowa wrote:
> It would be helpful to add a comment that profile_ may not be NULL.  I like
the
> const pointer thing... but maybe a note as well to indicate intent: ie. can
only
> be set in ctor.

Done.

http://codereview.chromium.org/8351027/diff/6001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.h:132:
AutofillDownloadManager::Observer* const observer_;  // WEAK
On 2011/10/26 15:58:33, dhollowa wrote:
> It would be helpful to add a comment that observer_ may not be NULL.

Done.

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

http://codereview.chromium.org/8351027/diff/6001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download_unittest.cc:115: enum TYPE_OF_RESPONSE
{
On 2011/10/26 15:58:33, dhollowa wrote:
> Please change this to ResponseType (or something).  ALL CAPs hurts.

Done.

http://codereview.chromium.org/8351027/diff/6001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/8351027/diff/6001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_manager.h:97: const
ViewHostMsg_FrameNavigate_Params& params);
On 2011/10/26 15:58:33, dhollowa wrote:
> OVERRIDE with these virtual methods.

Done.

[dhollowa, 2011-10-27 23:37:34]

LGTM.

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

http://codereview.chromium.org/8351027/diff/6001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.cc:51: DCHECK(observer_);
Ok, that's fine.  But if PrefService has no virtuals then it may not crash. 
I.e. in C++ it is possible to do: (Foo *)(NULL)->DoIt(), and DoIt() will execute
fine.

On 2011/10/26 23:06:32, Ilya Sherman wrote:
> On 2011/10/26 15:58:33, dhollowa wrote:
> > Should have a DCHECK(profile_) here if you're eliminating them below.
> 
> |profile_| is used on line 52, so the DCHECK would be redundant.

[commit-bot: I haz the power, 2011-10-28 00:08:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/8351027/9001

[commit-bot: I haz the power, 2011-10-28 01:39:48]

Try job failure for 8351027-9001 (retry) (retry) (retry) on linux_rel for step
"ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-10-28 01:57:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/8351027/9001

[commit-bot: I haz the power, 2011-10-28 02:55:21]

Change committed as 107680