[Webkit-unassigned] [Bug 136802] [GTK] Fix layering violations in PasteboardGtk

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #2 from Darin Adler <darin at apple.com>  2014-09-14 12:27:55 PST ---
(From update of attachment 238082)
View in context: https://bugs.webkit.org/attachment.cgi?id=238082&action=review

> Source/WebCore/editing/gtk/EditorGtk.cpp:47
> +static PassRefPtr<DocumentFragment> createFragmentFromPasteBoardData(Pasteboard& pasteboard, Frame& frame, Range& range, bool allowPlainText, bool& chosePlainText)

Pasteboard is one word, not two.

> Source/WebCore/editing/gtk/EditorGtk.cpp:57
> +        RefPtr<DocumentFragment> fragment = createFragmentFromMarkup(*frame.document(), dataObject->markup(), emptyString(), DisallowScriptingAndPluginContent);
> +        if (fragment)

Could bundle that definition into the if statement.

> Source/WebCore/editing/gtk/EditorGtk.cpp:67
> +        RefPtr<DocumentFragment> fragment = createFragmentFromText(range, dataObject->text());
> +        if (fragment)

Could bundle that definition into the if statement.

> Source/WebCore/editing/gtk/EditorGtk.cpp:86
> +static bool getImageAndURLForElement(Element& element, RefPtr<Image>& image, URL& url)

This seems like platform independent code. I am surprised to find it in a platform-specific file. Is there some other file with this code that we could share with?

> Source/WebCore/editing/gtk/EditorGtk.cpp:92
> +    CachedImage* cachedImage = toRenderImage(renderer)->cachedImage();

Since we know the renderer is non-null, could use * on it explicitly. The compiler is probably smart enough to optimize out the nil check, but this idiom guarantees there won’t be one:

    toRenderImage(*renderer).cachedImage()

> Source/WebCore/editing/gtk/EditorGtk.cpp:100
> +    AtomicString urlString;

Could use const AtomicString* here and avoid copying the attributes and churning the reference count.

> Source/WebCore/editing/gtk/EditorGtk.cpp:102
> +        urlString = element.getAttribute(HTMLNames::srcAttr);

fastGetAttribute

> Source/WebCore/editing/gtk/EditorGtk.cpp:103
> +    else if (element.hasTagName(SVGNames::imageTag))

Could use isSVGImageElement here.

> Source/WebCore/editing/gtk/EditorGtk.cpp:104
> +        urlString = element.getAttribute(XLinkNames::hrefAttr);

fastGetAttribute

> Source/WebCore/editing/gtk/EditorGtk.cpp:105
> +    else if (element.hasTagName(HTMLNames::embedTag) || isHTMLObjectElement(element))

Could use isHTMLEmbedElement here.

> Source/WebCore/editing/gtk/EditorGtk.cpp:108
> +    url = urlString.isEmpty() ? URL() : element.document().completeURL(stripLeadingAndTrailingHTMLSpaces(urlString));

Not sure it’s exactly right to special-case empty URLs before stripping the string. Maybe we should check for emptiness after stripping the leading and trailing spaces?

> Source/WebCore/platform/Pasteboard.h:85
> +    GClosure* callback;

I think this may create a problem in the future. It makes PasteboardWebContent not copyable on GTK. Is there a smart pointer we can use here?

> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:196
> +    PasteboardWebContent pasteboardContent;
> +    pasteboardContent.canSmartCopyOrDelete = false;
> +    pasteboardContent.text = range->text();
> +    pasteboardContent.markup = createMarkup(*range, nullptr, AnnotateForInterchange, false, ResolveNonLocalURLs);
> +    pasteboardContent.callback = callback;
> +    Pasteboard::createForGlobalSelection()->write(pasteboardContent);

I’m a little worried by this code. Building a PasteboardWebContent directly seems like a job for a function in the platform directory. Maybe we can find a different good place for this code, and just call it here.

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