1500155 - Refactor GeckoSessionSettings to provide dedicated methods for settings

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P3)

Description

[Eugen Sawin [:esawin]]

Currently, GeckoSessionSettings provides a generic set<DataType>(<Setting>, <Value>) interface.

We might want to move away from that and make GeckoSessionSettings consistent with GeckoRuntimeSettings by providing an individual method for each setting.

Comment 1

[Emily Toop (:fluffyemily)]

Do we want the existing get/set<DataType> methods to still be available publicly? If not, that will involve some refactoring of GeckoSessionTestRule Setting @interface to use a different strategy other than reflection to set it's values as it cannot find non-public functions and fields.

Comment 2

[Eugen Sawin [:esawin]]

I don't think we want to continue supporting the old interface.
Isn't it possible to access non-public fields via getDeclared{Field|Method}?

Comment 3

[Emily Toop (:fluffyemily)]

Turns out `Method.setAccessible(true)` was the missing link to calling private functions/fields using reflection.

Comment 4

[Emily Toop (:fluffyemily)]

Attachment: Part 1 - Add methods for each setting in `GeckoSessionSettings`. Part 2 - Make old methods and fields for get/set<DataType> in `GeckoSessionSettings` private.

Migrate existing code to use these new methods instead of the exisiting get/set<DataType>(Key, Value) methods.

This commit does not tackle making the old functions inaccessible publicly as this requires a complete refactoring of the Setting @interface in `GeckoSessionTestRule.java`. There may be a follow up commit if this is something we want to do.

Make `GeckoSessionSettings` fields and get<Type> methods private.

Update `GeckoSessionTestRule` to access private fields and methods using reflection.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84081d548703
Part 1 - Add methods for each setting in `GeckoSessionSettings`. r=geckoview-reviewers,snorp,esawin

Comment 6

[Narcis Beleuzu [:NarcisB]]

Backed out for build bustages on /GeckoViewActivity.java

Backout link: https://hg.mozilla.org/integration/autoland/rev/66e1668bb44c322c83e89bfebae59e0a4d2a2f07
Push link: https://hg.mozilla.org/integration/autoland/rev/84081d5487039fdb4aeca8245ed58bfa34d5051b
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=214375538&repo=autoland&lineNumber=42410

Comment 7

[Emily Toop (:fluffyemily)]

Attachment: Bug 1500155 - Part 1 - Add methods for each setting in `GeckoSessionSettings`.

Migrate existing code to use these new methods instead of the exisiting get/set<DataType>(Key, Value) methods.

This commit does not tackle making the old functions inaccessible publicly as this requires a complete refactoring of the Setting @interface in `GeckoSessionTestRule.java`. There may be a follow up commit if this is something we want to do.

Comment 8

[Emily Toop (:fluffyemily)]

Attachment: Bug 1500155 - Part 2 - Make old methods and fields for get/set<DataType> in `GeckoSessionSettings` private.

Depends on D13417

Comment 9

[Emily Toop (:fluffyemily)]

Attachment: Bug 1500155 - Part 3 - Migrate existing code to use these new methods instead of the exisiting get/set<DataType>(Key, Value) methods.

Depends on D13418

Comment 10

[Emily Toop (:fluffyemily)]

Attachment: Bug 1500155 - Part 4 - Add Builder to `GeckoSessionSettings` to handle setting of init only fields.

* Make setters for init only fields protected.
* Remove tests that ensure that init only fields throw an error when set on the fly as this is no longer possible.
* Update tests to use builder when init-ing settings.
* Update API doc to reflect new public API.

Depends on D13419

Comment 11

[Emily Toop (:fluffyemily)]

Attachment: Bug 1500155 - Part 6 - address review comments r=esawin,snorp. Update changelog.

Depends on D13420

Comment 12

[Emily Toop (:fluffyemily)]

Attachment: Bug 1500155 - Part 7 - Update `CHANGELOG.md`.

Depends on D13421

Comment 13

[Emily Toop (:fluffyemily)]

Attachment: Bug 1500155 - Part 8 - Update `geckoview_example` to use new methods.

