Closed Bug 92737 Opened 23 years ago Closed 8 years ago

DnD of multiple shortcuts from desktop only opens one

Categories

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

defect
Not set
minor

Tracking

()

VERIFIED FIXED
Future
Tracking Status
firefox52 --- verified

People

(Reporter: MatsPalmgren_bugz, Assigned: arai)

References

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(12 files, 46 obsolete files)

5.11 KB, text/plain
Details
7.93 KB, patch
arai
: review+
Details | Diff | Splinter Review
8.29 KB, patch
arai
: review+
Details | Diff | Splinter Review
9.40 KB, patch
arai
: review+
Details | Diff | Splinter Review
6.36 KB, patch
arai
: review+
Details | Diff | Splinter Review
9.25 KB, patch
arai
: review+
Details | Diff | Splinter Review
7.84 KB, patch
arai
: review+
Details | Diff | Splinter Review
5.83 KB, patch
arai
: review+
Details | Diff | Splinter Review
1.48 KB, patch
arai
: review+
Details | Diff | Splinter Review
12.39 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
11.90 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
25.04 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
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.
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.
Keywords: 4xp
Target Milestone: --- → Future
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.
Yes, tabs are an ideal solution.  Would focus go to one of the dropped URLs, or
stay with the current tab?
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.
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.
Attached file the modified contentAreaDD.js file (obsolete) —
I don't dare to call this a patch, but if someone helps me a little then it can
become one.
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?
Attachment #90662 - Attachment is obsolete: true
I'm adding the review keyword to see if someone cares a little about this
pseudo-patch.
Keywords: review
Keywords: reviewhelpwanted, patch
qa contact -> pmac
QA Contact: tpreston → pmac
*** Bug 185566 has been marked as a duplicate of this bug. ***
*** Bug 226931 has been marked as a duplicate of this bug. ***
Assignee: blake → alfonso-nospam
*** Bug 286062 has been marked as a duplicate of this bug. ***
*** Bug 248200 has been marked as a duplicate of this bug. ***
Severity: normal → minor
QA Contact: pmac → drag-drop
Whiteboard: [has draft patch]
Assignee: amla70 → nobody
Attached patch support DnD multiple files (obsolete) — Splinter Review
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?
Attachment #8367998 - Flags: feedback?(neil)
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.
Attachment #8367998 - Flags: feedback?(neil) → feedback?(enndeakin)
Thank you for your feedback!
Attachment #8367998 - Attachment is obsolete: true
Attachment #8367998 - Flags: feedback?(enndeakin)
Attachment #8368476 - Flags: feedback?(enndeakin)
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.
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.
Attachment #8368476 - Attachment is obsolete: true
Attachment #8368476 - Flags: feedback?(enndeakin)
Attachment #8369722 - Flags: feedback?(enndeakin)
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
Attachment #8369811 - Flags: feedback?(enndeakin)
Sorry, I forgot to remove the code which does not work.
Attachment #8369811 - Attachment is obsolete: true
Attachment #8369811 - Flags: feedback?(enndeakin)
Attachment #8369903 - Flags: feedback?(enndeakin)
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!
Attachment #8369722 - Flags: feedback?(enndeakin) → feedback+
Comment on attachment 8369903 [details] [diff] [review]
part2: support DnD files on Tab Groups

