Last Comment Bug 92737 - DnD of multiple shortcuts from desktop only opens one
: DnD of multiple shortcuts from desktop only opens one
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: Drag and Drop (show other bugs)
: Trunk
: All All
-- minor with 7 votes (vote)
: Future
Assigned To: Tooru Fujisawa [:arai]
:
: Neil Deakin
Mentors:
: 173658 185566 226931 248200 286062 296847 592634 601372 (view as bug list)
Depends on: 966986 1323276
Blocks: 597646
  Show dependency treegraph
 
Reported: 2001-07-29 12:27 PDT by Mats Palmgren (:mats)
Modified: 2017-02-08 07:31 PST (History)
21 users (show)
andrei.vaida: qe‑verify+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
the modified contentAreaDD.js file (4.56 KB, text/plain)
2002-07-09 14:14 PDT, Alfonso Martinez
no flags Details
Revised contentAreaDD.js file (5.11 KB, text/plain)
2002-07-10 07:05 PDT, Alfonso Martinez
no flags Details
support DnD multiple files (45.36 KB, patch)
2014-01-30 09:26 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
update uuid, remove unused QI, use multiline regex (45.43 KB, patch)
2014-01-31 02:53 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
fix text/x-moz-url and text/uri-list (47.52 KB, patch)
2014-02-03 15:42 PST, Tooru Fujisawa [:arai]
enndeakin: feedback+
Details | Diff | Splinter Review
part2: support DnD files on Tab Groups (9.95 KB, patch)
2014-02-03 18:00 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
part2: support DnD files on Tab Groups (8.74 KB, patch)
2014-02-04 00:38 PST, Tooru Fujisawa [:arai]
ttaubert: feedback-
Details | Diff | Splinter Review
Patches that supports dropping multiple files. (26.75 KB, application/zip)
2015-09-12 16:44 PDT, Tooru Fujisawa [:arai]
no flags Details
Part 1: Add nsIDroppedLinkHandler.dropLinks. (8.17 KB, patch)
2015-11-10 19:04 PST, Tooru Fujisawa [:arai]
enndeakin: review-
Details | Diff | Splinter Review
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area. (16.90 KB, patch)
2015-11-10 19:05 PST, Tooru Fujisawa [:arai]
dao+bmo: review-
Details | Diff | Splinter Review
Part 3: Open multiple tabs when multiple items are dropped on remote content area. (17.22 KB, patch)
2015-11-10 19:05 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 1: Add nsIDroppedLinkHandler.dropLinks. (7.92 KB, patch)
2015-11-23 07:29 PST, Tooru Fujisawa [:arai]
enndeakin: review+
Details | Diff | Splinter Review
Part 3: Open multiple tabs when multiple items are dropped on remote content area. (12.20 KB, patch)
2015-11-30 10:12 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area. (16.90 KB, patch)
2015-12-02 09:47 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 1: Add nsIDroppedLinkHandler.dropLinks. r=enndeakin (7.93 KB, patch)
2016-01-18 01:20 PST, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area. (16.84 KB, patch)
2016-01-18 01:21 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 3: Open multiple tabs when multiple items are dropped on remote content area. (12.20 KB, patch)
2016-01-18 01:23 PST, Tooru Fujisawa [:arai]
enndeakin: review+
Details | Diff | Splinter Review
Part 4: Open multiple tabs when multiple items are dropped on tab. (8.54 KB, patch)
2016-01-18 01:25 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 5: Set multiple homepage when multiple items are dropped on Home button. (10.89 KB, patch)
2016-01-18 01:25 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 6: Open multiple tabs when multiple items are dropped on New Tab button. (5.99 KB, patch)
2016-01-18 01:25 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 7: Open multiple windows when multiple items are dropped on New Window button. (6.79 KB, patch)
2016-01-18 01:26 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 8: Download multiple files when multiple items are dropped on Downloads button. (7.87 KB, patch)
2016-01-18 01:26 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 9: Download multiple files when multiple items are dropped on Downloads view in Library Window. (5.78 KB, patch)
2016-01-18 01:27 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 10: Use browserDragAndDrop.dropLinks in urlbarBindings.xml. (1.47 KB, patch)
2016-01-18 01:27 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 3: Open multiple tabs when multiple items are dropped on remote content area. r=enndeakin (13.40 KB, patch)
2016-02-10 21:19 PST, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 4: Open multiple tabs when multiple items are dropped on tab. (9.14 KB, patch)
2016-02-11 01:50 PST, Tooru Fujisawa [:arai]
enndeakin: review-
Details | Diff | Splinter Review
Part 5: Set multiple homepage when multiple items are dropped on Home button. (10.89 KB, patch)
2016-02-11 01:51 PST, Tooru Fujisawa [:arai]
enndeakin: review-
Details | Diff | Splinter Review
Part 6: Open multiple tabs when multiple items are dropped on New Tab button. (6.62 KB, patch)
2016-02-11 01:52 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 7: Open multiple windows when multiple items are dropped on New Window button. (7.43 KB, patch)
2016-02-11 01:53 PST, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 8: Download multiple files when multiple items are dropped on Downloads button. (7.87 KB, patch)
2016-02-11 01:53 PST, Tooru Fujisawa [:arai]
enndeakin: review+
Details | Diff | Splinter Review
Part 9: Download multiple files when multiple items are dropped on Downloads view in Library Window. (5.78 KB, patch)
2016-02-11 01:54 PST, Tooru Fujisawa [:arai]
enndeakin: review+
Details | Diff | Splinter Review
Part 10: Use browserDragAndDrop.dropLinks in urlbarBindings.xml. (1.47 KB, patch)
2016-02-11 01:54 PST, Tooru Fujisawa [:arai]
enndeakin: review+
Details | Diff | Splinter Review
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area. (16.61 KB, patch)
2016-03-15 02:20 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 4: Open multiple tabs when multiple items are dropped on tab. (7.91 KB, patch)
2016-03-15 02:36 PDT, Tooru Fujisawa [:arai]
enndeakin: review+
Details | Diff | Splinter Review
Part 5: Set multiple homepage when multiple items are dropped on Home button. (9.24 KB, patch)
2016-03-15 02:36 PDT, Tooru Fujisawa [:arai]
enndeakin: review+
Details | Diff | Splinter Review
Part 6: Open multiple tabs when multiple items are dropped on New Tab button. (6.17 KB, patch)
2016-03-15 02:37 PDT, Tooru Fujisawa [:arai]
enndeakin: review+
Details | Diff | Splinter Review
Part 7: Open multiple windows when multiple items are dropped on New Window button. (9.05 KB, patch)
2016-03-15 02:37 PDT, Tooru Fujisawa [:arai]
enndeakin: review+
Details | Diff | Splinter Review
Part 1: Add nsIDroppedLinkHandler.dropLinks. r=enndeakin (7.93 KB, patch)
2016-05-10 01:19 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area. (16.87 KB, patch)
2016-05-10 01:21 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 3: Open multiple tabs when multiple items are dropped on remote content area. r=enndeakin (14.84 KB, patch)
2016-05-10 01:21 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 4: Open multiple tabs when multiple items are dropped on tab. r=enndeakin (8.29 KB, patch)
2016-05-10 01:22 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 5: Set multiple homepage when multiple items are dropped on Home button. r=enndeakin (9.40 KB, patch)
2016-05-10 01:22 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 6: Open multiple tabs when multiple items are dropped on New Tab button. r=enndeakin (6.36 KB, patch)
2016-05-10 01:22 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 7: Open multiple windows when multiple items are dropped on New Window button. r=enndeakin (9.25 KB, patch)
2016-05-10 01:23 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 8: Download multiple files when multiple items are dropped on Downloads button. r=enndeakin (7.84 KB, patch)
2016-05-10 01:23 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 9: Download multiple files when multiple items are dropped on Downloads view in Library Window. r=enndeakin (5.83 KB, patch)
2016-05-10 01:23 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 10: Use browserDragAndDrop.dropLinks in urlbarBindings.xml. r=enndeakin (1.48 KB, patch)
2016-05-10 01:24 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area. (17.07 KB, patch)
2016-06-28 13:42 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area. (17.05 KB, patch)
2016-08-04 22:13 PDT, Tooru Fujisawa [:arai]
gijskruitbosch+bugs: review-
Details | Diff | Splinter Review
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area. (17.91 KB, patch)
2016-08-16 13:53 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area. (12.39 KB, patch)
2016-08-16 23:19 PDT, Tooru Fujisawa [:arai]
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review
(WIP) Bug 92737 - Part 3 fixed: Open multiple tabs when multiple items are dropped on remote content area. (15.26 KB, patch)
2016-08-17 11:55 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
(WIP) Bug 92737 - Part 3 fixed: Open multiple tabs when multiple items are dropped on remote content area. (17.22 KB, patch)
2016-08-24 14:58 PDT, Tooru Fujisawa [:arai]
enndeakin: feedback+
Details | Diff | Splinter Review
Part 3: Open multiple tabs when multiple items are dropped on remote content area. (12.60 KB, patch)
2016-08-30 13:22 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 3: Open multiple tabs when multiple items are dropped on remote content area. (11.90 KB, patch)
2016-08-30 14:09 PDT, Tooru Fujisawa [:arai]
enndeakin: review+
Details | Diff | Splinter Review
Part 3 fixup: Handle the first link in content process, and other links in parent process. (5.65 KB, patch)
2016-09-08 18:35 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 3 fixup: Move dropLinks to browser.xml, do not call RemoteDropLinks for empty links, and fix test to catch asynchronous handling. (13.87 KB, patch)
2016-09-09 20:06 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 3 fixup: Move dropLinks to browser.xml, do not call RemoteDropLinks for empty links, and fix test to catch asynchronous handling. (25.04 KB, patch)
2016-09-12 08:58 PDT, Tooru Fujisawa [:arai]
enndeakin: review+
Details | Diff | Splinter Review

Description User image Mats Palmgren (:mats) 2001-07-29 12:27:37 PDT
DESCRIPTION:
DnD of multiple shortcuts from desktop only opens one. It should open all.

STEPS TO REPRODUCE:
1. Create 2 or more shortcuts on the desktop by DnD from URL bar to desktop.
2. Select the shortcuts and drag them onto Mozilla main window.

ACTUAL RESULTS:
One of the shortcuts is loaded.

EXPECTED RESULTS:
All of the shortcuts should be loaded sequentually (as in Nav4.x).

DOES NOT WORK CORRECTLY ON:
Mozilla nightly build 2001-07-27-03 on Windows 98 SE.

