Bug 65237 – [Qt] Favicons can't be changed dynamically.

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKit Qt

[Pierre Rossi]

It would probably be nice to have since it has some nice use cases (e.g. gmail's unread mail notification)

Here are some websites demonstrating this:

http://www.p01.org/releases/DEFENDER_of_the_favicon/
http://ajaxify.com/run/favicon/

[Pierre Rossi]

Created attachment 102104 [details]
patch

Would probably be a nice thing to cherry-pick into 2.2 as well.

[Alexis Menard (darktears)]

Comment on attachment 102104 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102104&action=review

> Source/WebKit/qt/ChangeLog:3
> +        Load favicon dynamically.

You need to put the title of the bug with no extra space before the bug url.

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:752
> +            + QDateTime::currentDateTime().toString(QLatin1String("yyyyMMddhhmmss"));

I'm curious but by the time you are in ~tst_QWebFrame() it may be that the minute changed so you will not delete the dir. Or I miss something here?

[Pierre Rossi]

(In reply to comment #2)
> (From update of attachment 102104 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102104&action=review
> 
> > Source/WebKit/qt/ChangeLog:3
> > +        Load favicon dynamically.
> 
> You need to put the title of the bug with no extra space before the bug url.

Ok, will do.

> 
> > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:752
> > +            + QDateTime::currentDateTime().toString(QLatin1String("yyyyMMddhhmmss"));
> 
> I'm curious but by the time you are in ~tst_QWebFrame() it may be that the minute changed so you will not delete the dir. Or I miss something here?

Yeah I took that from some other autotests that need local storage and at first I thought the same, but taking another look at the getter function you'll notice the string is static. :)

[Alexis Menard (darktears)]

Comment on attachment 102104 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102104&action=review

>>> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:752
>>> +            + QDateTime::currentDateTime().toString(QLatin1String("yyyyMMddhhmmss"));
>> 
>> I'm curious but by the time you are in ~tst_QWebFrame() it may be that the minute changed so you will not delete the dir. Or I miss something here?
> 
> Yeah I took that from some other autotests that need local storage and at first I thought the same, but taking another look at the getter function you'll notice the string is static. :)

Ah yes :D

[Pierre Rossi]

Created attachment 102763 [details]
changelog fixup

[Benjamin Poulain]

Comment on attachment 102763 [details]
changelog fixup

View in context: https://bugs.webkit.org/attachment.cgi?id=102763&action=review

> Source/WebKit/qt/Api/qgraphicswebview.cpp:212
> +    \note This signal may not be emitted if the icon database is disabled.
> +

_may_ not be emitted?
I do not like fuzzy documentation because it is a bit scary for the reader. One might wonder if the signal is ever emitted, randomly emitted, etc. if the icon database is not set.

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:587
> +        if (entries[i].isDir())
> +            removeRecursive(entries[i].filePath());
> +        else
> +            dir.remove(entries[i].fileName());

Good practice: entires[i] -> entries.at(i) to avoid copying stuff.

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:754
> +    QString tmpDirPath() const
> +    {
> +        static QString tmpd = QDir::tempPath() + "/tst_qwebframe-"
> +            + QDateTime::currentDateTime().toString(QLatin1String("yyyyMMddhhmmss"));
> +        return tmpd;
> +    }

You do not use QLatin1String elsewhere, no need to put it here.

Please do not put this method inline. This should be a static function and not a method. A method that does not use any attribute and use a static variable -> something evil is going on :)

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3717
> +    QImage oldIcon  = frame->icon().pixmap(iconSize).toImage();

2 space before the equal sign.

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3718
> +    QImage image = QImage(":/favicon1.png").convertToFormat(QImage::Format_ARGB32_Premultiplied);

Why do you convert the image to ARGB32_PM?

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3723
> +    QString js = QLatin1String("document.getElementById('favicon').setAttribute(\"href\", \"favicon2.png\")");

Sometime you use QLatin1String, sometime not, better decide :)

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3734
> +

Space

[Pierre Rossi]

(In reply to comment #6)


> _may_ not be emitted?
> I do not like fuzzy documentation because it is a bit scary for the reader. One might wonder if the signal is ever emitted, randomly emitted, etc. if the icon database is not set.

Well technically it's going to be emitted regardless when the webframe is loaded, but then if the icon link is changed dynamically, we won't gen notified if there's no database.
 
Maybe I could try and rephrase that in the doc then.

> > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:587
> > +        if (entries[i].isDir())
> > +            removeRecursive(entries[i].filePath());
> > +        else
> > +            dir.remove(entries[i].fileName());
> 
> Good practice: entires[i] -> entries.at(i) to avoid copying stuff.
> 
> > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:754
> > +    QString tmpDirPath() const
> > +    {
> > +        static QString tmpd = QDir::tempPath() + "/tst_qwebframe-"
> > +            + QDateTime::currentDateTime().toString(QLatin1String("yyyyMMddhhmmss"));
> > +        return tmpd;
> > +    }
> 
> You do not use QLatin1String elsewhere, no need to put it here.
> 
> Please do not put this method inline. This should be a static function and not a method. A method that does not use any attribute and use a static variable -> something evil is going on :)
> 
> > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3717
> > +    QImage oldIcon  = frame->icon().pixmap(iconSize).toImage();
> 
> 2 space before the equal sign.
> 
> > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3718
> > +    QImage image = QImage(":/favicon1.png").convertToFormat(QImage::Format_ARGB32_Premultiplied);
> 
> Why do you convert the image to ARGB32_PM?
> 
> > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3723
> > +    QString js = QLatin1String("document.getElementById('favicon').setAttribute(\"href\", \"favicon2.png\")");
> 
> Sometime you use QLatin1String, sometime not, better decide :)
> 
> > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3734
> > +
> 
> Space

