[Webkit-unassigned] [Bug 202305] [GTK][WPE] Add about:gpu

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=202305

--- Comment #3 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 379717
  --> https://bugs.webkit.org/attachment.cgi?id=379717
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190927/34d86ef7/attachment.html>