Save password infobar should only work on schemes on which the bubble does

Save password infobar should only work on schemes on which the bubble does

Currently, the password bubble opens up only on "webby" URLs i.e. it
does not work on URLs such as chrome: or file: but the save password
infobar shows up also for the "non-webby" URLs making both the prompts
behave in a different manner. This patch makes the behaviour for both
the prompts consistent.

BUG=393138
Committed: https://crrev.com/25defaf729d72cb9ee8b6405d815259472edbb48
Cr-Commit-Position: refs/heads/master@{#296701}

[Sunil Ratnu, 2014-09-19 12:34:43]

sunil.ratnu@samsung.com changed reviewers:
+ gcasto@chromium.org, mkwst@chromium.org, vabr@chromium.org

[Sunil Ratnu, 2014-09-19 12:34:44]

Please have a look. I will soon add the tests. Thanks!

[vabr (Chromium), 2014-09-19 13:18:10]

Thanks, Sunil Ratnu.
The patch looks good so far (but do have a look at the comment).
Having said that, I'll wait with approving until you add the tests.

Cheers,
Vaclav

https://codereview.chromium.org/582833005/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/582833005/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/chrome_password_manager_client.cc:158: } else if
(BrowsingDataHelper::IsWebScheme(
While this will solve the current issue, please consider moving this test to the
very beginning of PromptUserToSavePassword() -- that way you ensure that neither
of the types of the prompt we have will show on non-webby pages, even if, e.g.,
the bubble code regresses in the future, or the UI team comes up with yet
another type of prompt.

[Sunil Ratnu, 2014-09-19 13:39:42]

Thanks Vaclav for the review. I will try to upload the tests asap.

[Sunil Ratnu, 2014-09-19 14:47:41]

Hi Vaclav, While trying to write the test case I am facing a little problem. I
don't think the NavigateToFile() method will create a non webby URL, so we need
to use NavigateAndCommit(GURL(file://....). I can manually check the working of
this patch locally as my system has a local file containing a password form and
I can go to a "file:///a/b/password.html" URL but I am not sure how to have a
file stored on server so that trybots can use it to go to a file:// URL. Can you
please help me with this. Thank you.

[vabr (Chromium), 2014-09-22 12:17:40]

On 2014/09/19 14:47:41, Sunil Ratnu wrote:
> Hi Vaclav, While trying to write the test case I am facing a little problem. I
> don't think the NavigateToFile() method will create a non webby URL, so we
need
> to use NavigateAndCommit(GURL(file://....). I can manually check the working
of
> this patch locally as my system has a local file containing a password form
and
> I can go to a "file:///a/b/password.html" URL but I am not sure how to have a
> file stored on server so that trybots can use it to go to a file:// URL. Can
you
> please help me with this. Thank you.

Hi Sunil Ratnu,

Try to have a look at PathService::Get. It looks like, e.g.,
DriveAppConverterTest::SetUpOnMainThread gets the (prefix of the) path you are
after.

Cheers,
Vaclav

[Sunil Ratnu, 2014-09-24 15:15:37]

Hi Vaclav,

I've written the browsertest for this case. I manually verified that this patch
was solving this particular bug  but I don't understand why the browser test
passes without the patch as well. I might be missing something here. Please have
a look at the new added tests.

Thanks!
Sunil

https://codereview.chromium.org/582833005/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/582833005/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/chrome_password_manager_client.cc:158: } else if
(BrowsingDataHelper::IsWebScheme(
On 2014/09/19 13:18:10, vabr (Chromium) wrote:
> While this will solve the current issue, please consider moving this test to
the
> very beginning of PromptUserToSavePassword() -- that way you ensure that
neither
> of the types of the prompt we have will show on non-webby pages, even if,
e.g.,
> the bubble code regresses in the future, or the UI team comes up with yet
> another type of prompt.

Done.

[vabr (Chromium), 2014-09-25 08:48:02]

LGTM with comments.

As for the test passing even without patch (thanks for checking that!), I
suspect it was the double Wait() you used (see my comments below). Another thing
to check would be whether the test used the bubble as a prompt, and not the
infobar (this bug only exists for the bubble).
Please let me know if you were able to make the test fail before patch.

Thanks for your work on this! :)
Vaclav

https://codereview.chromium.org/582833005/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/582833005/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:277: static GURL
GetURL(const char* filename) {
nit: indenting
(I strongly suggest running git cl format before uploading every patchset, to
make sure formatting is correct.)

https://codereview.chromium.org/582833005/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:277: static GURL
GetURL(const char* filename) {
This function has nothing to with the test class. Please move it out of the
class to the anonymous namespace. Also, please rename it to something more
descriptive, e.g., GetFileURL.

https://codereview.chromium.org/582833005/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:841: // Check
that no prompt is shown for non webby URLs such as file://.
Please simplify the comment to:
// Check that no prompt is shown for URLs with file: scheme.

In a similar spirit, please change the name of the test. "webby" is not a
standard English word, has unclear meaning, and this test only checks file:
anyway.

https://codereview.chromium.org/582833005/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:842:
NavigationObserver observer(WebContents());
Move the definition of the observer below, just before the prompt_observer.

https://codereview.chromium.org/582833005/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:845:
observer.Wait();
Don't Wait() here, NavigateToURL waits for that already.

[Sunil Ratnu, 2014-09-25 10:14:24]

Thanks for the review Vaclav. The test works fine now (i.e. fails without the
patch and passes with the patch). The problem was that earlier the test used
bubble as the prompt (i.e. I was not disabling the password bubble and hence It
was passing everytime as correctly pointed by you :)) but now I verified with
--disable-save-password-bubble.

Thanks!
Sunil

https://codereview.chromium.org/582833005/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/582833005/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:277: static GURL
GetURL(const char* filename) {
On 2014/09/25 08:48:02, vabr (Chromium) wrote:
> nit: indenting
> (I strongly suggest running git cl format before uploading every patchset, to
> make sure formatting is correct.)

Done.

https://codereview.chromium.org/582833005/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:277: static GURL
GetURL(const char* filename) {
On 2014/09/25 08:48:02, vabr (Chromium) wrote:
> This function has nothing to with the test class. Please move it out of the
> class to the anonymous namespace. Also, please rename it to something more
> descriptive, e.g., GetFileURL.

Done.

https://codereview.chromium.org/582833005/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:841: // Check
that no prompt is shown for non webby URLs such as file://.
On 2014/09/25 08:48:02, vabr (Chromium) wrote:
> Please simplify the comment to:
> // Check that no prompt is shown for URLs with file: scheme.
> 
> In a similar spirit, please change the name of the test. "webby" is not a
> standard English word, has unclear meaning, and this test only checks file:
> anyway.

Done.

https://codereview.chromium.org/582833005/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:842:
NavigationObserver observer(WebContents());
On 2014/09/25 08:48:02, vabr (Chromium) wrote:
> Move the definition of the observer below, just before the prompt_observer.

Done.

https://codereview.chromium.org/582833005/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:845:
observer.Wait();
On 2014/09/25 08:48:02, vabr (Chromium) wrote:
> Don't Wait() here, NavigateToURL waits for that already.

Done.

[Sunil Ratnu, 2014-09-25 10:20:04]

Hi Vaclav,

Please have a look at the final changes before we can commit it.

Thanks!
Sunil

[vabr (Chromium), 2014-09-25 11:07:27]

Still LGTM, with some nits. Feel free to land this after addressing them.

Also, thanks for checking that the test now works as it should. (And sorry that
I have confused which kind of prompt was supposed to work correctly; you got it
right in any case. :))

Cheers,
Vaclav

https://codereview.chromium.org/582833005/diff/40001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/582833005/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:155: // web
scheme URLs and DO NOT prompt in case of URLs with schemes like file:
optional nit: I think the patch set 2 version of this comment was actually
better.

Don't get me wrong, I do not object to the word "webby" in general. In the
special context of the browser test, where only file: scheme was tested, the use
of "webby" was confusing.

But here it was fine, because the code below makes it clear what it means, and
the comment was shorter. Also, right now it only has an incomplete list of
schemes, so it's not that much more precise (whereas in the test, saying 'file:'
was much more precise).

For similar reasons, you did not have to wipe out "webby" from the CL
description and other places. :) It was really just the one place I commented
on, where it needed to go away. Sorry for the confusion. :)

https://codereview.chromium.org/582833005/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:156: // and
schemes like 'chrome-extension'
nit: Whatever comment you choose, please end it with a full stop ('.'), as a
proper sentence.

https://codereview.chromium.org/582833005/diff/40001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/582833005/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:214: static GURL
GetFileURL(const char* filename) {
Please drop "static". The visibility is local already due to the anonymous
namespace.

https://codereview.chromium.org/582833005/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:256: }  //
anonymous namespace
Just "// namespace", not "anonymous".
See http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces
for examples (note that the guide uses the term "unnamed namespace").

[Sunil Ratnu, 2014-09-25 11:39:40]

Thanks Vaclav for the review. I've made the suggested changes.

--Sunil

https://codereview.chromium.org/582833005/diff/40001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/582833005/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:155: // web
scheme URLs and DO NOT prompt in case of URLs with schemes like file:
On 2014/09/25 11:07:27, vabr (Chromium) wrote:
> optional nit: I think the patch set 2 version of this comment was actually
> better.
> 
> Don't get me wrong, I do not object to the word "webby" in general. In the
> special context of the browser test, where only file: scheme was tested, the
use
> of "webby" was confusing.
> 
> But here it was fine, because the code below makes it clear what it means, and
> the comment was shorter. Also, right now it only has an incomplete list of
> schemes, so it's not that much more precise (whereas in the test, saying
'file:'
> was much more precise).
> 
> For similar reasons, you did not have to wipe out "webby" from the CL
> description and other places. :) It was really just the one place I commented
> on, where it needed to go away. Sorry for the confusion. :)

Done.

https://codereview.chromium.org/582833005/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:156: // and
schemes like 'chrome-extension'
On 2014/09/25 11:07:27, vabr (Chromium) wrote:
> nit: Whatever comment you choose, please end it with a full stop ('.'), as a
> proper sentence.

Done.

https://codereview.chromium.org/582833005/diff/40001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/582833005/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:214: static GURL
GetFileURL(const char* filename) {
On 2014/09/25 11:07:27, vabr (Chromium) wrote:
> Please drop "static". The visibility is local already due to the anonymous
> namespace.

Done.

https://codereview.chromium.org/582833005/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:256: }  //
anonymous namespace
On 2014/09/25 11:07:27, vabr (Chromium) wrote:
> Just "// namespace", not "anonymous".
> See http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces
> for examples (note that the guide uses the term "unnamed namespace").

Done.

[Sunil Ratnu, 2014-09-25 11:40:07]

The CQ bit was checked by sunil.ratnu@samsung.com

[commit-bot: I haz the power, 2014-09-25 11:40:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/582833005/60001

[commit-bot: I haz the power, 2014-09-25 12:33:19]

Committed patchset #4 (id:60001) as 3b5f8d6a64be0b5309a184d077d9473a02a356ac

[commit-bot: I haz the power, 2014-09-25 12:34:06]

Patchset 4 (id:??) landed as
https://crrev.com/25defaf729d72cb9ee8b6405d815259472edbb48
Cr-Commit-Position: refs/heads/master@{#296701}