New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #1039: Use browser-engine-system and feature-session components to replace WebView abstraction layer.. #1044

Merged
merged 1 commit into from Sep 5, 2018

Conversation

Projects
None yet
3 participants
@pocmo
Contributor

pocmo commented Aug 29, 2018

@mcomella @Baron-Severin I want to share the current state of the migration to use browser-engine. This version is functional and you can use it to browse the web. Like with the browser-session integration I used extension functions to work around functionality that is not available in the upstream components.

Missing pieces without a workaround in this version (list may be incomplete):

  • Tracking protection toggle not functional (TP is already in components 0.20, with 0.21 [Friday] we get the option to configure this globally for an engine or session and that will make this toggle simpler to implement.
  • We can't intercept URLs to inject HTML currently (used for firefox:home, firefox:about and error pages). This functionality is scheduled to land in 0.22 (next week). However I may move this into the current sprint so that we can have this in 0.21 on Friday. We moved this into the 0.21 sprint and the functionality should be available in 0.21.
    • Without loading "firefox:home" and adding it to the tab's history this currently breaks some assumptions when using the back button. This should fix itself once "firefox:home" gets loaded normally.
  • Fullscreen API (works with YouTube because the TV website doesn't use the web API and just shows the content at full size). Scheduled to land in 0.21 on Friday.

Added follow-up items to: #1063


As a side effect:

  • Replaces AmazonWebView with WebView (Issue #379)
  • Removes the product flavor dimensions that are not needed anymore.

@pocmo pocmo requested a review from mozilla-mobile/firetv-codeowners as a code owner Aug 29, 2018

@pocmo pocmo changed the title from Issue #1039: Use browser-engine-system and feature-session components to replace WebView abstraction layer. to WIP: Issue #1039: Use browser-engine-system and feature-session components to replace WebView abstraction layer. Aug 29, 2018

@pocmo pocmo changed the title from WIP: Issue #1039: Use browser-engine-system and feature-session components to replace WebView abstraction layer. to WIP: Issue #1039: Use browser-engine-system and feature-session components to replace WebView abstraction layer.. Aug 29, 2018

@Baron-Severin Baron-Severin added the WIP label Aug 29, 2018

@mcomella

With a quick skim, this looks reasonable to me. We can spend more time reviewing when we land for real (next week?). Note: I'll be PTO next week so I'll be unable to look over this until the following Monday.

image

You win Fire TV!

@mcomella

This comment has been minimized.

Show comment
Hide comment
@mcomella

mcomella Aug 30, 2018

Contributor

Note: I'll be PTO next week so I'll be unable to look over this until the following Monday.

Unless we can land something tomorrow. :)

Contributor

mcomella commented Aug 30, 2018

Note: I'll be PTO next week so I'll be unable to look over this until the following Monday.

Unless we can land something tomorrow. :)

@mcomella

This comment has been minimized.

Show comment
Hide comment
@mcomella

mcomella Aug 30, 2018

Contributor

Alternatively...

@pocmo How much is changing for the final PR?

Provided the changes aren't too big, I can look over the most recent WIP tomorrow, Severin can look at it next week and land it, and I can look at it to make sure we didn't miss anything when I come back. That way we can get it in sooner and test earlier. What do you think?

Contributor

mcomella commented Aug 30, 2018

Alternatively...

@pocmo How much is changing for the final PR?

Provided the changes aren't too big, I can look over the most recent WIP tomorrow, Severin can look at it next week and land it, and I can look at it to make sure we didn't miss anything when I come back. That way we can get it in sooner and test earlier. What do you think?

@pocmo

This comment has been minimized.

Show comment
Hide comment
@pocmo

pocmo Aug 31, 2018

Contributor

@pocmo How much is changing for the final PR?

For this iteration not much.

Components 0.21 should get released today and then with that I should be able to look at the three missing pieces mentioned above. The patches for tracking protection and URL interception landed in a-c. Fullscreen API is at risk for 0.21 because the PR is not up yet. We may need to ship that with 0.22 or can ship a 0.21.1 if this gets in early next week.

Provided the changes aren't too big, I can look over the most recent WIP tomorrow, Severin can look at it next week and land it, and I can look at it to make sure we didn't miss anything when I come back.

Depending on what you and @Baron-Severin think I'd be open to land this as-is (with review comments addressed) and open separate PRs for the next pieces. The idea is to unblock others by having to wait on a big code drop and to be able to have others test and iterate on the new code. The drawback is that we temporarily have a not 100% feature complete app though (-> at least the three points mentioned above). Apart from that the app seems to be "fully" functional in my tests.

Contributor

pocmo commented Aug 31, 2018

@pocmo How much is changing for the final PR?

For this iteration not much.

Components 0.21 should get released today and then with that I should be able to look at the three missing pieces mentioned above. The patches for tracking protection and URL interception landed in a-c. Fullscreen API is at risk for 0.21 because the PR is not up yet. We may need to ship that with 0.22 or can ship a 0.21.1 if this gets in early next week.

Provided the changes aren't too big, I can look over the most recent WIP tomorrow, Severin can look at it next week and land it, and I can look at it to make sure we didn't miss anything when I come back.

Depending on what you and @Baron-Severin think I'd be open to land this as-is (with review comments addressed) and open separate PRs for the next pieces. The idea is to unblock others by having to wait on a big code drop and to be able to have others test and iterate on the new code. The drawback is that we temporarily have a not 100% feature complete app though (-> at least the three points mentioned above). Apart from that the app seems to be "fully" functional in my tests.

@pocmo

This comment has been minimized.

Show comment
Hide comment
@pocmo

pocmo Aug 31, 2018

Contributor

Updated the PR. Made all checks pass (ktlint, detekt, checkstyle, lint), SessionUseCases initializes lazily now and cleaned up some of the code.

Contributor

pocmo commented Aug 31, 2018

Updated the PR. Made all checks pass (ktlint, detekt, checkstyle, lint), SessionUseCases initializes lazily now and cleaned up some of the code.

@mcomella

A few things missing here or there and a few nits but otherwise lgtm. I find the SessionUseCases abstraction strange: see below.

Some other things that have changed – please file a bug or fix:

  • Back/forward state takes a while to update

Not sure it was related to your changes but I got into a state where a white page was loaded and the progress bar was shown without a URL (i.e. empty str or null) after going backwards and forwards a few times.

I'd be open to land this as-is (with review comments addressed) and open separate PRs for the next pieces.

Me too: please send an email to the team letting them know 1) this is landing, 2) the features that need follow-ups, and 3) that they'll need to change their build variant. Also, don't forget to file issues to track this work! :)

