Fix favicons in firefox bookmarks runner
ClosedPublic

Authored by z3ntu on Feb 17 2018, 4:24 PM.

Details

Summary

The favicons are located in the file favicons.sqlite file since FF 37 and the old table was removed in FF 41 resulting in crashes because the table wasn't found.

BUG: 363136

Test Plan

Firefox bookmark favicons now work and don't crash Krunner and Kickoff

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
z3ntu created this revision.Feb 17 2018, 4:24 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 17 2018, 4:24 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
z3ntu requested review of this revision.Feb 17 2018, 4:24 PM
z3ntu edited the test plan for this revision. (Show Details)Feb 17 2018, 4:25 PM
z3ntu added a comment.Feb 17 2018, 4:28 PM

Two more notes:

The logic for the path of the cache file (bookmarkrunnerfirefoxfavdbfile.sqlite) is like in D10605.
If this patch doesn't get merged into the next Plasma bugfix release, then QList<QVariantMap> faviconFound = m_fetchsqlite->query(m_buildQuery, bindVariables); could be replaced with QList<QVariantMap> faviconFound; to just not run the sql code and avoid the crashes caused by this bug.

z3ntu updated this revision to Diff 27415.Feb 17 2018, 5:06 PM

Do the same change as in D10605

Firefox bookmark favicons now work and don't crash Krunner and Kickoff

Oh, that's fantastic news. Thank you so much!

Presumably this means it'll break on < Firefox 58?
As long as it just fails to load and doesn't crash, that's probably fine.


It's weird that it crashes on a table missing. Is that a Qt bug we can file?

runners/bookmarks/browsers/firefox.cpp
69

where does this get deleted?

z3ntu updated this revision to Diff 27421.Feb 17 2018, 7:29 PM
  • Add m_fetchsqlite_fav to teardown
z3ntu marked an inline comment as done.EditedFeb 17 2018, 7:33 PM

Thanks :)

Presumably this means it'll break on < Firefox 58?

It should work FF37+ where they switched to the new database schema.

It's weird that it crashes on a table missing. Is that a Qt bug we can file?

It seems to be QTBUG-66443 and yes, it's definitely a Qt bug as the same SQL statement just returns false in my test file as it should.

Two questions:

  • The destructor in the Firefox class doesn't do anything, should I add the new cache file there?
  • Should I do anything with the m_dbFile_fav variable and the KConfigGroup ?
runners/bookmarks/browsers/firefox.cpp
69

Sorry, fixed it.

z3ntu marked 2 inline comments as done.Feb 17 2018, 7:34 PM
davidedmundson accepted this revision.Feb 20 2018, 12:11 PM
This revision is now accepted and ready to land.Feb 20 2018, 12:11 PM
This revision was automatically updated to reflect the committed changes.