Tim should look at this instead.
Attachment #8369903 - Flags: feedback?(enndeakin) → feedback?(ttaubert)
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 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.
Attachment #8369903 - Flags: feedback?(ttaubert) → feedback-
Depends on: 966986
Blocks: 597646
Blocks: 417918
Just as a backup, here's draft patches, based on bug 966986.
I'll post them separately once bug 966986 gets resolved.
Assignee: nobody → arai.unmht
Attachment #8369722 - Attachment is obsolete: true
Attachment #8369903 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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
Attachment #8660384 - Attachment is obsolete: true
Attachment #8685763 - Flags: review?(bugs)
[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.
Attachment #8685764 - Flags: review?(dao)
[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
Attachment #8685765 - Flags: review?(dao)
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.
Attachment #8685763 - Flags: review?(bugs) → review?(enndeakin)
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.
Attachment #8685763 - Flags: review?(enndeakin) → review-
Thank you for reviewing :D
Addressed review comments.
Attachment #8685763 - Attachment is obsolete: true
Attachment #8690845 - Flags: review?(enndeakin)
Attachment #8690845 - Flags: review?(enndeakin) → review+
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?
Attachment #8685764 - Flags: review?(dao) → review?(enndeakin)
Attachment #8685765 - Flags: review?(dao) → review?(enndeakin)
I can review these next week.
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.
Attachment #8685764 - Flags: review?(enndeakin)
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?
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 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?
Attachment #8685764 - Flags: review?(dao)
> 
> 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.
(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 :)
Yeah, it works by just removing the |this.isRemoteBrowser| condition.
Attachment #8685765 - Attachment is obsolete: true
Attachment #8685765 - Flags: review?(enndeakin)
Attachment #8693662 - Flags: review?(enndeakin)
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?
Attachment #8685764 - Flags: review?(dao) → review-
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
Renamed replaceTab to targetTab.
Attachment #8685764 - Attachment is obsolete: true
Attachment #8694832 - Flags: review?(dao)
rebasing
Attachment #8690845 - Attachment is obsolete: true
Attachment #8708928 - Flags: review+
hi, can you review this?
Attachment #8694832 - Attachment is obsolete: true
Attachment #8694832 - Flags: review?(dao)
Attachment #8708929 - Flags: review?(dao)
hi, can you review this?
Attachment #8693662 - Attachment is obsolete: true
Attachment #8693662 - Flags: review?(enndeakin)
Attachment #8708931 - Flags: review?(enndeakin)
attaching remaining parts just as a backup :)
will ask review after Parts 2-3 (as changes there will need fix in them)
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:
Attachment #8708931 - Flags: review?(enndeakin) → review+
Thank you for reviewing :)
Updated the test and added some comments for each testcase.
Attachment #8708931 - Attachment is obsolete: true
Attachment #8718217 - Flags: review+
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
Attachment #8708933 - Attachment is obsolete: true
Attachment #8718261 - Flags: review?(enndeakin)
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?"
Attachment #8708935 - Attachment is obsolete: true
Attachment #8718262 - Flags: review?(enndeakin)
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
Attachment #8708936 - Attachment is obsolete: true
Attachment #8718263 - Flags: review?(enndeakin)
Behavior change:
  * When multiple links are dropped to New Window button, all items are
    opened in new windows, 1 window per each item
Attachment #8708937 - Attachment is obsolete: true
Attachment #8718264 - Flags: review?(enndeakin)
Behavior change:
  * When multiple (non-local) links are dropped to Downloads button
    (toolbar button), all items are downloaded
Attachment #8718265 - Flags: review?(enndeakin)
Attachment #8708938 - Attachment is obsolete: true
Behavior change:
  * When multiple (non-local) links are dropped to Downloads view in Library
    Window, all items are downloaded
Attachment #8708939 - Attachment is obsolete: true
Attachment #8718266 - Flags: review?(enndeakin)
* Changed browserDragAndDrop.drop to browserDragAndDrop.dropLinks in
    urlbarBindings.xml.

no behavior change.
Attachment #8708941 - Attachment is obsolete: true
Attachment #8718267 - Flags: review?(enndeakin)
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.
Attachment #8718261 - Flags: review?(enndeakin) → review-
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?
Attachment #8718262 - Flags: review?(enndeakin) → review-
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.
Attachment #8718263 - Flags: review?(enndeakin)
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.
Attachment #8718264 - Flags: review?(enndeakin)
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?
Attachment #8718265 - Flags: review?(enndeakin) → review+
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);
Attachment #8718266 - Flags: review?(enndeakin) → review+
Attachment #8718267 - Flags: review?(enndeakin) → review+
Just rebased.
Attachment #8708929 - Attachment is obsolete: true
Attachment #8708929 - Flags: review?(dao)
Attachment #8730614 - Flags: review?(enndeakin)
(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.
Attachment #8718261 - Attachment is obsolete: true
Attachment #8730625 - Flags: review?(enndeakin)
Changed testcase to use pushPrefs, BrowserTestUtils.domWindowOpened, and BrowserTestUtils.waitForEvent for "load" event.
Attachment #8718262 - Attachment is obsolete: true
Attachment #8730626 - Flags: review?(enndeakin)
Changed testcase to follow updated browser_tabDrop.js.
Attachment #8718263 - Attachment is obsolete: true
Attachment #8730627 - Flags: review?(enndeakin)
Changed testcase to follow updated browser_tabDrop.js,
also, applied requestLongerTimeout.
Attachment #8718264 - Attachment is obsolete: true
Attachment #8730628 - Flags: review?(enndeakin)
Attachment #8730625 - Flags: review?(enndeakin) → review+
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.
Attachment #8730626 - Flags: review?(enndeakin) → review+
Attachment #8730627 - Flags: review?(enndeakin) → review+
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.
Attachment #8730628 - Flags: review?(enndeakin) → review+
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.
Attachment #8730614 - Flags: review?(enndeakin) → review?(dao)
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.
> 
> 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.
rebasing
Attachment #8708928 - Attachment is obsolete: true
Attachment #8750656 - Flags: review+
Do you have time to review?  or can you suggest anyone else?
Attachment #8730614 - Attachment is obsolete: true
Attachment #8730614 - Flags: review?(dao+bmo)
Attachment #8750657 - Flags: review?(dao+bmo)
and rebasing remaining parts
Attachment #8718217 - Attachment is obsolete: true
Attachment #8750659 - Flags: review+
rebased, and fixed legacy generator to ES6 generator
Attachment #8750657 - Attachment is obsolete: true
Attachment #8750657 - Flags: review?(dao+bmo)
Attachment #8766050 - Flags: review?(dao+bmo)
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?
Attachment #8766050 - Flags: review?(vladimir)
rebased.  no changes in other parts.

dao, do you have time to review this?  or could you suggest someone else who can review this?
Attachment #8766050 - Attachment is obsolete: true
Attachment #8766050 - Flags: review?(vladimir)
Attachment #8766050 - Flags: review?(dao+bmo)
Flags: needinfo?(dao+bmo)
Attachment #8778092 - Flags: review?(dao+bmo)
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.
Attachment #8778092 - Flags: review?(dao+bmo) → review-
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).
Attachment #8778092 - Attachment is obsolete: true
Flags: needinfo?(dao+bmo)
Attachment #8781706 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8781706 - Flags: review?(gijskruitbosch+bugs)
Changed to use overloaded handleDroppedLink function, and keep same droppedLinkHandler property name.

