[webkit-reviews] review granted: [Bug 171110] Upstream the WPE port : [Attachment 309027] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 171110: Upstream the WPE port
https://bugs.webkit.org/show_bug.cgi?id=171110

Attachment 309027: Patch

https://bugs.webkit.org/attachment.cgi?id=309027&action=review




--- Comment #37 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 309027
  --> https://bugs.webkit.org/attachment.cgi?id=309027
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309027&action=review

What, nobody reviewed this?  Let's land this. r=me

> Source/WebKit2/UIProcess/InspectorServer/wpe/WebInspectorServerWPE.cpp:94
> +void WebInspectorServer::buildPageList(Vector<char>& data, String&
contentType)
> +{
> +    // chromedevtools (http://code.google.com/p/chromedevtools) 0.3.8
expected JSON format:

This is strange.  First of all, platformResourceForPath seems to only be called
in one place from WebInspectorServer.cpp, and there isn't an existing
implementation, so how does that work?	Second, we are trying to move away from
passing return values in as references in parameters.  Third, why are we trying
to match the Chrome devtools JSON format?  Are people using Chrome devtools
with WPE WebKit?  I'm not sure that's something we want to support.  If it is,
we should add this in another patch and add tests.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.h:265
> +    bool shouldPaintBrokenImage(const WebCore::URL&) const override;

This doesn't seem to be called.  Can it be added later if needed?

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:92
> +

This space is inconsistent.  I don't think it should be here.  The lack of
space here indicates that all these functions have to do with the pasteboard
stuff.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:428
> +    if options.platform in ["efl", "gtk", "wpe"]:

we need to go through all the tools and remove efl stuff.