Depends on D13422

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9410d9b3f16
Part 1 - Add methods for each setting in `GeckoSessionSettings`. r=snorp
https://hg.mozilla.org/integration/autoland/rev/fcfa609101f8
Part 2 - Make old methods and fields for get/set<DataType> in `GeckoSessionSettings` private. r=snorp
https://hg.mozilla.org/integration/autoland/rev/0d03623732b6
Part 3 - Migrate existing code to use these new methods instead of the exisiting get/set<DataType>(Key, Value) methods. r=snorp
https://hg.mozilla.org/integration/autoland/rev/4dd0b1c3524c
Part 4 - Add Builder to `GeckoSessionSettings` to handle setting of init only fields. r=snorp
https://hg.mozilla.org/integration/autoland/rev/c4bc2e839a9e
Part 6 - address review comments r=esawin,snorp. Update changelog.
https://hg.mozilla.org/integration/autoland/rev/85900c298b05
Part 7 - Update `CHANGELOG.md`. r=snorp
https://hg.mozilla.org/integration/autoland/rev/0f66513c5f23
Part 8 - Update `geckoview_example` to use new methods. r=snorp

Comment 15

[Andrei Ciure[:aciure]]

Backed out 7 changesets (bug 1500155) for multiple Android fails

push that caused the failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&group_state=expanded&selectedJob=216595830&searchStr=android&revision=0f66513c5f234d93889f4fd2c6e5e0bcee0b75ef

failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&group_state=expanded&selectedJob=216594113&searchStr=android%2C4.3%2Capi16%2B%2Cdebug%2Cmochitests%2Ctest-android-em-4.3-arm7-api-16%2Fdebug-mochitest-53%2Cm%2853%29

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=216594351&repo=autoland&lineNumber=1370

backout: https://hg.mozilla.org/integration/autoland/rev/f7c35130959cdc3d2711c2588d88d471f1d3beb8

Comment 16

[Emily Toop (:fluffyemily)]

Attachment: Bug 1500155 - Add methods for each setting in `GeckoSessionSettings`.

This patch has been hanging around for a while because it's been failing the mochitests on Android. The symptom was failure to load a webpage in Fennec running Android 4.1 - it would just hang. 
It turned out to have been a very simple error, possibly caused by fixing a merge conflict, which resulted in the `getChromeUri` function in `GeckoSessionSettings`
using the incorrect key, which produced an invalid URI which then caused Fennec to fail to fetch the ChromeUri, which was not handled in Android 4.1 for some reason. 
Fixing the function to use the correct key fixed the issue and now the mochitests pass.

Part 1 - Add methods for each setting in `GeckoSessionSettings`.
Migrate existing code to use these new methods instead of the exisiting get/set<DataType>(Key, Value) methods.

Part 2 - Make old methods and fields for get/set<DataType> in `GeckoSessionSettings` private.

Part 3 - Migrate existing code to use these new methods instead of the exisiting get/set<DataType>(Key, Value) methods.

Part 4 - Add Builder to `GeckoSessionSettings` to handle setting of init only fields.

* Make setters for init only fields protected.
* Remove tests that ensure that init only fields throw an error when set on the fly as this is no longer possible.
* Update tests to use builder when init-ing settings.
* Update API doc to reflect new public API.

Part 5 - Update `CHANGELOG.md`.

Part 6 - Update `geckoview_example` to use new methods.

Part 7 - Fetch `CHROME_URI` key when calling `getChromeUri` rather than incorrect `USER_AGENT_OVERRIDE`

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18cb79bb1c78
Add methods for each setting in `GeckoSessionSettings`. r=snorp

Comment 18

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/18cb79bb1c78

Comment 19

[Bob Clary [:bc] (inactive)]

fluffyemily: I believe this broke ./mach android archive-geckoview

mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSessionSettings.java:591: warning: no description for @param
* @param value

Execution failed for task ':geckoview:javadocWithGeckoBinariesDebug'.

Treating 1 javadoc warning(s) as error(s)

Would you like to file a follow up bug or just handle it here?

Comment 20

[Emily Toop (:fluffyemily)]

Let's just handle it here. It's an oversight possibly caused by one of the many conflicts encountered during rebasing. Thanks.

Comment 21

[Emily Toop (:fluffyemily)]

Attachment: Fix `./mach android archive-geckoview` by adding description into javadoc `@param` inside `GeckoSessionSettings.setUserAgentOverride`

Follow up issue

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e29a26d22b2
Fix `./mach android archive-geckoview` by adding description into javadoc `@param` inside `GeckoSessionSettings.setUserAgentOverride` r=geckoview-reviewers,agi

Comment 23

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/3e29a26d22b2