passed try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ad5f90a8e7b
Attachment #8781706 - Attachment is obsolete: true
Attachment #8781857 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8781857 - Flags: review?(gijskruitbosch+bugs) → review+
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.
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.
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?
Attachment #8782096 - Flags: feedback?(gijskruitbosch+bugs)
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.
Attachment #8782096 - Flags: feedback?(gijskruitbosch+bugs) → feedback?(mconley)
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.
Flags: needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin)
Attachment #8782096 - Flags: feedback?(mconley) → feedback?(enndeakin)
sorry, forgot to add dom/base/DroppedLinkItem.h and dom/base/DroppedLinkItemIPC.h files.
Attachment #8782096 - Attachment is obsolete: true
Attachment #8782096 - Flags: feedback?(enndeakin)
Attachment #8784578 - Flags: feedback?(enndeakin)
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.
Attachment #8784578 - Flags: feedback?(enndeakin) → feedback+
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
Attachment #8750659 - Attachment is obsolete: true
Attachment #8784578 - Attachment is obsolete: true
Attachment #8786464 - Flags: review?(enndeakin)
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
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
Attachment #8786464 - Attachment is obsolete: true
Attachment #8786464 - Flags: review?(enndeakin)
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
Attachment #8786493 - Flags: review?(enndeakin)
Attachment #8786493 - Flags: review?(enndeakin) → review+
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.
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
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.
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
Attachment #8789625 - Flags: review?(enndeakin)
(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).
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
Attachment #8789625 - Attachment is obsolete: true
Attachment #8789625 - Flags: review?(enndeakin)
Attachment #8789968 - Flags: review?(enndeakin)
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?
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
Attachment #8789968 - Attachment is obsolete: true
Attachment #8789968 - Flags: review?(enndeakin)
Attachment #8790312 - Flags: review?(enndeakin)
Attachment #8790312 - Flags: review?(enndeakin) → review+
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.
No longer blocks: 417918
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
OS: Windows 98 → All
Hardware: x86 → All
Whiteboard: [has draft patch]
Depends on: 1323276
Flags: qe-verify+
I have reproduced this bug following the comment 0 with Nightly 25.0a1 (2013-07-02) on Ubuntu 16.10, 64 bit!

The bug's fix is now verified on Latest Beta 52.0b8.

Build ID   :    20170220070057
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0

[bugday-20170222]
I have reproduced this bug with Firefox nightly 51.0a1 (2016-09-11)on
windows 7(64 bit)

I have verified this bug as fixed with Firefox beta 52.0b8 (build id:20170220070057)
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0

[bugday-20170222]
Thanks Md.Majedul isalm and Kazi Nuzhat Tasnem for verifying this issue.
I've also verified this fix on Firefox 52.0b9, under Mac OS 10.11.6.
Based on Comment 126, Comment 127 and on my results, I'm marking this issue Verified Fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1402380
Depends on: 1435910
You need to log in before you can comment on or make changes to this bug.