Bug 63820 – [Qt][WK2] Add API to retrieve page head link and meta elements

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKit2

Need an API for the UI process to access link and meta elements from the head of a page after it is loaded.

Git commit 5c5892af in qtwebkit-webkit2-dev added an API that retrieve link elements, but not meta elements.  We can adapt that API to get both.

Created attachment 99489 [details]
proposed patch

Adds new API to retrieve both link and meta element data from a page's head element.

While adding unit test code to tst_qwkpage.cpp noticed that pre-existing test loadEmptyUrl() causes an ASSERT failure in the WebProcess when waitForSignal() is used in a subsequent test.  Disabled the test via QSKIP and added appropriate comments for revisiting later.

[Benjamin Poulain]

I think we should wait until the client refactoring is upstreamed before looking at this.
The whole QWKPage is gone.

[Kenneth Rohde Christiansen]

Comment on attachment 99489 [details]
proposed patch

Also, personally I would like this to be done similar to how the viewport meta tag is received. At least that should be investigated.

(In reply to comment #2)
> I think we should wait until the client refactoring is upstreamed before looking at this.
> The whole QWKPage is gone.

When is this scheduled to be done?

(In reply to comment #3)
> (From update of attachment 99489 [details])
> Also, personally I would like this to be done similar to how the viewport meta tag is received. At least that should be investigated.

Meaning what exactly?  A separate message similar to viewportDataDidChange for each of a set of pre-defined "interesting" meta and link tags?  That seems unduly restrictive -- what if a client wants to see a meta or link tag we didn't think to support?  Maybe a combined message for the whole set, or whole subsets (e.g. link, meta with name attribute, meta with http-equiv attribute)?  A separate message for each link and meta tag?

The disadvantage for all of these is that they dispatch messages that will be ignored by many applications.  Is the expected cost small enough not to matter?  The advantages of Kimmo's approach is that the cost is only borne by clients that specifically ask for the data, and that there is no restriction on which link and meta tags can be retrieved.

[Benjamin Poulain]

Comment on attachment 99489 [details]
proposed patch

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?

[Benjamin Poulain]

You mention Kimmo in the comments. If this patch is based on his work, you could mention him as co-author in the Changelog.

[Kenneth Rohde Christiansen]

I still wonder whether we should instead make a callback like api, similar to what viewport Meta is doing?

[Benjamin Poulain]

(In reply to comment #8)
> I still wonder whether we should instead make a callback like api, similar to what viewport Meta is doing?

I would say that depends on the use case. One might not care about the change of values after page load.
The reporter should probably describe how this API is used.

This is gonna be private API so if this end up being a callback on value change, I would prefer it to be a opt-in so it does not affect the regular view.

[Kenneth Rohde Christiansen]

(In reply to comment #9)
> (In reply to comment #8)
> > I still wonder whether we should instead make a callback like api, similar to what viewport Meta is doing?
> 
> I would say that depends on the use case. One might not care about the change of values after page load.
> The reporter should probably describe how this API is used.
> 
> This is gonna be private API so if this end up being a callback on value change, I would prefer it to be a opt-in so it does not affect the regular view.

What people care about are the meta tags, so I guess asking for notification of all meta's or specific kinds would work fine.

[Jesus Sanchez-Palencia]

Kenneth, should we keep this open?

[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.