rAc: don't open sign in continue url in new tab

rAc: don't open sign in continue url in new tab

BUG=326882, 331127
TEST=rac sign in works

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

[Evan Stade, 2014-01-02 19:28:22]

not immediately obvious how to write an automated test for this.

[Dan Beam, 2014-01-02 19:33:18]

On 2014/01/02 19:28:22, Evan Stade wrote:
> not immediately obvious how to write an automated test for this.

i'll just leave this here to start...

diff --git
a/components/autofill/content/browser/wallet/wallet_service_url_unittest.cc
b/components/autofill/content/browser/wallet/wallet_service_ur
index 4825263..bf5d2f6 100644
--- a/components/autofill/content/browser/wallet/wallet_service_url_unittest.cc
+++ b/components/autofill/content/browser/wallet/wallet_service_url_unittest.cc
@@ -129,6 +129,7 @@ TEST(WalletServiceUrl, IsSignInContinueUrl) {
 
 TEST(WalletServiceUrl, IsSignInRelatedUrl) {
   EXPECT_TRUE(IsSignInRelatedUrl(GetSignInUrl()));
+  EXPECT_TRUE(IsSignInRelatedUrl(GetSignInContinueUrl()));
   EXPECT_FALSE(IsSignInRelatedUrl(GURL("http://google.com")));
 }

[Evan Stade, 2014-01-02 19:37:42]

On 2014/01/02 19:33:18, Dan Beam wrote:
> On 2014/01/02 19:28:22, Evan Stade wrote:
> > not immediately obvious how to write an automated test for this.
> 
> i'll just leave this here to start...
> 
> diff --git
> a/components/autofill/content/browser/wallet/wallet_service_url_unittest.cc
> b/components/autofill/content/browser/wallet/wallet_service_ur
> index 4825263..bf5d2f6 100644
> ---
a/components/autofill/content/browser/wallet/wallet_service_url_unittest.cc
> +++
b/components/autofill/content/browser/wallet/wallet_service_url_unittest.cc
> @@ -129,6 +129,7 @@ TEST(WalletServiceUrl, IsSignInContinueUrl) {
>  
>  TEST(WalletServiceUrl, IsSignInRelatedUrl) {
>    EXPECT_TRUE(IsSignInRelatedUrl(GetSignInUrl()));
> +  EXPECT_TRUE(IsSignInRelatedUrl(GetSignInContinueUrl()));
>    EXPECT_FALSE(IsSignInRelatedUrl(GURL("http://google.com")));
>  }

That's a pretty weak test, nearly worthless as it tests implementation more than
behavior.

[Dan Beam, 2014-01-02 19:39:48]

On 2014/01/02 19:37:42, Evan Stade wrote:
> On 2014/01/02 19:33:18, Dan Beam wrote:
> > On 2014/01/02 19:28:22, Evan Stade wrote:
> > > not immediately obvious how to write an automated test for this.
> > 
> > i'll just leave this here to start...
> > 
> > diff --git
> > a/components/autofill/content/browser/wallet/wallet_service_url_unittest.cc
> > b/components/autofill/content/browser/wallet/wallet_service_ur
> > index 4825263..bf5d2f6 100644
> > ---
> a/components/autofill/content/browser/wallet/wallet_service_url_unittest.cc
> > +++
> b/components/autofill/content/browser/wallet/wallet_service_url_unittest.cc
> > @@ -129,6 +129,7 @@ TEST(WalletServiceUrl, IsSignInContinueUrl) {
> >  
> >  TEST(WalletServiceUrl, IsSignInRelatedUrl) {
> >    EXPECT_TRUE(IsSignInRelatedUrl(GetSignInUrl()));
> > +  EXPECT_TRUE(IsSignInRelatedUrl(GetSignInContinueUrl()));
> >    EXPECT_FALSE(IsSignInRelatedUrl(GURL("http://google.com")));
> >  }
> 
> That's a pretty weak test, nearly worthless as it tests implementation more
than
> behavior.

then navigate the webcontents in a browser test and ensure no popups were
blocked or the dialog's in the right state (i.e. the signinwebview isn't showing
or something)

[Evan Stade, 2014-01-02 20:49:27]

tests improved

[Dan Beam, 2014-01-02 21:43:36]

lgtm

https://codereview.chromium.org/123363002/diff/70001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://codereview.chromium.org/123363002/diff/70001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1099:
content::PAGE_TRANSITION_LINK,
^ keep this as FORM_SUBMIT

https://codereview.chromium.org/123363002/diff/70001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1161:
content::PAGE_TRANSITION_LINK,
^ same

https://codereview.chromium.org/123363002/diff/70001/components/autofill/cont...
File components/autofill/content/browser/wallet/wallet_service_url.cc (right):

https://codereview.chromium.org/123363002/diff/70001/components/autofill/cont...
components/autofill/content/browser/wallet/wallet_service_url.cc:34: bool
g_use_testing_sign_in_continue_url = false;
^ this isn't really global if it's only available in this file, right?

[Dan Beam, 2014-01-02 22:05:09]

not lgtm

if we can't trust the page transition we should remove it from the
AutofillSignInDelegate code (and only go by URL) or figure out why it's
unexpectedly being changed to PAGE_TRANSITION_LINK.

[Evan Stade, 2014-01-02 22:07:20]

OK, it seems like should have put Rachel as the reviewer. Rachel can you review
this instead of Dan?

[groby-ooo-7-16, 2014-01-03 00:10:22]

Overall, seems to be doing the right thing - assuming that "the right thing" is
ensuring in-dialog transitions for the additional URL.

https://chromiumcodereview.appspot.com/123363002/diff/270001/chrome/browser/u...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/123363002/diff/270001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1065:
wallet::SetUseTestingSignInContinueUrl();
Since this is a browser test, I'd prefer it if we used a FakeURLFetcherFactory.
It's a bit more code, but it keeps us from creating global state for testing.

https://chromiumcodereview.appspot.com/123363002/diff/270001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1099:
content::PAGE_TRANSITION_LINK,
Question - why the move to calling OpenURLFromTab instead of LoadURL?

https://chromiumcodereview.appspot.com/123363002/diff/270001/components/autof...
File components/autofill/content/browser/wallet/wallet_service_url.cc (right):

https://chromiumcodereview.appspot.com/123363002/diff/270001/components/autof...
components/autofill/content/browser/wallet/wallet_service_url.cc:195:
IsSignInContinueUrl(url, &unused);
I assume we added this additional test because of accounts.youtube.com? Would it
make more sense to allow that as a second GAIA origin, instead of limiting it to
the passive auth URL?

[Evan Stade, 2014-01-03 00:14:57]

https://chromiumcodereview.appspot.com/123363002/diff/270001/chrome/browser/u...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/123363002/diff/270001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1099:
content::PAGE_TRANSITION_LINK,
On 2014/01/03 00:10:22, groby wrote:
> Question - why the move to calling OpenURLFromTab instead of LoadURL?

because I'm trying to test AutofillDialogSignInDelegate::OpenURLFromTab(). That
this test didn't cover that function before is the reason this test didn't catch
the regression.

https://chromiumcodereview.appspot.com/123363002/diff/270001/components/autof...
File components/autofill/content/browser/wallet/wallet_service_url.cc (right):

https://chromiumcodereview.appspot.com/123363002/diff/270001/components/autof...
components/autofill/content/browser/wallet/wallet_service_url.cc:195:
IsSignInContinueUrl(url, &unused);
On 2014/01/03 00:10:22, groby wrote:
> I assume we added this additional test because of accounts.youtube.com? Would
it
> make more sense to allow that as a second GAIA origin, instead of limiting it
to
> the passive auth URL?

Not sure where you're getting the youtube idea from. The sign in flow forwards
the user to the sign in continue url when it's done. That's why this is here.

[Evan Stade, 2014-01-03 01:47:44]

https://codereview.chromium.org/123363002/diff/270001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://codereview.chromium.org/123363002/diff/270001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1065:
wallet::SetUseTestingSignInContinueUrl();
On 2014/01/03 00:10:22, groby wrote:
> Since this is a browser test, I'd prefer it if we used a
FakeURLFetcherFactory.
> It's a bit more code, but it keeps us from creating global state for testing.

I added a FakeURLFetcherFactory (see second to last patchset), but then noticed
it doesn't appear to even be needed.

https://codereview.chromium.org/123363002/diff/450001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_sign_in_delegate.cc (left):

https://codereview.chromium.org/123363002/diff/450001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_sign_in_delegate.cc:24: return
params.transition == content::PAGE_TRANSITION_LINK &&
new change

https://codereview.chromium.org/123363002/diff/450001/components/autofill/con...
File components/autofill/content/browser/wallet/wallet_service_url.cc (right):

https://codereview.chromium.org/123363002/diff/450001/components/autofill/con...
components/autofill/content/browser/wallet/wallet_service_url.cc:187:
StartsWith(base::UTF8ToUTF16(url.GetOrigin().spec()),
new change

[groby-ooo-7-16, 2014-01-03 01:55:56]

Question: Without the fake factory, doesn't the code go and do actual network
requests? Should we use the fake factory so we can make the tests a bit more
resilient?

Otherwise, LGTM.

[groby-ooo-7-16, 2014-01-03 18:09:03]

Sigh. Still goes to a new page.

[groby-ooo-7-16, 2014-01-03 18:09:15]

(So, obviously, not LGTM)

[groby-ooo-7-16, 2014-01-03 18:54:49]

https://codereview.chromium.org/123363002/diff/450001/components/autofill/con...
File components/autofill/content/browser/wallet/wallet_service_url.cc (right):

https://codereview.chromium.org/123363002/diff/450001/components/autofill/con...
components/autofill/content/browser/wallet/wallet_service_url.cc:187:
StartsWith(base::UTF8ToUTF16(url.GetOrigin().spec()),
Ah. Here's the bug - you want to use Host(), not spec(). We need to compare the
hostname.

We should probably add a unit test to that effect, too.

[Evan Stade, 2014-01-03 23:08:11]

updated with a test, thanks for pointing out the bug.

[groby-ooo-7-16, 2014-01-04 02:08:37]

LGTM purely based on visual inspection - tried to patch in, but crashes for my
data:

autofill::AutofillDialogControllerImpl::IsCreditCardExpirationValid(std::basic_string<unsigned
short, base::string16_char_traits, std::allocator<unsigned short> > const&,
std::basic_string<unsigned short, base::string16_char_traits,
std::allocator<unsigned short> > const&) const + 180
(autofill_dialog_controller_impl.cc:3076)

Looks like ActiveInstrument() is NULL for whatever reason. (The account I tried
doesn't yet have an instrument, so I suppose that's it)

[commit-bot: I haz the power, 2014-01-04 02:12:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/123363002/710001

[commit-bot: I haz the power, 2014-01-04 12:37:27]

Change committed as 243030