Bug 104166 – Navigation begun in response to a drag should have a unique NavigationType

• Component: WebCore Misc.
• Status: ASSIGNED

[Conrad Shultz]

Currently, navigation commenced in DragController::performDrag(), via FrameLoader::load(), ends up as type NavigationTypeOther. In certain situations it would be desirable for client code to be able to distinguish drag-initiated navigation from other types of navigation, so a new navigation type should be declared, to be used by performDrag().

[Conrad Shultz]

Created attachment 178291 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:17:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:21:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:24:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:25:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:29:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/Shared/API/c/WKPageLoadTypes.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebKit2/Shared/API/c/WKPageLoadTypes.h:40:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 13 in 8 files


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

[Conrad Shultz]

Created attachment 178299 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/Shared/API/c/WKPageLoadTypes.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebKit2/Shared/API/c/WKPageLoadTypes.h:40:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 8 files


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

[Ryosuke Niwa]

Comment on attachment 178299 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Currently, navigation commenced in DragController::performDrag(), via FrameLoader::load(), ends up as type NavigationTypeOther. In certain situations it would be desirable for client code to be able to distinguish drag-initiated navigation from other types of navigation. To this end, this patch adds a new navigation type (NavigationTypeDragInitiated) and an associated WKFrameNavigationType (kWKFrameNavigationTypeDragInitiated), and revises, particularly, DragController and FrameLoader to set this navigation type accordingly.

Please wrap lines.

> Source/WebKit2/ChangeLog:8
> +        Currently, navigation commenced in DragController::performDrag(), via FrameLoader::load(), ends up as type NavigationTypeOther. In certain situations it would be desirable for client code to be able to distinguish drag-initiated navigation from other types of navigation. To this end, this patch adds a new navigation type (NavigationTypeDragInitiated) and an associated WKFrameNavigationType (kWKFrameNavigationTypeDragInitiated), and revises, particularly, DragController and FrameLoader to set this navigation type accordingly.

Ditto. Also, you don’t have to repeat this description here since it’s included in WebCore.

> Source/WebCore/loader/FrameLoader.cpp:1266
> +void FrameLoader::load(const FrameLoadRequest& passedRequest, bool isDrag)

Would it make sense to add a new member variable to FrameLoadRequest or add a new type to FrameLoadType instead?

[Ryosuke Niwa]

Comment on attachment 178299 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:1297
> +    if (isDrag)
> +        loader.get()->setTriggeringAction(NavigationAction(request.resourceRequest(), NavigationTypeDragInitiated));

I don’t think it makes much sense to override the action here.

[Conrad Shultz]

(In reply to comment #5)
> 
> Please wrap lines.

Will fix.

> 
> Ditto. Also, you don’t have to repeat this description here since it’s included in WebCore.

Ditto. (It wasn't clear to me from the contributor instructions what to do when a patch spans multiple sub-projects; now I know.

> 
> > Source/WebCore/loader/FrameLoader.cpp:1266
> > +void FrameLoader::load(const FrameLoadRequest& passedRequest, bool isDrag)
> 
> Would it make sense to add a new member variable to FrameLoadRequest or add a new type to FrameLoadType instead?

What were you thinking in terms of a new FrameLoadType? From what I can see, the only method that lets you pass one in, FrameLoader::loadWithNavigationAction(), is private, and I don't necessarily want to make it public just for this.

I do like the idea of adding a member variable to FrameLoadRequest, however, and that seems like a better approach than my admittedly inelegant first pass. I will take a look.

Thanks for reading the patch!

[Conrad Shultz]

(In reply to comment #6)
> (From update of attachment 178299 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178299&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:1297
> > +    if (isDrag)
> > +        loader.get()->setTriggeringAction(NavigationAction(request.resourceRequest(), NavigationTypeDragInitiated));
> 
> I don’t think it makes much sense to override the action here.

Do you think it should be done deeper down, such as in FrameLoader::loadWithDocumentLoader(), where the triggering action is currently set if empty?

[Sam Weinig]

What is the use case for distinguishing loads that are drag initiated? Also, please don't add an overload of FrameLoader::load().  If this data is needed, it should probably be incorporated into the FrameLoadRequest.

[Conrad Shultz]

(In reply to comment #9)
> What is the use case for distinguishing loads that are drag initiated? Also, please don't add an overload of FrameLoader::load().  If this data is needed, it should probably be incorporated into the FrameLoadRequest.

I agree that overloading load() is suboptimal; I will rework and submit a patch that modifies FrameLoadRequest.

[Ryosuke Niwa]

Comment on attachment 178299 [details]
Patch

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

>>> Source/WebCore/loader/FrameLoader.cpp:1297
>>> +        loader.get()->setTriggeringAction(NavigationAction(request.resourceRequest(), NavigationTypeDragInitiated));
>> 
>> I don’t think it makes much sense to override the action here.
> 
> Do you think it should be done deeper down, such as in FrameLoader::loadWithDocumentLoader(), where the triggering action is currently set if empty?

Yes. I thought I explicitly stated in my previous comment but it got cut off somehow :/

[Alexey Proskuryakov]

A few points from IRC discussion:

- care should be taken to not break WebKit1 API, as these navigation types are basically typecast to an API type;

- the concept of drag is not very well defined - a drag from Finder is very different from an HTML link drag.

- is drag really the only case that needs to be addressed? For example, I'd test AppleScript.