Bug 21358 - DragData should not depend on Clipboard, DocumentFragment, and Document
Summary: DragData should not depend on Clipboard, DocumentFragment, and Document
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 21354
  Show dependency treegraph
 
Reported: 2008-10-03 22:09 PDT by Sam Weinig
Modified: 2014-09-16 23:28 PDT (History)
8 users (show)

See Also:


Attachments
Patch (26.66 KB, patch)
2014-08-13 03:17 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix EFL and Win build (27.51 KB, patch)
2014-08-13 03:52 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix EFL and win build (27.90 KB, patch)
2014-08-13 05:43 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (28.58 KB, patch)
2014-08-14 00:26 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch (29.82 KB, patch)
2014-08-27 03:51 PDT, Carlos Garcia Campos
darin: review+
Details | Formatted Diff | Diff
Patch for landing (29.50 KB, patch)
2014-09-15 04:49 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix windows build (29.61 KB, patch)
2014-09-16 01:28 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix windows build again (29.60 KB, patch)
2014-09-16 08:14 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2008-10-03 22:09:39 PDT
It is a layering violation for DragData to depend on Clipboard, DocumentFragment, and Document.
Comment 1 Carlos Garcia Campos 2014-08-13 03:17:48 PDT
Created attachment 236510 [details]
Patch
Comment 2 Carlos Garcia Campos 2014-08-13 03:52:29 PDT
Created attachment 236511 [details]
Try to fix EFL and Win build
Comment 3 Carlos Garcia Campos 2014-08-13 05:43:03 PDT
Created attachment 236519 [details]
Try to fix EFL and win build
Comment 4 Sam Weinig 2014-08-13 11:04:56 PDT
Comment on attachment 236519 [details]
Try to fix EFL and win build

View in context: https://bugs.webkit.org/attachment.cgi?id=236519&action=review

> Source/WebCore/page/gtk/DragControllerGtk.cpp:83
> +    if (!dragData.platformData()->hasMarkup() || !frame->document())

