[Merge] lp:~osomon/webbrowser-app/remove-formFactor into lp:webbrowser-app
Alexandre Abreu
alexandre.abreu at canonical.com
Wed Feb 17 21:43:12 UTC 2016
Overall looks good, a few questions below
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 to stop() ? (not sure what the semantics is)
> + 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)
I guess that this was +1 by design ? I was wondering about the choice of a mouse as a predicate for this instead of "screen size"
> + ? 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
Is the "0.2" value arbitrary?
>
> 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) {
this is unlikely but it seems we could have a race there if the user closes a tab while we do this? (taking account that the timing is rather unlikely, but wondering if this could also trigger some other issues)
> + 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")
> }
> }
> }
>
> === modified file 'src/app/webcontainer/WebApp.qml'
> --- src/app/webcontainer/WebApp.qml 2016-01-08 16:09:01 +0000
> +++ src/app/webcontainer/WebApp.qml 2016-02-10 14:13:13 +0000
> @@ -250,8 +250,9 @@
> ChromeController {
> webview: webapp.currentWebview
> forceHide: webapp.chromeless
> - defaultMode: (formFactor == "desktop") ? Oxide.LocationBarController.ModeShown
> - : Oxide.LocationBarController.ModeAuto
> + defaultMode: webapp.hasTouchScreen
same thing as above about the predicate here
> + ? Oxide.LocationBarController.ModeAuto
> + : Oxide.LocationBarController.ModeShown
> }
> }
>
--
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