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

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com>  2014-09-15 03:45:00 PST ---
(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.

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