D6717: Expose base64-encoded favicons to the tabsrunner
fvogt updated this revision to Diff 17287. fvogt added a comment. braces + const A hasOwnProperty check is not needed here, that's accounted for by the if (!favIconUrl) below REPOSITORY R856 Plasma Browser Integration CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6717?vs=16759=17287 BRANCH favicons (branched from master) REVISION DETAIL https://phabricator.kde.org/D6717 AFFECTED FILES extension/extension.js extension/manifest.json tabsrunner/tabsrunner.cpp To: fvogt, #plasma, broulik, davidedmundson Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
D6717: Expose base64-encoded favicons to the tabsrunner
broulik accepted this revision. This revision is now accepted and ready to land. REPOSITORY R856 Plasma Browser Integration BRANCH favicons (branched from master) REVISION DETAIL https://phabricator.kde.org/D6717 To: fvogt, #plasma, broulik, davidedmundson Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
D6717: Expose base64-encoded favicons to the tabsrunner
This revision was automatically updated to reflect the committed changes. Closed by commit R856:d435a59b0514: Expose base64-encoded favicons to the tabsrunner (authored by fvogt). REPOSITORY R856 Plasma Browser Integration CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6717?vs=17287=17290 REVISION DETAIL https://phabricator.kde.org/D6717 AFFECTED FILES extension/extension.js extension/manifest.json tabsrunner/tabsrunner.cpp To: fvogt, #plasma, broulik, davidedmundson Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
D6717: Expose base64-encoded favicons to the tabsrunner
broulik added a comment.
Cool!
INLINE COMMENTS
> extension.js:495
> +var sendTabsIfComplete = function() {
> +if (--total > 0)
> +return;
Braces even for single line statements:
if (...) {
...
}
> extension.js:505
> +
> +for (let tabIndex in tabs) {
> +let currentIndex = tabIndex; // Not shared
Does this need a `tabs.hasOwnProperty(...)` check? (cf for in being horrible in
JS)
> tabsrunner.cpp:188
> +
> +QString favIconData =
> tab.value(QStringLiteral("favIconData")).toString();
> +int b64start = favIconData.indexOf(',');
const
> tabsrunner.cpp:191
> +if (b64start != -1) {
> +QByteArray b64 = favIconData.rightRef(favIconData.size() -
> b64start - 1).toLatin1();
> +QByteArray data = QByteArray::fromBase64(b64);
+1 for usage of `ref` :)
> tabsrunner.cpp:224
>
> -match.setIconName(iconName);
> +if (!iconName.isEmpty())
> +match.setIconName(iconName);
Braces
REPOSITORY
R856 Plasma Browser Integration
REVISION DETAIL
https://phabricator.kde.org/D6717
To: fvogt, #plasma, broulik, davidedmundson
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg,
abetts, sebas, apol, mart, lukas
D6717: Expose base64-encoded favicons to the tabsrunner
fvogt marked 4 inline comments as done. REPOSITORY R856 Plasma Browser Integration REVISION DETAIL https://phabricator.kde.org/D6717 To: fvogt, #plasma, broulik, davidedmundson Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
D6717: Expose base64-encoded favicons to the tabsrunner
fvogt updated this revision to Diff 16759. fvogt added a comment. Also send a request for the favicon without timeout to fill the cache for next time. Otherwise favicons from slow servers might never show up REPOSITORY R856 Plasma Browser Integration CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6717?vs=16745=16759 BRANCH favicons (branched from master) REVISION DETAIL https://phabricator.kde.org/D6717 AFFECTED FILES extension/extension.js extension/manifest.json tabsrunner/tabsrunner.cpp To: fvogt, #plasma, broulik, davidedmundson Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
D6717: Expose base64-encoded favicons to the tabsrunner
fvogt added a comment. It could also decode the base64 data in the plasma-browser-integration-host already and send it as bytearray, I'm not sure what's simpler/better/faster. REPOSITORY R856 Plasma Browser Integration REVISION DETAIL https://phabricator.kde.org/D6717 To: fvogt, #plasma, broulik, davidedmundson Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
D6717: Expose base64-encoded favicons to the tabsrunner
fvogt created this revision. Restricted Application added a project: Plasma. REVISION SUMMARY The browser has them in the cache already, so we avoid loading it ourselves, also we might not even have access to the icons from our context. Some browsers (Opera does) set the favIconUrl of the tab to the base64 data already, which is convenient. The base64 URL gets decoded in the krunner plugin and if it fails or is not available, the browser icon is shown. If the tab is audible or incognito, that icon is preferred. TEST PLAN Installed in Opera 40, Vivaldi 1.91 and Firefox 52, favicons appear in krunner. REPOSITORY R856 Plasma Browser Integration BRANCH favicons (branched from master) REVISION DETAIL https://phabricator.kde.org/D6717 AFFECTED FILES extension/extension.js extension/manifest.json tabsrunner/tabsrunner.cpp To: fvogt, #plasma, broulik, davidedmundson Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
