Bug 202305 – [GTK][WPE] Add about:gpu

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

A builtin protocol handler to show information about hardware acceleration. This is useful information we need from people reporting issues in accelerated compositing mode.

[Carlos Garcia Campos]

Created attachment 379717 [details]
Patch

[EWS Watchlist]

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

[Adrian Perez]

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.

[Carlos Garcia Campos]

(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.

[Adrian Perez]

(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.

[Carlos Garcia Campos]

(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)

[Carlos Garcia Campos]

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.

[Zan Dobersek]

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.

[Carlos Garcia Campos]

Committed r250517: <https://trac.webkit.org/changeset/250517>