Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

Closes #2267 AC WebExtensions + WebCompat + FxAWebChannels #3498

Merged
merged 2 commits into from
Jun 23, 2020

Conversation

keianhzo
Copy link
Contributor

@keianhzo keianhzo commented Jun 12, 2020

Closes #2267 Closes #3339 This PR adds support for the following:

  • AC 44.0.0
  • AC Web Extensions bridge. We are using a ComponentsAdapter as a bridge between our SessionStore and AC's BrowserStore which is required by the AC WebExtensions/Features lifecycle.
  • Moves our current builtin extensions (YouTube and Vimeo) to AC WebExtensionsRuntime.
  • Adds support for the WebCompat extension.
  • The SessionStore is now the WebExtensionDelegate
  • Session life cycle code refactor to establish the SessionStore as the single point of communications with the BrowserStore.

There are some Glean related updates, mainly initialization changes and lint errors in .md files, could you please @daoshengmu verify that everything is working as expected?

@bluemarvin @MortimerGoro I've have replaced the appcompat aar for the maven version so it stays aligned with the rest of the androidx components. I couldn't find any speech related classes in that module.

QA:
This should affect mainly to the WebExtensions. Things that we should keep an eye on:

  • Youtube and Vimeo WebExtensions lifecycle (correct load of the extensions in restored session)
  • FxA login: This shouldn't change, we are now using WebChannels but the flow is still the same.
  • Would be also worth keeping an eye on sessions state: creation, recreation, shutdown and restore.

@keianhzo keianhzo self-assigned this Jun 12, 2020
@keianhzo keianhzo requested a review from bluemarvin June 12, 2020 13:17
@keianhzo
Copy link
Contributor Author

@pocmo As you were involved with the ComponentsAdapter implementation, could you please help review? Thanks!

@keianhzo keianhzo requested a review from daoshengmu June 12, 2020 13:24
@keianhzo keianhzo added the QA Attention QA label Jun 12, 2020
@keianhzo keianhzo force-pushed the v11/fxa_webchannels branch from 2a5ce50 to 2df98c8 Compare June 12, 2020 13:39
@daoshengmu
Copy link
Contributor

Please rebase your patch with master first. I was helping you to rebase it in local, but you were working on your own remote, so I can't help you give a commit.

After my investigation, your patch will skip calling VRBrowserApplication::onActivityCreate() when running unit tests. That actually breaks our unit test mechanism. I need do more investigation to understand why.

@daoshengmu
Copy link
Contributor

If we move back

        TelemetryWrapper.init(this);
         GleanMetricsService.init(this);

to onCreate(), the unit tests can be executed. In addition, Configuration in Glean seems to be unable to access directly when using Java.

@bluemarvin bluemarvin added this to the #11 polish milestone Jun 12, 2020
Copy link
Contributor

@daoshengmu daoshengmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a workable version to fix unit testing failures. (https://github.com/MozillaReality/FirefoxReality/tree/keianhzo_v11_fxa_webchannel_1)

There are two things I did. The first is rewriting GleanMetricService with Kotlin. The other is moving TelemetryWrapper.init(this); GleanMetricsService.INSTANCE.init(this); back to onCreate().

In addition, I need to give a default PingUploader to the Config

val config = Configuration(
                channel = BuildConfig.BUILD_TYPE,
                httpClient = ConceptFetchHttpUploader(lazy { HttpURLConnectionClient() }))

Your version will crash in GeckoEngine somehow. You can try to still use GleanMetricService.java first with proper way to initialize Configuration. If we still need to switch to *.kt because their API constraint. I am going to give a PR to land my code.

@daoshengmu
Copy link
Contributor

Btw, we are better not to use the default client uploader. We should use GeckoView client uploader instead. I can work on it at another PR.

#3339

@keianhzo keianhzo force-pushed the v11/fxa_webchannels branch from 2df98c8 to 5a145fe Compare June 15, 2020 10:06
@keianhzo
Copy link
Contributor Author

keianhzo commented Jun 15, 2020

@daoshengmu I've fixed the rebase issues.

BTW, we are better not to use the default client uploader. We should use GeckoView client uploader instead. I can work on it at another PR.

This PR already uses the GeckoWebExecutor in the Configuration but it seems that you reverted that in your branch.

The main difference between this PR and master is that it uses the GeckoWebExecutor in the Glean configuration which forces a GeckoRuntime initialization in the Application's onCreate while before we weren't creating the runtime until the Activity's onCreate.

That seems to break Gecko somehow as nothing is renderer anymore and it also breaks the tests with:

java.lang.IllegalArgumentException: Crash handler service must run in a separate process

As it seems that the getSystemService call that happens in the GeckoRuntime initialization is not returning the correct process for the Crash handler service. I guess the testing context is not loading the correct manifest information when using the Application context?

The weird thing is that the GeckoRuntime docs states:
Create a new runtime with default settings and attach it to the given context. Create will throw if there is already an active Gecko instance running, to prevent that, bind the runtime to the process lifetime instead of the activity lifetime. So it should work.

@keianhzo keianhzo force-pushed the v11/fxa_webchannels branch from ecd15b0 to 6b99664 Compare June 16, 2020 13:40
@keianhzo
Copy link
Contributor Author

keianhzo commented Jun 16, 2020

@daoshengmu I've fixed the GV issue but I still can see issues with test, especially thee issue with the crashservice process not being correctly determined from the manifest in test. I'll have look into that but if you have any ideas, they are welcomed.

@daoshengmu
Copy link
Contributor

@daoshengmu I've fixed the GV issue but I still can see issues with test, especially thee issue with the crashservice process not being correctly determined from the manifest in test. I'll have look into that but if you have any ideas, they are welcomed.

You can run ./gradlew app:testNoapiArm64DebugUnitTest to execute the test. Or just pick one testXXX from ./gradlew tasks.

So far, I notice there is no one instances VRBrowserActivity, but it do enter VRBrowserApplication::onCreate().

@keianhzo keianhzo force-pushed the v11/fxa_webchannels branch from 6b99664 to a8df13f Compare June 17, 2020 08:22
@keianhzo
Copy link
Contributor Author

@daoshengmu I have fixed the Telemetry tests. The GV team recommended to move the GeckoRuntime startup code to the Activity onCreate so I moved all the initialization there and initialized the Telemetry services for tests in a setup method. Seems to be working fine but I needed to add a flag to avoid the legacy LegacyTelemetry initialization.

@keianhzo keianhzo force-pushed the v11/fxa_webchannels branch 4 times, most recently from 0748a7f to 872472e Compare June 17, 2020 14:21
Copy link
Contributor

@daoshengmu daoshengmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can just do changes in tests instead of adding a flag in GleanMetricsServiceTest.

@keianhzo keianhzo force-pushed the v11/fxa_webchannels branch 2 times, most recently from 8cef319 to ebdf8b3 Compare June 18, 2020 09:30
@keianhzo keianhzo requested a review from daoshengmu June 18, 2020 09:56
@keianhzo keianhzo force-pushed the v11/fxa_webchannels branch from ebdf8b3 to 4e55050 Compare June 23, 2020 13:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Attention QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Gecko as HTTP client for Glean Migrate FxA implemetation to WebChannels instead of interceptor
4 participants