Since we are not null-checking the Frame here, can we pass it by reference? Or do we need to null check it?
Comment 5 Carlos Garcia Campos 2014-08-14 00:10:20 PDT
(In reply to comment #4)
> (From update of attachment 236519 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236519&action=review
> 
> > Source/WebCore/page/gtk/DragControllerGtk.cpp:83
> > +    if (!dragData.platformData()->hasMarkup() || !frame->document())
> 
> Since we are not null-checking the Frame here, can we pass it by reference? Or do we need to null check it?

We can indeed pass it by reference, the caller should ensure a valid a frame is passed, and currently the only caller does it:

    RefPtr<Frame> innerFrame = element->document().frame();
    ASSERT(innerFrame);
Comment 6 Carlos Garcia Campos 2014-08-14 00:26:19 PDT
Created attachment 236580 [details]
Updated patch

Pass the Frame by reference
Comment 7 Carlos Garcia Campos 2014-08-27 03:51:28 PDT
Created attachment 237217 [details]
Rebased patch

Rebased to make it build on top of current git master
Comment 8 Darin Adler 2014-09-14 12:16:09 PDT
Comment on attachment 237217 [details]
Rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237217&action=review

We should make sure we are adding the minimum includes here.

> Source/WebCore/page/DragController.h:119
> +        static PassRefPtr<DocumentFragment> documentFragmentFromDragData(DragData&, Frame&, Range&, bool allowPlainText, bool& chosePlainText);
> +        static PassRefPtr<DocumentFragment> createFragmentFromDragData(DragData&, Frame&, Range&, bool allowPlainText, bool& chosePlainText);

The names of these two functions don’t make it at all clear why there are two of them and how they differ.

I think the second one exists only to have a platform-dependent hook to do things differently on every platform. Long term, I want to refactor this code so that it’s shared and not platform-specific. I think it can be done. In the meantime, I suggest a name that is clearer.

I’m also not sure that the first function needs to be a class member. Maybe it can just be a function with internal linkages inside the .cpp file and not in the header at all.

> Source/WebCore/page/win/DragControllerWin.cpp:93
> +     * Order is richest format first. On OSX this is:

You just moved this code, but this is a Windows-specific file. Not sure why a list of OS X formats is relevant. Also, it’s “OS X”, not “OSX”.

I suggest we just drop the comment entirely.

> Source/WebCore/page/win/DragControllerWin.cpp:122
> +    if (DragDataRef platformDragData = dragData.platformData()) {
> +        if (containsFilenames(platformDragData)) {
> +            if (PassRefPtr<DocumentFragment> fragment = fragmentFromFilenames(frame.document(), platformDragData))
> +                return fragment;
> +        }
> +
> +        if (containsHTML(platformDragData)) {
> +            if (PassRefPtr<DocumentFragment> fragment = fragmentFromHTML(frame.document(), platformDragData))
> +                return fragment;
> +        }
> +    } else {
> +        if (containsFilenames(&dragData.dragDataMap())) {
> +            if (PassRefPtr<DocumentFragment> fragment = fragmentFromFilenames(frame.document(), &dragData.dragDataMap()))
> +                return fragment;
> +        }
> +
> +        if (containsHTML(&dragData.dragDataMap())) {
> +            if (PassRefPtr<DocumentFragment> fragment = fragmentFromHTML(frame.document(), &dragData.dragDataMap()))
> +                return fragment;
> +        }
> +    }

You just moved this code, but sure would be nice to factor this so we don’t have to repeat everything twice.

> Source/WebCore/platform/DragData.h:95
> +    bool containsURL(FilenameConversionPolicy filenamePolicy = ConvertFilenames) const;

The argument name filenamePolicy doesn’t add anything and should be removed.

> Source/WebCore/platform/DragData.h:98
> +    String asURL(FilenameConversionPolicy filenamePolicy = ConvertFilenames, String* title = 0) const;

The argument name filenamePolicy doesn’t add anything and should be removed. Should use nullptr rather than 0.

> Source/WebCore/platform/mac/DragDataMac.mm:111
> +    // FIXME: It's not clear this is 100% correct since we know -[NSURL URLWithString:] does not handle
> +    // all the same cases we handle well in the URL code for creating an NSURL.

Where was this code moved from? It’s easy for us to rewrite this use WebCore::URL to make the NSURL.
Comment 9 Carlos Garcia Campos 2014-09-15 01:55:34 PDT
(In reply to comment #8)
> (From update of attachment 237217 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237217&action=review

Thanks for the review.

> We should make sure we are adding the minimum includes here.

I tried it for GTK+ port at least since it's the only port I can compile.

> > Source/WebCore/page/DragController.h:119
> > +        static PassRefPtr<DocumentFragment> documentFragmentFromDragData(DragData&, Frame&, Range&, bool allowPlainText, bool& chosePlainText);
> > +        static PassRefPtr<DocumentFragment> createFragmentFromDragData(DragData&, Frame&, Range&, bool allowPlainText, bool& chosePlainText);
> 
> The names of these two functions don’t make it at all clear why there are two of them and how they differ.
>
> I think the second one exists only to have a platform-dependent hook to do things differently on every platform. Long term, I want to refactor this code so that it’s shared and not platform-specific. I think it can be done. In the meantime, I suggest a name that is clearer.

We can create a Pasteboard for drag and drop with the given DragData, and use something like Editor::webContentFromPasteboard() implementing that for other platforms (it's mac specific now). But that will be easier once I land patch in bug #136802, so I'll leave just a FIXME here and will submit a new patch once this and #136802 are both fixed.
 
> I’m also not sure that the first function needs to be a class member. Maybe it can just be a function with internal linkages inside the .cpp file and not in the header at all.

Yes, I made it a member only because I needed to call the second one, that is implemented in port specific files. I could also make it public and pass the drag controller to the static function, but I thought it was better to keep them private. In any case, I'll remove the first one and make the second one static function again in a follow up patch, when moving the implementation to the editor.

> > Source/WebCore/page/win/DragControllerWin.cpp:93
> > +     * Order is richest format first. On OSX this is:
> 
> You just moved this code, but this is a Windows-specific file. Not sure why a list of OS X formats is relevant. Also, it’s “OS X”, not “OSX”.
> 
> I suggest we just drop the comment entirely.

Ok.

> > Source/WebCore/page/win/DragControllerWin.cpp:122
> > +    if (DragDataRef platformDragData = dragData.platformData()) {
> > +        if (containsFilenames(platformDragData)) {
> > +            if (PassRefPtr<DocumentFragment> fragment = fragmentFromFilenames(frame.document(), platformDragData))
> > +                return fragment;
> > +        }
> > +
> > +        if (containsHTML(platformDragData)) {
> > +            if (PassRefPtr<DocumentFragment> fragment = fragmentFromHTML(frame.document(), platformDragData))
> > +                return fragment;
> > +        }
> > +    } else {
> > +        if (containsFilenames(&dragData.dragDataMap())) {
> > +            if (PassRefPtr<DocumentFragment> fragment = fragmentFromFilenames(frame.document(), &dragData.dragDataMap()))
> > +                return fragment;
> > +        }
> > +
> > +        if (containsHTML(&dragData.dragDataMap())) {
> > +            if (PassRefPtr<DocumentFragment> fragment = fragmentFromHTML(frame.document(), &dragData.dragDataMap()))
> > +                return fragment;
> > +        }
> > +    }
> 
> You just moved this code, but sure would be nice to factor this so we don’t have to repeat everything twice.

Agree.

> > Source/WebCore/platform/DragData.h:95
> > +    bool containsURL(FilenameConversionPolicy filenamePolicy = ConvertFilenames) const;
> 
> The argument name filenamePolicy doesn’t add anything and should be removed.
> 
> > Source/WebCore/platform/DragData.h:98
> > +    String asURL(FilenameConversionPolicy filenamePolicy = ConvertFilenames, String* title = 0) const;
> 
> The argument name filenamePolicy doesn’t add anything and should be removed. Should use nullptr rather than 0.
> 
> > Source/WebCore/platform/mac/DragDataMac.mm:111
> > +    // FIXME: It's not clear this is 100% correct since we know -[NSURL URLWithString:] does not handle
> > +    // all the same cases we handle well in the URL code for creating an NSURL.
> 
> Where was this code moved from? It’s easy for us to rewrite this use WebCore::URL to make the NSURL.

From Editor::plainTextFromPasteboard() in EditorMac.mm
Comment 10 Carlos Garcia Campos 2014-09-15 04:49:00 PDT
Created attachment 238114 [details]
Patch for landing

Addressed review comments
Comment 11 Carlos Garcia Campos 2014-09-15 05:10:14 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > > Source/WebCore/page/DragController.h:119
> > > +        static PassRefPtr<DocumentFragment> documentFragmentFromDragData(DragData&, Frame&, Range&, bool allowPlainText, bool& chosePlainText);
> > > +        static PassRefPtr<DocumentFragment> createFragmentFromDragData(DragData&, Frame&, Range&, bool allowPlainText, bool& chosePlainText);
> > 
> > The names of these two functions don’t make it at all clear why there are two of them and how they differ.
> >
> > I think the second one exists only to have a platform-dependent hook to do things differently on every platform. Long term, I want to refactor this code so that it’s shared and not platform-specific. I think it can be done. In the meantime, I suggest a name that is clearer.
> 
> We can create a Pasteboard for drag and drop with the given DragData, and use something like Editor::webContentFromPasteboard() implementing that for other platforms (it's mac specific now). But that will be easier once I land patch in bug #136802, so I'll leave just a FIXME here and will submit a new patch once this and #136802 are both fixed.

See https://bugs.webkit.org/show_bug.cgi?id=136819
Comment 12 Carlos Garcia Campos 2014-09-16 01:28:56 PDT
Created attachment 238158 [details]
Try to fix windows build

Ok, so it seems I can't use the ternary operator for the windows refactoring so I'll try my luck again using a template.
Comment 13 Carlos Garcia Campos 2014-09-16 08:14:35 PDT
Created attachment 238181 [details]
Try to fix windows build again
Comment 14 Carlos Garcia Campos 2014-09-16 23:28:19 PDT
Committed r173686: <http://trac.webkit.org/changeset/173686>