[Merge] lp:~osomon/webbrowser-app/remove-formFactor into lp:webbrowser-app
Olivier Tilloy
olivier.tilloy at canonical.com
Thu Feb 18 15:38:14 UTC 2016
Thanks for your thorough review, I’ve replied to your questions (with more questions!) inline.
Diff comments:
>
> === added file 'src/app/meminfo.cpp'
> --- src/app/meminfo.cpp 1970-01-01 00:00:00 +0000
> +++ src/app/meminfo.cpp 2016-02-10 14:13:13 +0000
> @@ -0,0 +1,137 @@
> +/*
> + * Copyright 2016 Canonical Ltd.
> + *
> + * This file is part of webbrowser-app.
> + *
> + * webbrowser-app is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 3.
> + *
> + * webbrowser-app is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "meminfo.h"
> +
> +// Qt
> +#include <QtCore/QByteArray>
> +#include <QtCore/QFile>
> +#include <QtCore/QRegExp>
> +#include <QtCore/QString>
> +#include <QtCore/QtGlobal>
> +
> +MemInfo::MemInfo(QObject* parent)
> + : QObject(parent)
> + , m_total(0)
> + , m_free(0)
> +{
> + // Default interval: 5000 ms
> + m_timer.setInterval(5000);
> + connect(&m_timer, SIGNAL(timeout()), SLOT(update()));
> + // Active by default
> + m_timer.start();
> +}
> +
> +MemInfo::~MemInfo()
> +{}
> +
> +const bool MemInfo::active() const
> +{
> + return m_timer.isActive();
> +}
> +
> +void MemInfo::setActive(bool active)
> +{
> + if (active != m_timer.isActive()) {
> + if (active) {
> + m_timer.start();
> + } else {
> + m_timer.stop();
> + }
> + Q_EMIT activeChanged();
> + }
> +}
> +
> +const int MemInfo::interval() const
> +{
> + return m_timer.interval();
> +}
> +
> +void MemInfo::setInterval(int interval)
> +{
> + if (interval != m_timer.interval()) {
> + m_timer.setInterval(interval);
No need for it. The implementation of QTimer::setInterval() kills the existing timer and starts a new one if it was already running.
> + Q_EMIT intervalChanged();
> + }
> +}
> +
> +const int MemInfo::total() const
> +{
> + return m_total;
> +}
> +
> +const int MemInfo::free() const
> +{
> + return m_free;
> +}
> +
> +void MemInfo::update()
> +{
> +#if defined(Q_OS_LINUX)
> + // Inspired by glibtop_get_mem_s()
> + QFile meminfo(QStringLiteral("/proc/meminfo"));
> + if (!meminfo.open(QIODevice::ReadOnly)) {
> + return;
> + }
> + static QRegExp memTotalRegexp(QStringLiteral("MemTotal:\\s*(\\d+) kB\\n"));
> + static QRegExp memFreeRegexp(QStringLiteral("MemFree:\\s*(\\d+) kB\\n"));
> + static QRegExp buffersRegexp(QStringLiteral("Buffers:\\s*(\\d+) kB\\n"));
> + static QRegExp cachedRegexp(QStringLiteral("Cached:\\s*(\\d+) kB\\n"));
> + int parsedTotal = -1;
> + int parsedFree = -1;
> + int parsedBuffers = -1;
> + int parsedCached = -1;
> + while ((parsedTotal == -1) || (parsedFree == -1) ||
> + (parsedBuffers == -1) || (parsedCached == -1)) {
> + QByteArray line = meminfo.readLine();
> + if (line.isEmpty()) {
> + break;
> + }
> + if (memTotalRegexp.exactMatch(line)) {
> + parsedTotal = memTotalRegexp.cap(1).toInt();
> + } else if (memFreeRegexp.exactMatch(line)) {
> + parsedFree = memFreeRegexp.cap(1).toInt();
> + } else if (buffersRegexp.exactMatch(line)) {
> + parsedBuffers = buffersRegexp.cap(1).toInt();
> + } else if (cachedRegexp.exactMatch(line)) {
> + parsedCached = cachedRegexp.cap(1).toInt();
> + }
> + }
> + meminfo.close();
> + if ((parsedTotal != -1) && (parsedFree != -1) &&
> + (parsedBuffers != -1) && (parsedCached != -1)) {
> + bool totalUpdated = false;
> + if (parsedTotal != m_total) {
> + m_total = parsedTotal;
> + totalUpdated = true;
> + }
> + bool freeUpdated = false;
> + int newFree = parsedFree + parsedCached + parsedBuffers;
> + if (newFree != m_free) {
> + m_free = newFree;
> + freeUpdated = true;
> + }
> + if (totalUpdated) {
> + Q_EMIT totalChanged();
> + }
> + if (freeUpdated) {
> + Q_EMIT freeChanged();
> + }
> + }
> +#endif // Q_OS_LINUX
> +}
>
> === modified file 'src/app/webbrowser/Browser.qml'
> --- src/app/webbrowser/Browser.qml 2016-01-28 22:12:27 +0000
> +++ src/app/webbrowser/Browser.qml 2016-02-10 14:13:13 +0000
> @@ -460,8 +462,9 @@
> webview: browser.currentWebview
> forceHide: browser.fullscreen
> forceShow: recentView.visible
> - defaultMode: (formFactor == "desktop") ? Oxide.LocationBarController.ModeShown
> - : Oxide.LocationBarController.ModeAuto
> + defaultMode: (internal.hasMouse && !internal.hasTouchScreen)
Actually no, I didn’t run this past design. The reason I chose the mouse predicate is because my assumption is that if you have a wide touch screen without a mouse, you’d still want the chrome to autohide when scrolling down (that’s how chrome on android behaves on e.g. a 10" tablet). This might or might not be a correct assumption, I’ll ask design their opinion. Given that they’re overwhelmed and slow to answer lately, I wouldn’t block on this though, we can refine this behaviour whenever we get feedback from them. What do you think?
> + ? Oxide.LocationBarController.ModeShown
> + : Oxide.LocationBarController.ModeAuto
> }
>
> Chrome {
> @@ -1432,6 +1434,10 @@
> }
>
> readonly property bool hasMouse: (miceModel.count + touchPadModel.count) > 0
> + readonly property bool hasTouchScreen: touchScreenModel.count > 0
> +
> + readonly property real freeMemRatio: (MemInfo.total > 0) ? (MemInfo.free / MemInfo.total) : 1.0
> + readonly property bool lowOnMemory: freeMemRatio < 0.2
Yes, absolutely arbitrary. If you want to suggest another value based on actual data, I’m all for it (and in any case that value can be changed in the future if it turns out not to be good enough for the job).
Note that for my tests, I modified the value to 0.7 so that I didn’t have to overload my machine too much, and the tab-unloading functionality worked pretty well there.
>
> function getOpenPages() {
> var urls = []
> @@ -1817,15 +1824,41 @@
> }
>
> Connections {
> - // On mobile, ensure that at most n webviews are instantiated at all
> - // times, to reduce memory consumption (see http://pad.lv/1376418).
> - // Note: this works only in narrow mode, where the list of tabs is a
> - // stack. Switching from wide mode to narrow mode will result in
> - // undefined behaviour (tabs previously loaded won’t be unloaded).
> - target: ((formFactor == "mobile") && !browser.wide) ? tabsModel : null
> - onCurrentTabChanged: {
> - if (tabsModel.count > browser.maxLiveWebviews) {
> - tabsModel.get(browser.maxLiveWebviews).unload()
> + target: internal
> + onFreeMemRatioChanged: {
> + if (internal.lowOnMemory) {
> + // Unload an inactive tab to (hopefully) free up some memory
> + function getCandidate(model) {
> + // Naive implementation that only takes into account the
> + // last time a tab was current. In the future we might
> + // want to take into account other parameters such as
> + // whether the tab is currently playing audio/video.
> + var candidate = null
> + for (var i = 0; i < model.count; ++i) {
> + var tab = model.get(i)
> + if (tab.current || !tab.webview) {
That’s a good point, I hadn’t thought about it. Not sure what to do about it though? Would you suggest some sort of locking mechanism to prevent closing a tab while the tab-unloader is running?
> + continue
> + }
> + if (!candidate || (candidate.lastCurrent > tab.lastCurrent)) {
> + candidate = tab
> + }
> + }
> + return candidate
> + }
> + var candidate = getCandidate(publicTabsModel)
> + if (candidate) {
> + console.warn("Unloading background tab (%1) to free up some memory".arg(candidate.url))
> + candidate.unload()
> + return
> + } else if (browser.incognito) {
> + candidate = getCandidate(privateTabsModelLoader.item)
> + if (candidate) {
> + console.warn("Unloading a background incognito tab to free up some memory")
> + candidate.unload()
> + return
> + }
> + }
> + console.warn("System low on memory, but unable to pick a tab to unload")
> }
> }
> }
--
https://code.launchpad.net/~osomon/webbrowser-app/remove-formFactor/+merge/285539
Your team Ubuntu Phablet Team is requested to review the proposed merge of lp:~osomon/webbrowser-app/remove-formFactor into lp:webbrowser-app.
More information about the Ubuntu-reviews
mailing list