-
Notifications
You must be signed in to change notification settings - Fork 1.3k
For #11660: added prefetch for topsites and update in onCreateView() #11668
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -142,6 +142,7 @@ open class FenixApplication : LocaleAwareApplication() { | |
} | ||
} | ||
|
||
prefetchForHomeFragment() | ||
setupLeakCanary() | ||
if (settings().isTelemetryEnabled) { | ||
components.analytics.metrics.start(MetricServiceType.Data) | ||
|
@@ -228,6 +229,13 @@ open class FenixApplication : LocaleAwareApplication() { | |
// no-op, LeakCanary is disabled by default | ||
} | ||
|
||
// This is for issue https://github.com/mozilla-mobile/fenix/issues/11660. We prefetch our info for startup | ||
// so that we're sure that we have all the data available as our fragment is launched. | ||
private fun prefetchForHomeFragment() { | ||
StrictMode.allowThreadDiskReads().resetPoliciesAfter { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this necessary? I don't see anywhere that would cause us to read on the main thread but perhaps I misunderstand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why, but our strictmodes complained when I didn't add it and the CI tests failed ( some of them). I could try to remove it |
||
components.core.topSiteStorage.prefetch() | ||
} | ||
} | ||
private fun setupPush() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: whitespace above |
||
// Sets the PushFeature as the singleton instance for push messages to go to. | ||
// We need the push feature setup here to deliver messages in the case where the service | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ import mozilla.components.feature.top.sites.TopSite | |
import mozilla.components.feature.top.sites.TopSiteStorage | ||
import mozilla.components.support.locale.LocaleManager | ||
import org.mozilla.fenix.R | ||
import org.mozilla.fenix.ext.observeOnce | ||
import org.mozilla.fenix.ext.settings | ||
import org.mozilla.fenix.settings.SupportUtils | ||
import org.mozilla.fenix.settings.advanced.getSelectedLocale | ||
|
@@ -86,4 +87,10 @@ class TopSiteStorage(private val context: Context) { | |
context.settings().defaultTopSitesAdded = true | ||
} | ||
} | ||
|
||
fun prefetch() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think we need to explain why we do this. We should probably only do so once in the code so maybe say to reference some other method (or the other explanation should reference here?) We should probably also explain why this is innocuous. |
||
getTopSites().observeOnce { | ||
cachedTopSites = it | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/* This Source Code Form is subject to the terms of the Mozilla Public | ||
* License, v. 2.0. If a copy of the MPL was not distributed with this | ||
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
||
package org.mozilla.fenix.ext | ||
|
||
import androidx.lifecycle.LiveData | ||
import androidx.lifecycle.Observer | ||
|
||
/** | ||
* Observe a LiveData once and unregister from it as soon as the live data returns a value | ||
*/ | ||
fun <T> LiveData<T>.observeOnce(observer: (T) -> Unit) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the name isn't very indicative. I could put There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this |
||
observeForever(object : Observer<T> { | ||
override fun onChanged(value: T) { | ||
removeObserver(this) | ||
observer(value) | ||
} | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -215,11 +215,10 @@ class HomeFragment : Fragment() { | |
sessionControlInteractor, | ||
homeViewModel | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: In the extended commit summary, I would have included information on why it's okay to remove this code (though I guess it's fine if it's just in the PR text? maybe you can link to what's in the PR). |
||
) | ||
activity.themeManager.applyStatusBarTheme(activity) | ||
|
||
view.consumeFrom(homeFragmentStore, viewLifecycleOwner) { | ||
sessionControlView?.update(it) | ||
} | ||
updateSessionControlView(view) | ||
|
||
activity.themeManager.applyStatusBarTheme(activity) | ||
|
||
view.consumeFrom(requireComponents.core.store, viewLifecycleOwner) { | ||
val tabCount = if (currentMode.getCurrentMode() == Mode.Normal) { | ||
|
@@ -234,6 +233,20 @@ class HomeFragment : Fragment() { | |
return view | ||
} | ||
|
||
/** | ||
* The [SessionControlView] is forced to update with our current state when we call | ||
* [HomeFragment.onCreateView] in order to be able to draw everything at once with the current | ||
* data in our store. The [View.consumeFrom] coroutine dispatch | ||
* doesn't get run right away which means that we won't draw on the first layout pass. | ||
*/ | ||
fun updateSessionControlView(view: View) { | ||
sessionControlView?.update(homeFragmentStore.state) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I like the inner function version I suggested because it makes it clear that the code is repeated and prevents someone from missing that if they decide to refactor this. But I can go either way |
||
|
||
view.consumeFrom(homeFragmentStore, viewLifecycleOwner) { | ||
sessionControlView?.update(it) | ||
} | ||
} | ||
|
||
private fun updateLayout(view: View) { | ||
val shouldUseBottomToolbar = view.context.settings().shouldUseBottomToolbar | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's confusing that we say, "our info" but really mean "top sites" so I'd rather say "We prefetch our top sites for startup" and it can be changed if the code becomes more general later.