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 #929: Use browser-session component for browser state. #982

Merged
merged 1 commit into from Jul 20, 2018

Conversation

Projects
None yet
2 participants
@pocmo
Contributor

pocmo commented Jul 12, 2018

Closes #929: This PR removes the custom session code from the app and replaces it with the browser-session component.

There are three missing pieces in the component (webview state, session source, blocking state) that I added via property extensions for now. I created upstream issues to get that into the component and once that has happened we can remove the extensions here.

For now this is almost a 1:1 replacement. In further iterations we can start to clean some things up. I want to look into how we can make usage of the component for "single-session" users a bit easier.

I did test this on a Fire TV 4k stick a bit. I seems to work fine. However as this touches a lot of areas I think this should get good test coverage by QA too.

@pocmo

This comment has been minimized.

Show comment
Hide comment
@pocmo

pocmo Jul 12, 2018

Contributor

Codecov: Unfortunately the coverage went down because there were some tests for the removed code. On the positive side the components have a good/better coverage. :)

Sputnik: That's something we saw in other repos too: If we remove files then sputnik fails because it cannot look at those files (obviously). This seems to be a sputnik bug.

Contributor

pocmo commented Jul 12, 2018

Codecov: Unfortunately the coverage went down because there were some tests for the removed code. On the positive side the components have a good/better coverage. :)

Sputnik: That's something we saw in other repos too: If we remove files then sputnik fails because it cannot look at those files (obviously). This seems to be a sputnik bug.

@pocmo

This comment has been minimized.

Show comment
Hide comment
@pocmo

pocmo Jul 12, 2018

Contributor

Sputnik: That's something we saw in other repos too: If we remove files then sputnik fails because it cannot look at those files (obviously). This seems to be a sputnik bug.

Filed an issue for that: TouK/sputnik-ci#23

Contributor

pocmo commented Jul 12, 2018

Sputnik: That's something we saw in other repos too: If we remove files then sputnik fails because it cannot look at those files (obviously). This seems to be a sputnik bug.

Filed an issue for that: TouK/sputnik-ci#23

@mcomella

This comment has been minimized.

Show comment
Hide comment
@mcomella

mcomella Jul 12, 2018

Contributor

I'm trying to land some MediaSession stuff which needs to get uplifted to v2.2 so I'm going to review that after the core work is done (which should be today or tomorrow).

Contributor

mcomella commented Jul 12, 2018

I'm trying to land some MediaSession stuff which needs to get uplifted to v2.2 so I'm going to review that after the core work is done (which should be today or tomorrow).

@mcomella

Looks pretty good but there are a few areas I'm concerned about that I think we should discuss the best approach on. The detailed comments are inline but to highlight these areas:

  • Components as a member of ApplicationContext
  • Can we have multiple sessions? We should avoid changing logic around that interaction if it's possible
  • The session observer callbacks not passing in a reference to the changed values
  • How lifecycle owners work for Fragments with view hierarchies

This is a long PR and GH doesn't have interdiff so I'd prefer if you can make your changes as separate commits and squash at the end!

@pocmo

This comment has been minimized.

Show comment
Hide comment
@pocmo

pocmo Jul 17, 2018

Contributor

@mcomella Updated the PR and added a separate commit to address your review comments.

To address your review summary:

Components as a member of ApplicationContext

As mentioned in the comments: Components will need application context going forward. Especially for #959 and #960. We use a similar concept in our sample browser:
https://github.com/mozilla-mobile/android-components/blob/master/samples/browser/src/main/java/org/mozilla/samples/browser/Components.kt#L28

Can we have multiple sessions? We should avoid changing logic around that interaction if it's possible

Yes, I changed everything back to handle multiple sessions. Let's address that separately. We are also looking into making single session use cases easier in the components.

The session observer callbacks not passing in a reference to the changed values

As mentioned above: I had the same feeling when using the component here and a patch for that already landed in the components repository.

How lifecycle owners work for Fragments with view hierarchies

This was a 1:1 conversion from the previous code where the LiveData observer was bound to the fragment too. I updated this code to bind the observer to the view instead of the Fragment.

Contributor

pocmo commented Jul 17, 2018

@mcomella Updated the PR and added a separate commit to address your review comments.

To address your review summary:

Components as a member of ApplicationContext

As mentioned in the comments: Components will need application context going forward. Especially for #959 and #960. We use a similar concept in our sample browser:
https://github.com/mozilla-mobile/android-components/blob/master/samples/browser/src/main/java/org/mozilla/samples/browser/Components.kt#L28

Can we have multiple sessions? We should avoid changing logic around that interaction if it's possible

Yes, I changed everything back to handle multiple sessions. Let's address that separately. We are also looking into making single session use cases easier in the components.

The session observer callbacks not passing in a reference to the changed values

As mentioned above: I had the same feeling when using the component here and a patch for that already landed in the components repository.

How lifecycle owners work for Fragments with view hierarchies

This was a 1:1 conversion from the previous code where the LiveData observer was bound to the fragment too. I updated this code to bind the observer to the view instead of the Fragment.

@pocmo

This comment has been minimized.

Show comment
Hide comment
@pocmo

pocmo Jul 18, 2018

Contributor

@mcomella Updated the PR (added the comment to FocusApplication)!

Contributor

pocmo commented Jul 18, 2018

@mcomella Updated the PR (added the comment to FocusApplication)!

@pocmo

This comment has been minimized.

Show comment
Hide comment
@pocmo

pocmo Jul 18, 2018

Contributor

... and rebased the PR. Let me know if there are more things. Otherwise I'll squash the commits.

Contributor

pocmo commented Jul 18, 2018

... and rebased the PR. Let me know if there are more things. Otherwise I'll squash the commits.

@mcomella

This comment has been minimized.

Show comment
Hide comment
@mcomella

mcomella Jul 19, 2018

Contributor

Sorry, I forgot to get to this today: I'll get to it tomorrow.

Contributor

mcomella commented Jul 19, 2018

Sorry, I forgot to get to this today: I'll get to it tomorrow.

@mcomella

lgtm w/ comments addressed. Thanks!

Don't forget to squash! :)

@pocmo

This comment has been minimized.

Show comment
Hide comment
@pocmo

pocmo Jul 20, 2018

Contributor

@mcomella Adressed your last comments. Updated and squashed the commits. Rebased. And just tested again a build of that a bit on a Fire TV 4k stick. I think that's ready to merge now.

Contributor

pocmo commented Jul 20, 2018

@mcomella Adressed your last comments. Updated and squashed the commits. Rebased. And just tested again a build of that a bit on a Fire TV 4k stick. I think that's ready to merge now.

@pocmo

This comment has been minimized.

Show comment
Hide comment
@pocmo

pocmo Jul 20, 2018

Contributor

Taskcluster and travis seem to agree. Landing!

Contributor

pocmo commented Jul 20, 2018

Taskcluster and travis seem to agree. Landing!

@pocmo pocmo merged commit f627a6d into mozilla-mobile:master Jul 20, 2018

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
codecov/project Absolute coverage decreased by -2.16% but relative coverage increased by +12.56% compared to 0fda8e2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pocmo pocmo deleted the pocmo:session branch Jul 20, 2018

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