Bug 238734 - [WPE] Mark "backend" parameters of web view constructors not nullable
Summary: [WPE] Mark "backend" parameters of web view constructors not nullable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-04 06:11 PDT by Adrian Perez
Modified: 2022-04-05 02:01 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.82 KB, patch)
2022-04-04 06:15 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Screenshot of the generated documentation with patch applied (167.45 KB, image/png)
2022-04-04 06:26 PDT, Adrian Perez
no flags Details
Patch v2 (7.18 KB, patch)
2022-04-04 13:45 PDT, Adrian Perez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrian Perez 2022-04-04 06:11:30 PDT
In theory according to the documentation NULL is mentioned as a possible
value to use “the default backend” but in practice there has always been
a g_return_value_if_fail(backend, NULL) guarding such usages.
Comment 1 Adrian Perez 2022-04-04 06:15:11 PDT
Created attachment 456564 [details]
Patch
Comment 2 Martin Robinson 2022-04-04 06:20:53 PDT
Comment on attachment 456564 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456564&action=review

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:65
> + * @backend: (transfer full) (not nullable): the WPE view backend to use.

Hrm. "WPE view backend" seems a bit more ambiguous here than [class@WebKitWebViewBackend]. Is there some reason to use phrases like this rather than the class name?
Comment 3 Adrian Perez 2022-04-04 06:23:51 PDT
(In reply to Martin Robinson from comment #2)
> Comment on attachment 456564 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456564&action=review
> 
> > Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:65
> > + * @backend: (transfer full) (not nullable): the WPE view backend to use.
> 
> Hrm. "WPE view backend" seems a bit more ambiguous here than
> [class@WebKitWebViewBackend]. Is there some reason to use phrases like this
> rather than the class name?

Yes: the documentation output already writes the type by itself, there is
no need to repeat it every single time. You can find example output here:

  https://people.igalia.com/aperez/Documentation/wpe-webkit-1.1/ctor.WebView.new.html

The documentation comment is expected to *describe* what each parameter
is and/or does, not repeating the type, which the documentation generator
already knows.
Comment 4 Adrian Perez 2022-04-04 06:26:27 PDT
Created attachment 456565 [details]
Screenshot of the generated documentation with patch applied

Hopefully this helps understand how it is unneeded to manually
repeat the type of parameters and returned values, given that
the documentation generator will anyway write them in the output
by itself :-)
Comment 5 Martin Robinson 2022-04-04 06:32:01 PDT
(In reply to Adrian Perez from comment #3)

> Yes: the documentation output already writes the type by itself, there is
> no need to repeat it every single time. You can find example output here:
> 
>  
> https://people.igalia.com/aperez/Documentation/wpe-webkit-1.1/ctor.WebView.
> new.html
> 
> The documentation comment is expected to *describe* what each parameter
> is and/or does, not repeating the type, which the documentation generator
> already knows.

I think the issue is that a phrase like "WPE view backend" doesn't really describe what the parameter does, because it's more of less the type name with spaces in it. As an outsider to WPEWebKit, it is difficult to understand what purpose this parameter serves. It's even a little inaccurate in that a "WPE view backend" isn't the same as a WebKitWebViewBackend, which is a wrapper around a WPE view backend.
Comment 6 Adrian Perez 2022-04-04 13:45:37 PDT
Created attachment 456622 [details]
Patch v2

Now with (hopefully) better wording :)
Comment 7 EWS 2022-04-05 00:29:43 PDT
Committed r292379 (249244@main): <https://commits.webkit.org/249244@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456622 [details].
Comment 8 Radar WebKit Bug Importer 2022-04-05 00:30:19 PDT
<rdar://problem/91281170>