WORKS CORRECTLY ON:
Communicator 4.7 on Windows 98 SE.
Comment 1 User image xyzzy 2001-07-29 15:17:02 PDT
While this sounds like a valid backward-compatability issue, I'm not sure I like 
this behavior as the default.  I have proposed an alternative, bug 91832, of a 
queue, to which the user could then apply a particular opening behavior, perhaps 
defined as a preference.  As stated, it applies to bookmarks, but shortcuts 
isn't a big stretch...

Marking 4xp.
Comment 2 User image James Henstridge 2001-11-20 05:14:40 PST
This bug is almost definitely caused by the DnD observer for the main browser
window having the canHandleMultipleItems flag set to false, in which case, only
the first item dropped gets handled.  How to handle the extra items is another
matter.

Are you saying the 4.x behaviour was to spawn extra windows for a multi item
drop?    I wonder if using tabs would make sense for this sort of drop.
Comment 3 User image xyzzy 2001-11-20 10:01:22 PST
Yes, tabs are an ideal solution.  Would focus go to one of the dropped URLs, or
stay with the current tab?
Comment 4 User image James Henstridge 2001-11-20 20:31:40 PST
I don't have time to fix this right now, but I can give a few pointers on how to
fix the bug if someone else wants to attempt it.  The fix would only involve
patching javascript files, and would be similar to the patch attached to bug 69528.

You need to modify contentAreaDD.js in the
mozilla/xpfe/communicator/resources/content subdirectory.  The DND observer
needs to have canHandleMultipleItems set to true, and the onDrop function needs
to be changed to handle an array as the aXferData argument (see the patch on the
other bug for details).

For details on opening new tabs, the open() function in openLocation.js (same
directory) may help.
Comment 5 User image Alfonso Martinez 2002-07-09 14:12:33 PDT
I was gonna submit a RFE about this when I've found this bug, then I've read the
instructions of James Henstridge, and copying lines of code I think that maybe I
got the fix for this bug, but I don't have the tools to create a nice patch, all
I can do is add all the file and then someone else should create the diff.

I'll attach my file right now.
Comment 6 User image Alfonso Martinez 2002-07-09 14:14:39 PDT
Created attachment 90662 [details]
the modified contentAreaDD.js file

I don't dare to call this a patch, but if someone helps me a little then it can
become one.
Comment 7 User image Alfonso Martinez 2002-07-10 07:05:15 PDT
Created attachment 90773 [details]
Revised contentAreaDD.js file

I've made a few revisions of the changes in the file, moving some variable
declarations and checks out of the loop, and changing the way the DnD is
handled if the window is a View-source.
Some comments about my changes:
For the first file, the behaviour is the same as it was previously, except in
view-source.

I've defined a variable outside the loop:
  var tabsEnabled=(getBrowser && getBrowser().localName == "tabbrowser");
If this test fails then instead of opening the files in new tabs I open them in
new windows.

Previously for a file droped in a view-Source window the following command was
executed:
	      viewSource(url);
For me it always lead to a prompt to save the file or pickup an application. So
I've changed it to this code:
	      openDialog("chrome://navigator/content/viewSource.xul",
		"_blank",
		"scrollbars,resizable,chrome,dialog=no",
		url, null, null);
	      ViewSourceClose();
So a new view source window opens and the current one is closed. This leads to
a visual gap.

