[Webkit-unassigned] [Bug 63820] [Qt] Add API to retrieve page head link and meta elements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=63820


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #99489|review?                     |review-
               Flag|                            |




--- Comment #6 from Benjamin Poulain <benjamin at webkit.org>  2011-07-09 05:44:12 PST ---
(From update of attachment 99489)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.