1264835 - No way to set doorhanger checkbox default off

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

For getusermedia permission granting & blocking, we want to grant as little persistent permissions as possible. So we need to set the doorhanger checkbox default off.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

We allow to set the text but not the default state:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/widget/DefaultDoorHanger.java#124-127

Comment 2

[Nicholas Rosbrook [:enr0n]]

So do we want to allow for the default state of the checkbox to be passed in with options? Or are we just assuming a default of unchecked?

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

We should allow the default state to be passed in.

Comment 4

[Nicholas Rosbrook [:enr0n]]

Attachment: Patch attempt 1

Here's my first try at it.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8762766 [details] [diff] [review]
Patch attempt 1

Review of attachment 8762766 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good! I think with the little change below we can land this. Did you test this?

We have a test for some doorhangers here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testDoorHanger.java

Maybe you can use this to write a new test just for the checkbox state?

::: mobile/android/base/java/org/mozilla/gecko/widget/DefaultDoorHanger.java
@@ +131,5 @@
> +        if (options.has("checkboxState")) {
> +            final boolean checkBoxState = options.optBoolean("checkboxState");
> +            mCheckBox = (CheckBox) findViewById(R.id.doorhanger_checkbox);
> +            mCheckBox.setChecked(checkBoxState);
> +            mCheckBox.setVisibility(VISIBLE);

I wonder if we should move this inside the checkboxText block: I guess it only makes sense to set the default for the checkbox if we are actually going to display a checkbox *with* text. This also avoids some of the code because we do not need to search the view again and update the visibility.

Comment 6

[Nicholas Rosbrook [:enr0n]]

Attachment: Patch attempt 2

Here's the patch with the suggested fix. I'll try to write the test for it--but here's the patch itself for now.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8763724 [details] [diff] [review]
Patch attempt 2

LGTM!

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77a5370625b1

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Let's get this into the tree (Adding checkin-needed..).

@Nicholas: If you write a UI test for this then just file a new bug, attach the patch and assign me for review. :)

Comment 10

[Nicholas Rosbrook [:enr0n]]

@Sebastian: Okay, sounds good!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/a5e39700fd44
Updated DefaultDoorHanger so that the default CheckBox state could be specified as an option. r=sebastian

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a5e39700fd44