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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wire up the history tracking delegate for GeckoView #1238

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@linacambridge
Contributor

linacambridge commented Oct 31, 2018

I tested this with my local GeckoView build, the sample browser, and @grigoryk's in-memory history storage, and it works! 馃帀 There's also a commit that points the Nightly x86 target to m-c instead, for folks to try out if they'd like. 馃槃

@pocmo

This comment has been minimized.

Contributor

pocmo commented Oct 31, 2018

Woo! Awesome. :)

If this is in mozilla-central already... will updating nightly_version be enough to get that landed (not sure if there are other breaking API changes since the last update)? :)

@pocmo

This comment has been minimized.

Contributor

pocmo commented Nov 16, 2018

@linacambridge linacambridge force-pushed the linacambridge:gv-history branch 2 times, most recently from 34d3fa5 to 163924a Nov 20, 2018

@codecov

This comment has been minimized.

codecov bot commented Nov 20, 2018

Codecov Report

鉂楋笍 No coverage uploaded for pull request base (master@2475162). Click here to learn what that means.
The diff coverage is 61.53%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1238   +/-   ##
=========================================
  Coverage          ?   82.46%           
  Complexity        ?     1632           
=========================================
  Files             ?      197           
  Lines             ?     6508           
  Branches          ?     1123           
=========================================
  Hits              ?     5367           
  Misses            ?      637           
  Partials          ?      504
Impacted Files Coverage 螖 Complexity 螖
...mponents/feature/storage/HistoryTrackingFeature.kt 90% <酶> (酶) 1 <0> (?)
...mponents/browser/engine/system/SystemEngineView.kt 78.53% <0%> (酶) 31 <0> (?)
...nents/browser/storage/sync/PlacesHistoryStorage.kt 82.85% <0%> (酶) 12 <0> (?)
...s/browser/storage/memory/InMemoryHistoryStorage.kt 88.88% <88.88%> (酶) 27 <5> (?)

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 2475162...ff39a88. Read the comment docs.

@linacambridge

This comment has been minimized.

Contributor

linacambridge commented Nov 20, 2018

@mozilla-mobile/act This is ready for re-review, now that the GV API has landed! Please take a look at how I'm using scope.launch in particular, as I'm not sure that I'm doing it right.

From talking to @grigoryk today, it sounds like this is fine for now, but we'll want to agree on a story for scopes eventually (#1347). @thomcc might have insights here, too.

@grigoryk

As noted before, we're likely going to change the scoping story, but that's fine for now. This can land with a few changes.

For bonus points, I think you should be able to add tests for this. See GeckoEngineSessionTest

@linacambridge linacambridge force-pushed the linacambridge:gv-history branch 5 times, most recently from 4b821fa to 86374e1 Nov 29, 2018

@linacambridge

This comment has been minimized.

Contributor

linacambridge commented Nov 29, 2018

@grigoryk Whew! I think this is ready for re-review. Changes since last time:

  • Removed the privateMode argument from HistoryTrackingDelegate. Instead, we'll skip the call to the delegate entirely.
  • Bail early instead of launching a coroutine if a delegate isn't set.
  • Tests actually work! 馃帀
  • Made both overloads of getVisited suspend functions.
@linacambridge

This comment has been minimized.

Contributor

linacambridge commented Nov 29, 2018

Looks like the browser-awesomebar failure is #1300; I didn't see it fail on previous runs.

@grigoryk

A few small nits, but this look good to me! Thanks!

@linacambridge linacambridge force-pushed the linacambridge:gv-history branch from 86374e1 to 4461ef4 Nov 29, 2018

@linacambridge linacambridge force-pushed the linacambridge:gv-history branch from 4461ef4 to 8f7280d Nov 29, 2018

@grigoryk

This comment has been minimized.

Contributor

grigoryk commented Nov 30, 2018

This landed!

@grigoryk grigoryk closed this Nov 30, 2018

@linacambridge linacambridge deleted the linacambridge:gv-history branch Nov 30, 2018

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