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

Add experimental browser-engine-servo component. #1003

Merged
merged 3 commits into from Oct 11, 2018

Conversation

Projects
None yet
5 participants
@pocmo
Contributor

pocmo commented Oct 8, 2018

Closes #1002.

@pocmo pocmo requested a review from mozilla-mobile/act as a code owner Oct 8, 2018

@codecov

This comment has been minimized.

codecov bot commented Oct 8, 2018

Codecov Report

Merging #1003 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1003      +/-   ##
============================================
+ Coverage     79.92%   79.98%   +0.05%     
  Complexity     1204     1204              
============================================
  Files           165      164       -1     
  Lines          4529     4526       -3     
  Branches        658      658              
============================================
  Hits           3620     3620              
+ Misses          606      603       -3     
  Partials        303      303
Impacted Files Coverage Δ Complexity Δ
...la/components/support/ktx/android/os/StrictMode.kt

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce54644...e498295. Read the comment docs.

@pocmo

This comment has been minimized.

Contributor

pocmo commented Oct 8, 2018

WIP: Currently only renders a white screen and I do not know why yet. :)

I never added the actual ServoView to the ServoEngineView container 😅

@pocmo pocmo force-pushed the pocmo:browser-engine-servo branch 2 times, most recently from d6854fb to d29cb8a Oct 8, 2018

@pocmo pocmo changed the title from WIP: Add experimental browser-engine-servo component. to Add experimental browser-engine-servo component. Oct 8, 2018

@pocmo pocmo force-pushed the pocmo:browser-engine-servo branch 2 times, most recently from 80eacff to c48150f Oct 8, 2018

@pocmo

This comment has been minimized.

Contributor

pocmo commented Oct 9, 2018

Some updates for follow-up issues:

  • There's now a maven repository: https://download.servo.org/nightly/maven
  • They are working on a View/Session split. With that we do not need this wonky WebView-like workaround anymore.
  • There's an x86 build now too.

@pocmo pocmo requested a review from csadilek Oct 10, 2018

@csadilek

Found two small things, otherwise 🚢 :)

Show resolved Hide resolved components/browser/engine-servo/build.gradle Outdated
}
override fun loadUrl(url: String) {
val view = view

This comment has been minimized.

@csadilek

csadilek Oct 10, 2018

Contributor

This seems like an unneeded assignment?

This comment has been minimized.

@pocmo

pocmo Oct 11, 2018

Contributor

The view property is nullable and with the local reference I circumvent the issue that it may change after the null check. Usually we do this with view?.let {} but I need an else branch here as well.

This comment has been minimized.

@csadilek

csadilek Oct 11, 2018

Contributor

heh, yes, that's how we usually end up with view?.let {} ?: (run) {}. Still on the fence what's easier to read...

@@ -53,6 +53,11 @@
name: browser-engine-gecko-nightly
old_artifact_id: engine-gecko-nightly
new_group_id: org.mozilla.components
public/build/browser.engine-servo.maven.zip:

This comment has been minimized.

@MihaiTabara

MihaiTabara Oct 11, 2018

Contributor

👍

@pocmo pocmo force-pushed the pocmo:browser-engine-servo branch from c48150f to 4345f75 Oct 11, 2018

@pocmo pocmo force-pushed the pocmo:browser-engine-servo branch from 4345f75 to 2487e6d Oct 11, 2018

@@ -0,0 +1,21 @@
# [Android Components](../../../README.md) > Browser > Engine-Servo
[*Engine*](../../concept/engine/README.md) implementation based on the [Servo Browser Engine](https://servo.org/).

This comment has been minimized.

@Amejia481

Amejia481 Oct 11, 2018

Contributor

Should we add some basic docs with the new component?

This comment has been minimized.

@pocmo

pocmo Oct 11, 2018

Contributor

Yeah, let's wait a bit. The Servo team is working an actual View/Session split so this may change some things here and there. For now no one is probably seriously going to use this component. :)

}
override fun loadUrl(url: String) {
val view = view

This comment has been minimized.

@csadilek

csadilek Oct 11, 2018

Contributor

heh, yes, that's how we usually end up with view?.let {} ?: (run) {}. Still on the fence what's easier to read...

@pocmo pocmo merged commit f08921d into mozilla-mobile:master Oct 11, 2018

1 of 3 checks passed

Taskcluster (pull_request) TaskGroup: failure
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
WIP ready for review
Details

@pocmo pocmo deleted the pocmo:browser-engine-servo branch Oct 11, 2018

@pocmo

This comment has been minimized.

Contributor

pocmo commented Oct 11, 2018

heh, yes, that's how we usually end up with view?.let {} ?: (run) {}. Still on the fence what's easier to read...

It definitely took me a while to understand what you did in that line in the other PR :)

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