I was lazy and just copied all that stuff over from qwebpage's autotest, let me move it to util.h and fix the things you pointed out.

[Pierre Rossi]

Created attachment 103174 [details]
Patch

[Pierre Rossi]

(In reply to comment #6)

> > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3718
> > +    QImage image = QImage(":/favicon1.png").convertToFormat(QImage::Format_ARGB32_Premultiplied);
> 
> Why do you convert the image to ARGB32_PM?
> 

Probably something I did wrong when making my test image. I just wanted QCOMPARE to play nice, maybe not the best approach.

[Early Warning System Bot]

Comment on attachment 103174 [details]
Patch

Attachment 103174 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9327417

[Benjamin Poulain]

Comment on attachment 103174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103174&action=review

Would it be possible to have a fake icon database to get the signal?
A icon database mockup that would just forward the right signals at the right time?
Of modify the icon database to handle this kind of cases (which could be useful if the disk is full for example).

> Source/WebKit/qt/Api/qgraphicswebview.cpp:213
> +    \note This signal will not be emitted again after the initial page load
> +    if the icon database is disabled.
> +

This is better, but I think we should do even better :)
This could be read as "you get the signal once for the first page ever being loaded". While what you really want to communicate is: "the signal is not emitted again if a loaded page change the icon".

> Source/WebKit/qt/tests/util.h:22
> +#include <QDateTime>
> +#include <QDir>

No need to remove those from tst_qdeclarativewebview.cpp?

> Source/WebKit/qt/tests/util.h:92
> +static QString tmpDirPath()
> +{
> +    static QString tmpd = QDir::tempPath() + QLatin1String("/tst_qwebframe-")
> +        + QDateTime::currentDateTime().toString(QLatin1String("yyyyMMddhhmmss"));
> +    return tmpd;
> +}

Better put the implementation in a new util.cpp (like Jocelyn did for WebKit2 IIRC).
I guess this causes the other tests to have warnings on compilation because static functions are defined but not used?

[Pierre Rossi]

Created attachment 103496 [details]
Patch

Actually, we could emit that signal when the iconDatabase is disabled, but then it'd be inconsistent since we don't even emit it on page load. Benjamin: good thing you made me double check, the comment was not only scary and confusing, it was plain wrong. :)

[WebKit Review Bot]

Attachment 103496 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qgraphicswebview.cpp'..." exit_code: 1

Source/WebKit/qt/tests/util.cpp:20:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

> Actually, we could emit that signal when the iconDatabase is disabled, but then it'd be inconsistent since we don't even emit it on page load. Benjamin: good thing you made me double check, the comment was not only scary and confusing, it was plain wrong. :)

Then also add tests for that use case :)

[Pierre Rossi]

Created attachment 103605 [details]
Patch

Benjamin: Here you go boss ! ;)

[Rafael Brandao]

Comment on attachment 103605 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103605&action=review

Looks good to me, but I'm not a reviewer.

> Source/WebKit/qt/tests/qwebframe/resources/favicon.html:4
> +</script>

You should remove "</script>" from here.

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3820
> +    ::waitForSignal(frame, SIGNAL(iconChanged()), 1000);

You should use "waitForSignal" instead of "::waitForSignal".

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3826
> +    QImage oldIcon  = frame->icon().pixmap(iconSize).toImage();

There's an extra space between "oldIcon" and "=".

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3833
> +    ::waitForSignal(frame, SIGNAL(iconChanged()), 1000);

