STR: - Go to URL - Scroll down to http://willemzwijgtnietmeer.web-log.nl/ (use findbar to locate it) - Select and attempt to drag this URL to bookmarks toolbar Expected result: dragged URL becomes bookmark. Actual result: none. This regressed on 27 Aug 2008: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-08-25+22%3A00%3A00&enddate=2008-08-28+13%3A00%3A00 between 20080827052043 and 20080827061801 so probably from Bug 356295.

And neither can you drag a url (or partial url) from the navigation toolbar and drop it on the bookmark toolbar. (do not confuse this with dragging the favicon to the bookmark toolbar) All working: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20080919032843 Minefield/3.1b1pre Cursor 'animation' still working in this one, but doesn't create a bookmark. Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20080920033605 Minefield/3.1b1pre Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081011 Minefield/3.1b2pre In this one, i can't even drag the text in the url bar at all Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081018 Minefield/3.1b2pre Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081020 Minefield/3.1b2pre In this one, i can drag again, but not onto bookmark bar, shows blocked cursor (= current behaviour also): Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081022 Minefield/3.1b2pre

comment 1 is now fixed in bug 478070.

This is a pretty serious regression, just updated a friend of mine to 3.5, this was the first thing he ran into. It's an extremely common action to drag a url from the bar to the bookmark toolbar, IMO, putting this on the radar. Note: this probably belongs in Browser::<something> as it doesn't really seem to be a Core bug, rather a bookmark or Navigation Bar bug...

while this bug is still relevant for 3.5.2, it seems to be fixed in 3.6a2

my bad - i was referring to comment #1. the original bug is still broken in 3.6a2

can the mechanism of Bug 454518 be of use here?

Ran into this upgrading systems from 3.x to 4. This is a rather serious oversight, as many users bookmark exclusively by dragging the text from the location bar to the bookmarks toolbar- It isn't obvious there is a work-around, it just appears that bookmarking is no longer supported in the new version.

The problem was that we were doing a string compare on the transferable flavor mime type. If we were given something of type text/plain we should have made the best matched flavor to text/unicode (conversion would be made internally).

Comment on attachment 548198 [details] [diff] [review] Patch for bookmarking unlinked URLs If it's not to be generally used, it's probably best to avoid adding a PlacesUtils constant for it (just hardcode it in the one place where you need it). :mak and/or Mano should probably review this, flagging them both and they can sort it out :)

OK thanks for the feedback Gavin. I'll wait for the additional feedback from others before doing any changes here.

Comment on attachment 548198 [details] [diff] [review] Patch for bookmarking unlinked URLs Review of attachment 548198 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/content/controller.js @@ +1378,5 @@ > } > + > + // We couldn't find a supported flavor, we need to ensure that we aren't > + // trying to use text/plain though and if so indicate to use text/unicode > + // A conversion internally will be made automatically to text/unicode. let's compress this coment a bit. "If no supported flavor is found, check if data includes text/plain contents. If so request them as text/unicode, conversion will happen automatically." @@ +1381,5 @@ > + // trying to use text/plain though and if so indicate to use text/unicode > + // A conversion internally will be made automatically to text/unicode. > + if (aFlavors.contains(PlacesUtils.TYPE_PLAIN_TEXT) != -1) { > + return PlacesUtils.TYPE_UNICODE; > + } I agree with Gavin, this is not useful to be exposed from utils for now, just hardcode "text/plain" here. @@ +1383,5 @@ > + if (aFlavors.contains(PlacesUtils.TYPE_PLAIN_TEXT) != -1) { > + return PlacesUtils.TYPE_UNICODE; > + } > + > + // We couldn't find a flavor match This comment doesn't seem that useful, just remove it

A couple things yet, do we really have a contains() method on arrays? Also making a test for this should be possible and most likely required for a positive review.

Thanks for the review Marco. aFlavors is of type DOMStringList: https://developer.mozilla.org/En/DOM/DOMStringList I'll implement those changes and look into a chrome mochitest

