Refactor GeckoSessionSettings to provide dedicated methods for settings

RESOLVED FIXED in Firefox 66

Status

enhancement
P3
normal
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: esawin, Assigned: fluffyemily)

Tracking

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

51 Branch
mozilla66
All
Android

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
(Reporter)

Description

7 months ago
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)

Updated

6 months ago
Assignee: nobody → etoop
(Assignee)

Comment 1

6 months ago
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)
(Reporter)

Comment 2

6 months ago
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)
(Assignee)

Comment 3

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

Comment 4

6 months ago
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

6 months ago
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
(Assignee)

Comment 7

6 months ago
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.
(Assignee)

Comment 10

6 months ago
* 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
(Assignee)

Comment 12

6 months ago
Depends on D13421

Comment 14

5 months ago
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

Updated

5 months ago
Product: Firefox for Android → GeckoView
(Assignee)

Comment 16

5 months ago
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

4 months ago
Pushed by etoop@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18cb79bb1c78
Add methods for each setting in `GeckoSessionSettings`. r=snorp

Comment 18

4 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
(Assignee)

Updated

4 months ago
Flags: needinfo?(etoop)

Comment 19

4 months ago

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)
(Assignee)

Comment 20

4 months ago

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)

Comment 22

4 months ago
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.