Fail autocheckout if there is no forms in the page.

Fail autocheckout if there is no forms in the page.


BUG=229617
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193588

[benquan, 2013-04-10 18:02:22]

PTAL

[ahutter, 2013-04-10 20:00:59]

https://codereview.chromium.org/13947035/diff/1/chrome/browser/ui/autofill/ta...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/13947035/diff/1/chrome/browser/ui/autofill/ta...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc:101: if
(dialog_controller_)
Is this change necessary? Ideally this method won't be called if there isn't a
|dialog_controller_|.

https://codereview.chromium.org/13947035/diff/1/components/autofill/browser/a...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/13947035/diff/1/components/autofill/browser/a...
components/autofill/browser/autofill_manager.cc:471: // no form in the page.
This give autocheckout_manager a chance to terminate
nit: |autocheckout_manager_|

https://codereview.chromium.org/13947035/diff/1/components/autofill/browser/a...
components/autofill/browser/autofill_manager.cc:472: // autocheckout and send
autocheckout status.
nit: Autocheckout and either SendAutocheckoutStatus or "send Autocheckout
status".

[benquan, 2013-04-10 20:12:59]

https://codereview.chromium.org/13947035/diff/1/chrome/browser/ui/autofill/ta...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/13947035/diff/1/chrome/browser/ui/autofill/ta...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc:101: if
(dialog_controller_)
On 2013/04/10 20:00:59, ahutter wrote:
> Is this change necessary? Ideally this method won't be called if there isn't a
> |dialog_controller_|.

Yes, it is called from AutocheckoutManager::OnLoadedPageMetaData().

|dialog_controller_| is a WeakPtr, we should always null-test before using
WeakPtr according to class docstring.

https://codereview.chromium.org/13947035/diff/1/components/autofill/browser/a...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/13947035/diff/1/components/autofill/browser/a...
components/autofill/browser/autofill_manager.cc:471: // no form in the page.
This give autocheckout_manager a chance to terminate
On 2013/04/10 20:00:59, ahutter wrote:
> nit: |autocheckout_manager_|

Done.

https://codereview.chromium.org/13947035/diff/1/components/autofill/browser/a...
components/autofill/browser/autofill_manager.cc:472: // autocheckout and send
autocheckout status.
On 2013/04/10 20:00:59, ahutter wrote:
> nit: Autocheckout and either SendAutocheckoutStatus or "send Autocheckout
> status".

Done.

[ahutter, 2013-04-10 20:29:01]

https://codereview.chromium.org/13947035/diff/1/chrome/browser/ui/autofill/ta...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/13947035/diff/1/chrome/browser/ui/autofill/ta...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc:101: if
(dialog_controller_)
On 2013/04/10 20:12:59, benquan wrote:
> On 2013/04/10 20:00:59, ahutter wrote:
> > Is this change necessary? Ideally this method won't be called if there isn't
a
> > |dialog_controller_|.
> 
> Yes, it is called from AutocheckoutManager::OnLoadedPageMetaData().
> 
> |dialog_controller_| is a WeakPtr, we should always null-test before using
> WeakPtr according to class docstring. 
> 
> 

If it's called and |dialog_controller_| is NULL there's a bug in our code.  The
reason I wrote this the way I did is explicitly so it _will_ crash if
|dialog_controller_| is NULL.  I see nothing in your changes that make this
check now necessary.  If this check is necessary something got/is messed up in
the logic of OnLoadedPageMetaData.

[Ilya Sherman, 2013-04-10 20:52:18]

https://codereview.chromium.org/13947035/diff/6001/components/autofill/browse...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/13947035/diff/6001/components/autofill/browse...
components/autofill/browser/autofill_manager.cc:487: ParseForms(forms);
What if |forms| is nonempty, but only contains forms that we end up ignoring?

[benquan, 2013-04-10 22:08:15]

https://codereview.chromium.org/13947035/diff/6001/components/autofill/browse...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/13947035/diff/6001/components/autofill/browse...
components/autofill/browser/autofill_manager.cc:487: ParseForms(forms);
On 2013/04/10 20:52:18, Ilya Sherman wrote:
> What if |forms| is nonempty, but only contains forms that we end up ignoring?

Good catch. I moved the logic into |ParseForms|.

[Ilya Sherman, 2013-04-10 22:25:26]

LGTM, but please also wait for Alex's review.

[ahutter, 2013-04-10 22:38:05]

https://codereview.chromium.org/13947035/diff/10001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/13947035/diff/10001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc:101: if
(dialog_controller_)
I still don't think this change should be here.

[benquan, 2013-04-11 02:17:56]

https://codereview.chromium.org/13947035/diff/1/chrome/browser/ui/autofill/ta...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/13947035/diff/1/chrome/browser/ui/autofill/ta...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc:101: if
(dialog_controller_)
When we navigated to a page with different domain, all dialog windows will be
closed, and thus |dialog_controller_| will be NULL. We assume checkout pages
should be within the same domain. We should re-visit this when we do have pages
from different domains. For now, I replace it with a comment.

On 2013/04/10 20:29:01, ahutter wrote:
> On 2013/04/10 20:12:59, benquan wrote:
> > On 2013/04/10 20:00:59, ahutter wrote:
> > > Is this change necessary? Ideally this method won't be called if there
isn't
> a
> > > |dialog_controller_|.
> > 
> > Yes, it is called from AutocheckoutManager::OnLoadedPageMetaData().
> > 
> > |dialog_controller_| is a WeakPtr, we should always null-test before using
> > WeakPtr according to class docstring. 
> > 
> > 
> 
> If it's called and |dialog_controller_| is NULL there's a bug in our code. 
The
> reason I wrote this the way I did is explicitly so it _will_ crash if
> |dialog_controller_| is NULL.  I see nothing in your changes that make this
> check now necessary.  If this check is necessary something got/is messed up in
> the logic of OnLoadedPageMetaData.

https://codereview.chromium.org/13947035/diff/10001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/13947035/diff/10001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc:101: if
(dialog_controller_)
On 2013/04/10 22:38:06, ahutter wrote:
> I still don't think this change should be here.

replaced the null check with a comment

[benquan, 2013-04-11 02:21:26]

Alex gave LGTM in chat. I'm going to submit it and circle back to him when he is
back.

[commit-bot: I haz the power, 2013-04-11 02:22:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benquan@chromium.org/13947035/15001

[commit-bot: I haz the power, 2013-04-11 06:10:26]

Change committed as 193588