A builtin protocol handler to show information about hardware acceleration. This is useful information we need from people reporting issues in accelerated compositing mode.
Created attachment 379717 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 379717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379717&action=review This will be neat to have; I have added a few comments with improvement proposals, but by no means they are blockers for landing the patch. Feel free to use a follow-up patch instead of uploading a new one to this bug. > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:97 > + return "WPEWEbKit"; As this is user-visible, I suppose we may want to use "WPE WebKit", which our preferred spelling for the port name ;-) > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:190 > + " <td><div class=\"titlename\">WebKit version</div></td>" Instead of <td> and <div class="..."> you could directly use <td class="...">. Or, even better: <tr><th scope="row">Key</th><td>Value</td></tr> Then in the CSS snippet above you can just use: th { font-weight: bold; } This makes the needed HTML shorter (small is good!) and much, much more semantic, with “scope="row"” attribute indicating that the <th> cell contains the title for data in the table row (for example a11y technologies can use this information). > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:200 > + " <tbody><tr>" Is there any practical reason why you are putting each HTML table row inside its own <tbody> element, instead of all <tr> rows inside a single <tbody>? Currently the code in the patch is generating: <table> ... <tbody><tr><td>Key1</td><td>Value1</td></tr></tbody> ... <tbody><tr><td>KeyN</td><td>ValueN</td></tr></tbody> </table> while the following is more common: <table> ... <tbody> <tr><th scope="row">Key1</th><td>Value1</td></tr> ... <tr><th scope="row">KeyN</th><td>ValueN</td></tr> </tbody> </table> > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:219 > + CAIRO_VERSION_MAJOR, CAIRO_VERSION_MINOR, CAIRO_VERSION_MICRO); You could use “CAIRO_VERSION_STRING” for simplicity here. I would call this line “Cairo build version” and add an additional “Cairo runtime version”, using the value returned by “cairo_version_string()” — the version being loaded at run time may not match the headers used when building; for example if a distribution builds WebKitGTK, then updates to the next Cairo micro version but does not rebuild the WebKitGTK package because the ABI of the Cairo library has not changed. Having both versions can be useful to diagnose situations in which users might be doing unexpected things like setting “$LD_LIBRARY_PATH” and accidentally loading an older Cairo version which happens to have the same ABI but we know is bugged. > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:227 > + GTK_MAJOR_VERSION, GTK_MINOR_VERSION, GTK_MICRO_VERSION); Same here, I would have an additional line with the GTK version being loaded at runtime, which can obtained with “gtk_get_{major,minor,micro}_version()” functions. > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:236 > + WPE_FDO_MAJOR_VERSION, WPE_FDO_MINOR_VERSION, WPE_FDO_MICRO_VERSION, Here we could use “wpe_get_{major,minor,micro}_version()” for the runtime version of libwpe — sadly we do not have an equivalent for WPEBackend-fdo, so I have filed at issue for that at https://github.com/Igalia/WPEBackend-fdo/issues/80 > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:304 > + hardwareAccelerationPolicy(request)); Would it be useful to also print the value of the “WEBKIT_FORCE_COMPOSITING_MODE” environment variable? I suppose it can be good to know whether the setting has been forced due to the environment variable being set.
(In reply to Adrian Perez from comment #3) > Comment on attachment 379717 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=379717&action=review > > This will be neat to have; I have added a few comments with improvement > proposals, but by no means they are blockers for landing the patch. Feel > free to use a follow-up patch instead of uploading a new one to this bug. Sure, I have also a couple of improvements in mind to be added in followup patches, like adding a button to copy the report to the clipboard (or save it to a file that can be easily uploaded to bz). We could also add info about the distro. > > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:97 > > + return "WPEWEbKit"; > > As this is user-visible, I suppose we may want to use "WPE WebKit", > which our preferred spelling for the port name ;-) Sure. > > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:190 > > + " <td><div class=\"titlename\">WebKit version</div></td>" > > Instead of <td> and <div class="..."> you could directly use <td > class="...">. > Or, even better: > > <tr><th scope="row">Key</th><td>Value</td></tr> > > Then in the CSS snippet above you can just use: > > th { font-weight: bold; } > > This makes the needed HTML shorter (small is good!) and much, much more > semantic, > with “scope="row"” attribute indicating that the <th> cell contains the > title for > data in the table row (for example a11y technologies can use this > information). I don't know HTML, I copy pasted this from remoce inspector custom protocol impl, which was copied from ephy. > > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:200 > > + " <tbody><tr>" > > Is there any practical reason why you are putting each HTML table row > inside its own <tbody> element, instead of all <tr> rows inside a > single <tbody>? > > Currently the code in the patch is generating: > > <table> > ... > <tbody><tr><td>Key1</td><td>Value1</td></tr></tbody> > ... > <tbody><tr><td>KeyN</td><td>ValueN</td></tr></tbody> > </table> > > while the following is more common: > > <table> > ... > <tbody> > <tr><th scope="row">Key1</th><td>Value1</td></tr> > ... > <tr><th scope="row">KeyN</th><td>ValueN</td></tr> > </tbody> > </table> No reason, as I said I don't know html, nor css either, so I just copy pasted. > > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:219 > > + CAIRO_VERSION_MAJOR, CAIRO_VERSION_MINOR, CAIRO_VERSION_MICRO); > > You could use “CAIRO_VERSION_STRING” for simplicity here. I would call this > line “Cairo build version” and add an additional “Cairo runtime version”, > using the value returned by “cairo_version_string()” — the version being > loaded at run time may not match the headers used when building; for > example if a distribution builds WebKitGTK, then updates to the next > Cairo micro version but does not rebuild the WebKitGTK package because > the ABI of the Cairo library has not changed. Having both versions can > be useful to diagnose situations in which users might be doing unexpected > things like setting “$LD_LIBRARY_PATH” and accidentally loading an older > Cairo version which happens to have the same ABI but we know is bugged. Good point. Maybe we could keep at as a single Foo Version and then add x.y.z (build) x.y.z (runtime). Both are indeed useful. > > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:227 > > + GTK_MAJOR_VERSION, GTK_MINOR_VERSION, GTK_MICRO_VERSION); > > Same here, I would have an additional line with the GTK version being loaded > at runtime, which can obtained with “gtk_get_{major,minor,micro}_version()” > functions. Right. > > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:236 > > + WPE_FDO_MAJOR_VERSION, WPE_FDO_MINOR_VERSION, WPE_FDO_MICRO_VERSION, > > Here we could use “wpe_get_{major,minor,micro}_version()” for > the runtime version of libwpe — sadly we do not have an equivalent > for WPEBackend-fdo, so I have filed at issue for that at > https://github.com/Igalia/WPEBackend-fdo/issues/80 > > > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:304 > > + hardwareAccelerationPolicy(request)); > > Would it be useful to also print the value of the > “WEBKIT_FORCE_COMPOSITING_MODE” environment variable? I suppose > it can be good to know whether the setting has been forced due > to the environment variable being set. I'm not sure that's useful, but I don't mind to add it.
(In reply to Carlos Garcia Campos from comment #4) > (In reply to Adrian Perez from comment #3) > > Comment on attachment 379717 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=379717&action=review > > > > This will be neat to have; I have added a few comments with improvement > > proposals, but by no means they are blockers for landing the patch. Feel > > free to use a follow-up patch instead of uploading a new one to this bug. > > Sure, I have also a couple of improvements in mind to be added in followup > patches, like adding a button to copy the report to the clipboard (or save > it to a file that can be easily uploaded to bz). We could also add info > about the distro. Cool! For the information about the distribution, the “modern” way of doing it seems to be reading from “/etc/os-release” (I think even Debian has it nowadays). I don't expect it to be available in the BSDs, but for those the information from uname() is enough to know what the user is running (e.g. there is only one FreeBSD, and for “remixes” like Debian/kFreeBSD probably there will be an “/etc/os-release” anyway). > > > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:97 > > > + return "WPEWEbKit"; > > > > As this is user-visible, I suppose we may want to use "WPE WebKit", > > which our preferred spelling for the port name ;-) > > Sure. > > > > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:190 > > > + " <td><div class=\"titlename\">WebKit version</div></td>" > > > > Instead of <td> and <div class="..."> you could directly use <td > > class="...">. > > Or, even better: > > > > <tr><th scope="row">Key</th><td>Value</td></tr> > > > > Then in the CSS snippet above you can just use: > > > > th { font-weight: bold; } > > > > This makes the needed HTML shorter (small is good!) and much, much more > > semantic, > > with “scope="row"” attribute indicating that the <th> cell contains the > > title for > > data in the table row (for example a11y technologies can use this > > information). > > I don't know HTML, I copy pasted this from remoce inspector custom protocol > impl, which was copied from ephy. > > > > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:200 > > > + " <tbody><tr>" > > > > Is there any practical reason why you are putting each HTML table row > > inside its own <tbody> element, instead of all <tr> rows inside a > > single <tbody>? > > > > Currently the code in the patch is generating: > > > > <table> > > ... > > <tbody><tr><td>Key1</td><td>Value1</td></tr></tbody> > > ... > > <tbody><tr><td>KeyN</td><td>ValueN</td></tr></tbody> > > </table> > > > > while the following is more common: > > > > <table> > > ... > > <tbody> > > <tr><th scope="row">Key1</th><td>Value1</td></tr> > > ... > > <tr><th scope="row">KeyN</th><td>ValueN</td></tr> > > </tbody> > > </table> > > No reason, as I said I don't know html, nor css either, so I just copy > pasted. That's fine, like I said it's just a suggestion. At the end of the day “about:gpu” will be a debugging feature and it does not need to be pretty :) > > > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:219 > > > + CAIRO_VERSION_MAJOR, CAIRO_VERSION_MINOR, CAIRO_VERSION_MICRO); > > > > You could use “CAIRO_VERSION_STRING” for simplicity here. I would call this > > line “Cairo build version” and add an additional “Cairo runtime version”, > > using the value returned by “cairo_version_string()” — the version being > > loaded at run time may not match the headers used when building; for > > example if a distribution builds WebKitGTK, then updates to the next > > Cairo micro version but does not rebuild the WebKitGTK package because > > the ABI of the Cairo library has not changed. Having both versions can > > be useful to diagnose situations in which users might be doing unexpected > > things like setting “$LD_LIBRARY_PATH” and accidentally loading an older > > Cairo version which happens to have the same ABI but we know is bugged. > > Good point. Maybe we could keep at as a single Foo Version and then add > x.y.z (build) x.y.z (runtime). Both are indeed useful. A single line sounds good as well 👍 > > Would it be useful to also print the value of the > > “WEBKIT_FORCE_COMPOSITING_MODE” environment variable? I suppose > > it can be good to know whether the setting has been forced due > > to the environment variable being set. > > I'm not sure that's useful, but I don't mind to add it. No strong opinion. I was thinking of cases where we ask users to set the variable and test something for us, so we can be sure that they have set it to the value we have asked them to use. Mainly that, because I don't expect it to be set accidentally.
(In reply to Adrian Perez from comment #5) [...] > > > Would it be useful to also print the value of the > > > “WEBKIT_FORCE_COMPOSITING_MODE” environment variable? I suppose > > > it can be good to know whether the setting has been forced due > > > to the environment variable being set. > > > > I'm not sure that's useful, but I don't mind to add it. > > No strong opinion. > > I was thinking of cases where we ask users to set the variable and > test something for us, so we can be sure that they have set it to > the value we have asked them to use. Mainly that, because I don't > expect it to be set accidentally. The env vars change the setting, so if you force the AC mode using the env var, you will see "always" in about:gpu (and never if you disable it)
Created attachment 379720 [details] Updated patch Added some of Adri's suggestions and also added the WPE version and backend lib name for WPE that I had forgotten.
Comment on attachment 379720 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=379720&action=review > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:97 > + return "WPE WEbKit"; Typo. > Source/WebKit/UIProcess/API/glib/WebKitProtocolHandler.cpp:134 > +#if USE(LIBEPOXY) > + if (epoxy_is_desktop_gl()) > + return "OpenGL"; > + return "OpenGL ES 2"; In this case we could append "(libepoxy)" to the end of the GL string.
Committed r250517: <https://trac.webkit.org/changeset/250517>