Refactor GeckoSessionSettings to provide dedicated methods for settings

RESOLVED FIXED in Firefox 66

Status

enhancement
P3
normal
RESOLVED FIXED
9 months ago
6 months ago

People

(Reporter: esawin, Assigned: fluffyemily)

Tracking

(Blocks 1 bug, {good-first-bug})

51 Branch
mozilla66
All
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(10 attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
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.
Assignee: nobody → etoop
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.
Flags: needinfo?(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}?
Flags: needinfo?(esawin)
Turns out `Method.setAccessible(true)` was the missing link to calling private functions/fields using reflection.
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.
Pushed by etoop@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84081d548703
Part 1 - Add methods for each setting in `GeckoSessionSettings`. r=geckoview-reviewers,snorp,esawin
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 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
Depends on D13421
Pushed by etoop@mozilla.com:
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
Product: Firefox for Android → GeckoView
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`
Pushed by etoop@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18cb79bb1c78
Add methods for each setting in `GeckoSessionSettings`. r=snorp
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Flags: needinfo?(etoop)

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?

Flags: needinfo?(etoop)

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

Flags: needinfo?(etoop)
Pushed by etoop@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.