@Baron-Severin FYI that this may land.

@mcomella

This comment has been minimized.

Show comment
Hide comment
@mcomella

mcomella Sep 1, 2018

Contributor

fwiw, I am a little concerned that we're rushing this and should maybe wait until the next release. We merged the home screen into the navigation overlay, which isn't stable yet, and we're landing this complex refactor (whose components haven't been used in a production app before) into that. For example, there are regressions in back/forward state that I couldn't reproduce consistently and I'm not sure who is to blame; debugging may get complicated when there's so much changing at once!

That being said, we can't expect this to land without bugs, the components are well tested in isolation, and it's pretty important to get the components tested in production sooner rather than later so we can add them to our other projects.

Perhaps we should land but not hesitate to back it out if it causes problems? What do you think @pocmo ? If so, let's ask QA to start testing this as soon as it lands. Note that Chenxia, Severin, and I won't be working on this next week so we'll have fewer people around if something goes wrong and after that we only have two weeks to iron out any issues.

@liuche @Baron-Severin Please share if you have any thoughts!

Contributor

mcomella commented Sep 1, 2018

fwiw, I am a little concerned that we're rushing this and should maybe wait until the next release. We merged the home screen into the navigation overlay, which isn't stable yet, and we're landing this complex refactor (whose components haven't been used in a production app before) into that. For example, there are regressions in back/forward state that I couldn't reproduce consistently and I'm not sure who is to blame; debugging may get complicated when there's so much changing at once!

That being said, we can't expect this to land without bugs, the components are well tested in isolation, and it's pretty important to get the components tested in production sooner rather than later so we can add them to our other projects.

Perhaps we should land but not hesitate to back it out if it causes problems? What do you think @pocmo ? If so, let's ask QA to start testing this as soon as it lands. Note that Chenxia, Severin, and I won't be working on this next week so we'll have fewer people around if something goes wrong and after that we only have two weeks to iron out any issues.

@liuche @Baron-Severin Please share if you have any thoughts!

@pocmo

This comment has been minimized.

Show comment
Hide comment
@pocmo

pocmo Sep 3, 2018

Contributor

From the migration I feel like the overlay and the things touching the engine are pretty well separated. We are definitely introducing risk with the migration but it doesn't feel like it gets worse by having both things (engine migration + overlay migration) happen at the same time.

whose components haven't been used in a production app before

Yeah, that's a chicken egg problem. FFFTV is the first candidate because the feature set is the smallest and the first milestone we have reached. :)

there are regressions in back/forward state that I couldn't reproduce consistently

I noticed that there is a regression because we can't intercept "firefox:home" and inject custom content. With that the page is not in the history stack and breaks some of the logic. The functionality for that landed in 0.21 - I added an item about testing back/forward after that to #1063.

