[webkit-reviews] review denied: [Bug 63820] [Qt] Add API to retrieve page head link and meta elements : [Attachment 99489] proposed patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied mike.zraly at nokia.com's
request for review:
Bug 63820: [Qt] Add API to retrieve page head link and meta elements
https://bugs.webkit.org/show_bug.cgi?id=63820

Attachment 99489: proposed patch
https://bugs.webkit.org/attachment.cgi?id=99489&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=99489&action=review

Quick review.

Before this patch is updated, one should find the best way to expose private
API on trunk. Then the patch needs some cleaning.

> Source/WebKit2/UIProcess/API/qt/qwkpage.h:149
> +    typedef QList<QMap<QString, QString> > HeadTagData;

QMap seems to be a weird type for this. Why did you choose this type?
You should make a wrapper class to expose this, QList<QMap<QString, QString> >
is just plain confusing.

> Source/WebKit2/UIProcess/API/qt/tests/qwkpage/tst_qwkpage.cpp:93
> +    // Loading an empty URL crashes QtWebProcess. This will be evident to
the
> +    // test program only after processing the event loop to wait for a
signal.
> +    //
> +    // Here is the error message reported by QtWebProcess before it dies:
> +    //
> +    // ASSERTION FAILED: !newRequest.isNull()
> +    // ../../../Source/WebCore/loader/MainResourceLoader.cpp(177) : virtual
void
> +    //  
WebCore::MainResourceLoader::willSendRequest(WebCore::ResourceRequest&,
> +    //   const WebCore::ResourceResponse&)
> +
> +    QSKIP("Loading an empty URL crashes QtWebProcess with an ASSERTION
FAILURE", SkipSingle);

Totally unrelated change.

> Source/WebKit2/UIProcess/WebPageProxy.h:499
> +    void retrieveHeadTags();

This should be part of WebFrameProxy, not WebPageProxy.
A convenience method could be on WebPageProxy, but I am not sure that is
necessary.

> Source/WebKit2/WebProcess/WebPage/qt/WebPageQt.cpp:304
> +void WebPage::retrieveHeadTags()

This name does not describe what the method do. It only retrieve some sort of
nodes: link || meta.

> Source/WebKit2/WebProcess/WebPage/qt/WebPageQt.cpp:319
> +    for (Node* n = headElement->firstChild(); n; n = n->nextSibling()) {
> +	   if (!n->isElementNode())
> +	       continue;
> +
> +	   Element* e = static_cast<Element*>(n);

Variable names like "n" and "e" should be avoided. Just use "node", and
"element".

> Source/WebKit2/WebProcess/WebPage/qt/WebPageQt.cpp:324
> +	   const WebCore::NamedNodeMap * const attrs = e->attributes();

How come you need to qualify the name here and not for the other types?
(Document for example)

The style should be "const NamedNodeMap* const attr", no space before the *.
Const pointer are generally not used in WebKit for some reason, you should
probably drop the second const.

> Source/WebKit2/WebProcess/WebPage/qt/WebPageQt.cpp:329
> +	   const unsigned attrsCount = attrs->length();

size_t != unsigned.

> Source/WebKit2/WebProcess/WebPage/qt/WebPageQt.cpp:335
> +	       const WebCore::Attribute* const attribute =
attrs->attributeItem(i);
> +		if (attribute->namespaceURI().isEmpty())

Indent?

> Source/WebKit2/WebProcess/WebPage/qt/WebPageQt.cpp:338
> +		else
> +		    name = attribute->namespaceURI() + String(":") +
attribute->localName();

This is one ugly solution to the problem of namespaces. What about giving
namespace their own field in the output?