[webkit-reviews] review denied: [Bug 178205] [WPE] Remove GLib API functions which use Cairo : [Attachment 323937] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 16 15:42:23 PDT 2017


Michael Catanzaro <mcatanzaro at igalia.com> has denied Adrian Perez
<aperez at igalia.com>'s request for review:
Bug 178205: [WPE] Remove GLib API functions which use Cairo
https://bugs.webkit.org/show_bug.cgi?id=178205

Attachment 323937: Patch

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




--- Comment #13 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 323937
  --> https://bugs.webkit.org/attachment.cgi?id=323937
Patch

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

This is way better than before, thanks. But still r- due to a couple remaining
problems.

> Source/WebKit/UIProcess/API/glib/WebKitFaviconDatabasePrivate.h:33
> +#if PLATFORM(GTK)
>  void webkitFaviconDatabaseGetLoadDecisionForIcon(WebKitFaviconDatabase*,
const WebCore::LinkIcon&, const String&, Function<void(bool)>&&);
>  void webkitFaviconDatabaseSetIconForPageURL(WebKitFaviconDatabase*, const
WebCore::LinkIcon&, API::Data&, const String&);
> +#endif

Won't removing these break the webkit_favicon_database_get_favicon_uri() API? I
think these probably need to stay in.

Also: you added guards here, but forgot to add them in
WebKitFaviconDatabase.cpp, which is bad. So please ensure it's consistent.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:500
>      webkitWebViewUpdateFaviconURI(webView, faviconURI);

Pretty sure this breaks webkit_favicon_database_get_favicon_uri(). Do we not
have any API tests for that, or did you run them?

We need to be careful to remove only the portions of these functions that
implement webkit_favicon_database_get_favicon() so that we keep
webkit_favicon_database_get_favicon_uri() working.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1948
> +#if PLATFORM(GTK)
>      case WEBKIT_LOAD_COMMITTED: {
>	   WebKitFaviconDatabase* database =
webkit_web_context_get_favicon_database(priv->context.get());
>	   GUniquePtr<char>
faviconURI(webkit_favicon_database_get_favicon_uri(database,
priv->activeURI.data()));
>	   webkitWebViewUpdateFaviconURI(webView, faviconURI.get());
>	   break;
>      }
> +#endif

This should stay too.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:588
>  void webkitWebPageDidReceiveMessage(WebKitWebPage* page, const String&
messageName, API::Dictionary& message)
>  {
> +#if PLATFORM(GTK)

I don't think the guards are needed here, especially since you didn't remove
the web process functionality that backs the GetSnapshot message, right?

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitFaviconDatabase.cpp:246
>  static void testGetFaviconURI(FaviconDatabaseTest* test)
>  {

Is this test really still passing?

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitFaviconDatabase.cpp:292
> +#if PLATFORM(GTK)
>      testGetFavicon(test);
> +#endif
> +
>      testGetFaviconURI(test);
> +
> +#if PLATFORM(GTK)
>      testWebViewFavicon(test);
> +#endif

I bet they can be reordered so you only need one set of guards, right?

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitFindController.cpp:261
> +// TODO: Rewrite this test to avoid using snapshots so it can be re-enabled
> +//	    for WPE or, alternatively, make snapshots work for WPE as well.

Don't indent the second line of the comment, that's not WebKit style for
comments.

> Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:373
>  static void onSnapshotReady(WebKitWebView* web_view, GAsyncResult* res,
WebViewTest* test)

Please move this and WebViewTest::getSnapshotAndWaitUntilReady into
WebViewTestGtk.cpp.

(Also, now that Carlos Garcia is back, we can ask him why he has left so many
functions in WebKitWebView.cpp that could be moved to WebKitWebViewGtk.cpp.)


More information about the webkit-reviews mailing list