Chromium: Make password saving option independent from autofill option in Privacy settings

[haanhvu]

Chromium: Make password saving option independent from autofill option in Privacy settings

Currently in Privacy settings in Chromium backend, the password saving option depends on the autofill option. If autofill is disabled, password saving doesn't work whether enabled or not. This is because Chromium doesn't provide an autofill option but provides a login selection prompt that asks users whether to autofill the saved password or not. This commit makes password saving option independent from autofill option. The autofill option now only affects whether to show login selection prompt.

Fixes #1707

[svillar]

I have to admit that I don't really understand what the problem is. I don't see the relationship between password saving and autofill. Actually if you check the implementation of PromptDelegate.java the onLoginSave method does depend on LoginAutocompleteEnabled not in AutoFillEnabled. So I'm a bit lost about why this is needed and whether this fixes anything

[haanhvu]

I have to admit that I don't really understand what the problem is. I don't see the relationship between password saving and autofill. Actually if you check the implementation of PromptDelegate.java the onLoginSave method does depend on LoginAutocompleteEnabled not in AutoFillEnabled. So I'm a bit lost about why this is needed and whether this fixes anything

As stated in #1707, the problem is if users enable password saving and disable autofill, password saving dialog doesn't show. And this only happens in Chromium. You could reproduce this with the main branch.

You're right about onLoginSave depending on LoginAutocompleteEnabled not AutoFillEnabled. However in Chromium implementation, onLoginSave is called by saveOrUpdatePassword in PromptDelegateImpl.java:

wolvic/app/src/common/chromium/com/igalia/wolvic/browser/api/impl/PromptDelegateImpl.java

Line 997 in 9b1e83d

mDelegate.onLoginSave(mSession, mLoginSavePrompt);

This is in turn called by WolvicPasswordManagerClient in native code: https://github.com/Igalia/wolvic-chromium/blob/40804fadc808686bdebc54a71196c4b9a3ff68c7/wolvic/browser/autocomplete/wolvic_password_manager_client.cc#L164

However, password manager client checks if saving and autofill enabled too, as I stated in #1707 (comment)

Does this make sense to you? I don't know if I missed something, but I checked my change on Quest 3 and it fixed the issue. When I enabled password saving and disabled autofill, password saving dialog now showed.

Also, unlike Gecko, Chromium shows an autofill prompt to ask if you want to autofill in every login after saving. I made this dependent on the autofill option in the Settings now. If users disable autofill, this autofill prompt now doesn't show.

[svillar]

I have to admit that I don't really understand what the problem is. I don't see the relationship between password saving and autofill. Actually if you check the implementation of PromptDelegate.java the onLoginSave method does depend on LoginAutocompleteEnabled not in AutoFillEnabled. So I'm a bit lost about why this is needed and whether this fixes anything

As stated in #1707, the problem is if users enable password saving and disable autofill, password saving dialog doesn't show. And this only happens in Chromium. You could reproduce this with the main branch.

You're right about onLoginSave depending on LoginAutocompleteEnabled not AutoFillEnabled. However in Chromium implementation, onLoginSave is called by saveOrUpdatePassword in PromptDelegateImpl.java:

wolvic/app/src/common/chromium/com/igalia/wolvic/browser/api/impl/PromptDelegateImpl.java

Line 997 in 9b1e83d

mDelegate.onLoginSave(mSession, mLoginSavePrompt);

This is in turn called by WolvicPasswordManagerClient in native code: https://github.com/Igalia/wolvic-chromium/blob/40804fadc808686bdebc54a71196c4b9a3ff68c7/wolvic/browser/autocomplete/wolvic_password_manager_client.cc#L164

However, password manager client checks if saving and autofill enabled too, as I stated in #1707 (comment)

Does this make sense to you? I don't know if I missed something, but I checked my change on Quest 3 and it fixed the issue. When I enabled password saving and disabled autofill, password saving dialog now showed.

Yes it does make sense. What I don't see

I have to admit that I don't really understand what the problem is. I don't see the relationship between password saving and autofill. Actually if you check the implementation of PromptDelegate.java the onLoginSave method does depend on LoginAutocompleteEnabled not in AutoFillEnabled. So I'm a bit lost about why this is needed and whether this fixes anything

As stated in #1707, the problem is if users enable password saving and disable autofill, password saving dialog doesn't show. And this only happens in Chromium. You could reproduce this with the main branch.

You're right about onLoginSave depending on LoginAutocompleteEnabled not AutoFillEnabled. However in Chromium implementation, onLoginSave is called by saveOrUpdatePassword in PromptDelegateImpl.java:

wolvic/app/src/common/chromium/com/igalia/wolvic/browser/api/impl/PromptDelegateImpl.java

Line 997 in 9b1e83d

mDelegate.onLoginSave(mSession, mLoginSavePrompt);

This is in turn called by WolvicPasswordManagerClient in native code: https://github.com/Igalia/wolvic-chromium/blob/40804fadc808686bdebc54a71196c4b9a3ff68c7/wolvic/browser/autocomplete/wolvic_password_manager_client.cc#L164

However, password manager client checks if saving and autofill enabled too, as I stated in #1707 (comment)

Does this make sense to you? I don't know if I missed something, but I checked my change on Quest 3 and it fixed the issue. When I enabled password saving and disabled autofill, password saving dialog now showed.

Also, unlike Gecko, Chromium shows an autofill prompt to ask if you want to autofill in every login after saving. I made this dependent on the autofill option in the Settings now. If users disable autofill, this autofill prompt now doesn't show.

Thanks for the explanation, it now makes sense to me, sorry I haven't previously read the conversation in #1707

After analyzing the problem (thanks for your detailed analysis) I think that the best we can do it to remove the autofill option from the UI in the Chromium port because as you said, it does not make sense as they're linked.

Then in isAutoFillEnabled() we need to return instead whether saving logins is enabled or not. We should add a comment there explaining how both concepts are tied in Chromium otherwise it'd look like a bug to return isLoginSaveEnabled() (or whatever is called) in the isFillingEnabled() method

[haanhvu]

Then in isAutoFillEnabled() we need to return instead whether saving logins is enabled or not. We should add a comment there explaining how both concepts are tied in Chromium otherwise it'd look like a bug to return isLoginSaveEnabled() (or whatever is called) in the isFillingEnabled() method

Actually we don't need to do that. Because in Chromium it calls isSavingAndFillingEnabled(): https://github.com/Igalia/wolvic-chromium/blob/3d364f2d324a6087bdd02867604f4d4b77abebc7/wolvic/java/org/chromium/wolvic/PasswordManager.java#L66-L71 So we just need to make sure isAutoFillEnabled() return true. That's what I did in this PR.

After analyzing the problem (thanks for your detailed analysis) I think that the best we can do it to remove the autofill option from the UI in the Chromium port because as you said, it does not make sense as they're linked.

Yes that's an option. But as @javifernandez explained in #1707 (comment), Chromium has a login selection dialog to ask if users want to autofill in each login after saving password. So my suggestion in #1707 (comment) is the better option would be let the autofill option enable/disable this login selection dialog. I.e., if users turn on password saving but turn off autofill, password saving dialog would still show but login selection dialog would not show (in next logins after saving password). That's what I did in this PR. Wdyt?