Bug 136802 – [GTK] Fix layering violations in PasteboardGtk

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Fix layering violations in PasteboardGtk and friends

[Carlos Garcia Campos]

Created attachment 238082 [details]
Patch

[Darin Adler]

Comment on attachment 238082 [details]
Patch

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.

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 238082 [details])
> 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.

Oops

> > 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.

Sure.

> > Source/WebCore/editing/gtk/EditorGtk.cpp:67
> > +        RefPtr<DocumentFragment> fragment = createFragmentFromText(range, dataObject->text());
> > +        if (fragment)
> 
> Could bundle that definition into the if statement.

Sure.

> > 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?

Yes, there's similar method in EditorMac.mm, getImage() but it only gets the image not the URL. I don't know exactly why we are not using the given URL instead, like mac, I think it's because one of the callers use the document URL. I've split the methods anyway, so that we can share the getImage(), but I'm not sure where to move the code, since it's a static function in both cases and doesn't seem to fit as members of Editor, no?

> > 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()

Ok.

> > Source/WebCore/editing/gtk/EditorGtk.cpp:100
> > +    AtomicString urlString;
> 
> Could use const AtomicString* here and avoid copying the attributes and churning the reference count.

I moved this to a new helper function that returns a const AtomicString&

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

Ok.

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

Ok.

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

Ok.

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

Ok.

> > 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?

Actually we don't even need to check if the string is empty, stripLeadingAndTrailingHTMLSpaces returns a null string when the given string is null and completeURL returns a null URL when the given string is null.

> > 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?

Indeed, and I've noticed we are indeed leaking the closure in some cases. I'll use GRefPtr to fix both issues.

> > 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.

Current methods building a PasteboardWebContent are not actually in platform, but in port-specific files in WebCore, Editor files mainly I think.

[Carlos Garcia Campos]

Created attachment 238115 [details]
Patch for landing

Addressed review comments

[WebKit Commit Bot]

Attachment 238115 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/gtk/EditorGtk.cpp:85:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 238118 [details]
Fixed coding style issues

[Carlos Garcia Campos]

Committed r173687: <http://trac.webkit.org/changeset/173687>