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

Closes #2267 AC WebExtensions + WebCompat + FxAWebChannels#3498

Merged
bluemarvin merged 2 commits into
MozillaReality:masterfrom
keianhzo:v11/fxa_webchannels
Jun 23, 2020
Merged

Closes #2267 AC WebExtensions + WebCompat + FxAWebChannels#3498
bluemarvin merged 2 commits into
MozillaReality:masterfrom
keianhzo:v11/fxa_webchannels

Conversation

@keianhzo

@keianhzo keianhzo commented Jun 12, 2020

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@daoshengmu daoshengmu left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

keianhzo commented Jun 15, 2020

Copy link
Copy Markdown
Contributor Author

@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

keianhzo commented Jun 16, 2020

Copy link
Copy Markdown
Contributor Author

@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.

Comment thread app/src/common/shared/org/mozilla/vrbrowser/browser/engine/Session.java Outdated
Comment thread app/src/common/shared/org/mozilla/vrbrowser/browser/engine/SessionStore.java Outdated
@daoshengmu

Copy link
Copy Markdown
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
Copy Markdown
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

@daoshengmu daoshengmu left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread app/src/test/java/org/mozilla/vrbrowser/GleanMetricsServiceTest.kt Outdated
Comment thread app/src/test/java/org/mozilla/vrbrowser/GleanMetricsServiceTest.kt
Comment thread app/src/common/shared/org/mozilla/vrbrowser/telemetry/GleanMetricsService.java Outdated
Comment thread app/src/test/java/org/mozilla/vrbrowser/GleanMetricsServiceTest.kt Outdated
Comment thread app/src/common/shared/org/mozilla/vrbrowser/VRBrowserApplication.java Outdated
Comment thread app/src/common/shared/org/mozilla/vrbrowser/browser/Services.kt Outdated
Comment thread app/src/common/shared/org/mozilla/vrbrowser/telemetry/GleanMetricsService.java Outdated
@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
Comment thread app/src/common/shared/org/mozilla/vrbrowser/browser/engine/Session.java Outdated
@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