Bug 178205

...of course you'll also remove webkit_web_view_get_snapshot.

Please actually guard the function implementations (including the doc comments) with #if PLATFORM(GTK). I think this can all be done in one patch, so I'm not sure why this is a separate bug from bug #178157.

(In reply to Michael Catanzaro from comment #2) > Please actually guard the function implementations (including the doc > comments) with #if PLATFORM(GTK). Or move them to WebKitWebViewGtk.cpp, but Carlos Garcia has been preferring #if PLATFORM(GTK) instead. Not sure why.

Created attachment 323518 [details] Patch

(In reply to Michael Catanzaro from comment #2) > Please actually guard the function implementations (including the doc > comments) with #if PLATFORM(GTK). > > I think this can all be done in one patch, so I'm not sure why this is a > separate bug from bug #178157. What I was thinking would be better is to guard the definitions with “WEBKIT2_COMPILATION” inside the WPE headers, because some other parts inside WebKit can use the function. For example: WebKitWebView uses the webkit_favicon_database_get_favicon{,_finish} functions. The reason why I have made this new bug is that #178157 is for *replacing* Cairo in the WPE public API, and this one is only for *removing* Cairo, which is a stopgap measure to avoid third-parties using the affected parts of the API.

(In reply to Adrian Perez from comment #5) > (In reply to Michael Catanzaro from comment #2) > > Please actually guard the function implementations (including the doc > > comments) with #if PLATFORM(GTK). > > > > I think this can all be done in one patch, so I'm not sure why this is a > > separate bug from bug #178157. > > What I was thinking would be better is to guard the definitions with > “WEBKIT2_COMPILATION” inside the WPE headers, because some other parts > inside WebKit can use the function. For example: WebKitWebView uses the > webkit_favicon_database_get_favicon{,_finish} functions. Also, web page snapshots are used in the unit tests and that made the EWS fail. Let's make the function definitions available for tests as well.

Created attachment 323521 [details] Patch

Comment on attachment 323521 [details] Patch The unit tests should not be testing a function that's not in our API. And the symbol should not be exported by the installed library. Code using these functions in WebKitWebView should be guarded by #if PLATFORM(GTK). When entire API functions are guarded with PLATFORM(GTK), I would move them to WebKitWebViewGtk, but Carlos Garcia has not been doing that, so let's not start now. We can ask him why when he's available again.

(In reply to Michael Catanzaro from comment #8) > Comment on attachment 323521 [details] > Patch > > The unit tests should not be testing a function that's not in our API. I mean, the tests should be guarded with #if PLATFORM(GTK), just like we do for the rest of the GLib API that's not exposed to WPE.

(In reply to Michael Catanzaro from comment #9) > (In reply to Michael Catanzaro from comment #8) > > Comment on attachment 323521 [details] > > Patch > > > > The unit tests should not be testing a function that's not in our API. > > I mean, the tests should be guarded with #if PLATFORM(GTK), just like we do > for the rest of the GLib API that's not exposed to WPE. If we do that, any test that uses “WebViewTest::getSnapshotAndWaitUntilReady()” will *NOT* be built for WPE. The following tests would need to be disabled: - WebKitFindController/hide - WebKitWebView/snapshot - WebKitFaviconDatabase/favicon-database-test (some parts). Of course it's okay to have the two last ones disabled, as we are “removing” the associated API functions for WPE. I do not feel very comfortable by disabling “WebKitFindController/hide” as well, that's why I thought it could be a good idea to *build* the code but use #ifdefs in the headers. If everybody is okay with removing the “WebKitFindController/hide” test, I'll submit a patch that does as suggested.

Yes, please disable all three tests for WPE. For the FindController test, you could add a TODO suggesting that the test should be enhanced to not depend on the snapshot API so that it can be enabled for WPE.

Created attachment 323937 [details] Patch

Comment on attachment 323937 [details] 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.)

(In reply to Michael Catanzaro from comment #13) > Comment on attachment 323937 [details] > 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. Theorically no: removing these only prevents favicon bitmap data from being fetched, and the favicon URIs in the database will still be kept updated. (The “ensureFaviconDatabase()” function in “WebKitWebContext.cpp” is the one that creates the “WebKitFaviconDatabase” instance, and then when setting the database path, “webkitFaviconDatabaseOpen()” will be called and that instantiates and attaches an instance of “WebKitFaviconDatabaseClient” to listen for favicon URL changes.) That being said, the “webkitFaviconDatabaseSetIconForPageURL()” does also set the URI. I wonder whether there can be cases in which the URI is only set using this, and not from the “WebKitFaviconDatabaseClient”. I'll take a deeper into the code to figure out this. > Also: you added guards here, but forgot to add them in > WebKitFaviconDatabase.cpp, which is bad. So please ensure it's consistent. Correct, let's make the guards 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? Not necessarily, for the reasons outlined above. As said, I am not 100% sure so I'll double check. > 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? The web process functionality is also guarded. > > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitFaviconDatabase.cpp:246 > > static void testGetFaviconURI(FaviconDatabaseTest* test) > > { > > Is this test really still passing? I'm having some trouble running tests, but I'll get to have them run and passing before landing. > > 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? Most likely yes, I'll try. > > 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. Ok. > > 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. Ok. > (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.) Could it be just in case we would want to re-enable them later on? Keeping them in the same file but guarded makes it somewhat easier to follow VCS history and using functionality like “git blame”.

(In reply to Adrian Perez from comment #14) > The web process functionality is also guarded. IMO it would be better to not guard it, since we'll eventually want to use it for WPE and the reason we don't expose it for WPE is entirely specific to the API layer, not the implementation. But I'm OK with this if you prefer it this way, since you're doing the work. > Could it be just in case we would want to re-enable them later on? Keeping > them in the same file but guarded makes it somewhat easier to follow VCS > history and using functionality like “git blame”. Maybe. Carlos has been available for a week now, you can just ask him. ;)

(In reply to Michael Catanzaro from comment #13) > Comment on attachment 323937 [details] > 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.) What functions do you mean? there are only a few GTK ifdefs in WebKitWebView for several reasons: - Defining signal, properties or vfuncs, because all of them happen in class_init and I think it's better to have those together. We would need to make the prop and signal enums public (but private) somehow, and adding a platformClassInit or something like that. I don't think it's worth it. - Things that are not currently supported by WPE, but might be eventually, like the context menu API. I don't want to move that to Gtk to mov them back to the common file once WPE implements it. - Functions that emit signals. Again, I don't want to make the signals enum public or switch to use g_signal_emit_by_name. - Functions using private members. The priv struct is also private to the common cpp file, we could move it to the private header, but the web view private header is included by many other files that shouldn't have access to the struct directly. Maybe we could add another private header Internal.h or something like that. Again, I don't think it's worth it.

Thanks for the reply. (In reply to Carlos Garcia Campos from comment #16) > - Things that are not currently supported by WPE, but might be eventually, > like the context menu API. I don't want to move that to Gtk to mov them back > to the common file once WPE implements it. Adrian, I think it's fine to stick with #ifdefs instead of moving the functions, like you wanted.

(In reply to Michael Catanzaro from comment #17) > Thanks for the reply. > > (In reply to Carlos Garcia Campos from comment #16) > > - Things that are not currently supported by WPE, but might be eventually, > > like the context menu API. I don't want to move that to Gtk to mov them back > > to the common file once WPE implements it. > > Adrian, I think it's fine to stick with #ifdefs instead of moving the > functions, like you wanted. Okay. Also I have finally managed to run the API tests and the ones for “WebKitFaviconDatabase” pass fine with all the guards added by the patch, which mans the URIs for favicons are being updated correctly. I'm right now addressing the rest of review comments before re-uploading.

Created attachment 324680 [details] Patch

(In reply to Adrian Perez from comment #19) > Created attachment 324680 [details] > Patch Updated, with the missing guards added (which allowed to guard a couple more of unused functions), and *not* moving the implementations to the “*Gtk.cpp” files as agreed after having Carlos' feedback.

I still think it's better to not add #if PLATFORM(GTK) guards except for our API layer, but this is OK. Thanks for your patience when I repeatedly requested more changes. And be sure to add it to the 2.18 backports list.

(In reply to Michael Catanzaro from comment #21) > I still think it's better to not add #if PLATFORM(GTK) guards except for our > API layer, but this is OK. Thanks for your patience when I repeatedly > requested more changes. And be sure to add it to the 2.18 backports list. No prob, sometimes things just take time. Thanks for reviewing and helping out with this ;-)

Comment on attachment 324680 [details] Patch Clearing flags on attachment: 324680 Committed r223953: <https://trac.webkit.org/changeset/223953>

All reviewed patches have been landed. Closing bug.