[webkit-reviews] review requested: [Bug 73215] Split QWebPermissionRequest into QWebSecurityOrigin : [Attachment 122049] Imported code, implemented missing methods/operators, integrated with QWebPermissionRequest

[bugzilla-daemon at webkit.org]

Rafael Brandao <rafael.lobo at openbossa.org> has asked  for review:
Bug 73215: Split QWebPermissionRequest into QWebSecurityOrigin
https://bugs.webkit.org/show_bug.cgi?id=73215

Attachment 122049: Imported code, implemented missing methods/operators,
integrated with QWebPermissionRequest
https://bugs.webkit.org/attachment.cgi?id=122049&action=review

------- Additional Comments from Rafael Brandao <rafael.lobo at openbossa.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=122049&action=review


Do you have any use case in mind for this API? Maybe adding a test to show how
it could be used would be very nice. :-)

> Source/WebKit2/ChangeLog:16
> +

I fail to see how this change log is related to the rest of the patch. You've
added QtWebSecurityOrigin, but did not mention it here... neither the new files
are listed there.

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:65
> +bool QtWebSecurityOrigin::isLocalScheme(const QString &scheme)

Style (const QString&)

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:67
> +    // FIXME: how to say if a scheme is local or not?

Maybe you could use SchemeRegistry::shouldTreatURLSchemeAsLocal, not sure if
this alone is enough.

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:77
> +	   s_localSchemes << QLatin1String("file") << QLatin1String("qrc");

I believe you don't need to do this... we already have code inside WebCore to
deal with local schemes.

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:32
> +class QWEBKIT_EXPORT QtWebSecurityOrigin : public QObject {

I believe the naming is incorrect here, the others start with Q prefix instead
of Qt, but I see that we already have QWebSecurityOrigin. Maybe someone else
has a suggestion how to fix this, I don't know.

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:67
> +    bool isLocalScheme(const QString &);

Style (use const QString&, no whitespace)

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest_p.h:53
> +    QtWebSecurityOrigin &securityOrigin();

Style (use QWebSecurityOrigin& instead)