[Webkit-unassigned] [Bug 202847] [GTK][WPE] Add user messages API

[bugzilla-daemon at webkit.org]

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

--- Comment #15 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Michael Catanzaro from comment #14)
> Comment on attachment 380972 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380972&action=review
> 
> LGTM. I'll leave r+ for Adrian, though.

Thanks!

> > Source/WebKit/Shared/API/glib/WebKitUserMessage.cpp:46
> > + * and Web extensions. A WebKitUserMessage always have a name, and it can also include parameters and
> 
> We've never capitalized web before, at least not in web process or web
> extension. I wouldn't start now.

I don't know who corrected me recently changing web to Web somewhere, anyway let's be consistent in our docs, I'll change it.

> Also: have -> has

Oops.

> > Source/WebKit/Shared/API/glib/WebKitUserMessage.cpp:49
> > + * corresponding #WebKitWebPage (and vice versa). One to one messages can be replied directly with
> 
> "replied to"

Ok.

> > Source/WebKit/Shared/API/glib/WebKitUserMessage.cpp:299
> > +    GRefPtr<WebKitUserMessage> adoptedReply = reply;
> 
> Probably a good idea to add your one-line comment about sinking the message
> here, since you did everywhere else, and it's not obvious.

Didn't add it in this case because I thought the adopted prefix in the name was self-explanatory. I'll add the comment in any case, it won't hurt.

> > Source/WebKit/Shared/glib/ArgumentCodersGLib.cpp:46
> > +bool decode(Decoder& decoder, GRefPtr<GVariant>& variant)
> 
> Shouldn't it return Optional<GRefPtr<GVariant>&>, using Optional return
> instead of an out parameter, as you did for the UserMessage class? I think
> this is an old-style decoder that we're supposed to be moving away from,
> when you used the modern style in UserMessage.

Right, I'll use Optional instead.

> > Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp:162
> > +     * replied. Calling webkit_user_message_send_reply() will do nothing.
> 
> replied to.

Ok.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:586
> > +    void sendMedssageToAllExtensions(WebKitUserMessage* message)
> 
> Medssage -> Message

Oops.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:641
> > +static bool readFileDescritpor(int fd, char** contents, gsize* length)
> 
> Descritpor -> Descriptor

You are good catching my typos :-D

-- 
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/20191015/b72b7a07/attachment.html>