[webkit-reviews] review granted: [Bug 136802] [GTK] Fix layering violations in PasteboardGtk : [Attachment 238082] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 136802: [GTK] Fix layering violations in PasteboardGtk
https://bugs.webkit.org/show_bug.cgi?id=136802

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

------- Additional Comments from Darin Adler <darin at apple.com>
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.