[webkit-reviews] review denied: [Bug 71082] [Qt][WK2] Implement favicon support : [Attachment 114991] Use a QDeclarativeImageProvider to display FavIcons instead of creating a new component

[bugzilla-daemon at webkit.org]

Caio Marcelo de Oliveira Filho <cmarcelo at webkit.org> has denied Rafael Brandao
<rafael.lobo at openbossa.org>'s request for review:
Bug 71082: [Qt][WK2] Implement favicon support
https://bugs.webkit.org/show_bug.cgi?id=71082

Attachment 114991: Use a QDeclarativeImageProvider to display FavIcons instead
of creating a new component
https://bugs.webkit.org/attachment.cgi?id=114991&action=review

------- Additional Comments from Caio Marcelo de Oliveira Filho
<cmarcelo at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114991&action=review


Rafael, there are a bunch of comments. I like that the final solution is
smaller than we expected in the beginning, but I feel we need another iteration
:-)

I think you could improve the patch ChangeLog by explaining that the icons are
actually stored on the UIProcess and how this relates to the IconDatabase in
the WebProcess. Understanding this big picture helps understanding the patch.

> Source/WebKit/qt/declarative/plugin.cpp:30
> +#include "qwebfaviconimageprovider.h"

Shouldn't be public API, so either use "qwebfaviconimageprovider_p.h" or move
it to non-API directory if possible.

> Source/WebKit/qt/declarative/plugin.cpp:48
> +
> +#endif

Nit pick: swap these two lines.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:358
> +	   QObject::connect(q_ptr, SIGNAL(urlChanged(QUrl)),
qtWebIconDatabase(), SLOT(requestIconIdForPageURL(QUrl)));

Nit pick: use q instead of q_ptr.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:433
> +void QQuickWebView::requestIconIdForPageURL(const QUrl& pageURL)
> +{
> +    Q_D(const QQuickWebView);
> +    d->pageProxy->requestIconIdForPageURL(pageURL);
> +}

I think this shouldn't be exposed at API level. If other entities in the future
want to expose different favicons (History Items), they can do by exposing a
favIconSource like the WebView did.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:439
> +void QQuickWebView::cleanupIconDatabase()
> +{
> +    Q_D(const QQuickWebView);
> +    d->pageProxy->cleanupIconDatabase();
> +}

I think this shouldn't be exposed at API level. At least not in QML. Specially
considering that cleanupIconDatabase() would clean for all WebViews, which
could be a bit surprising. Maybe a candidate to be in a future object that
represents the webcontext in C++?

> Source/WebKit2/UIProcess/API/qt/qwebfaviconimageprovider.cpp:38
> +QPixmap QWebFavIconImageProvider::requestPixmap(const QString &id, QSize
*size, const QSize &requestedSize)

Style.

> Source/WebKit2/UIProcess/API/qt/qwebfaviconimageprovider.cpp:44
> +	   if (size)
> +	       *size = m_defaultFavIcon.size();
> +	   return m_defaultFavIcon;

What about replacing this with "icon = m_defaultFavIcon;"?

> Source/WebKit2/UIProcess/API/qt/qwebfaviconimageprovider.h:31
> +    QPixmap requestPixmap(const QString & id, QSize * size, const QSize &
requestedSize);

Style.

> Source/WebKit2/UIProcess/WebContext.h:194
> +    String iconDatabasePath() const;

Could add comment why you need that? I think iconDatabasePath() is serving as a
getter for the property set as well as a way to get the default value for the
property, maybe you can improve this. (Relates to previous comment about setter
before)

> Source/WebKit2/UIProcess/qt/QtViewInterface.h:88
> +    virtual void didChangeFavIconSource(const QUrl&);

Is this a fit for ViewInterface? I wonder if a SIGNAL in QtWebPageProxy would
suffice -- specially because later you ask the QtWebPageProxy in the property
getter.

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:36
> +QtWebIconDatabase* qtWebIconDatabase()
> +{
> +    return globalQtIconDatabase;
> +}

Not very important, but what about changing it to a static class function and
storing the globalQtIconDatabase as a static class member?

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:49
> +    // FIXME: user should be able to set its own icon database path
> +    // rather than always using the default path.

You can remove this. This is not a concern right now, we don't support this
level of customization for other things as well.

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:50
> +    m_context->setIconDatabasePath(m_context->iconDatabasePath());

I think this line deserve some explanation: looks like setting something to its
current value. I know that setIconDatabasePath have a side effect that you want
to trigger, and the default path is accessible by the "getter". Maybe you can
even improve this in WebContext in another patch ;-)

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:61
> +    if (!m_context)
> +	   return QPixmap();

I think this case will never happen with current code. Same applies for the
next functions.

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:72
> +    QPixmap* nativeImage = image->nativeImageForCurrentFrame();
> +    if (!nativeImage)
> +	   return QPixmap();
> +
> +    return *nativeImage;

When image is destroyed, will this new QPixmap still be valid? Or at this point
we know more about the image's lifetime?

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:84
> +    m_pixmapForIconId[++m_lastIconId] = icon;

Maybe create a getNextAvailableIconId() function? I'll let Alexis complain
about the overflow ;-)

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:98
> +    QMap<int, QPixmap>::iterator it = m_pixmapForIconId.find(id);

const_iterator?

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:108
> +    if (it != m_retainCountForIconId.constEnd()) {

end() instead of constEnd()?

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:109
> +	   ++m_retainCountForIconId[id];

You can change the iterator value here, instead of looking up again.

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:118
> +    if (it == m_retainCountForIconId.constEnd())

end() instead of constEnd()?

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.cpp:127
> +    --m_retainCountForIconId[id];

Same as before, use iterator value instead of looking it up again.

> Source/WebKit2/UIProcess/qt/QtWebIconDatabase.h:59
> +    WebContext* m_context;

Maybe just save a reference to the IconDatabase is enough?

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:88
> +	   // FIXME: This is where the icondatabase's client is injected,
> +	   // if there's no instance of it by now, then it's created.

I don't think you need this comment.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:177
> +	       if (qtWebIconDatabase())
> +		   delete qtWebIconDatabase();

You don't need the "if" here, just delete.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:943
> +void QtWebPageProxy::setFavIconId(int id)

Nit pick: updateFavIconId()? :)

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:945
> +    if (!qtWebIconDatabase() || m_favIconId == id)

Do we need to check qtWebIconDatabase() here?

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:271
> +    int m_favIconId;
> +    QUrl m_favIconSource;

It doesn't feel right that we need to keep two representations of the same
data. Can the string be always generated? Usually this property will be queried
once each time it changed (or twice if you count the value passed in the
signal).