[webkit-reviews] review granted: [Bug 42317] Pasteboard doesn't work in WebKit2 : [Attachment 68788] Patch5

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Enrica Casucci <enrica at apple.com>'s
request for review:
Bug 42317: Pasteboard doesn't work in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=42317

Attachment 68788: Patch5
https://bugs.webkit.org/attachment.cgi?id=68788&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=68788&action=review

This patch seems to break the Qt WebKit2. If you resolve that seems OK to land.


> WebKit2/UIProcess/API/mac/WKView.mm:72
> +	   MenuItemInfo(): _isEnabled(false), _state(0) {};
> +	   MenuItemInfo(bool isEnabled, int state): _isEnabled(isEnabled),
_state(state) {};

Need a space before the ":". No semicolon needed after the "{}". We normally
put a space in the "{ }".

You call this EditCommandState in the PageClient API. Why not call it that here
too?

No need for underscores in this class’s members. We normally don’t use them for
struct-style classes like this one. We do use them for Objective-C classes.

I suggest struct instead of class for something where everything is public.

Not sure it’s good to nest a C++ class inside an Objective-C class definition.
I suggest putting it outside WKViewData.

> WebKit2/UIProcess/API/mac/WKView.mm:207
> +// FIXME: we should add here all the commands as we implement them.

Grammar mistake. Should be "We should add all the commands here as we implement
them."

> WebKit2/UIProcess/API/mac/WKView.mm:227
> +	   return NO;	   // FIXME: we need to be able to handle other user
interface elements.

We capitalize comments, using sentence style. We normally put comments only one
space past the code, not 6 spaces.

> WebKit2/UIProcess/API/mac/WKView.mm:496
> +    _data->_needsDataForValidation = false;

I think the “needs data for validation” mechanism is a bit roundabout. We could
try replacing it with a boolean that means “I am inside the update call”
instead to see if that results in easier to read code.