Without "::".

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3851
> +    ::waitForSignal(frame, SIGNAL(loadFinished(bool)), 1000);

Without the "::". By the way, to make sure the icon was not changed, you should actually wait for the signal iconChanged, because there's no guarantee that you'll get it before loadFinished happens.

[Pierre Rossi]

Created attachment 158571 [details]
Patch

[WebKit Review Bot]

Attachment 158571 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qgraphicswebview.cpp'..." exit_code: 1
Source/WebKit/qt/tests/util.cpp:20:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rafael Brandao]

Comment on attachment 158571 [details]
Patch

LGTM.

[Simon Hausmann]

Comment on attachment 158571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158571&action=review

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:472
> +    m_frame->loader()->icon()->startLoader();

I wonder how this works in the other ports. It seems the others merely emit a notification. Can we do the same or do we need to start the loader?

[Pierre Rossi]

Created attachment 158811 [details]
Patch

[WebKit Review Bot]

Attachment 158811 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qgraphicswebview.cpp'..." exit_code: 1
Source/WebKit/qt/tests/util.cpp:20:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pierre Rossi]

Created attachment 158847 [details]
Patch

Actually that last patch was nonsense, the signal is emitted in FrameLoaderClientQt::dispatchDidReceiveIcon, the problem is that this only gets called again if we restart the loader when the url is changed. The difference with other ports could stem from the fact that they don't use the IconDatabase maybe ?

[WebKit Review Bot]

Attachment 158847 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qgraphicswebview.cpp'..." exit_code: 1
Source/WebKit/qt/tests/util.cpp:20:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rafael Brandao]

(In reply to comment #23)
> Created an attachment (id=158847) [details]
> Patch
> 
> Actually that last patch was nonsense, the signal is emitted in FrameLoaderClientQt::dispatchDidReceiveIcon, the problem is that this only gets called again if we restart the loader when the url is changed. The difference with other ports could stem from the fact that they don't use the IconDatabase maybe ?

If I remember correctly, you don't need to do that. When you know the icon has changed, then the client can ask at some point for the favicon to the database (check how's implemented the function that returns the favicon's QPixmap or QImage) and then if it figure out it still need to fetch the new icon, then it will do right away. So it's just a matter of asking which is the current favicon data. So you just need to connect emit "icon changed" just like you do when you get the first one, and the UI can rely on this signal to update the data displayed. Please double check this because I may be wrong.

[Rafael Brandao]

(In reply to comment #25)
> (In reply to comment #23)
> > Created an attachment (id=158847) [details] [details]
> > Patch
> > 
> > Actually that last patch was nonsense, the signal is emitted in FrameLoaderClientQt::dispatchDidReceiveIcon, the problem is that this only gets called again if we restart the loader when the url is changed. The difference with other ports could stem from the fact that they don't use the IconDatabase maybe ?
> 
> If I remember correctly, you don't need to do that. When you know the icon has changed, then the client can ask at some point for the favicon to the database (check how's implemented the function that returns the favicon's QPixmap or QImage) and then if it figure out it still need to fetch the new icon, then it will do right away. So it's just a matter of asking which is the current favicon data. So you just need to connect emit "icon changed" just like you do when you get the first one, and the UI can rely on this signal to update the data displayed. Please double check this because I may be wrong.

Sorry about the typo. I mean you just need to emit "iconChanged" signal just like you do when you get the first favicon, and then the machinery behind it will do the trick for you.

[Pierre Rossi]

Created attachment 162503 [details]
Patch

[Rafael Brandao]

Comment on attachment 162503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162503&action=review

LGTM :)

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:1615
> +    icon  = frame->icon().pixmap(iconSize).toImage();

Extra whitespace on "icon  =".

[Early Warning System Bot]

Comment on attachment 162503 [details]
Patch

Attachment 162503 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13765636

[Early Warning System Bot]

Comment on attachment 162503 [details]
Patch

Attachment 162503 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13770358

[Brady Eidson]

We are *not* going to be adding this to all platforms without having a switch to opt in.

We (Apple) intentionally *do not* want the ability to script favicons.

[Brady Eidson]

(In reply to comment #31)
> We are *not* going to be adding this to all platforms without having a switch to opt in.
> 
> We (Apple) intentionally *do not* want the ability to script favicons.

Sorry about this comment and the review-churn.  I had multiple bugs open and acted on the wrong one!

(Meant to act on https://bugs.webkit.org/show_bug.cgi?id=95979 FWIW)

[Anders Carlsson]

Comment on attachment 162503 [details]
Patch

Qt has been removed, clearing review flags.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.