[Webkit-unassigned] [Bug 42317] Pasteboard doesn't work in WebKit2

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #68788|review?                     |review+
               Flag|                            |




--- Comment #20 from Darin Adler <darin at apple.com>  2010-09-24 17:25:38 PST ---
(From update of attachment 68788)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.