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

[bugzilla-daemon at webkit.org]

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

--- Comment #5 from Adrian Perez <aperez at igalia.com> ---
(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.

-- 
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/46bd2957/attachment.html>