Hook up autofill popup control elements

Hook up autofill popup control elements

BUG=403597
Committed: https://crrev.com/57722ec5e7cc0020fe065e172c578190b520502d
Cr-Commit-Position: refs/heads/master@{#292272}

[Evan Stade, 2014-08-25 21:49:50]

estade@chromium.org changed reviewers:
+ aurimas@chromium.org

[Evan Stade, 2014-08-25 21:52:28]

https://codereview.chromium.org/490193003/diff/20001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/autofill/AutofillSuggestion.java
(right):

https://codereview.chromium.org/490193003/diff/20001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/autofill/AutofillSuggestion.java:15: private
final int mSuggestionId;
this is just a name change

[aurimas (slooooooooow), 2014-08-26 00:01:49]

https://codereview.chromium.org/490193003/diff/20001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):

https://codereview.chromium.org/490193003/diff/20001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:137: if
(suggestion.getSuggestionId() == ITEM_ID_AUTOFILL_OPTIONS) {
Take a look
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
for an example how to launch into Settings.

[Evan Stade, 2014-08-26 00:38:10]

https://codereview.chromium.org/490193003/diff/20001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):

https://codereview.chromium.org/490193003/diff/20001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:137: if
(suggestion.getSuggestionId() == ITEM_ID_AUTOFILL_OPTIONS) {
On 2014/08/26 00:01:49, aurimas wrote:
> Take a look
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> for an example how to launch into Settings.

What is the advantage of that approach? I guess you don't have to parse a URL,
but otherwise I don't see why it would be preferable. It creates extra cruft in
places like ChromiumShellApplication. I'm far from an android expert, but it
seems like this is a good use case for intents.

[aurimas (slooooooooow), 2014-08-26 00:44:49]

On 2014/08/26 00:38:10, Evan Stade wrote:
>
https://codereview.chromium.org/490193003/diff/20001/ui/android/java/src/org/...
> File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):
> 
>
https://codereview.chromium.org/490193003/diff/20001/ui/android/java/src/org/...
> ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:137: if
> (suggestion.getSuggestionId() == ITEM_ID_AUTOFILL_OPTIONS) {
> On 2014/08/26 00:01:49, aurimas wrote:
> > Take a look
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> > for an example how to launch into Settings.
> 
> What is the advantage of that approach? I guess you don't have to parse a URL,
> but otherwise I don't see why it would be preferable. It creates extra cruft
in
> places like ChromiumShellApplication. I'm far from an android expert, but it
> seems like this is a good use case for intents.

This is for consistency with the rest of the code. We do not use intents for
other places where we open settings.

[Evan Stade, 2014-08-26 01:07:58]

On 2014/08/26 00:44:49, aurimas wrote:
> On 2014/08/26 00:38:10, Evan Stade wrote:
> >
>
https://codereview.chromium.org/490193003/diff/20001/ui/android/java/src/org/...
> > File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java
(right):
> > 
> >
>
https://codereview.chromium.org/490193003/diff/20001/ui/android/java/src/org/...
> > ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:137: if
> > (suggestion.getSuggestionId() == ITEM_ID_AUTOFILL_OPTIONS) {
> > On 2014/08/26 00:01:49, aurimas wrote:
> > > Take a look
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> > > for an example how to launch into Settings.
> > 
> > What is the advantage of that approach? I guess you don't have to parse a
URL,
> > but otherwise I don't see why it would be preferable. It creates extra cruft
> in
> > places like ChromiumShellApplication. I'm far from an android expert, but it
> > seems like this is a good use case for intents.
> 
> This is for consistency with the rest of the code. We do not use intents for
> other places where we open settings.

I understand the desire for consistency. But I'm not sure why the other code is
not using intents. Wouldn't it be better if we could whittle down
ChromiumApplication.java (or even remove it completely)? Smaller APIs are
better. So non-existent APIs must be best!

[aurimas (slooooooooow), 2014-08-26 01:45:32]

On 2014/08/26 01:07:58, Evan Stade wrote:
> On 2014/08/26 00:44:49, aurimas wrote:
> > On 2014/08/26 00:38:10, Evan Stade wrote:
> > >
> >
>
https://codereview.chromium.org/490193003/diff/20001/ui/android/java/src/org/...
> > > File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/490193003/diff/20001/ui/android/java/src/org/...
> > > ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:137: if
> > > (suggestion.getSuggestionId() == ITEM_ID_AUTOFILL_OPTIONS) {
> > > On 2014/08/26 00:01:49, aurimas wrote:
> > > > Take a look
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> > > > for an example how to launch into Settings.
> > > 
> > > What is the advantage of that approach? I guess you don't have to parse a
> URL,
> > > but otherwise I don't see why it would be preferable. It creates extra
cruft
> > in
> > > places like ChromiumShellApplication. I'm far from an android expert, but
it
> > > seems like this is a good use case for intents.
> > 
> > This is for consistency with the rest of the code. We do not use intents for
> > other places where we open settings.
> 
> I understand the desire for consistency. But I'm not sure why the other code
is
> not using intents. Wouldn't it be better if we could whittle down
> ChromiumApplication.java (or even remove it completely)? Smaller APIs are
> better. So non-existent APIs must be best!

Adding Intent support is not free. Using intents exposes all sort of possible
security bugs. I still think that you should use ChromiumApplication.java.

[Evan Stade, 2014-08-26 01:49:09]

On 2014/08/26 01:45:32, aurimas wrote:
> On 2014/08/26 01:07:58, Evan Stade wrote:
> > On 2014/08/26 00:44:49, aurimas wrote:
> > > On 2014/08/26 00:38:10, Evan Stade wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/490193003/diff/20001/ui/android/java/src/org/...
> > > > File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/490193003/diff/20001/ui/android/java/src/org/...
> > > > ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:137: if
> > > > (suggestion.getSuggestionId() == ITEM_ID_AUTOFILL_OPTIONS) {
> > > > On 2014/08/26 00:01:49, aurimas wrote:
> > > > > Take a look
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> > > > > for an example how to launch into Settings.
> > > > 
> > > > What is the advantage of that approach? I guess you don't have to parse
a
> > URL,
> > > > but otherwise I don't see why it would be preferable. It creates extra
> cruft
> > > in
> > > > places like ChromiumShellApplication. I'm far from an android expert,
but
> it
> > > > seems like this is a good use case for intents.
> > > 
> > > This is for consistency with the rest of the code. We do not use intents
for
> > > other places where we open settings.
> > 
> > I understand the desire for consistency. But I'm not sure why the other code
> is
> > not using intents. Wouldn't it be better if we could whittle down
> > ChromiumApplication.java (or even remove it completely)? Smaller APIs are
> > better. So non-existent APIs must be best!
> 
> Adding Intent support is not free. Using intents exposes all sort of possible
> security bugs. I still think that you should use ChromiumApplication.java.

Ok, I can switch to ChromiumApplication. But for my understanding, can you
elaborate on the security bugs that this opens up? Is checking the originating
application id insufficient to ensure Chrome launched the intent?

[aurimas (slooooooooow), 2014-08-26 02:44:43]

aurimas@chromium.org changed reviewers:
+ cpalmer@chromium.org

[aurimas (slooooooooow), 2014-08-26 02:44:43]

+cpalmer who can tell you all about why intents in Android cause security
issues.

In short I think the idea is that having one specialized
ChromiumApplication#openAutofillSettings() call is safer because you do not have
to do any URL parsing, you don't have to make it is sent by the right package,
and you don't have to pass anything to Android system.

[Evan Stade, 2014-08-26 17:26:51]

On 2014/08/26 02:44:43, aurimas wrote:
> +cpalmer who can tell you all about why intents in Android cause security
> issues.
> 
> In short I think the idea is that having one specialized
> ChromiumApplication#openAutofillSettings() call is safer because you do not
have
> to do any URL parsing, you don't have to make it is sent by the right package,
> and you don't have to pass anything to Android system.

Are intents slower? That could be another reason to avoid them.

[aurimas (slooooooooow), 2014-08-26 17:34:15]

On 2014/08/26 17:26:51, Evan Stade wrote:
> On 2014/08/26 02:44:43, aurimas wrote:
> > +cpalmer who can tell you all about why intents in Android cause security
> > issues.
> > 
> > In short I think the idea is that having one specialized
> > ChromiumApplication#openAutofillSettings() call is safer because you do not
> have
> > to do any URL parsing, you don't have to make it is sent by the right
package,
> > and you don't have to pass anything to Android system.
> 
> Are intents slower? That could be another reason to avoid them.

Yes they would be as you they leave your process to the system and then system
passes it back to your process.

[Evan Stade, 2014-08-26 19:41:38]

patch updated

[aurimas (slooooooooow), 2014-08-26 21:49:37]

lgtm

https://codereview.chromium.org/490193003/diff/40001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):

https://codereview.chromium.org/490193003/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:34: * Constants
defining types of Autofill suggestion entries.
Update this comment as there is only one constant here now.

[aurimas (slooooooooow), 2014-08-26 21:50:03]

aurimas@chromium.org changed reviewers:
+ tedchoc@chromium.org

[aurimas (slooooooooow), 2014-08-26 21:50:03]

+tedchoc who can review the rest of the changes.

[Ted C, 2014-08-27 00:22:53]

On 2014/08/26 21:50:03, aurimas wrote:
> +tedchoc who can review the rest of the changes.

lgtm for non-autofill code

[Evan Stade, 2014-08-27 22:05:55]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-08-27 22:07:14]

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

[Evan Stade, 2014-08-27 22:21:57]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2014-08-27 22:22:16]

https://codereview.chromium.org/490193003/diff/40001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):

https://codereview.chromium.org/490193003/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:34: * Constants
defining types of Autofill suggestion entries.
On 2014/08/26 21:49:37, aurimas wrote:
> Update this comment as there is only one constant here now.

Done.

[commit-bot: I haz the power, 2014-08-27 22:24:02]

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

[commit-bot: I haz the power, 2014-08-27 23:22:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[commit-bot: I haz the power, 2014-08-27 23:52:40]

Committed patchset #4 (id:60001) as 0eb0f5bfe042b84d308895b9ef71f9c2ab282eb7

[commit-bot: I haz the power, 2014-09-10 02:55:47]

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