[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 #67878|review?                     |review-
               Flag|                            |




--- Comment #8 from Darin Adler <darin at apple.com>  2010-09-17 10:33:56 PST ---
(From update of attachment 67878)
View in context: https://bugs.webkit.org/attachment.cgi?id=67878&action=prettypatch

I think the load of the other WebKit framework in WebKit2 should be dynamic. Under many circumstances we can use WebKit2 without ever needing to run this code. The code should only load the WebKit framework if it’s actually run.

Loading the WebKit framework for converting RTF seems OK. Loading it just to convert the URL seems like a bad idea, although perhaps OK for now.

> WebKit2/WebProcess/WebPage/WebPage.h:53
> +#ifdef __OBJC__
> + at class NSAttributedString;
> + at class NSString;
> + at class NSURL;
> +#else

Is this part needed? I know the !OBJC part is needed, but I thought that when compiling Obj-C we would always have the data types from Foundation included as part of the precompiled header.

> WebKit2/WebProcess/WebPage/WebPage.h:158
> +#if PLATFORM(MAC)
> +    NSString* userVisibleString(NSURL*);
> +    WebCore::DocumentFragment* documentFragmentFromAttributedString(NSAttributedString*, Vector<WebCore::ArchiveResource*>&);
> +#endif

Do we really need to use the Cocoa types here? That makes this code Mac-specific when it could otherwise possibly be shared with other platforms like Windows when they use CF. What about the CF types, CFStringRef, CFURLRef, and CFAttributedStringRef? At least the first two are “toll-free bridged”.

The new code is inconsistent about the position of the "*" characters. Our strange rule is that Obj-C types always have a space before the *. I would like to change that rule in the future, but I would like you to be consistent within the patch at least, if possible.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:291
> +static NSArray* excludedElementsForAttributedStringConversion()

A better style for this is to make a separate create function for the one time code, and then write:

    static NSArray *elements = createExcludedElementsArray();
    return elements;

It’s cleaner without the "== nil" and if statement and such.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:305
> +    static NSArray *elements = nil;
> +    if (elements == nil) {
> +        elements = [[NSArray alloc] initWithObjects:
> +                    // Omit style since we want style to be inline so the fragment can be easily inserted.
> +                    @"style",
> +                    // Omit xml so the result is not XHTML.
> +                    @"xml", 
> +                    // Omit tags that will get stripped when converted to a fragment anyway.
> +                    @"doctype", @"html", @"head", @"body",
> +                    // Omit deprecated tags.
> +                    @"applet", @"basefont", @"center", @"dir", @"font", @"isindex", @"menu", @"s", @"strike", @"u",
> +                    // Omit object so no file attachments are part of the fragment.
> +                    @"object", nil];

I understand that you were copying existing code, but typically we avoid code lined up with the previous lines like this, because it’s not friendly when renaming among other reasons.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:313
> +    WebView* view = [[WebView alloc] initWithFrame:NSMakeRect(0, 0, 0, 0)];

Calling the init method will call initWithFrame, so there’s no need to call initWithFrame:. Also, you can use NSEmptyRect in cases like this if you do need this rectangle.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:317
> +    NSDictionary *dictionary = [[NSDictionary alloc] initWithObjectsAndKeys:
> +                                excludedElementsForAttributedStringConversion(), NSExcludedElementsDocumentAttribute,
> +                                nil, @"WebResourceHandler", nil];

Again, I prefer to avoid lining up like this.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:319
> +    NSArray* s;

I much prefer words for local variable names over letters. I would name this “subresources”.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:324
> +    NSEnumerator *e = [s objectEnumerator];

And this, “enumerator”.

Since this code doesn’t need to run on systems older than Snow Leopard, you can use Objective-C 2.0 style enumeration which is a lot cleaner. I don’t know the syntax myself, but I know it’s the preferred style.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:327
> +        RefPtr<ArchiveResource>  ar = [r _coreResource];

There’s an extra space here. I don’t think you need to put this into a local variable. It would read better if you didn’t.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:328
> +        resources.append(ar.get());

Why is resources an array of raw pointers? What guarantees the ArchiveResource objects will stay alive? I think it probably needs to be a vector of RefPtr, unless I’m missing something.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:338
> +NSString* WebPage::userVisibleString(NSURL* URL)
> +{
> +    return [URL _web_userVisibleString];
> +}

I don’t understand why we are indirecting here through a WebPage function. Why can’t WebEditorClient just call this function directly?

In fact, in both cases I am a little puzzled. It seems that putting this code into WebPage just makes WebPage more of a god class. Perhaps there’s a way we can avoid this. These functions don’t seem to rely on being members of WebPage, so they could be static members if they are going to be members, but I am not sure they get any benefit from being class members.

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