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.

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.

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?

I'm adding the review keyword to see if someone cares a little about this pseudo-patch.

qa contact -> pmac

*** Bug 185566 has been marked as a duplicate of this bug. ***

*** Bug 226931 has been marked as a duplicate of this bug. ***

*** Bug 286062 has been marked as a duplicate of this bug. ***

*** Bug 248200 has been marked as a duplicate of this bug. ***

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 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.

Thank you for your feedback!

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.

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

Sorry, I forgot to remove the code which does not work.

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 on attachment 8369903 [details] [diff] [review] part2: support DnD files on Tab Groups Tim should look at this instead.

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.

Just as a backup, here's draft patches, based on bug 966986. I'll post them separately once bug 966986 gets resolved.

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

[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.

[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 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 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.

Thank you for reviewing :D Addressed review comments.

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?

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.

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?

> > 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.

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?

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.

rebasing

hi, can you review this?

hi, can you review this?

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:

Thank you for reviewing :) Updated the test and added some comments for each testcase.

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

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?"

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

Behavior change: * When multiple links are dropped to New Window button, all items are opened in new windows, 1 window per each item

Behavior change: * When multiple (non-local) links are dropped to Downloads button (toolbar button), all items are downloaded

Behavior change: * When multiple (non-local) links are dropped to Downloads view in Library Window, all items are downloaded

* Changed browserDragAndDrop.drop to browserDragAndDrop.dropLinks in urlbarBindings.xml. no behavior change.

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 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 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 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 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 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);

Just rebased.

(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.

Changed testcase to use pushPrefs, BrowserTestUtils.domWindowOpened, and BrowserTestUtils.waitForEvent for "load" event.

Changed testcase to follow updated browser_tabDrop.js.

Changed testcase to follow updated browser_tabDrop.js, also, applied requestLongerTimeout.

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 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 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.

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

Do you have time to review? or can you suggest anyone else?

and rebasing remaining parts

testing rebased patches https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f58cdf062cb

rebased, and fixed legacy generator to ES6 generator

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?

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 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.

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 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.

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 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.

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?

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.

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 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.

sorry, forgot to add dom/base/DroppedLinkItem.h and dom/base/DroppedLinkItemIPC.h files.

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.

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 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

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

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

(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

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

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.

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

Updated following documents: https://developer.mozilla.org/en-US/Firefox/Releases/52 https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIDroppedLinkHandler https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIDroppedLinkItem https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/tabbrowser https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/loadTabs https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/browser https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/droppedLinkHandler

https://hg.mozilla.org/mozilla-central/rev/e51a6f9a0ebb https://hg.mozilla.org/mozilla-central/rev/1d44acbb76fe https://hg.mozilla.org/mozilla-central/rev/978a6615d7cc https://hg.mozilla.org/mozilla-central/rev/40e1dbd5409b https://hg.mozilla.org/mozilla-central/rev/9b53031d587c https://hg.mozilla.org/mozilla-central/rev/3e65ab5b5f38 https://hg.mozilla.org/mozilla-central/rev/2d201f01953f https://hg.mozilla.org/mozilla-central/rev/1107b20c6c51 https://hg.mozilla.org/mozilla-central/rev/b1976ce06a09 https://hg.mozilla.org/mozilla-central/rev/d87a29e5c993

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.