(In reply to comment #19) > aFlavors is of type DOMStringList: > https://developer.mozilla.org/En/DOM/DOMStringList ugh! my memory fails, please add a note in the javadoc above getFirstValidFlavor, on the aFlavors param.

Just updating with review comments. Before I ask for another review though I'll implement the chrome mochitest.

according to http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/core/nsIDOMDOMStringList.idl#51 contains() returns a boolean, so checking it against -1 sounds still wrong. Also, please use nsIDOMDOMStringList in the javadoc, since it's what we really get

Fixed, it was left over from me also thinking originally it was an array and using indexOf. Pending test case.

- The mochitest passes with the new code and fails before the new code. - It tests mainly for text/plain but I also tried for unicode and text/x-moz-url - It tests for each drop effect type as well

Just had to qrefresh a comment change

Comment on attachment 549094 [details] [diff] [review] Patch for bookmarking unlinked URLs + review comments + chrome mochitest Review of attachment 549094 [details] [diff] [review]: ----------------------------------------------------------------- the patch is fine, the test needs some minor cleanup, r=me with the following fixed (as usual nits are not needed to get a positive review, but appreciated if you fix them). if you want a further final check, feel free to ask it. ::: browser/components/places/tests/browser/browser_475045.js @@ +13,5 @@ > + * > + * The Original Code is Places test code. > + * > + * The Initial Developer of the Original Code is > + * Mozilla Foundation. nit: "the Mozilla Foundation." but see below @@ +32,5 @@ > + * and other provisions required by the GPL or the LGPL. If you do not delete > + * the provisions above, a recipient may use your version of this file under > + * the terms of any one of the MPL, the GPL or the LGPL. > + * > + * ***** END LICENSE BLOCK ***** */ nit: if you wish you may use the PD license in tests (http://www.mozilla.org/MPL/boilerplate-1.1/pd-c) you don't have to, if you don't want to. The clear advantage is it's shorter and allows any kind of code reuse. @@ +41,5 @@ > + let toolbar = document.getElementById("PersonalToolbar"); > + let wasCollapsed = toolbar.collapsed; > + if (wasCollapsed) { > + setToolbarVisibility(toolbar, true); > + } if the test timeouts or fails for whatever reason, it may leave toolbar in an unwanted status, so rather use registerCleanupFunction here to restore it at original status after test run @@ +46,5 @@ > + > + // Setup a node we will use to be dropped. The actual node used does not > + // matter, because we will set its data, effect, and mimeType manually > + let placesItems = document.getElementById("PlacesToolbarItems"); > + var srcElement = placesItems.childNodes[0]; I think it's safer if you test that is not null and its localName, since previous tests may do funny stuff. nit: please don't mix up let and var unless you have really good reasons to, let is usually fine. @@ +49,5 @@ > + let placesItems = document.getElementById("PlacesToolbarItems"); > + var srcElement = placesItems.childNodes[0]; > + > + // simulateDragDrop will simulate a drop of a URI onto the bookmarks bar > + // using the specified effect and mime type use a real javadoc here, no reason to use single-lines comments to document a function @@ +51,5 @@ > + > + // simulateDragDrop will simulate a drop of a URI onto the bookmarks bar > + // using the specified effect and mime type > + var simulateDragDrop = function(aEffect, aMimeType) { > + // Simulate a dorp of a URI typo: dorp nit: we prefer to make comments read like phrases, so punctuation may help readability (uc first, end with period, and such) @@ +52,5 @@ > + // simulateDragDrop will simulate a drop of a URI onto the bookmarks bar > + // using the specified effect and mime type > + var simulateDragDrop = function(aEffect, aMimeType) { > + // Simulate a dorp of a URI > + let uriSpec = "http://www.mozilla.org/D1995729-A152-4e30-8329-469B01F30AA7"; nit: you can use a const URL = "..." here @@ +66,5 @@ > + let bookmarkIds = PlacesUtils.bookmarks > + .getBookmarkIdsForURI(uri); > + > + // Verify that there is exactly one URI only > + ok(bookmarkIds.length == 1, "There should be exactly one bookmark"); I think the second check here (getBookmarksIdsForURI) makes the first check (isBookmarked) pretty much useless, I'd remove the isBookmarked check. @@ +68,5 @@ > + > + // Verify that there is exactly one URI only > + ok(bookmarkIds.length == 1, "There should be exactly one bookmark"); > + > + // Verify that we removed the bookmark successfully nit: we have not yet removed it, we are about to do that. @@ +78,5 @@ > + // Simulate a bookmark drop for all of the mime types and effects > + var mimeTypes = ["text/plain", "text/unicode", "text/x-moz-url"]; > + var effects = ["move", "copy", "link"]; > + for (e in effects) { > + for (m in mimeTypes) { per good code practices, don't use for...in on arrays, rather use .forEach or a plain for/while loop. While that doesn't matter here, we know lots of addons developers use tests to get inspiration for their code, so we should avoid common pitfalls.

Thanks for the review, I'll implement all comments and r? you again to be sure.

Implemented review comments on the chrome mochitest.

Minor change from last patch I forgot to qrefresh

Comment on attachment 550128 [details] [diff] [review] Patch for bookmarking unlinked URLs + review comments + chrome mochitest Review of attachment 550128 [details] [diff] [review]: ----------------------------------------------------------------- only nits, thanks for taking care of this. ::: browser/components/places/tests/browser/browser_475045.js @@ +15,5 @@ > + registerCleanupFunction(function() { > + if (wasCollapsed) { > + setToolbarVisibility(toolbar, false); > + } > + }); nit: you may directly if (toolbar.collapsed) { setToolbarVisibility(toolbar, true); registerCleanupFunction(function() { setToolbarVisibility(toolbar, false); }); } @@ +48,5 @@ > + PlacesUtils.bookmarks.removeItem(bookmarkIds[0]); > + > + // Verify that we removed the bookmark successfully. > + let bookmarked = PlacesUtils.bookmarks.isBookmarked(uri); > + ok(!bookmarked, "URI should be removed"); nit: no need for the local, you can just ok(!PlacesUtils...

Fixed nits, thanks for the suggestions.

Comment on attachment 550345 [details] [diff] [review] Patch for bookmarking unlinked URLs + review comments + chrome mochitest it's fine, doesn't need a further review

Does it need an r+ flag on the patch for someone to pick it up for checkin?

(In reply to comment #33) > Does it need an r+ flag on the patch for someone to pick it up for checkin? nope, it just needs a r+ somewhere in the bug and a checkin-needed in the whiteboard

ops, I meant in the keywords field :)

backed out, the test is permaorange :( http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1312574380.1312575673.18633.gz http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1312574263.1312575676.18642.gz

Comment on attachment 550345 [details] [diff] [review] Patch for bookmarking unlinked URLs + review comments + chrome mochitest Sorry :( Will try again locally with pulling recent changes and will run through try after that.

Mentioned in IRC already but I think the dependent bug needed to be in first. Before asking again for a checkin though I'll definitely investigate more and run through try :S

hm, the test had some green run too, looks like it's a frequent random failure, not a permanent failure.

> the test had some green run too, Where was the test run? Do you have a link?

hm I may have been wrong, it was permafailure on osx, green on others... http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a8bca81215ea but note I had to backout bug 673301 for similar reasons, there may be a D&D bug on OSX that hits both.

bug 673301 is in inbound now, with a possible workaround for the exception, may you try to push this to try on top of inbound tip, checking mochitest-oth on osx?

Thanks for letting me know Marco, I'll run it through try today and if it passes I'll put it back onto inbound.

I wanted to ensure everything worked so I pushed to all tests and platforms here: http://tbpl.mozilla.org/?tree=Try&rev=a4e2a8487328

Sorry I realized that I pushed to mozilla-central try so I cancelled that try request, I'll put another one out.

OK I repushed to try from a fully updated mozilla-inbound. I think that should work now.

you probably want to use tbpl.allizom.org for now (it's the temporary replacement for the official tbpl, due to excessive load on Tinderbox): http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=aae42b831c3b

Ya sorry someone told me on IRC after I posted, thanks though.

If I put the synthesizeDrop in a try/catch statement the test succeeds fine and the bookmarks do get created as they should on OS X. I setup my dev environment on OS X and debugged a bit so far and found that the nsBaseDragService::EndDragSession method is returning NS_ERROR_FAILURE because of a NULL mDoingDrag. (See code snippet below) I still need to investigate the cause, for example maybe it is being called twice. Also it consistently fails on EndDragSession no matter what the mime type and drop effect is on OS X. On Windows nsBaseDragService::EndDragSession is returning fine so mDoingDrag is non NULL. Note: We actually ignore the return result on Windows of EndDragSession but if I properly return the real return result is succeeds anyway. nsBaseDragService::EndDragSession(PRBool aDoneDrag) { if (!mDoingDrag) { return NS_ERROR_FAILURE; } I'll investigate more and update again.

So the problem on OS X was that a window move was happening which cancels the drag, and then another end drag was issued a second time which threw the exception in JS from EndDragSession. Call stack: nsBaseDragService::EndDragSession nsDragService::EndDragSession nsBaseDragService::Suppress nsGlobalWindow::CanMoveResizeWindows Problem stems from the call stack from this: nsGlobalWindow::CanMoveResizeWindows { if (ds) { gDragServiceDisabled = PR_TRUE; ds->Suppress(); <-- } There were 2 changes I needed to do so that the window wouldn't move. 1) EventUtils.synthesizeDrop needed to use the source element mouse down from center point instead of (2,2) 2) I needed to use a bookmark as the source element instead of the places toolbar. I ran this on try as well and I think it's good: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=4585f77734fa

Comment on attachment 554776 [details] [diff] [review] Bug 475045 - Can't drag unlinkified URL to bookmarks toolbar (regression from Firefox 3) + OS X fix Review of attachment 554776 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/tests/browser/browser_475045.js @@ +29,5 @@ > + */ > + let simulateDragDrop = function(aEffect, aMimeType) { > + const uriSpec = "http://www.mozilla.org/D1995729-A152-4e30-8329-469B01F30AA7"; > + let uri = makeURI(uriSpec); > + EventUtils.synthesizeDrop(placesItems.childNodes[0], Can you sanity check that this is not null above, where you test for placesItems? ::: testing/mochitest/tests/SimpleTest/EventUtils.js @@ +637,5 @@ > > try { > // need to use real mouse action > aWindow.addEventListener("dragstart", trapDrag, true); > + synthesizeMouseAtCenter(srcElement, { type: "mousedown" }, aWindow); Imo this is fine, but just in case I'd prefer Neil to approve the change to EventUtils

Does comment 51 describe an underlying bug?

> Does comment 51 describe an underlying bug? Ya even if I supply a bookmark button in synthesizeDrop as the source element it would move the whole window on OS X. When it moves the window instead of the bookmark it cancels the drag. Setting the mouse down to be the center of the source element instead fixes the problem.

may be due to rounded corners on those buttons? dragging the toolbar moves the window, isn't it? so if you hit the space out of the button border, you are dragging the window.

> may be due to rounded corners on those buttons? That sounds possible ya. I thought those coordinates 2,2 were relative to the source element so it didn't make sense to me why. But that would explain it. > dragging the toolbar moves the window, isn't it? On OS X yes, on Windows no. > so if you hit the space out of the button border, you are dragging the window Ya so I think dragging from the center of the source element is best.

Updated test case for no children check. Review needed for the EventUtils change.

Thanks for the fix! I think bug 670587 is related to this bug because without it it doesn't work for majority of users.

submitted Bug 701647 to allow dragging when the leading protocol is missing

Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0 Mozilla/5.0 (X11; Linux x86_64; rv:10.0a2) Gecko/20111117 Firefox/10.0a2 Mozilla/5.0 (X11; Linux x86_64; rv:11.0a1) Gecko/20111117 Firefox/11.0a1 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0) Gecko/20100101 Firefox/9.0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111116 Firefox/10.0a2 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111117 Firefox/11.0a1 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:9.0) Gecko/20100101 Firefox/9.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a2) Gecko/20111116 Firefox/10.0a2 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0a1) Gecko/20111117 Firefox/11.0a1 Mozilla/5.0 (Windows NT 5.1; rv:9.0) Gecko/20100101 Firefox/9.0 Mozilla/5.0 (Windows NT 5.1; rv:10.0a2) Gecko/20111116 Firefox/10.0a2 Mozilla/5.0 (Windows NT 5.1; rv:11.0a1) Gecko/20111117 Firefox/11.0a1 Verified on F9 Beta 2, Aurora and Nightly. STR used: 1. Load "data:text/html,http://www.mozilla.org" (any site) 2. Select the unlinkified text. 3. Drag the text to the bookmark toolbar. As expected, the dragged URL becomes a bookmark.