I don't know if it's bad idea calling
OpenDialog("chrome://navigator/content/viewSource.xul" ... , but if there's
some other way to open a view source window I'll change it.

Please, can somebody tell me if my changes are valid?
Comment 8 User image Alfonso Martinez 2002-07-20 13:52:16 PDT
I'm adding the review keyword to see if someone cares a little about this
pseudo-patch.
Comment 9 User image Terri Preston 2002-10-08 15:40:15 PDT
qa contact -> pmac
Comment 10 User image Brian Rogers 2002-12-16 00:59:19 PST
*** Bug 185566 has been marked as a duplicate of this bug. ***
Comment 11 User image Alfonso Martinez 2003-11-27 06:29:11 PST
*** Bug 226931 has been marked as a duplicate of this bug. ***
Comment 12 User image Elmar Ludwig 2005-03-14 07:08:49 PST
*** Bug 286062 has been marked as a duplicate of this bug. ***
Comment 13 User image Elmar Ludwig 2005-03-14 07:11:16 PST
*** Bug 248200 has been marked as a duplicate of this bug. ***
Comment 14 User image Wayne Mery (:wsmwk, NI for questions) 2009-04-29 04:10:51 PDT
*** Bug 296847 has been marked as a duplicate of this bug. ***
Comment 15 User image Tooru Fujisawa [:arai] 2014-01-30 09:26:54 PST
Created attachment 8367998 [details] [diff] [review]
support DnD multiple files

Hi, I've created a draft patch for this.

Currently DnD multiple files is supported in following place:
- Browser Window
  * Browser Content Area
    open in current browser and new tabs
  * Tab
    open in current tab and new tabs
  * Between 2 Tabs (including both ends)
    open in new tabs
  - Toolbar
    * Downloads Indicator
      download all files
    * New Tab Button
      open in new tabs
    * New Window Button
      open in new windows
    * Home Button
      set multiple home pages
- Library Window
  * Downloads view
    download all files

Following item will became multiple tabs/windows if it contains 2 or more URLs:
* text/uri-list
* text/x-moz-url
* text/plain
* text/x-moz-text-internal


Following interfaces are changed:
- browser/base/content/browser.js
  * added browserDragAndDrop.dropLinks method
    replacement for browserDragAndDrop.drop (*1)
  * added handleDroppedLinks function
    replacement for handleDroppedLink (*1)
- browser/base/content/tabbrowser.xml
  * if tabbrowser.loadTabs method is called with exactly 2 arguments and
    2nd argument is an object, parameter is extracted from it
    ex: tabbrowser.loadTabs(urls, { inBackground: true });
- toolkit/content/widgets/browser.xml
  * added browser.droppedLinksHandler field (= handleDroppedLinks),
    replacement for browser.droppedLinkHandler (*1, *2)
- content/base/public/nsIDroppedLinkHandler.idl
  * added nsIDroppedLinkHandler.dropLinks method
    replacement for nsIDroppedLinkHandler.dropLink (*1)
  * added nsIDroppedLink interface
    return value of nsIDroppedLinkHandler.dropLinks
- content/base/src/contentAreaDropListener.js
  * added ContentAreaDropListener.prototype.dropLinks method
    replacement for ContentAreaDropListener.prototype.dropLink (*1)
  * added ContentAreaDropListener.prototype._getDropLinks method
    replacement for ContentAreaDropListener.prototype._getDropLink
  * removed ContentAreaDropListener.prototype._getDropLink method

*1: They are no longer called, but not removed for compatibility.
*2: browser.droppedLinkHandler is called
    only if browser.droppedLinksHandler is not set.


Then, I have some questions about implementation:
Q1.
  If I drop 10 files on New Window Button, 10 new windows are opened.
  Is it better to open 1 new window with 10 tabs instead?
Q2.
  If I drop 2 add-on files on normal tab other than Add-on Manager,
  2 install dialogs are shown for 2 tabs,
  and after I install them, notifications are shown in each tab.
  Is it okay? or perhaps should I create dedicated code for add-on files
  to handle all dropped add-ons in single install dialog and notification?
Comment 16 User image neil@parkwaycc.co.uk 2014-01-30 13:38:25 PST
Comment on attachment 8367998 [details] [diff] [review]
support DnD multiple files

Other Neil is more appropriate here, but some things I happened to notice:

> [scriptable, uuid(6B58A5A7-76D0-4E93-AB2E-4DE108683FF8)]
> interface nsIDroppedLinkHandler : nsISupports
When changing an interface you need to change its uuid too.

>+    let link = {
>+      QueryInterface: XPCOMUtils.generateQI([Ci.nsIDroppedLinkItem]),
You only need this if someone is going to explicitly invoke QueryInterface. Otherwise XPConnect will synthesize the interface for you based on the fact that it's the outparam value.

>+          let urls = dt.mozGetDataAt("URL", i).replace(/^\s+|\s+$/g, "").split("\n");
Need to use mulitline regex semantics here so that each line is trimmed.
Comment 17 User image Tooru Fujisawa [:arai] 2014-01-31 02:53:32 PST
Created attachment 8368476 [details] [diff] [review]
update uuid, remove unused QI, use multiline regex

Thank you for your feedback!
Comment 18 User image Tooru Fujisawa [:arai] 2014-02-03 06:44:15 PST
Depend on following bug:
  Bug 966986 - DataTransfer.getData/mozGetDataAt return incorrect data for text/x-moz-url on Mac

Duplicate of following bugs:
  Bug 173658 - Dragging multiple webloc files or bookmarks to content area, only one opens 
  Bug 592634 - Unable to open multiple files at once using drag and drop 
  Bug 597646 - Dragging multiple extensions into a browser window, only installs the first one 
  Bug 601372 - drag and drop of multiple local html files into the browser opens just the focussed one 

Bug 592634 refers to panorama view, I'm working on it now.
Comment 19 User image Tooru Fujisawa [:arai] 2014-02-03 15:42:35 PST
Created attachment 8369722 [details] [diff] [review]
fix text/x-moz-url and text/uri-list

While making patch for tab groups, I found some bugs in my last patch:

1. I misunderstood behavior of mozGetDataAt with text/x-moz-url,
and when I drop webloc files, duplicated tabs are opened.

2. text/uri-list returns first URL only.
Comment 20 User image Tooru Fujisawa [:arai] 2014-02-03 18:00:21 PST
Created attachment 8369811 [details] [diff] [review]
part2: support DnD files on Tab Groups

Draft patch for DnD on Tab Groups.

implemented:
* drop files/links to empty area
    create new group with new tabs
* drop files/links to group
    add new tabs to the group
* drop tab to empty area
    create new group with copied tab
* drop tab to group
    add copied tab to the group

not implemented:
* drop tab and move tab
    current implementation: always copy
* drop pinned tab and move/copy pinned tab
    current implementation: pinned tab is copied as normal tab
* drag tab group to other window
* drag tab in tab group to other window
Comment 21 User image Tooru Fujisawa [:arai] 2014-02-04 00:38:37 PST
Created attachment 8369903 [details] [diff] [review]
part2: support DnD files on Tab Groups

Sorry, I forgot to remove the code which does not work.
Comment 22 User image Neil Deakin 2014-03-14 06:57:26 PDT
Comment on attachment 8369722 [details] [diff] [review]
fix text/x-moz-url and text/uri-list

>   drop: function (aEvent, aName, aDisallowInherit) {
>-    return Services.droppedLinkHandler.dropLink(aEvent, aName, aDisallowInherit);
>+    let links = Services.droppedLinkHandler.dropLinks(aEvent, aDisallowInherit);
>+    if (links.length === 0)
>+      return undefined;
>+    aName.value = links[0].name;
>+    return links[0].url;
>+  },
>+
>+  dropLinks: function (aEvent, aDisallowInherit) {
>+    return Services.droppedLinkHandler.dropLinks(aEvent, aDisallowInherit);

Just remove the drop method as it shouldn't be needed now that you've replaced all the callers with dropLinks.


>diff --git a/browser/components/downloads/content/indicator.js b/browser/components/downloads/content/indicator.js
>index 7547b53..55c3367 100644
>--- a/browser/components/downloads/content/indicator.js
>+++ b/browser/components/downloads/content/indicator.js
>@@ -512,25 +512,23 @@ const DownloadsIndicatorView = {
>   onDrop: function DIV_onDrop(aEvent)
>   {
>     let dt = aEvent.dataTransfer;
>     // If dragged item is from our source, do not try to
>     // redownload already downloaded file.
>     if (dt.mozGetDataAt("application/x-moz-file", 0))
>       return;

You removed this check from allDownloadsViewOverlay.js and made it check each item separately. But here you
leave it as is. Is there a reason for the difference?


>diff --git a/content/base/public/nsIDroppedLinkHandler.idl b/content/base/public/nsIDroppedLinkHandler.idl
>+  /**
>+   * Returns the URL of link.
>+   */
 
Add 'the' to become 'Returns the URL of the link'


>+  /**
>+   * Given a drop event aEvent, determines links being dragged and returns
>+   * them. If links are returned the caller can, for instance, load them. If
>+   * count of links is 0, there is no valid link to be dropped.
>+   *

Add 'the' to become 'If the count of links is 0' ...


>diff --git a/content/base/src/contentAreaDropListener.js b/content/base/src/contentAreaDropListener.js
>+
>+  _getDropLinks : function (dt)
>+  {
>+    let links = [];
>     let types = dt.types;
>     for (let t = 0; t < types.length; t++) {
>       let type = types[t];
...

This function only handles the first item in the dataTransfer.

> 
>+  dropLinks: function(aEvent, aDisallowInherit, aCount, aLinks)
>+  {

aLinks isn't used or needed.


>+    if (this._eventTargetIsDisabled(aEvent))
>+      return [];
>+
>+    let dataTransfer = aEvent.dataTransfer;
>+    let links = this._getDropLinks(dataTransfer);
>+
>+    for (let link of links) {
>+      try {
>+        link.url = this._validateURI(dataTransfer, link.url, aDisallowInherit);
>+      } catch (ex) {
>+        aEvent.stopPropagation();
>+        aEvent.preventDefault();
>+        throw ex;

You might want to add a comment that we intentionally want to prevent the drop entirely
if any of the links are invalid even if one of them is valid.


>diff --git a/dom/events/nsDOMDataTransfer.cpp b/dom/events/nsDOMDataTransfer.cpp
>@@ -320,28 +320,30 @@ nsDOMDataTransfer::GetData(const nsAString& aFormat, nsAString& aData)

The 'url' type is a special type intended for backwards compatibility and should only return the first url in the list.
So the changes to nsDOMDataTransfer.cpp should not be made.


I didn't look at the changes to tabbrowser.xml much. It would better if Mano or Dao reviewed those changes.

Otherwise, this looks good. If you make the needed changes I can test it out. Thanks!
Comment 23 User image Neil Deakin 2014-03-14 07:02:17 PDT
Comment on attachment 8369903 [details] [diff] [review]
part2: support DnD files on Tab Groups

Tim should look at this instead.
Comment 24 User image Tooru Fujisawa [:arai] 2014-03-14 16:10:36 PDT
Thank you for your feedback.

(In reply to Neil Deakin from comment #22)
> >diff --git a/browser/components/downloads/content/indicator.js b/browser/components/downloads/content/indicator.js
> >index 7547b53..55c3367 100644
> >--- a/browser/components/downloads/content/indicator.js
> >+++ b/browser/components/downloads/content/indicator.js
> >@@ -512,25 +512,23 @@ const DownloadsIndicatorView = {
> >   onDrop: function DIV_onDrop(aEvent)
> >   {
> >     let dt = aEvent.dataTransfer;
> >     // If dragged item is from our source, do not try to
> >     // redownload already downloaded file.
> >     if (dt.mozGetDataAt("application/x-moz-file", 0))
> >       return;
> 
> You removed this check from allDownloadsViewOverlay.js and made it check
> each item separately. But here you
> leave it as is. Is there a reason for the difference?

It's my mistake. I forgot to modify it.

> >diff --git a/content/base/src/contentAreaDropListener.js b/content/base/src/contentAreaDropListener.js
> >+
> >+  _getDropLinks : function (dt)
> >+  {
> >+    let links = [];
> >     let types = dt.types;
> >     for (let t = 0; t < types.length; t++) {
> >       let type = types[t];
> ...
> 
> This function only handles the first item in the dataTransfer.

Sorry in advance if I'm wrong.

I thought that flavors other than "application/x-moz-file" are
not designed to return content of multiple entries separately,
since standard DataTransfer object has only "files" property,
and no "index" argument for "getData" method:
  https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer

and some contents are generated from multiple files, such as "text/x-moz-url":
  http://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsDragService.mm#403
so "mozItemCount" property has meaning only with "application/x-moz-file".
(may be related with bug 966986)
Comment 25 User image Tim Taubert [:ttaubert] 2014-03-31 09:22:28 PDT
Comment on attachment 8369903 [details] [diff] [review]
part2: support DnD files on Tab Groups

Review of attachment 8369903 [details] [diff] [review]:
-----------------------------------------------------------------

Tooru, I appreciate your effort but at this point I unfortunately can't accept feature patches for Panorama. The feature is mostly on its way out (bug 836758), respectively to be completely rewritten. We don't currently have the time and people to risk breaking existing Panorama features or maintain new ones.
Comment 26 User image Tooru Fujisawa [:arai] 2014-12-20 03:34:35 PST
*** Bug 173658 has been marked as a duplicate of this bug. ***
Comment 27 User image Tooru Fujisawa [:arai] 2014-12-20 03:36:34 PST
*** Bug 592634 has been marked as a duplicate of this bug. ***
Comment 28 User image Tooru Fujisawa [:arai] 2014-12-20 03:36:57 PST
*** Bug 601372 has been marked as a duplicate of this bug. ***
Comment 29 User image Tooru Fujisawa [:arai] 2015-09-12 16:44:07 PDT
Created attachment 8660384 [details]
Patches that supports dropping multiple files.

Just as a backup, here's draft patches, based on bug 966986.
I'll post them separately once bug 966986 gets resolved.
Comment 30 User image Tooru Fujisawa [:arai] 2015-11-10 19:04:09 PST
Created attachment 8685763 [details] [diff] [review]
Part 1: Add nsIDroppedLinkHandler.dropLinks.

Prepared 10 patches, here's rough plan:
  1) Change API to be capable of handling DnD multiple links (Part 1-3)
  2) Fix each place that accepts DnD to use new API, one by one (Part 2-10)

I'd like to get feedback for Part 1-3 first, as they're the key parts, and others are just changing consumers to follow them.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7edc85808830

[Part 1]

API Change:
  * Added nsIDroppedLinkHandler.dropLinks that extracts all links from drop
    event
    * nsIDroppedLinkHandler.dropLink are kept as it's still used in some places
      that does not need to handle multiple links
  * Added nsIDroppedLinkItem inferface that is the element of the array
    returned by nsIDroppedLinkHandler.dropLinks
Comment 31 User image Tooru Fujisawa [:arai] 2015-11-10 19:05:11 PST
Created attachment 8685764 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

[Part 2]

Behavior change:
  * When multiple links are dropped to non-remote content area, 1st item is
    opened in current browser and remaining items are opened in new tabs next
    to current tab

API Change
  * Added handleDroppedLinks function that handles dropping multiple links
    and opens them in tabs
    * handleDroppedLink is kept for compatibility with other products and some
      Add-ons
  * Added browser.droppedLinksHandler that will become handleDroppedLinks
    * browser.droppedLinkHandler is kept for compatibility (can we remove it??),
      it's called only if browser.droppedLinksHandler is not defined
  * Acced browserDragAndDrop.dropLinks that calls
    Services.droppedLinkHandler.dropLinks
    * Removed browserDragAndDrop.drop, that is used in some places, that are all
      fixed in remaining patches
  * Made 2nd argument of tabbrowser.loadTabs can be object, that contains
    several information needed to handle opening multiple tabs

test is added in Part 3.
Comment 32 User image Tooru Fujisawa [:arai] 2015-11-10 19:05:57 PST
Created attachment 8685765 [details] [diff] [review]
Part 3: Open multiple tabs when multiple items are dropped on remote content area.

[Part 3]

Behavior change:
  * When multiple links are dropped to remote content area, 1st item is opened
    in current browser and remaining items are opened in new tabs next to
    current tab

API Change
  * Added RemoteDropLinks to chrome process, that is called by receiving
    a message from content process
  * Added DropLinksListener to content process, that propagates a message for
    drop event from browser element to chrome process
  * Made browser elementin content process handle dragover/drop event, and send
    a message to chrome process via DropLinksListener
Comment 33 User image Olli Pettay [:smaug] 2015-11-11 12:06:03 PST
Comment on attachment 8685763 [details] [diff] [review]
Part 1: Add nsIDroppedLinkHandler.dropLinks.

Neil was looking this already before, so perhaps he could review this.
If not, move the review back to my queue.
Comment 34 User image Neil Deakin 2015-11-23 06:00:09 PST
Comment on attachment 8685763 [details] [diff] [review]
Part 1: Add nsIDroppedLinkHandler.dropLinks.

>-          var url = dt.getData(type).replace(/^\s+|\s+$/g, "");
>-          return [url, url];
>-        case "text/x-moz-url":
>-          return dt.getData(type).split("\n");
>+    let types = new Set(Array.from(dt.mozTypesAt(i)));

The StringList returned by mozTypesAt has a contains() method which you can just use later instead of creating an array/set.
  

>+    let type, data;
>+
>+    type = "text/uri-list";
>+    if (types.has(type)) {
>+      data = dt.mozGetDataAt(type, i);
>+      if (data) {
>+        let urls = data.split("\n");
>+        for (let url of urls) {
>+          // lines beginning with # are comments
>+          if (url.startsWith("#"))
>+            continue;
>+          url = url.replace(/^\s+|\s+$/g, "");
>+          this._addLink(links, url, url, type);
>+        }
>+        return;
>+      }
>+    }
>+
>+    type = "text/plain";
>+    if (types.has(type)) {
>+      data = dt.mozGetDataAt(type, i);
>+      if (data) {
>+        let lines = data.replace(/^\s+|\s+$/mg, "").split("\n");
>+        for (let line of lines) {
>+          this._addLink(links, line, line, type);
>+        }
>+        return;
>+      }
>+    }
>+    type = "text/x-moz-text-internal";
>+    if (types.has(type)) {
>+      data = dt.mozGetDataAt(type, i);
>+      if (data) {
>+        let lines = data.replace(/^\s+|\s+$/mg, "").split("\n");
>+        for (let line of lines) {
>+          this._addLink(links, line, line, type);
>+        }
>+        return;
>+      }

It seems like we should be able to combine these two to reduce the amount of code.


>+    }
>+    type = "text/x-moz-url";
>+    if (types.has(type)) {

This part should appear before the text handlng blocks, so we find things we know are links first.


>+      data = dt.mozGetDataAt(type, i);
>+      if (data) {
>+        let tmp = data.split("\n");

Use a more descriptive name than 'tmp'.


>   dropLink: function(aEvent, aName, aDisallowInherit)
>   {
>     aName.value = "";
>     if (this._eventTargetIsDisabled(aEvent))
>       return "";


dropLinks calls _eventTargetIsDisabled as well and returns "" if true, so you could remove this redundant call.


You didn't address the comment changes from my comment 22.
Comment 35 User image Tooru Fujisawa [:arai] 2015-11-23 07:29:41 PST
Created attachment 8690845 [details] [diff] [review]
Part 1: Add nsIDroppedLinkHandler.dropLinks.

Thank you for reviewing :D
Addressed review comments.
Comment 36 User image Tooru Fujisawa [:arai] 2015-11-27 00:30:43 PST
Comment on attachment 8685764 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

thanks again :)

can you review other parts as well?
Comment 37 User image Neil Deakin 2015-11-27 08:22:38 PST
I can review these next week.
Comment 38 User image Neil Deakin 2015-11-30 08:44:01 PST
Comment on attachment 8685764 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

After looking at this, I don't think I can review all of this. Dao probably is the best person to review the changes to 'loadTabs', and the part in handleDroppedLinks that calls it.

The rest of the patch looks ok though.
Comment 39 User image Neil Deakin 2015-11-30 08:53:34 PST
Comment on attachment 8685765 [details] [diff] [review]
Part 3: Open multiple tabs when multiple items are dropped on remote content area.

I'm not clear on what this patch is doing. The DropLinksListener seems to receive a message and then immediately send the same thing back again. Why does it do this?

> +            let plainLinks = [];
> +            for (let link of links) {
> +              plainLinks.push({ url: link.url, name: link.name, type: link.type });
> +            }
> +            this.messageManager.sendAsyncMessage("Browser:DropLinks", plainLinks);

What is different about this new array versus the existing 'links' one?

Doesn't the code in nsDocShellTreeOwner::HandleEvent get called as well for dragover/drop?

Is the reason you've changed this away from using nsDocShellTreeOwner because there isn't a convenient way to open multiple tabs?
Comment 40 User image Tooru Fujisawa [:arai] 2015-11-30 09:15:27 PST
Thank you for reviewing :)

(In reply to Neil Deakin from comment #39)
> Comment on attachment 8685765 [details] [diff] [review]
> Part 3: Open multiple tabs when multiple items are dropped on remote content
> area.
> 
> I'm not clear on what this patch is doing. The DropLinksListener seems to
> receive a message and then immediately send the same thing back again. Why
> does it do this?

I thought it's needed to send message from "drop" event handler in toolkit/content/widgets/browser.xml (in content process) to RemoteDropLinks message listener (in chrome process).
so, for now, the message sent by |this.messageManager.sendAsyncMessage("Browser:DropLinks", plainLinks);| in "drop" event handler is received by DropLinksListener (also in content process, right?), and it's sent again to RemoteDropLinks listener.
Is there any way to send the message directly from "drop" event handler to RemoteDropLinks listener?


> > +            let plainLinks = [];
> > +            for (let link of links) {
> > +              plainLinks.push({ url: link.url, name: link.name, type: link.type });
> > +            }
> > +            this.messageManager.sendAsyncMessage("Browser:DropLinks", plainLinks);
> 
> What is different about this new array versus the existing 'links' one?

|links| is the array of objects that implements nsIDroppedLinkItem.  I had to convert them to plain objects to send with sendAsyncMessage.


> Doesn't the code in nsDocShellTreeOwner::HandleEvent get called as well for
> dragover/drop?

nsDocShellTreeOwner::HandleEvent is called but it returns here:
https://dxr.mozilla.org/mozilla-central/rev/7883e81f3c305078353ca27a6b1adb8c769d5904/embedding/browser/nsDocShellTreeOwner.cpp#939
>   bool defaultPrevented;
>   aEvent->GetDefaultPrevented(&defaultPrevented);
>   if (defaultPrevented) {
>     return NS_OK;
>   }

so those events are handled only in browser.xml.
maybe I can remove the code in nsDocShellTreeOwner.cpp?


> Is the reason you've changed this away from using nsDocShellTreeOwner
> because there isn't a convenient way to open multiple tabs?

Yes, it's easier to send the message in JS code.
Comment 41 User image Tooru Fujisawa [:arai] 2015-11-30 09:27:22 PST
Comment on attachment 8685764 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

as suggested in comment #38.
Dao, can you review this?
Comment 42 User image Neil Deakin 2015-11-30 09:34:08 PST
> 
> I thought it's needed to send message from "drop" event handler in
> toolkit/content/widgets/browser.xml (in content process) to RemoteDropLinks

browser.xml is in the parent process. You can just call droppedLinksHandler directly, no?

> so those events are handled only in browser.xml.
> maybe I can remove the code in nsDocShellTreeOwner.cpp?

I think that would prevent an embedded browser from working.
Comment 43 User image Tooru Fujisawa [:arai] 2015-11-30 09:54:33 PST
(In reply to Neil Deakin from comment #42)
> > 
> > I thought it's needed to send message from "drop" event handler in
> > toolkit/content/widgets/browser.xml (in content process) to RemoteDropLinks
> 
> browser.xml is in the parent process. You can just call droppedLinksHandler
> directly, no?

Oh...  I guess I misunderstood the |this.isRemoteBrowser| flag.
Will fix the patch.

> > so those events are handled only in browser.xml.
> > maybe I can remove the code in nsDocShellTreeOwner.cpp?
> 
> I think that would prevent an embedded browser from working.

Okay, I don't touch it :)
Comment 44 User image Tooru Fujisawa [:arai] 2015-11-30 10:12:30 PST
Created attachment 8693662 [details] [diff] [review]
Part 3: Open multiple tabs when multiple items are dropped on remote content area.

Yeah, it works by just removing the |this.isRemoteBrowser| condition.
Comment 45 User image Dão Gottwald [:dao] 2015-12-02 06:21:22 PST
Comment on attachment 8685764 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

>         </body>
>       </method>
> 
>       <method name="loadTabs">
>         <parameter name="aURIs"/>
>         <parameter name="aLoadInBackground"/>
>         <parameter name="aReplace"/>
>         <body><![CDATA[
>+          let aAllowThirdPartyFixup;
>+          let aReplaceTab;
>+          let aNewIndex;
>+          let aPostDatas = [];
>+          if (arguments.length == 2 &&
>+              typeof arguments[1] == "object") {
>+            let params = arguments[1];
>+            aLoadInBackground     = params.inBackground;
>+            aReplace              = params.replace;
>+            aAllowThirdPartyFixup = params.allowThirdPartyFixup;
>+            aReplaceTab           = params.replaceTab;
>+            aNewIndex             = params.newIndex;
>+            aPostDatas            = params.postDatas || aPostDatas;
>+          }

replace vs. replaceTab seems like pretty horrible API design. Even I can't tell the difference off-hand. Can these names be made more distinctive?

Also, postDatas is kind of weird. Do we need to support this at all?
Comment 46 User image Tooru Fujisawa [:arai] 2015-12-02 07:42:36 PST
Thank you for reviewing :)

(In reply to Dão Gottwald [:dao] from comment #45)
> replace vs. replaceTab seems like pretty horrible API design. Even I can't
> tell the difference off-hand. Can these names be made more distinctive?

oh thanks, it should be renamed to targetTab, as the index of the tab is assigned to targetTabIndex inside the method.  omitting the property fallbacks to current tab.

the property is used in Part 4, that part handles drag and drop to a tab.  that property holds a tab that the drop event happens.
A dropped link can be opened in the tab, or a new tab next to it, so the replacetTab name was simply wrong.


> Also, postDatas is kind of weird. Do we need to support this at all?

Dropped text can use search keyword, current handleDroppedLink handles that case by calling getShortcutOrURIAndPostData [1].  Newly-added handleDroppedLinks also does same thing for each link, so there can be postData for each link.

For example:
  1. add a keyword for search in web content that uses POST, with "test" keyword
  2. add a keyword for another search in web content that uses POST, with "test2" keyword
  3. enter following text in some textarea in web content

test foo
test bar
test2 baz

  4. select all of the text entered in step 3
  5. open new window
  6. drag and drop selected text into new window's content area

it should open "test foo" search in current tab, and "test bar" search and "test2 baz" search in next 2 new tabs.  I think it's consistent with current behavior with single link.

(the text can also come from other application)


[1] https://dxr.mozilla.org/mozilla-central/rev/470f4f8c2b2d6f82e56e161a4b05262c85f55b59/browser/base/content/browser.js#5855
Comment 47 User image Tooru Fujisawa [:arai] 2015-12-02 09:47:54 PST
Created attachment 8694832 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

Renamed replaceTab to targetTab.
Comment 48 User image Tooru Fujisawa [:arai] 2016-01-18 01:20:04 PST
Created attachment 8708928 [details] [diff] [review]
Part 1: Add nsIDroppedLinkHandler.dropLinks. r=enndeakin

rebasing
Comment 49 User image Tooru Fujisawa [:arai] 2016-01-18 01:21:40 PST
Created attachment 8708929 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

hi, can you review this?
Comment 50 User image Tooru Fujisawa [:arai] 2016-01-18 01:23:40 PST
Created attachment 8708931 [details] [diff] [review]
Part 3: Open multiple tabs when multiple items are dropped on remote content area.

hi, can you review this?
Comment 51 User image Tooru Fujisawa [:arai] 2016-01-18 01:25:08 PST
Created attachment 8708933 [details] [diff] [review]
Part 4: Open multiple tabs when multiple items are dropped on tab.

attaching remaining parts just as a backup :)
will ask review after Parts 2-3 (as changes there will need fix in them)
Comment 52 User image Tooru Fujisawa [:arai] 2016-01-18 01:25:23 PST
Created attachment 8708935 [details] [diff] [review]
Part 5: Set multiple homepage when multiple items are dropped on Home button.
Comment 53 User image Tooru Fujisawa [:arai] 2016-01-18 01:25:46 PST
Created attachment 8708936 [details] [diff] [review]
Part 6: Open multiple tabs when multiple items are dropped on New Tab button.
Comment 54 User image Tooru Fujisawa [:arai] 2016-01-18 01:26:05 PST
Created attachment 8708937 [details] [diff] [review]
Part 7: Open multiple windows when multiple items are dropped on New Window button.
Comment 55 User image Tooru Fujisawa [:arai] 2016-01-18 01:26:47 PST
Created attachment 8708938 [details] [diff] [review]
Part 8: Download multiple files when multiple items are dropped on Downloads button.
Comment 56 User image Tooru Fujisawa [:arai] 2016-01-18 01:27:04 PST
Created attachment 8708939 [details] [diff] [review]
Part 9: Download multiple files when multiple items are dropped on Downloads view in Library Window.
Comment 57 User image Tooru Fujisawa [:arai] 2016-01-18 01:27:26 PST
Created attachment 8708941 [details] [diff] [review]
Part 10: Use browserDragAndDrop.dropLinks in urlbarBindings.xml.
Comment 58 User image Neil Deakin 2016-02-10 07:42:20 PST
Comment on attachment 8708931 [details] [diff] [review]
Part 3: Open multiple tabs when multiple items are dropped on remote content area.

>+    expectLink(child,
>+               [ { url: "http://www.example.com",
>+                   name: "Example.com" },
>+                 { url: "http://www.mozilla.org",
>+                   name: "http://www.mozilla.org" }],
>+               [ [ { type: "text/x-moz-url", data: "http://www.example.com\nExample.com" } ],
>+                 [ { type: "text/plain", data: "http://www.mozilla.org" } ] ],
>+               "text/x-moz-url and text/plain drop on browser " + type);


Could you also add some tests around here for when there is only one item, but contains multiple types with links in them, to ensure that only one of the types is used. For example:
Comment 59 User image Tooru Fujisawa [:arai] 2016-02-10 21:19:41 PST
Created attachment 8718217 [details] [diff] [review]
Part 3: Open multiple tabs when multiple items are dropped on remote content area. r=enndeakin

Thank you for reviewing :)
Updated the test and added some comments for each testcase.
Comment 60 User image Tooru Fujisawa [:arai] 2016-02-11 01:50:38 PST
Created attachment 8718261 [details] [diff] [review]
Part 4: Open multiple tabs when multiple items are dropped on tab.

Behavior change:
  * When multiple links are dropped to a tab, 1st item is opened in the tab,
    and remaining items are opened in new tabs next to the tab
  * When multiple links are dropped between tabs, all items are opened in new
    tabs between them
  * When multiple links are dropped before the 1st tab, all items are opened in
    new tabs before the tab
  * When multiple links are dropped after the last tab, all items are opened in
    new tabs after the tab
Comment 61 User image Tooru Fujisawa [:arai] 2016-02-11 01:51:42 PST
Created attachment 8718262 [details] [diff] [review]
Part 5: Set multiple homepage when multiple items are dropped on Home button.

Behavior change:
  * When multiple links are dropped to Home button, all of them are set to
    homepages after a confirm dialog
    * The dialog asks "Do you want these documents to be your new home pages?",
      instead of "Do you want this document to be your new home page?"
Comment 62 User image Tooru Fujisawa [:arai] 2016-02-11 01:52:36 PST
Created attachment 8718263 [details] [diff] [review]
Part 6: Open multiple tabs when multiple items are dropped on New Tab button.

Behavior change:
  * When multiple links are dropped to New Tab button, all items are opened
    in new tabs next to the last tab
    This code works only when New Tab button is placed outside of Tab bar,
    that is non-default configuration, otherwise it's handled in Part 4
Comment 63 User image Tooru Fujisawa [:arai] 2016-02-11 01:53:19 PST
Created attachment 8718264 [details] [diff] [review]
Part 7: Open multiple windows when multiple items are dropped on New Window button.

Behavior change:
  * When multiple links are dropped to New Window button, all items are
    opened in new windows, 1 window per each item
Comment 64 User image Tooru Fujisawa [:arai] 2016-02-11 01:53:43 PST
Created attachment 8718265 [details] [diff] [review]
Part 8: Download multiple files when multiple items are dropped on Downloads button.

Behavior change:
  * When multiple (non-local) links are dropped to Downloads button
    (toolbar button), all items are downloaded
Comment 65 User image Tooru Fujisawa [:arai] 2016-02-11 01:54:16 PST
Created attachment 8718266 [details] [diff] [review]
Part 9: Download multiple files when multiple items are dropped on Downloads view in Library Window.

Behavior change:
  * When multiple (non-local) links are dropped to Downloads view in Library
    Window, all items are downloaded
Comment 66 User image Tooru Fujisawa [:arai] 2016-02-11 01:54:43 PST
Created attachment 8718267 [details] [diff] [review]
Part 10: Use browserDragAndDrop.dropLinks in urlbarBindings.xml.

  * Changed browserDragAndDrop.drop to browserDragAndDrop.dropLinks in
    urlbarBindings.xml.

no behavior change.
Comment 67 User image Neil Deakin 2016-03-14 05:39:29 PDT
Comment on attachment 8718261 [details] [diff] [review]
Part 4: Open multiple tabs when multiple items are dropped on tab.

>-              // Just ignore invalid urls
>-            }
>-          }
>+          let replace = (tab && dropEffect != "copy");

Note that the 'dropEffect != "copy"' check has been removed by another recent bug.


>+          let newIndex = this._getDropIndex(event, true);
>-  var openedTabs = 0;
>+  let resolveTabOpenPromise = null;
>+  let validOpenedTabCount = 0;
>+  let openedTabCount = 0;
>+  let totalOpenedTabCount = 0;
>+  let removeTabPromises = [];

It is hard to follow this test with the many similar variable names. I suggest using names containing 'expected' for the expected counts and 'actual' or 'received' for the counts you actually got.

>   function tabOpenListener(e) {
>-    openedTabs++;
>+    openedTabCount++;
>+    totalOpenedTabCount++;
>+
>     let tab = e.target;
>-    executeSoon(function () {
>-      gBrowser.removeTab(tab);
>-    });
>+    removeTabPromises.push(new Promise(resolve => {
>+      executeSoon(function () {
>+        gBrowser.removeTab(tab);
>+        resolve();

Remove tabs with BrowserTestUtils.removeTab.
You might also consider just removing all of the tabs at the end of the test rather than keeping track of them.

Does this test work if an extra tab is opened? The test was hard to follow.

The patch looks good but I'd like to see a test that is a bit easier to read.

Similar comments apply to the other patches with almost identical tests.
Comment 68 User image Neil Deakin 2016-03-14 05:47:49 PDT
Comment on attachment 8718262 [details] [diff] [review]
Part 5: Set multiple homepage when multiple items are dropped on Home button.

>-function test() {
>-  waitForExplicitFinish();
>+add_task(function*() {
>+  let HOMEPAGE_PREF = "browser.startup.homepage";
> 
>   let str = Cc["@mozilla.org/supports-string;1"]
>               .createInstance(Ci.nsISupportsString);
>   str.data = "about:mozilla";

'str' is used in a number of places. Rename it to a clearer name, such as 'homepageStr'.


>-  let newTab = gBrowser.selectedTab = gBrowser.addTab();
>-  registerCleanupFunction(function () {
>-    gBrowser.removeTab(newTab);
>+    Services.prefs.clearUserPref(HOMEPAGE_PREF);
>   });
> 

Can the pushPrefs function (in head.js) be used here?


>-  let dialogListener = new WindowListener("chrome://global/content/commonDialog.xul", function (domwindow) {
>-    ok(true, "dialog appeared in response to home button drop");
>-    domwindow.document.documentElement.cancelDialog();
>-    Services.wm.removeListener(dialogListener);
>+  function* task_drop(dragData, homepage) {
>+    yield new Promise(resolve => {

This can just return the promise, not yield it. Then remove the asterisks.

>+
>+  function* task_dropInvalidURI() {
>+    yield new Promise(resolve => {

Same here.


> 
>+function createTemporaryFiles(n) {

This function isn't used.


>+
>+function GenericWindowListener(aURL, aCallback) {

Can we just use BrowserTestUtils.domWindowOpened instead of adding GenericWindowListener?
Comment 69 User image Neil Deakin 2016-03-14 05:48:54 PDT
Comment on attachment 8718263 [details] [diff] [review]
Part 6: Open multiple tabs when multiple items are dropped on New Tab button.

Similar comments here as part 4.
Comment 70 User image Neil Deakin 2016-03-14 05:50:35 PDT
Comment on attachment 8718264 [details] [diff] [review]
Part 7: Open multiple windows when multiple items are dropped on New Window button.

Similar comments here as part 4.


+[browser_newWindowDrop.js]
+skip-if = debug # takes long time to opening many new windows in debug build

You can use requestLongerTimeout to help here.
Comment 71 User image Neil Deakin 2016-03-14 05:55:09 PDT
Comment on attachment 8718265 [details] [diff] [review]
Part 8: Download multiple files when multiple items are dropped on Downloads button.

>+    if (handled)
>       aEvent.preventDefault();
>-    }

Add some braces around this.


>+        onDownloadAdded: function(download) {
>+          added.add(download.source.url);
>+          if (added.size == urls.length) {
>+            list.removeView(view).then(function() {
>+              resolve();
>+            });

Just call resolve directly:

  list.removeView(view).then(resolve);


>+  Services.prefs.setIntPref(folderListPrefName, 2);
>+  Services.prefs.setComplexValue(dirPrefName, Ci.nsIFile, tmpDir);

Can SpecialPowers.pushPrefEnv be used here to avoid manual cleanup?
Comment 72 User image Neil Deakin 2016-03-14 05:58:28 PDT
Comment on attachment 8718266 [details] [diff] [review]
Part 9: Download multiple files when multiple items are dropped on Downloads view in Library Window.

>+    yield new Promise(function(resolve) {
>+      let view = {
>+        onDownloadAdded: function(download) {
>+          added.add(download.source.url);
>+          if (added.size == urls.length) {
>+            list.removeView(view).then(function() {
>+              resolve();
>+            });

Call resolve directly.

>+  let win = yield new Promise(function(resolve) {
>+    function onLibraryReady(win) {
>+      resolve(win);
>+    }
>+    openLibrary(onLibraryReady, "Downloads");
>+  });

Same here:
  openLibrary(resolve, "Downloads");

Or better, just use promiseFocus instead of waitForFocus and yield on OpenLibrary:

>+  waitForFocus(function () {
>+    callback(library);
>+  }, library);
Comment 73 User image Tooru Fujisawa [:arai] 2016-03-15 02:20:52 PDT
Created attachment 8730614 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

Just rebased.
Comment 74 User image Tooru Fujisawa [:arai] 2016-03-15 02:36:18 PDT
Created attachment 8730625 [details] [diff] [review]
Part 4: Open multiple tabs when multiple items are dropped on tab.

(In reply to Neil Deakin from comment #67)
> >+          let newIndex = this._getDropIndex(event, true);
> >-  var openedTabs = 0;
> >+  let resolveTabOpenPromise = null;
> >+  let validOpenedTabCount = 0;
> >+  let openedTabCount = 0;
> >+  let totalOpenedTabCount = 0;
> >+  let removeTabPromises = [];
> 
> It is hard to follow this test with the many similar variable names. I
> suggest using names containing 'expected' for the expected counts and
> 'actual' or 'received' for the counts you actually got.

browser_tabDrop.js testcase was rewritten in bug 1253163, so applied minimal change to make it compatible with opening multiple tabs.

  * Separated drop function to drop and dropText wrapper
    drop takes dragData as 1st argument, and expectedTabOpenCount as 2nd argument
  * Changed awaitTabOpen to wait for expectedTabOpenCount-times,
    and stores opened tabs into openedTabs array
  * Remove all opened tabs in each test
    previously the test takes the opened tab from awaitTabOpen,
    but now there could be multiple tabs, so it takes opened tabs from openedTabs array,
    generated in check function for awaitTabOpen


> Does this test work if an extra tab is opened? The test was hard to follow.

Yes, it doesn't touch inactive tabs until cleanup.
Comment 75 User image Tooru Fujisawa [:arai] 2016-03-15 02:36:44 PDT
Created attachment 8730626 [details] [diff] [review]
Part 5: Set multiple homepage when multiple items are dropped on Home button.

Changed testcase to use pushPrefs, BrowserTestUtils.domWindowOpened, and BrowserTestUtils.waitForEvent for "load" event.
Comment 76 User image Tooru Fujisawa [:arai] 2016-03-15 02:37:06 PDT
Created attachment 8730627 [details] [diff] [review]
Part 6: Open multiple tabs when multiple items are dropped on New Tab button.

Changed testcase to follow updated browser_tabDrop.js.
Comment 77 User image Tooru Fujisawa [:arai] 2016-03-15 02:37:30 PDT
Created attachment 8730628 [details] [diff] [review]
Part 7: Open multiple windows when multiple items are dropped on New Window button.

Changed testcase to follow updated browser_tabDrop.js,
also, applied requestLongerTimeout.
Comment 78 User image Neil Deakin 2016-03-24 06:11:49 PDT
Comment on attachment 8730626 [details] [diff] [review]
Part 5: Set multiple homepage when multiple items are dropped on Home button.

>   if (pressedVal == 0) {
>     try {
>-      var str = Components.classes["@mozilla.org/supports-string;1"]
>+      var homepageStr = Components.classes["@mozilla.org/supports-string;1"]
>                           .createInstance(Components.interfaces.nsISupportsString);


Fix up the indenting here. (the . should line up)


>-  waitForExplicitFinish();
>+add_task(function*() {
>+  let HOMEPAGE_PREF = "browser.startup.homepage";
> 
>-  let str = Cc["@mozilla.org/supports-string;1"]
>+  let homepageStr = Cc["@mozilla.org/supports-string;1"]
>               .createInstance(Ci.nsISupportsString);

Same indenting here.
 
>-
>+  yield drop([[{type: "text/plain",
>+                data: "http://mochi.test:8888/"}]],
>+             "http://mochi.test:8888/");
>+  yield drop([[{type: "text/plain",
>+                data: "http://mochi.test:8888/\nhttp://mochi.test:8888/b\nhttp://mochi.test:8888/c"}]],
>+             "http://mochi.test:8888/|http://mochi.test:8888/b|http://mochi.test:8888/c");

I think you need 'yield* drop(...)' as the drop function as multiple yields.
Comment 79 User image Neil Deakin 2016-03-24 06:30:58 PDT
Comment on attachment 8730628 [details] [diff] [review]
Part 7: Open multiple windows when multiple items are dropped on New Window button.

>+  let checkCount = function(window) {
>+    // Add observer as soon as domWindow is opened to avoid missing the topic.
>+    let awaitStartup = tmp.TestUtils.topicObserved("browser-delayed-startup-finished",
>+                                                   subject => subject == window);
>+    openedWindows.push([window, awaitStartup]);
>+    actualWindowOpenCount++;
>+    return actualWindowOpenCount == expectedWindowOpenCount;
>+  };
>+  let awaitWindowOpen = expectedWindowOpenCount && BrowserTestUtils.domWindowOpened(null, checkCount);

If you use BrowserTestUtils.waitForNewWindow instead of domWindowOpened, it will wait for the "browser-delayed-startup-finished" message as well.

Then you don't need TestUtils.
Comment 80 User image Neil Deakin 2016-03-24 06:34:11 PDT
Comment on attachment 8730614 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

Dao I think is still a better reviewer for the tabbrowser.xml changes.
Comment 81 User image Tooru Fujisawa [:arai] 2016-03-24 16:38:31 PDT
Thank you for reviewing :)

(In reply to Neil Deakin from comment #79)
> Comment on attachment 8730628 [details] [diff] [review]
> Part 7: Open multiple windows when multiple items are dropped on New Window
> button.
> 
> >+  let checkCount = function(window) {
> >+    // Add observer as soon as domWindow is opened to avoid missing the topic.
> >+    let awaitStartup = tmp.TestUtils.topicObserved("browser-delayed-startup-finished",
> >+                                                   subject => subject == window);
> >+    openedWindows.push([window, awaitStartup]);
> >+    actualWindowOpenCount++;
> >+    return actualWindowOpenCount == expectedWindowOpenCount;
> >+  };
> >+  let awaitWindowOpen = expectedWindowOpenCount && BrowserTestUtils.domWindowOpened(null, checkCount);
> 
> If you use BrowserTestUtils.waitForNewWindow instead of domWindowOpened, it
> will wait for the "browser-delayed-startup-finished" message as well.
> 
> Then you don't need TestUtils.

It doesn't work, as I need to wait for both "domwindowopened" notification and "browser-delayed-startup-finished" topic, for each window.

waitForNewWindow yields domWindowOpened and topicObserved sequentially, so it cannot wait for both of them for multiple windows.
Comment 82 User image Neil Deakin 2016-03-24 18:11:19 PDT
> 
> It doesn't work, as I need to wait for both "domwindowopened" notification
> and "browser-delayed-startup-finished" topic, for each window.
> 
> waitForNewWindow yields domWindowOpened and topicObserved sequentially, so
> it cannot wait for both of them for multiple windows.

OK, that's fine then.
Comment 83 User image Tooru Fujisawa [:arai] 2016-05-10 01:19:58 PDT
Created attachment 8750656 [details] [diff] [review]
Part 1: Add nsIDroppedLinkHandler.dropLinks. r=enndeakin

rebasing
Comment 84 User image Tooru Fujisawa [:arai] 2016-05-10 01:21:22 PDT
Created attachment 8750657 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

Do you have time to review?  or can you suggest anyone else?
Comment 85 User image Tooru Fujisawa [:arai] 2016-05-10 01:21:56 PDT
Created attachment 8750659 [details] [diff] [review]
Part 3: Open multiple tabs when multiple items are dropped on remote content area. r=enndeakin

and rebasing remaining parts
Comment 86 User image Tooru Fujisawa [:arai] 2016-05-10 01:22:12 PDT
Created attachment 8750660 [details] [diff] [review]
Part 4: Open multiple tabs when multiple items are dropped on tab. r=enndeakin
Comment 87 User image Tooru Fujisawa [:arai] 2016-05-10 01:22:28 PDT
Created attachment 8750661 [details] [diff] [review]
Part 5: Set multiple homepage when multiple items are dropped on Home button. r=enndeakin
Comment 88 User image Tooru Fujisawa [:arai] 2016-05-10 01:22:45 PDT
Created attachment 8750662 [details] [diff] [review]
Part 6: Open multiple tabs when multiple items are dropped on New Tab button. r=enndeakin
Comment 89 User image Tooru Fujisawa [:arai] 2016-05-10 01:23:01 PDT
Created attachment 8750664 [details] [diff] [review]
Part 7: Open multiple windows when multiple items are dropped on New Window button. r=enndeakin
Comment 90 User image Tooru Fujisawa [:arai] 2016-05-10 01:23:17 PDT
Created attachment 8750665 [details] [diff] [review]
Part 8: Download multiple files when multiple items are dropped on Downloads button. r=enndeakin
Comment 91 User image Tooru Fujisawa [:arai] 2016-05-10 01:23:40 PDT
Created attachment 8750666 [details] [diff] [review]
Part 9: Download multiple files when multiple items are dropped on Downloads view in Library Window. r=enndeakin
Comment 92 User image Tooru Fujisawa [:arai] 2016-05-10 01:24:06 PDT
Created attachment 8750667 [details] [diff] [review]
Part 10: Use browserDragAndDrop.dropLinks in urlbarBindings.xml. r=enndeakin
Comment 93 User image Tooru Fujisawa [:arai] 2016-06-28 05:53:02 PDT
testing rebased patches
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f58cdf062cb
Comment 94 User image Tooru Fujisawa [:arai] 2016-06-28 13:42:37 PDT
Created attachment 8766050 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

rebased, and fixed legacy generator to ES6 generator
Comment 95 User image Tooru Fujisawa [:arai] 2016-06-29 01:48:31 PDT
Comment on attachment 8766050 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

vlad, can you review this?
Comment 96 User image Tooru Fujisawa [:arai] 2016-08-04 22:13:50 PDT
Created attachment 8778092 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

rebased.  no changes in other parts.

dao, do you have time to review this?  or could you suggest someone else who can review this?
Comment 97 User image :Gijs 2016-08-16 10:58:12 PDT
Comment on attachment 8778092 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

Review of attachment 8778092 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing per IRC. r- for the duplicate stuff and the breaking change of using third party fixup when we didn't use to. AIUI this means if you have e.g. "foo bar" as a the 'link' we'll do a web search with the default engine. That doesn't seem right. To be clear, you shouldn't need those flags for the keyword searches.

::: browser/base/content/browser.js
@@ +5652,5 @@
> +    }
> +    if (lastLocationChange == gBrowser.selectedBrowser.lastLocationChange) {
> +      gBrowser.loadTabs(urls, { inBackground: inBackground,
> +                                replace: true,
> +                                allowThirdPartyFixup: true,

This used to pass false for allowThirdPartyFixup, so this should too.

@@ +5654,5 @@
> +      gBrowser.loadTabs(urls, { inBackground: inBackground,
> +                                replace: true,
> +                                allowThirdPartyFixup: true,
> +                                postDatas: postDatas,
> +                                userContextId: userContextId,

You don't need to repeat the variable and property names here for inBackground, postDatas, and userContextId.

::: browser/base/content/tabbrowser.xml
@@ +1529,5 @@
> +            aAllowThirdPartyFixup = params.allowThirdPartyFixup;
> +            aTargetTab            = params.targetTab;
> +            aNewIndex             = params.newIndex;
> +            aPostDatas            = params.postDatas || aPostDatas;
> +            aPostDatas            = params.postDatas || aPostDatas;

Duplicate line?

@@ +1564,5 @@
> +            }
> +            let flags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
> +            if (aAllowThirdPartyFixup) {
> +              flags |= Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
> +              flags |= Ci.nsIWebNavigation.LOAD_FLAGS_FIXUP_SCHEME_TYPOS;

You can do this in one assignment with the | operator.

@@ +1570,3 @@
>              try {
> +              browser.loadURIWithFlags(aURIs[0], {
> +                                       flags: flags,

Again, don't need to repeat the flags literal.

I also think the indenting here is off. Don't line up properties with parameters when they're actually part of an object that is a parameter. In this case, it might even just all fit on one line if you remove the duplication, but otherwise either use a temp or fix the indentation to use the better-accepted notation of {
  propA: valA,
  propB: valB,
});

@@ +1576,5 @@
>                // opening the next ones.
>              }
> +          } else {
> +            firstTabAdded = this.addTab(aURIs[0], {
> +                                        ownerTab: owner,

Similar indenting problems here.

@@ +1581,5 @@
> +                                        skipAnimation: multiple,
> +                                        allowThirdPartyFixup: aAllowThirdPartyFixup,
> +                                        postData: aPostDatas[0],
> +                                        userContextId: aUserContextId });
> +            if (aNewIndex !== undefined) {

Seems like it'd be more natural to default this to -1 and check for that here.

@@ +1953,5 @@
>              this._tabListeners.set(aTab, tabListener);
>              this._tabFilters.set(aTab, filter);
>  
>              browser.droppedLinkHandler = handleDroppedLink;
> +            browser.droppedLinksHandler = handleDroppedLinks;

You should make up your mind whether you intend to keep both of these or only the plural version. Currently your patch sometimes keeps the old thing and sometimes not. Not keeping the old thing is likely a breaking change so I'd lean towards having both if we really have to - but you're not being consistent about keeping both, and your patch claims handleDroppedLink is not used, which is not true.

We should also extend the frontend tests that check this functionality and ensure it works in the e10s case as well.
Comment 98 User image Tooru Fujisawa [:arai] 2016-08-16 13:53:01 PDT
Created attachment 8781706 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

Thank you for reviewing :)
Addressed review comments.

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #97)
> @@ +1953,5 @@
> >              this._tabListeners.set(aTab, tabListener);
> >              this._tabFilters.set(aTab, filter);
> >  
> >              browser.droppedLinkHandler = handleDroppedLink;
> > +            browser.droppedLinksHandler = handleDroppedLinks;
> 
> You should make up your mind whether you intend to keep both of these or
> only the plural version. Currently your patch sometimes keeps the old thing
> and sometimes not. Not keeping the old thing is likely a breaking change so
> I'd lean towards having both if we really have to - but you're not being
> consistent about keeping both, and your patch claims handleDroppedLink is
> not used, which is not true.

Thanks, fixed to keep all.
singular function and property are kept for compatibility.

singular handleDroppedLink is surely referred in the code, but not called anywhere unless add-ons or another application do so.
Removed the misleading comment.


> We should also extend the frontend tests that check this functionality and
> ensure it works in the e10s case as well.

Tests are added in part 3 (toolkit/content/tests/chrome/window_browser_drop.xul) and remaining parts for each UI part (tab, new tab button, new window button, home, download).
Comment 99 User image :Gijs 2016-08-16 14:27:21 PDT
Comment on attachment 8781706 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

Clearing review for now per IRC discussion.
Comment 100 User image Tooru Fujisawa [:arai] 2016-08-16 23:19:40 PDT
Created attachment 8781857 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

Changed to use overloaded handleDroppedLink function, and keep same droppedLinkHandler property name.

passed try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ad5f90a8e7b
Comment 101 User image :Gijs 2016-08-17 04:05:38 PDT
Comment on attachment 8781857 [details] [diff] [review]
Part 2: Open multiple tabs when multiple items are dropped on non-remote content area.

Review of attachment 8781857 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the issues below addressed. Thanks for persevering!

::: browser/base/content/browser.js
@@ +3340,5 @@
>      }
>    },
>  
> +  dropLinks: function (aEvent, aDisallowInherit) {
> +    return Services.droppedLinkHandler.dropLinks(aEvent, aDisallowInherit);

Note that this and the handleDroppedLink change both imply add-on compat changes. We should ensure we update the relevant MDN docs and get this in the "notes for developers" for the release it ships in.

@@ +5571,5 @@
>    // LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL for those.
>    return pasteData.replace(/^(?:\s*javascript:)+/i, "");
>  }
>  
> +// handleDroppedLink have the following 2 overloads:

Tiny English nit: "has", not "have".

::: browser/base/content/tabbrowser.xml
@@ +1543,5 @@
> +            aLoadInBackground     = params.inBackground;
> +            aReplace              = params.replace;
> +            aAllowThirdPartyFixup = params.allowThirdPartyFixup;
> +            aTargetTab            = params.targetTab;
> +            aNewIndex             = params.newIndex || aNewIndex;

This doesn't work if params.newIndex == 0. You'll need an actual typeof check here to ensure that newIndex is not a number before using the default.

@@ +5593,5 @@
>        <method name="_getDropEffectForTabDrag">
>          <parameter name="event"/>
>          <body><![CDATA[
>            var dt = event.dataTransfer;
>            // Disallow dropping multiple items

This comment is now outdated, right? We can just remove it.
Comment 102 User image Tooru Fujisawa [:arai] 2016-08-17 07:17:36 PDT
Thanks again :)

while testing more with the latest patch, I noticed that drag and drop event in website doesn't work in e10s,
the website cannot accept drop event, like, google image doesn't search dropped image, but the dropped image is opened in the tab, that's from patch Part 3.

will investigate it.
Comment 103 User image Tooru Fujisawa [:arai] 2016-08-17 07:31:43 PDT
with current patch's approach, drag and drop event is not sent to content process,
but we should send it, for web pages, and handle in content process, and send back to chrome process if the web pages don't handle it.
Comment 104 User image Tooru Fujisawa [:arai] 2016-08-17 11:55:16 PDT
Created attachment 8782096 [details] [diff] [review]
(WIP) Bug 92737 - Part 3 fixed: Open multiple tabs when multiple items are dropped on remote content area.

Here's WIP patch that at least works.

Here's the summary of the issue and the approach in this patch:

In current non-patched code, drag and drop in e10s content area is handled in nsDocShellTreeOwner::HandleEvent.
previous Part 3 tries to handle drag and drop in parent process, but that was wrong approach, as child process cannot handle drag and drop, even if web content tries to handle it.

So, now we need to handle the drag and drop in child process first, and if the event is not defaultPrevented, pass the dropped data back to parent process (from nsDocShellTreeOwner::HandleEvent), and handle it in parent process.

With this patch, I used newly added DropLinks function in PBrowser, to pass the data from child process to parent process, and in parent process, convert it to observer notification, and observe it in browser.js, and call browser.droppedLinkHandler (I know whole this process is super redundant...).

What would be the correct way to do this?
Comment 105 User image :Gijs 2016-08-17 13:04:42 PDT
Comment on attachment 8782096 [details] [diff] [review]
(WIP) Bug 92737 - Part 3 fixed: Open multiple tabs when multiple items are dropped on remote content area.

I don't know whether this makes sense. I think mconley is likely to know more than me.
Comment 106 User image Mike Conley (:mconley) 2016-08-18 14:33:25 PDT
I have zero context for this bug... maybe a good idea to rope Neil in again, since he's been involved with this one before.
Comment 107 User image Mike Conley (:mconley) 2016-08-18 14:33:47 PDT
Comment on attachment 8782096 [details] [diff] [review]
(WIP) Bug 92737 - Part 3 fixed: Open multiple tabs when multiple items are dropped on remote content area.

Actually, meant to set the feedback? on Enn.
Comment 108 User image Tooru Fujisawa [:arai] 2016-08-24 14:58:53 PDT
Created attachment 8784578 [details] [diff] [review]
(WIP) Bug 92737 - Part 3 fixed: Open multiple tabs when multiple items are dropped on remote content area.

sorry, forgot to add dom/base/DroppedLinkItem.h and dom/base/DroppedLinkItemIPC.h files.
Comment 109 User image Neil Deakin 2016-08-30 06:50:30 PDT
Comment on attachment 8784578 [details] [diff] [review]
(WIP) Bug 92737 - Part 3 fixed: Open multiple tabs when multiple items are dropped on remote content area.

>+    json.AppendLiteral("{\"url\":\"");
>+    // FIXME: escape string
>+    json.Append(aLinks[i].mUrl);
>+    json.AppendLiteral("\",\"name\":\"");
>+    json.Append(aLinks[i].mName);
>+    json.AppendLiteral("\",\"type\":\"");
>+    json.Append(aLinks[i].mType);
>+    json.AppendLiteral("\"}");
>+  }
>+  json.Append(']');
>+  os->NotifyObservers(NS_ISUPPORTS_CAST(nsITabParent*, this),
>+                      "browser-remote-drop-links",
>+                      json.get());

This will send the notification to all windows and you don't look to be filtering on the receiving side. Better is to QueryInterface mFrameElement to nsIRemoteBrowser (or perhaps the new nsIBrowser interface) and add a method there. That way you can just pass an array as well instead or serializing the data.
Comment 110 User image Tooru Fujisawa [:arai] 2016-08-30 13:22:01 PDT
Created attachment 8786464 [details] [diff] [review]
Part 3: Open multiple tabs when multiple items are dropped on remote content area.

Thank you for your feedback :)

Here's updated patch that adds dropLinks methods to following:
  * firefoxBrowserBindings (remote-browser.xml)
  * RemoteController (RemoteController.jsm)

Now the data flow is following:

  nsDocShellTreeOwner::HandleEvent
    |
    | an array of nsIDroppedLinkItem
    v
  TabChild::RemoteDropLinks
    |
    | a flat array of nsString
    v
  TabChild::SendDropLinks
    |
    | a flat array of nsString
    v
  TabParent::RecvDropLinks
    |
    | a flat array of char16_t*
    v
  dropLinks method of remote-browser binding
    |
    | a flat array of string
    v
  RemoteController.prototype.dropLinks
    |
    | an array of nsIDroppedLinkItem
    v
  browser.droppedLinkHandler
Comment 111 User image Neil Deakin 2016-08-30 13:27:39 PDT
Comment on attachment 8786464 [details] [diff] [review]
Part 3: Open multiple tabs when multiple items are dropped on remote content area.

There's no reason to change RemoteController which is unrelated to drag and drop. Just put the code directly in remote-browser.xml
Comment 112 User image Tooru Fujisawa [:arai] 2016-08-30 14:09:21 PDT
Created attachment 8786493 [details] [diff] [review]
Part 3: Open multiple tabs when multiple items are dropped on remote content area.

oh, I didn't notice that it's browser element

updated data flow:

  nsDocShellTreeOwner::HandleEvent
    |
    | an array of nsIDroppedLinkItem
    v
  TabChild::RemoteDropLinks
    |
    | a flat array of nsString
    v
  TabChild::SendDropLinks
    |
    | a flat array of nsString
    v
  TabParent::RecvDropLinks
    |
    | a flat array of char16_t*
    v
  dropLinks method of remote-browser binding
    |
    | an array of nsIDroppedLinkItem
    v
  browser.droppedLinkHandler
Comment 113 User image Tooru Fujisawa [:arai] 2016-09-01 17:00:53 PDT
Comment on attachment 8786493 [details] [diff] [review]
Part 3: Open multiple tabs when multiple items are dropped on remote content area.

forgot to add review flag
Comment 114 User image Tooru Fujisawa [:arai] 2016-09-08 12:11:15 PDT
Thank you for reviewing :)

unfortunately, the change broke the testcase's assumption.
window_browser_drop.xul assumes that page load happens synchronously after dispatching "drop" event, in remote content.
but now the pageload is handled in parent process and the page load happens asynchronously, so the testcase fails to catch the page load.

https://dxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/window_browser_drop.xul?q=path%3Achrome%2Fwindow_browser_drop.xul&redirect_type=single#56
>     let dataTransfer = new content.DataTransfer("dragstart", false);
>     dataTransfer.mozSetDataAt(data.type, data.data, 0);
>     let event = content.document.createEvent("DragEvent");
>     event.initDragEvent("drop", true, true, content, 0, 0, 0, 0, 0,
>                         false, false, false, false, 0, null, dataTransfer);
>     content.document.body.dispatchEvent(event);
> ...
>     is(shouldExpectStateChange, gotLoad, "Should have gotten a load only if we expected it.");

I'll try finding some solution.
Comment 115 User image Tooru Fujisawa [:arai] 2016-09-08 12:14:04 PDT
for now, I have 2 possible solutions.

  A. wait for parent process before checking page load
  B. handle page load for current tab in content process, and handle page load for other tabs in parent process

patch for B is ready, but I'm not sure if it makes sense to change the implementation only for test.
so I'll try A
Comment 116 User image Tooru Fujisawa [:arai] 2016-09-08 14:26:48 PDT
Just noticed <browser remote="true"> doesn't inherit remote-browser, and TabParent fails to get nsIRemoteBrowser interface for the test's case.
So, if we try to handle all links in parent process, dropped links are not handled for it.
I guess it may break some use case.

So, anyway We should choose B.
Will post the patch after some more tests.
Comment 117 User image Tooru Fujisawa [:arai] 2016-09-08 18:35:08 PDT
Created attachment 8789625 [details] [diff] [review]
Part 3 fixup: Handle the first link in content process, and other links in parent process.

If we get links dropped onto content process, we need to handle the first one in the process, with the same way as before, to keep <browser remote="true"> working.
(as mentioned above, it doesn't implement nsIRemoteBrowser and the message will just get lost without handling any links)
and then pass links back to parent process, and, in parent process, open other links in newly created background tabs, next to current tab.

Here, we don't need the first link in parent process, but it would be better passing back all links to parent process and call droppedLinkHandler with them.
So that droppedLinkHandler will receive the same links array regardless of e10s/non-e10s.


https://treeherder.mozilla.org/#/jobs?repo=try&revision=510ed205e19f
Comment 118 User image Neil Deakin 2016-09-09 06:17:39 PDT
(In reply to Tooru Fujisawa [:arai] from comment #116)
> Just noticed <browser remote="true"> doesn't inherit remote-browser, and
> TabParent fails to get nsIRemoteBrowser interface for the test's case.
> So, if we try to handle all links in parent process, dropped links are not
> handled for it.
> I guess it may break some use case.
> 

Use nsIBrowser instead and move the dropLinks method there. (You should also null-check droppedLinkHandler before calling it).
Comment 119 User image Tooru Fujisawa [:arai] 2016-09-09 20:06:44 PDT
Created attachment 8789968 [details] [diff] [review]
Part 3 fixup: Move dropLinks to browser.xml,  do not call RemoteDropLinks for empty links, and fix test to catch asynchronous handling.

Thank you for suggestion :)

Moved dropLinks method to nsIBrowser from nsIRemoteBrowser,
and moved implementation from remote-browser.xml to browser.xml.

Also, in nsDocShellTreeOwner.cpp, changed nsDocShellTreeOwner::HandleEvent not to call RemoteDropLinks if the links array is empty.
that should match non-e10s behavior that also doesn't call droppedLinkHandler when the links array is empty

Then, changed remote-content case of window_browser_drop.xul to observe droppedLinkHandler in parent process,
as now everything goes asynchronously, and we need to catch one of them.
here's the testing flow for each drop:

  1. in parent process
    1-1. replace browser.droppedLinkHandler to catch it
  2. in content process
    2-1. wait for the testing page gets loaded
         as now we don't do any page navigation, we need to explicitly wait for
         page load, otherwise content.document.body can be null
    2-2. dispatch drop event on content.document.body
    2-3. call Services.droppedLinkHandler.dropLinks
         (to verify it returns the same thing also on content process,
          especially for the case the expected links array is empty)
    2-4. return the links array returned by Services.droppedLinkHandler.dropLinks
  3. in parent process
    3-1. verify the links array returned by content process matches to expected links array
    3-2. if the expected links array is empty
      3-2-1. finish testing
             because there is no way to check if nothing happens asynchronously
    3-3. else if the expected links array is not empty
      3-3-1. wait for browser.droppedLinkHandler
      3-3-2. verify the links array caught by browser.droppedLinkHandler matches to expected links array

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61b32e6c7762
Comment 120 User image Neil Deakin 2016-09-12 08:47:36 PDT
Comment on attachment 8789968 [details] [diff] [review]
Part 3 fixup: Move dropLinks to browser.xml,  do not call RemoteDropLinks for empty links, and fix test to catch asynchronous handling.

> bool
> TabParent::RecvDropLinks(nsTArray<nsString>&& aLinks)
> {
>-  nsCOMPtr<nsIRemoteBrowser> remoteBrowser = do_QueryInterface(mFrameElement);
>-  if (remoteBrowser) {
>+  nsCOMPtr<nsIBrowser> broeser = do_QueryInterface(mFrameElement);

Spelling: browser


>-    let links = [];
>-    let gotLoad = false;
>-    function stopContent(uriLoaded) {
>-      content.stop();
>-      gotLoad = true;
>+    if (!content.document.documentElement) {
>+      // Wait for the testing document gets loaded.

Wait *until* the ...


>-function expectLink(browser, expectedLinks, data, testid, onbody=false) {
>-  function dropOnBrowserSync() {
>-    let lastLinks = [];
>+var expectLink = Task.async(function*(browser, expectedLinks, data, testid, onbody=false) {


You can use yield* to do nested yields without needing to use Tasks.


>+  is(links.length, expectedLinks.length, testid + " links length");
>+  for (let i = 0, length = links.length; i < length; i++) {
>+    is(links[i].url, expectedLinks[i].url, testid + "[" + i + "] link");
>+    is(links[i].name, expectedLinks[i].name, testid + "[" + i + "] name");
>+  }


This is ok, but you intended to do this verification twice for the remote case? Is this to handle the zero expected links case?
Comment 121 User image Tooru Fujisawa [:arai] 2016-09-12 08:58:45 PDT
Created attachment 8790312 [details] [diff] [review]
Part 3 fixup: Move dropLinks to browser.xml,  do not call RemoteDropLinks for empty links, and fix test to catch asynchronous handling.

Thank you :D

> This is ok, but you intended to do this verification twice for the remote
> case? Is this to handle the zero expected links case?

yeah, there are 2 reasons:
  * to verify Services.droppedLinkHandler.dropLinks returns the same array on content process
  * to verify zero case
Comment 122 User image Tooru Fujisawa [:arai] 2016-09-12 17:57:16 PDT
Thank you :D
I'll land this after next merge, to keep this nightly-only for a while,
as this will break some add-ons that interacts with tabs and/or drag and drop.
Comment 123 User image Tooru Fujisawa [:arai] 2016-09-20 00:58:33 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e51a6f9a0ebb3a533b34bb4fb0585eeaec1b3c59
Bug 92737 - Part 1: Add nsIDroppedLinkHandler.dropLinks. r=enndeakin

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d44acbb76fe1ffcb52ce1d99ddd898b2c5ab5f8
Bug 92737 - Part 2: Open multiple tabs when multiple items are dropped on non-remote content area. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/978a6615d7cce1d5970fd014c72ec474dfa02629
Bug 92737 - Part 3: Open multiple tabs when multiple items are dropped on remote content area. r=enndeakin

https://hg.mozilla.org/integration/mozilla-inbound/rev/40e1dbd5409b5d7f54eb39c787d643af5139265f
Bug 92737 - Part 4: Open multiple tabs when multiple items are dropped on tab. r=enndeakin

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b53031d587cb51af43095f7f86e5137a524470d
Bug 92737 - Part 5: Set multiple homepage when multiple items are dropped on Home button. r=enndeakin

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e65ab5b5f3878a6d2d7357a0b0852e2efe2ad75
Bug 92737 - Part 6: Open multiple tabs when multiple items are dropped on New Tab button. r=enndeakin

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d201f01953fe44d5b37dc1bbec8650f8352533f
Bug 92737 - Part 7: Open multiple windows when multiple items are dropped on New Window button. r=enndeakin

https://hg.mozilla.org/integration/mozilla-inbound/rev/1107b20c6c51c030d4f013361d5b3ae185691423
Bug 92737 - Part 8: Download multiple files when multiple items are dropped on Downloads button. r=enndeakin

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1976ce06a096f7d4e381416148a04285d88e218
Bug 92737 - Part 9: Download multiple files when multiple items are dropped on Downloads view in Library Window. r=enndeakin

https://hg.mozilla.org/integration/mozilla-inbound/rev/d87a29e5c993089a50126693d9b71773917c33ac
Bug 92737 - Part 10: Use browserDragAndDrop.dropLinks in urlbarBindings.xml. r=enndeakin

Note You need to log in before you can comment on or make changes to this bug.