Perhaps we should land but not hesitate to back it out if it causes problems? What do you think @pocmo ?

Yeah, that sounds good to me. It will get more complicated to back out later on after we added more commits on top of it though.

I'll talk to QA once this lands. :)

Contributor

pocmo commented Sep 3, 2018

From the migration I feel like the overlay and the things touching the engine are pretty well separated. We are definitely introducing risk with the migration but it doesn't feel like it gets worse by having both things (engine migration + overlay migration) happen at the same time.

whose components haven't been used in a production app before

Yeah, that's a chicken egg problem. FFFTV is the first candidate because the feature set is the smallest and the first milestone we have reached. :)

there are regressions in back/forward state that I couldn't reproduce consistently

I noticed that there is a regression because we can't intercept "firefox:home" and inject custom content. With that the page is not in the history stack and breaks some of the logic. The functionality for that landed in 0.21 - I added an item about testing back/forward after that to #1063.

Perhaps we should land but not hesitate to back it out if it causes problems? What do you think @pocmo ?

Yeah, that sounds good to me. It will get more complicated to back out later on after we added more commits on top of it though.

I'll talk to QA once this lands. :)

@pocmo pocmo changed the title from WIP: Issue #1039: Use browser-engine-system and feature-session components to replace WebView abstraction layer.. to Issue #1039: Use browser-engine-system and feature-session components to replace WebView abstraction layer.. Sep 3, 2018

@pocmo pocmo removed the WIP label Sep 3, 2018

@pocmo pocmo requested a review from Baron-Severin Sep 3, 2018

@pocmo

This comment has been minimized.

Show comment
Hide comment
@pocmo

pocmo Sep 3, 2018

Contributor

@Baron-Severin From my side this is ready to land. I addressed most of @mcomella's review comments, rebased the branch and added follow-up items to #1063. I started working on the follow-up items now (using 0.21 of the components) but I'll create separate PRs for those changes once this PR lands.

Contributor

pocmo commented Sep 3, 2018

@Baron-Severin From my side this is ready to land. I addressed most of @mcomella's review comments, rebased the branch and added follow-up items to #1063. I started working on the follow-up items now (using 0.21 of the components) but I'll create separate PRs for those changes once this PR lands.

@Baron-Severin

Had a few questions, nothing major. LGTM

@@ -13,7 +13,7 @@ android {
compileSdkVersion 27
defaultConfig {
applicationId "org.mozilla.tv"
applicationId "org.mozilla.tv.firefox"

This comment has been minimized.

@Baron-Severin

Baron-Severin Sep 4, 2018

Contributor

Nit: Why are we changing the app ID? Is there an issue that explains this?

@Baron-Severin

Baron-Severin Sep 4, 2018

Contributor

Nit: Why are we changing the app ID? Is there an issue that explains this?

This comment has been minimized.

@pocmo

pocmo Sep 5, 2018

Contributor

Previously we had product flavors for the engine and those flavors added a suffix:

roductFlavors {
        amazonWebview {
            applicationIdSuffix ".firefox"
            // [..]
        }
        gecko {
            applicationIdSuffix ".gecko"
            // [..]
        }
    }

We have been shipping the amazonWebview flavor only. Now that the flavor is gone I moved the suffix to the applicationId setup directly.

@pocmo

pocmo Sep 5, 2018

Contributor

Previously we had product flavors for the engine and those flavors added a suffix:

roductFlavors {
        amazonWebview {
            applicationIdSuffix ".firefox"
            // [..]
        }
        gecko {
            applicationIdSuffix ".gecko"
            // [..]
        }
    }

We have been shipping the amazonWebview flavor only. Now that the flavor is gone I moved the suffix to the applicationId setup directly.

private fun initAmazonFactory() {
if (!isAmazonFactoryInit) {
factory = AmazonWebKitFactories.getDefaultFactory()
if (factory!!.isRenderProcess(this)) {

This comment has been minimized.

@Baron-Severin

Baron-Severin Sep 4, 2018

Contributor

I love how much code this PR deletes.

@Baron-Severin

Baron-Severin Sep 4, 2018

Contributor

I love how much code this PR deletes.

Show outdated Hide outdated app/src/main/java/org/mozilla/focus/browser/InfoActivity.kt
Issue #1039: Use browser-engine-system and feature-session components…
… to replace WebView abstraction layer.

As a side effect:
 * Replaces AmazonWebView with WebView (Issue #379)
 * Removes the product flavor dimensions that are not needed anymore.

@pocmo pocmo merged commit 996999e into mozilla-mobile:master Sep 5, 2018

2 of 3 checks passed

codecov/project 30.3% (-4.7%) compared to bcc0bd4
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pocmo pocmo deleted the pocmo:engine-integration branch Sep 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment