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

[bugzilla-daemon at webkit.org]

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

--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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.

-- 
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/d4e4ba3a/attachment-0001.html>