The Mac specific code that creates the drag image has some inconsistencies between the way the image is created from a selection and the one created from a link. Also both call into the NSView directly from the WebCore level. It would be nice to clean up this code, to avoid using the NSView from WebCore. This is requirement for WebKit2.
Created attachment 79033 [details] Patch
Comment on attachment 79033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79033&action=review Just nit comments since I don't know Cocoa / Objective-C. > Source/WebCore/page/mac/FrameMac.mm:281 > + Nit: space. > Source/WebCore/page/mac/FrameMac.mm:284 > + Ditt. > WebKit2/UIProcess/API/mac/WKView.mm:201 > + //[self _registerDraggedTypes]; Maybe you want to delete this line? > WebKit2/UIProcess/API/mac/WKView.mm:1018 > +/* > - (NSDragOperation)draggingEntered:(id <NSDraggingInfo>)draggingInfo Ditto. > WebKit2/UIProcess/API/mac/WKView.mm:1059 > +*/ Ditto. > WebKit/mac/WebCoreSupport/WebDragClient.mm:64 > +#define DRAG_LABEL_BORDER_Y_OFFSET 2.0f > + > +#define MIN_DRAG_LABEL_WIDTH_BEFORE_CLIP 120.0f > +#define MAX_DRAG_LABEL_WIDTH 320.0f > + > +#define DRAG_LINK_LABEL_FONT_SIZE 11.0f > +#define DRAG_LINK_URL_FONT_SIZE 10.0f Maybe nice to align these numbers to above 3 numbers?
Comment on attachment 79033 [details] Patch r- for now because of your commenting out code.
(In reply to comment #3) > (From update of attachment 79033 [details]) > r- for now because of your commenting out code. Sorry that code was commented out by mistake. I was testing something and forgot to remove the comments before I prepared the patch. Uploading now the correct patch.
Created attachment 79044 [details] Patch2
Comment on attachment 79044 [details] Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=79044&action=review I don't really understand why DragClient is a better place for drag image generation, but OK. > Source/WebCore/page/mac/FrameMac.mm:-265 > + PaintBehavior oldBehavior = m_view->paintBehavior(); > + m_view->setPaintBehavior(oldBehavior | PaintBehaviorFlattenCompositingLayers); > > - PaintBehavior oldPaintBehavior = m_view->paintBehavior(); > - m_view->setPaintBehavior(oldPaintBehavior | PaintBehaviorFlattenCompositingLayers); What's wrong about "oldPaintBehavior" name? It seems more clear to me. > Source/WebCore/page/mac/FrameMac.mm:-275 > - // Round image rect size in window coordinate space to avoid pixel cracks at HiDPI (4622794) > - rect = [view convertRect:rect toView:nil]; > rect.size.height = roundf(rect.size.height); > rect.size.width = roundf(rect.size.width); > - rect = [view convertRect:rect fromView:nil]; So, this is no longer necessary for 4622794? > Source/WebCore/page/mac/FrameMac.mm:275 > + m_view->paintContents(&graphicsContext, IntRect(rect)); Can m_view be null? I'm asking myself the same question as there's almost identical code in PrintContext, and it doesn't check for null - but general Frame code has a lot of null checks for null m_view. > WebKit/mac/WebCoreSupport/WebDragClient.mm:65 > +#define DRAG_LABEL_BORDER_X 4.0f > +//Keep border_y in synch with DragController::LinkDragBorderInset > +#define DRAG_LABEL_BORDER_Y 2.0f > +#define DRAG_LABEL_RADIUS 5.0f > +#define DRAG_LABEL_BORDER_Y_OFFSET 2.0f > + > +#define MIN_DRAG_LABEL_WIDTH_BEFORE_CLIP 120.0f > +#define MAX_DRAG_LABEL_WIDTH 320.0f > + > +#define DRAG_LINK_LABEL_FONT_SIZE 11.0f > +#define DRAG_LINK_URL_FONT_SIZE 10.0f > + We're now using consts (appropriately named, not in ALL_CAPS) instead of defines. Const values needn't have ".0f" suffixes. > WebKit/mac/WebCoreSupport/WebDragClient.mm:165 > + NSSize imageSize, urlStringSize; Please don't declare two variables on one line. In this case, urlStringSize should go inside an if block below. > WebKit/mac/WebCoreSupport/WebDragClient.mm:177 > + } else { > + imageSize.width = max(labelSize.width + DRAG_LABEL_BORDER_X * 2, urlStringSize.width + DRAG_LABEL_BORDER_X * 2); > + } No braces around single line blocks. > WebKit/mac/WebCoreSupport/WebDragClient.mm:182 > + [[NSColor colorWithDeviceRed: 0.7f green: 0.7f blue: 0.7f alpha: 0.8f] set]; I don't think that we're putting spaces after semicolons. This style mistake is repeated elsewhere in the moved code. > WebKit/mac/WebCoreSupport/WebDragClient.mm:184 > + // Drag a rectangle with rounded corners/ s/\//./ > WebKit/mac/WebCoreSupport/WebDragClient.mm:186 > + [path appendBezierPathWithOvalInRect: NSMakeRect(0.0f, 0.0f, DRAG_LABEL_RADIUS * 2.0f, DRAG_LABEL_RADIUS * 2.0f)]; 0.0.f -> 0; 2.0f -> 2
(In reply to comment #6) > (From update of attachment 79044 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79044&action=review > > I don't really understand why DragClient is a better place for drag image generation, but OK. The best place to have this code would be WebCore and avoid using a platform specific image, but I did not want to make too many changes to this code, since the goal is to support drag and drop. At least not it is consistent with the Windows implementation. As fas as all the other comments, I just copied the code from WebHTMLView.mm. I'll make the suggested changes. Thank you.
> - // Round image rect size in window coordinate space to avoid pixel cracks at HiDPI (4622794) > > - rect = [view convertRect:rect toView:nil]; > > rect.size.height = roundf(rect.size.height); > > rect.size.width = roundf(rect.size.width); > > - rect = [view convertRect:rect fromView:nil]; > > So, this is no longer necessary for 4622794? I spoke with Anders and he confirmed that we changed the way we handle this issue. Therefore I'll remove the 2 roundf statements as well.
http://trac.webkit.org/changeset/75963