Closed Bug 410196 Opened 17 years ago Closed 16 years ago

Clearly allow or disallow tagging of history items

Categories

(Firefox :: Bookmarks & History, defect, P2)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3.6a1

People

(Reporter: bielawski1, Assigned: ddahl)

References

Details

(Keywords: verified1.9.1)

Attachments

(2 files, 14 obsolete files)

for checkin
11.99 KB, patch
Details | Diff | Splinter Review
new patch for branch
13.50 KB, patch
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2007122605 Minefield/3.0b3pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2007122605 Minefield/3.0b3pre There are two possible ways to tag history entries (through the properties pane and copy-pasting it into a tag listing in the Library), but the properties pane is disabled for history entries (still works for bookmark entries). Reproducible: Always Steps to Reproduce: 1. Visit a page 2. Locate the page in the Library (Places Organizer) and select it 3. Make the Properties pane visible. All of the controls in it are disabled, preventing tagging. 4. Create a tag by tagging another page in the Bookmarks section of the Library 5. Right-click the page's entry and select Copy. 6. Paste the page into the tag you just created. Actual Results: It is impossible to tag a page using Properties pane, but it is possible to do this using copy-paste. Expected Results: Either block the usage of tags on non-bookmarked pages, or allow adding tags through the properties pane.
Flags: blocking-firefox3?
OS: Windows Vista → All
Hardware: PC → All
Version: unspecified → Trunk
Flags: blocking-firefox3?
??? Hundreds of bugs newer than this have been triaged, but not this one?
This is by design: tags are properties of bookmarks. However, it maybe desirable to allow tagging via non-bookmark UI, silently creating an unfiled bookmark.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Cannot add tags to history entries directly even though copy-paste to tag folder is possible → Clearly allow or disallow tagging of history items
Doing the steps above literally tags the history item, not putting it into a bookmark folder, but showing it as a bookmark in the awesomebar.
(In reply to comment #3) > Doing the steps above literally tags the history item, not putting it into a > bookmark folder, but showing it as a bookmark in the awesomebar. does it appear under Unfiled Bookmarks?
It doesn't.
Depends on: 419731
after bug 419731 drag & drop should create an unfiled bookmark when dropping an history item on a tag. have to check for cut/copy/paste.
Fixed at least as of: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9pre) Gecko/2008041206 Minefield/3.0pre ID:2008041206 Think bug 419731 fixed this.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
This is not fixed. I can still see this issue with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko Minefield/3.0pre ID:2008041321 Marco, you have said in comment 6 that a new bookmark should be created under "Unsorted Bookmarks". This is not done when using the above version. Tags are still added to the history item directly. D&D even doesn't work. If you place such a history item onto a tag nothing happens. But in using c&p I can reproduce it every time.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #8) > This is not fixed. I can still see this issue with Mozilla/5.0 (Macintosh; U; > Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko Minefield/3.0pre ID:2008041321 > > Marco, you have said in comment 6 that a new bookmark should be created under > "Unsorted Bookmarks". This is not done when using the above version. Tags are > still added to the history item directly. D&D even doesn't work. If you place > such a history item onto a tag nothing happens. But in using c&p I can > reproduce it every time. > Really have you attempted on a new profile? I can't tag history items at all anymore.
Sure, c&p a history entry to a tag. It will be placed there and automatically tagged.
(In reply to comment #10) > Sure, c&p a history entry to a tag. It will be placed there and automatically > tagged. > Sorry, there used to be another way as well to tag history that caused this problem. Forgot to test the orriginal bug reports method.
for me drag&drop a history item to a tag creates a new bookmark in unfiled, and i cannot past in a tag. This has clearly to be tested on a new profile untile left pane query is replaced. > Sure, c&p a history entry to a tag. It will be placed there and automatically > tagged. paste is disabled here since tag are now read only containers
(In reply to comment #10) > Sure, c&p a history entry to a tag. It will be placed there and automatically > tagged. > Yup, tested again on: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9pre) Gecko/2008041306 Minefield/3.0pre ID:2008041306 This time following the method of copying and pasting in to the tag. This is now fixed on a clean profile, at least for Windows (though obviously not yet fixed on an a old profile). Are you sure you tried it on a clean profile? Because otherwise this might be a mac issue.
I created a fresh profile again and used Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008041404 Minefield/3.0pre for testing: 1. If I'm doing d&d to a tag container, the history entry gets tagged. The new tag is listed under History and the bookmark can be seen under the tag container. Hitting delete on the bookmark under the tag container removes the tag (even when its not directly updated - you have to reopen the container) 2. Using c&p I'm able to add the same tag multiple times. Copying the history item and after selecting the appropriate tag container just hit paste multiple times. On OS X you will hear a beep each time you hit this shortcut. Opening the tag container gives you a list of the same tag e.g. "test, test, test, test". Hitting delete on this bookmark removes the tags step by step (please remember reselecting the tag container). Marcia or Stephend could you both have a look at this if you can reproduce with an OS X trunk build?
so that's through keyboard shortcut for copy&paste, not through context menu paste
Yes, using Cmd+C and Cmd+V on OS X. Also tested on Windows XP and there I can see the same behavior. The only difference is that the history item doesn't show up under the tag but also has tags listed under History. D&D also sets the tag to the history item. So its not only OS X which is affected and where the bug is not fixed.
This seems fixed on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008041804 Minefield/3.0pre. C&P nor D&D a history item to tag container do not create a tag on that history item.
Marco, I found the reason why history items can be tagged for me. If I do the steps mentioned above (d&d or c&p) a new bookmark is created for this history item under "Unsorted Bookmarks". This one gets the tag and will be shown in the history list. I think this is a nice feature but if this is intended, moving a history item to e.g. "Unsorted Bookmarks" should also do the same. Here we have actually an inconsistency. Which is the correct behavior? If its the former one I'll file a new bug for moving history items into bookmark folders.
- dropping history items in a bookmark folder should bookmark it in that folder. - dropping history items in a tag container should tag it and bookmark it in unfiled bookmarks. - C&P should ideally have the same behaviour as D&D (In reply to comment #17) > C&P nor D&D a history item to tag container do not create a tag on that history item. this is bad, d&d should be working, when this regressed?
Marco, I checked with a hourly build which was build directly after your patch on bug 419731 was checked-in. Even this build shows strange behaviors. I did some further testing and following issues are still open. Shall I file a new bug therefor or do you want to handle it in this bug? * D&D into a bookmark folder doesn't create a new bookmark * C&P into a bookmark folder creates the new bookmark twice. Removing one of these bookmarks deletes both.
they are different bugs, this is only about tagging HISTORY items with C&P or D&D, so please fill them separately
Using a new profile on: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9pre) Gecko/2008041807 Minefield/3.0pre ID:2008041807 I can confirm: D&D a history item in to a tag creates a new bookmark in unsorted bookmarks C&P using Ctrl+C and Ctrl+V tags the history item, but does NOT create a new bookmark in unsorted bookmarks.
Sorry, but the new bugs shouldn't be on the depends list here. Marking as FIXED again. I will file a follow-up bug to the remaining missing update issue when using c&p.
No longer blocks: 429808, 429809
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Given this bug isn't really fixed then, could we have a link asap to the new bug concerning c&p then please.
(In reply to comment #22) > C&P using Ctrl+C and Ctrl+V tags the history item, but does NOT create a new > bookmark in unsorted bookmarks. For me this is WFM but there is a problem with updating the tag containers view. I filed bug 429811 therefor.
(In reply to comment #25) > (In reply to comment #22) > > C&P using Ctrl+C and Ctrl+V tags the history item, but does NOT create a new > > bookmark in unsorted bookmarks. > > For me this is WFM but there is a problem with updating the tag containers > view. I filed bug 429811 therefor. > Triple checked it on: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9pre) Gecko/2008041807 Minefield/3.0pre ID:2008041807 1. Create New Profile 2. Create Tag using a default bookmark called "Test" 3. Visit a page (I used one the BBC News pages from the Live Headlines) 4. Go to places Library, go to History 5. Select a non-bookmarked History, press Ctrl+C 6. Select the Test tag container 7. Click in to the right pane, to make sure the window is properly selected 8. Press Ctrl+V, nothing appears to happen 9. Go to Unsorted Bookmarks, nothing there 10. Go to History, notice the item is now tagged with "Test" I can duplicate 100% of the time. Please check these steps quickly, maybe it's just a Windows issue.
Ok, I can reproduce it. It looks like a Windows only issue. As I understand Marcos comment it should be filed as a new bug. Can you do this? Just as a small additional note: the context menu within the tag container has a disabled state for paste...
(In reply to comment #27) > Ok, I can reproduce it. It looks like a Windows only issue. As I understand > Marcos comment it should be filed as a new bug. Can you do this? Just as a > small additional note: the context menu within the tag container has a disabled > state for paste... > Marco comment was this bug affected history items only, this still affects history items only. If you still think I should file a new bug I'll file one in about 30 mins if there's been no comment to this.
this bug is ONLY about a. drag & drop an history item onto a tag container b. copy & paste an history item in a tag container everything other should be filled elsewhere. Since a. appear to be fixed, while b. is not, i'm reopening (Diaman steps are clearly b.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Alex: you wanted to discuss the ui design of tagging history items over in bug 459438, any decisions?
Blocks: 459438
Ok, this bug is still present on each OS with Minefield. Marco, doing a c&p or d&d action on history items they get tagged but aren't placed into the unsorted bookmarks folder. That's different as the tagging via the property pane. Any chance to get this fixed for FF3.1? Would be nice to have a clear way in being able to tag history items or not. Alex, what's your position?
Flags: wanted-firefox3.1?
(In reply to comment #32) > Ok, this bug is still present on each OS with Minefield. > > Marco, doing a c&p or d&d action on history items they get tagged but aren't > placed into the unsorted bookmarks folder. That's different as the tagging via > the property pane. > > Any chance to get this fixed for FF3.1? Would be nice to have a clear way in > being able to tag history items or not. > > Alex, what's your position? blocking+ we should either not allow drop history items onto tag containers, or properly bookmark them (into unfiled).
Flags: wanted-firefox3.1? → wanted-firefox3.1+
Flags: wanted-firefox3.1+ → blocking-firefox3.1+
Priority: -- → P2
Assignee: nobody → ddahl
Test case https://litmus.mozilla.org/show_test.cgi?id=7477 needs to be re-activated after this bug has been fixed and verified per Henrik's suggestion.
we should clearly allow for tagging through paste as we do for drag&drop, adding an unsorted bookmark,
Keywords: uiwanted
(In reply to comment #34) > Test case https://litmus.mozilla.org/show_test.cgi?id=7477 needs to be > re-activated after this bug has been fixed and verified per Henrik's For that just set the in-litmus? flag, so we know that some work has to be done here.
Flags: in-litmus?
>> Alex, what's your position? > >blocking+ > >we should either not allow drop history items onto tag containers, or properly >bookmark them (into unfiled). same as dietrich's :) properly bookmarking them into unfield will probably result in less negative feedback from the (likely) very small population of people currently relying on this feature.
Blocks: 474066
changing from blocking+ to wanted+. this is not a regression, and not in primary UI.
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1-
Flags: blocking-firefox3.1+
Depends on: 475977
So far in my testing, via js shell - (will post a test case soon), using copy and paste, the tag is created, but the bookmark is not. Selecting the history node clearly shows a tag. Sounds like if you detect that the copied or pasted object is a history node, then the paste code should call a method that creates the bookmark (and tags) as well.
Just want to get some feedback, the patch does indeed fix this issue, but I think the bookmark that is created is using the url for the title. test case included.
Attachment #362821 - Flags: review?(mak77)
Comment on attachment 362821 [details] [diff] [review] First test case and possible fix. >diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js if you are going to fix all trailing spaces in the file, your patch will bitrot fast, practically with every change to this file > // adjusted to make sure that items are given the correct index - > // transactions insert differently if index == -1 > if (ip.index > -1) > index = ip.index + i; >- transactions.push(PlacesUIUtils.makeTransaction(items[i], type.value, >- ip.itemId, index, >- true)); >+ if (ip.isTag) { >+ var uri = PlacesUtils._uri(items[i].uri); >+ transactions.push(PlacesUIUtils.ptm.tagURI(uri,[ip.itemId])); >+ } >+ else { >+ transactions.push(PlacesUIUtils.makeTransaction(items[i], >+ type.value, >+ ip.itemId, >+ index, >+ true)); >+ >+ } >+ the approach looks correct, but as we talked on IRC, since we are special-casing both drop and paste, could you check if it would be hard to manage tags directly into PlacesUIUtils.makeTransaction while there take a look at getFolderCopyTransaction, i now see it contains some code to handle tags that could be wrong/unused some drive-by comment on the test >diff --git a/browser/components/places/tests/browser/browser_410196.js b/browser/components/places/tests/browser/browser_410196.js >+ * Portions created by the Initial Developer are Copyright (C) 2008 nit: 2009 >+function add_visit(aURI, aReferrer) { >+ var placeID = histsvc.addVisit(aURI, >+ Date.now() * 1000, >+ aReferrer, >+ histsvc.TRANSITION_TYPED, // user typed in URL bar >+ false, // not redirect >+ 0); >+ dump("### Added visit with id of " + placeID + "\n"); >+ return placeID; >+} addvisit returns visitId, not placeID, i know we have some miscomprehension in other tests, we should fix them. >+ >+function test() { >+ waitForExplicitFinish(); >+ // individual tests for each step of tagging a history item >+ var win = window.openDialog("chrome://browser/content/places/places.xul", >+ "", >+ "chrome,toolbar=yes,dialog=no,resizable"); >+ >+ win.addEventListener("load", function onload() { >+ executeSoon(function () { remember to remove listener also better if you create local vars like PU = win.PlacesUtils, would make it more readable >+ makeTag: function() { >+ // create an initial tag to work with >+ // (also creates another history object and bookmark) >+ try { >+ taggingSvc.tagURI(PlacesUtils._uri("http://example.com/"), >+ ["foo"]); this tags the uri, but won't expose it in the ui, you need to make the uri a bookmark before so double check your focusTag test below. even if it works, we don't support tagging history entries.
Attachment #362821 - Flags: review?(mak77)
Attached patch WIP patch for zpao to peruse (obsolete) — — Splinter Review
Attached patch another approach (obsolete) — — Splinter Review
Another approach - This does not try to fix the underlying issue of consolidating the d&d and copy/paste code. Because of thescope, perhaps that should be another bug?
Attachment #362821 - Attachment is obsolete: true
Attachment #365549 - Attachment is obsolete: true
Attachment #365552 - Flags: review?(mak77)
Comment on attachment 365552 [details] [diff] [review] another approach >diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js > if (ip.index > -1) > index = ip.index + i; >- transactions.push(PlacesUIUtils.makeTransaction(items[i], type.value, >- ip.itemId, index, >- true)); the only thing you should do is to create the correct transaction for the tag container case, that's because makeTransaction won't do that for you. As i said previously you could fix makeTransaction (best choice) or special case tags here (acceptable choice, since we are going to make changes to tags for 3.2, i won't stop you from doing that) >+ if (ip.isTag) { >+ var uri = PlacesUtils._uri(items[i].uri); >+ var taggedURI = PlacesUIUtils.ptm.tagURI(uri,[ip.itemId]); the result of ptm.tagURI is not a tagged URI, is a transaction, so you should call this something like "tagURITxn" >+ transactions.push(taggedURI); >+ >+ var bookmarkIds = []; >+ bookmarkIds = PlacesUtils.bookmarks.getBookmarkIdsForURI(uri,{}); All of this is useless, because placesTagURITransaction (See nsPlacesTransactionsService) will already create a bookmark for you if needed. Also notice you could have used getLastBookmarkForURI util here. I'm going on commenting the following code for you, even if it's useless for the above reasons >+ // history item also needs actual bookmark created >+ var unfiledFolder = PlacesUtils.bookmarks.unfiledBookmarksFolder; notice PU should also gives you PlacesUtils.unfiledBookmarksFolderId >+ try { >+ var node = PlacesUtils.history.getHistoryItemsForURI(uri); this is completely useless to get the title, you coul use bookmarks.getPageTitle(uri); >+ var idx = Ci.nsINavBookmarksService.DEFAULT_INDEX; >+ PlacesUtils.bookmarks.insertBookmark(unfiledFolder, >+ uri, >+ idx, >+ node.title); probably no need for all of these temp vars, you can format as PlacesUtils.bookmarks .insertBookmark(unfiledFolder, and you have plenty of space for arguments >+ } >+ catch(ex) { >+ // XXX: throw an error? pass silent? why could adding a bookmark throw? if the throw is unexpected, you should not hide it with a catch, i don't think there are valid causes for this to throw, unless something is really messed up! >+ } >+ } >+ } >+ else { >+ transactions.push(PlacesUIUtils.makeTransaction(items[i], >+ type.value, >+ ip.itemId, >+ index, >+ true)); >+ } for clarity, create a different transaction based on the type of the receiving container, and then push in transactions out of the if/else >diff --git a/toolkit/components/places/src/utils.js b/toolkit/components/places/src/utils.js >+ getHistoryNodeForURI: function PU_getHistoryNodeForURI(uri) { params should also start with a, so aURI >+ var hs = this.history; >+ var options = hs.getNewQueryOptions(); >+ var query = hs.getNewQuery(); >+ query.searchTerms = uri.spec; >+ var result = hs.executeQuery(query, options); this would be a perf hog, for many nodes, you would end up executing a query and filtering results every time >+ result.root.containerOpen = true; when you open a container, you should always close it (unless it's already open but that's a different case when you use nodes in one of our views) So the next for should have a bool found and cycle till i < result.root.childCount && !found
Attachment #365552 - Flags: review?(mak77) → review-
Attached patch much simplified patch (obsolete) — — Splinter Review
At some point someone needs to whiteboard out the places transactions system for me.
Attachment #365552 - Attachment is obsolete: true
Attachment #367849 - Flags: review?(mak77)
Attached patch added else path to if isTag (obsolete) — — Splinter Review
Attachment #367849 - Attachment is obsolete: true
Attachment #367877 - Flags: review?(mak77)
Attachment #367849 - Flags: review?(mak77)
Attachment #367877 - Attachment is obsolete: true
Attachment #367880 - Flags: review?(mak77)
Attachment #367877 - Flags: review?(mak77)
Comment on attachment 367880 [details] [diff] [review] removed duplicate tagging svc in test file >diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js >--- a/browser/components/places/content/controller.js >+++ b/browser/components/places/content/controller.js >+ if (ip.isTag) { >+ var uri = PlacesUtils._uri(items[i].uri); >+ var txn = PlacesUIUtils.ptm.tagURI(uri,[ip.itemId]); space after the comma please >+ transactions.push(txn); >+ } else { don't use this style, use } else { >+ // adjusted to make sure that items are given the correct index - >+ // transactions insert differently if index == -1 while there please fix this confusing comment: "Adjust index so that items are given a correct position, if index == -1 transaction will enqueue the item." >+ if (ip.index > -1) >+ index = ip.index + i; >+ transactions.push(PlacesUIUtils.makeTransaction(items[i], type.value, >+ ip.itemId, index, >+ true)); >+ } please have a local txn var, set it in the if-else and call transactions.push(txn) out of it, should be more clear >diff --git a/browser/components/places/tests/browser/browser_410196.js b/browser/components/places/tests/browser/browser_410196.js >new file mode 100644 >--- /dev/null >+++ b/browser/components/places/tests/browser/browser_410196.js >@@ -0,0 +1,254 @@ >+/* vim:set ts=2 sw=2 sts=2 et: */ >+/* ***** BEGIN LICENSE BLOCK ***** >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+ * >+ * The contents of this file are subject to the Mozilla Public License Version >+ * 1.1 (the "License"); you may not use this file except in compliance with >+ * the License. You may obtain a copy of the License at >+ * http://www.mozilla.org/MPL/ >+ * >+ * Software distributed under the License is distributed on an "AS IS" basis, >+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+ * for the specific language governing rights and limitations under the >+ * License. >+ * >+ * The Original Code is Places test code. >+ * >+ * The Initial Developer of the Original Code is Mozilla Corp. >+ * Portions created by the Initial Developer are Copyright (C) 2009 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): >+ * David Dahl <ddahl@mozilla.com> >+ * >+ * Alternatively, the contents of this file may be used under the terms of >+ * either the GNU General Public License Version 2 or later (the "GPL"), or >+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+ * in which case the provisions of the GPL or the LGPL are applicable instead >+ * of those above. If you wish to allow use of your version of this file only >+ * under the terms of either the GPL or the LGPL, and not to allow others to >+ * use your version of this file under the terms of the MPL, indicate your >+ * decision by deleting the provisions above and replace them with the notice >+ * 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 ***** */ >+ >+// Get history services >+var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"]. >+ getService(Ci.nsINavHistoryService); if you use short names "convention" this should be hs >+var gh = histsvc.QueryInterface(Ci.nsIGlobalHistory2); >+var bh = histsvc.QueryInterface(Ci.nsIBrowserHistory); >+ >+var taggingSvc = Cc["@mozilla.org/browser/tagging-service;1"] >+ .getService(Components.interfaces.nsITaggingService); >+ >+function add_visit(aURI, aReferrer) { >+ var visitID = histsvc.addVisit(aURI, visitId please >+ Date.now() * 1000, >+ aReferrer, >+ histsvc.TRANSITION_TYPED, // user typed in URL bar >+ false, // not redirect >+ 0); >+ //dump("### Added visit with id of " + visitID + "\n"); >+ return visitID; >+} >+ >+// Get bookmark service >+try { >+ var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"]. >+ getService(Ci.nsINavBookmarksService); >+} >+catch(ex) { >+ do_throw("Could not get the nav-bookmarks-service\n"); >+} make this bs, and move it up with other services, try/catch is not needed, we used them in old tests. >+ >+function test() { >+ waitForExplicitFinish(); >+ // individual tests for each step of tagging a history item >+ var win = window.openDialog("chrome://browser/content/places/places.xul", >+ "", >+ "chrome,toolbar=yes,dialog=no,resizable"); >+ >+ win.addEventListener("load", function onload() { >+ win.removeEventListener("load", onload, false); >+ executeSoon(function () { >+ var PU = win.PlacesUtils; >+ var PO = win.PlacesOrganizer; >+ var PUIU = win.PlacesUIUtils; >+ >+ var tests = { >+ >+ sanity: function(){ >+ // sanity check >+ ok(PU !== null, "PlacesUtils in scope"); >+ ok(PUIU !== null, "PlacesUIUtils in scope"); >+ ok(PO !== null,"Places organizer in scope"); >+ }, >+ >+ makeHistObj: function() { >+ // need to add a history object what's an history "object"? i would simply define this AddVisits or populateHistory, history object is practically a visit or a generic page >+ // make a uri, add it to the history >+ let testURI = PU._uri("http://example.com"); if you're going to use this url all around, define a const TEST_URL above >+ isnot(testURI,null,testURI +"is not null"); spaces after commas please, all around in your code, those are appreciated by readers :) >+ let tP = add_visit(testURI); tP? var names should generally be more "verbose" about their contents, this is a visit id >+ ok(tP > 0,"A visit was added to the history"); >+ ok(gh.isVisited(testURI),"example.com is a visited url."); >+ testURI = PU._uri("http://mozilla.com"); >+ isnot(testURI,null,testURI +"is not null"); >+ tP = add_visit(testURI); >+ ok(tP > 0,"A visit was added to the history"); >+ ok(gh.isVisited(testURI),"Mozilla.com is a visited url."); >+ }, >+ >+ makeTag: function() { >+ // create an initial tag to work with >+ // (also creates another history object and bookmark) >+ try { >+ taggingSvc.tagURI(PlacesUtils._uri("http://example.com/"), >+ ["foo"]); >+ var tags = taggingSvc.getTagsForURI( >+ PU._uri("http://example.com/"),{}); >+ ok(tags[0] == 'foo',"tag is foo"); this should be is(tags[0], "foo", "tag is foo"); avoid ok when you can do direct comparison with is, since is prints both strings in case of error >+ } >+ catch(e){ >+ dump("makeTag Failed: " + e); if this is a real unexpected failure you should throw or use ok(false, "makeTag Failed... dumping is not enough and this should probably block next tests >+ } >+ }, >+ >+ focusTag: function (paste){ >+ // focus the new tag >+ try { >+ PO.selectLeftPaneQuery("Tags"); >+ var tags = PO._places.view.nodeForTreeIndex(1); >+ tags.containerOpen = true; >+ var fooTag = tags.getChild(0); >+ this.tagNode = fooTag; >+ PO._places.selectNode(fooTag); // no side-efects >+ ok(this.tagNode !== null,"tagNode exists"); >+ var ip = PO._places.insertionPoint; >+ ok(ip,"IP is " + ip.itemId); what about checking isTag too? >+ if (paste) { >+ PO._places.controller.paste(); >+ } >+ } catch(e){ >+ ok(false,'focusTag: ' + e); >+ } >+ }, >+ >+ histNode: null, >+ >+ copyHistNode: function (){ >+ // focus the history object >+ try { >+ PO.selectLeftPaneQuery("History"); >+ this.histNode = PO._content.view.nodeForTreeIndex(0); >+ PO._content.selectNode(this.histNode); >+ ok(this.histNode.uri === 'http://mozilla.com/', >+ "historyNode exists: " + >+ this.histNode.uri); >+ // copy the history object >+ PO._content.controller.copy(); >+ } >+ catch(e){ >+ dump(e); as above, is this an error that should cause us to fail? if so dump is not enough >+ } >+ }, >+ >+ pasteToTag: function (){ >+ // paste histobj into tag cryptic comment >+ this.focusTag(true); >+ }, >+ >+ mozURISpec: 'http://mozilla.com/', can this be a const? do you need it as a property? >+ >+ historyNode: function (){ >+ // re-focus the history again >+ PO.selectLeftPaneQuery("History"); >+ var histNode = PO._content.view. >+ nodeForTreeIndex(1); >+ ok(histNode,"histNode exists: " + histNode.title); >+ PO._content.selectNode(histNode); >+ // check to see if the history node is tagged! >+ var tags = PU.tagging.getTagsForURI( >+ PU._uri(this.mozURISpec),{}); >+ ok(histNode.tags.length > 0,"history node is tagged: " + >+ histNode.tags.length); > 0 is not enough if you know how many tags you added, do a direct compare with number of tags >+ // check if a bookmark was created >+ var bkmk_true = PU.bookmarks.isBookmarked( >+ PU._uri(this.mozURISpec)); strange name for a var, isPageBookmarked would tell us more >+ //dump("Bkmk_true \n"); >+ //dump(bkmk_true); these need cleanup >+ ok(bkmk_true === true,"Mozilla.com is bookmarked"); ok(yourvar, "message") is enough >+ var ids = PU.bookmarks.getBookmarkIdsForURI( >+ PU._uri(histNode.uri),{}); >+ ok(ids > -1,"bookmark exists for the tagged history item: " + ids); add some comments, for example, what are ids, why are you testing them? >+ }, >+ >+ checkForBookmarkInUI: function(){ >+ try { >+ // is the bookmark visible in the UI? >+ PO.selectLeftPaneQuery("AllBookmarks"); >+ // we selected all bookmarks, lets get the node >+ var nodeList = PO._places.getSelectionNodes(); >+ // select the All Bookmarks node in the UI >+ PO._places.selectNode(nodeList[0]); >+ //get the Unsorted Bookmarks node >+ var node = nodeList[0].getChild(2); >+ dump(node.title); >+ // select Unsorted Bookmarks in the UI >+ PO._places.view.nodeForTreeIndex( >+ node.viewIndex); >+ PO._places.selectNode(node); >+ // now we can see what is in the _content tree >+ var unsrtdNode = PO._content.view. >+ nodeForTreeIndex(0); don't eat vowels in var names please, this makes us AS/400 fans. >+ //dump("unsrtdNode.uri: " + unsrtdNode.uri); >+ PO._content.selectNode(unsrtdNode); >+ ok(unsrtdNode !== null,"unsrtdNode is not null: " + unsrtdNode.uri); >+ ok(unsrtdNode.uri === this.histNode.uri,"node uri's are the same"); >+ } >+ catch(e){ >+ ok(false,"checkForBookmarkInUI: " + e); >+ } >+ }, >+ >+ tagNode: null, >+ >+ queryForURI: function (){ >+ // test placesQuery >+ >+ var hs = Cc["@mozilla.org/browser/nav-history-service;1"] >+ .getService(Components.interfaces.nsINavHistoryService); you already have this, right? >+ var options = hs.getNewQueryOptions(); >+ var query = hs.getNewQuery(); >+ query.searchTerms = "http://mozilla.com"; >+ var result = hs.executeQuery(query, options); >+ result.root.containerOpen = true; >+ if (result.root.hasChildren) { >+ for (var i=0; i < result.root.childCount; i++){ >+ let n = result.root.getChild(i); >+ ok(PU.uri(n.uri) === PU._uri(this.mozURISpec)); >+ } >+ } >+ } what's the purpose here, if it's simply check if the uri is in history, you have better ways to do that.
Attachment #367880 - Flags: review?(mak77)
Just for the record, the code in this test reflects a new (to Moz) developer trying to understand how to use Places code and the chrome tests. Some of what you see here (including comments) was copied from other tests or introspected via javascript shell, since it is not very apparent how any of this works. There are 14 ways to do some things and zero obvious ways to do other things. when writing a comment like this: > what's the purpose here, if it's simply check if the uri is in history, you have better ways to do that. perhaps you could give an example? that would be better. Thanks for the feedback, I am cleaning this up now.
all clean (i hope)
Attachment #367880 - Attachment is obsolete: true
Attachment #368071 - Flags: review?(mak77)
Attachment #368071 - Flags: review?(mak77)
Comment on attachment 368071 [details] [diff] [review] cleaned up style of tests and controller >diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js >+ if (ip.isTag) { >+ var uri = PlacesUtils._uri(items[i].uri); >+ var txn = PlacesUIUtils.ptm.tagURI(uri, [ip.itemId]); >+ transactions.push(txn); since you do this for both if and else, you can do it out of it, defining var txn before the if/else var txn; if/else transactions.push(txn) >diff --git a/browser/components/places/tests/browser/browser_410196.js b/browser/components/places/tests/browser/browser_410196.js >+// Get history services >+var hs = Cc["@mozilla.org/browser/nav-history-service;1"]. >+ getService(Ci.nsINavHistoryService); getService aligned with start of Cc >+var gh = hs.QueryInterface(Ci.nsIGlobalHistory2); >+var bh = hs.QueryInterface(Ci.nsIBrowserHistory); >+var ts = Cc["@mozilla.org/browser/tagging-service;1"] >+ .getService(Components.interfaces.nsITaggingService); dot on the first line and getService aligned with start of Cc >+var bs = Cc["@mozilla.org/browser/nav-bookmarks-service;1"]. >+ getService(Ci.nsINavBookmarksService); as above >+ >+function add_visit(aURI, aReferrer) { >+ var visitId = hs.addVisit(aURI, >+ Date.now() * 1000, >+ aReferrer, >+ hs.TRANSITION_TYPED, // user typed in URL bar >+ false, // not redirect >+ 0); >+ return visitId; >+} >+ >+const TEST_URL = "http://example.com/"; >+const MOZURISPEC = "http://mozilla.com/"; >+ >+function test() { >+ waitForExplicitFinish(); >+ // individual tests for each step of tagging a history item this comment looks misplaced, should probably go near var tests? >+ var win = window.openDialog("chrome://browser/content/places/places.xul", >+ "", >+ "chrome,toolbar=yes,dialog=no,resizable"); >+ >+ win.addEventListener("load", function onload() { >+ win.removeEventListener("load", onload, false); >+ executeSoon(function () { >+ var PU = win.PlacesUtils; >+ var PO = win.PlacesOrganizer; >+ var PUIU = win.PlacesUIUtils; >+ >+ var tests = { >+ >+ sanity: function(){ >+ // sanity check >+ ok(PU, "PlacesUtils in scope"); >+ ok(PUIU, "PlacesUIUtils in scope"); >+ ok(PO, "Places organizer in scope"); >+ }, >+ >+ makeHistVisit: function() { >+ // need to add a history object >+ // make a uri, add it to the history First comment looks useless since second comment tells me everything i need to know >+ let testURI = PU._uri(TEST_URL); >+ isnot(testURI, null, testURI + "is not null"); this will print out the nsIURI "signature" attached to is not null, i think you only want "testURI is not null" >+ let visitId = add_visit(testURI); >+ ok(visitId > 0, "A visit was added to the history"); >+ ok(gh.isVisited(testURI), "example.com is a visited url."); use testURI.spec or the const >+ testURI = PU._uri("http://mozilla.com"); please don't use the same variable, could be confusing, call this testURI2. should not you use MOZURISPEC const? >+ isnot(testURI, null, testURI + "is not null"); as above >+ visitId = add_visit(testURI); >+ ok(visitId > 0, "A visit was added to the history"); >+ ok(gh.isVisited(testURI), "Mozilla.com is a visited url."); use testURI.spec or the const >+ }, >+ >+ makeTag: function() { >+ // create an initial tag to work with >+ // (also creates another history object and bookmark) >+ ts.tagURI(PlacesUtils._uri(TEST_URL), ["foo"]); >+ var tags = ts.getTagsForURI(PU._uri(TEST_URL), {}); >+ is(tags[0], 'foo', "tag is foo"); >+ }, >+ >+ focusTag: function (paste){ >+ // focus the new tag >+ PO.selectLeftPaneQuery("Tags"); >+ var tags = PO._places.view.nodeForTreeIndex(1); this is fragile, since you know Tags is tree index 1 but in future it could change position try looking here for a possible alternative to select the query and get the node (aka selectedNode) http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js#51 >+ tags.containerOpen = true; >+ var fooTag = tags.getChild(0); you must check the tag node you have found is "foo", so check node's title >+ this.tagNode = fooTag; >+ PO._places.selectNode(fooTag); // no side-efects nit: effects, btw what does this comment mean? >+ ok(this.tagNode, "tagNode exists"); this should be done before you try to select the node, right? >+ var ip = PO._places.insertionPoint; >+ ok(ip, "IP is " + ip.itemId); a mere itemId != null doesn't tell me much, you should also ensure the ip is your tag, so check ip.isTag and if you get the itemId you should compare it with tag's itemId >+ if (paste) { >+ PO._places.controller.paste(); >+ } >+ }, >+ >+ histNode: null, >+ >+ copyHistNode: function (){ >+ // focus the history object >+ PO.selectLeftPaneQuery("History"); >+ this.histNode = PO._content.view.nodeForTreeIndex(0); as above >+ PO._content.selectNode(this.histNode); >+ ok(this.histNode.uri === 'http://mozilla.com/', >+ "historyNode exists: " + this.histNode.uri); use the const? >+ // copy the history node >+ PO._content.controller.copy(); >+ }, >+ >+ pasteToTag: function (){ >+ // paste history node into tag >+ this.focusTag(true); >+ }, >+ >+ historyNode: function (){ >+ // re-focus the history again >+ PO.selectLeftPaneQuery("History"); >+ var histNode = PO._content.view.nodeForTreeIndex(1); as above >+ ok(histNode, "histNode exists: " + histNode.title); >+ PO._content.selectNode(histNode); >+ // check to see if the history node is tagged! >+ var tags = PU.tagging.getTagsForURI(PU._uri(MOZURISPEC), {}); >+ ok(histNode.tags.length == 3, "history node is tagged: " + >+ histNode.tags.length); you mix histNode.tags with tags variable and don't check for "foo" (btw the tag "foo" should probably be defined as a const with uris. It's fine if you check both histNode.tags and also tags got through getTagsForURI Also pay attention to formatting please (the second line is not aligned) >+ // check if a bookmark was created >+ var isBookmarked = PU.bookmarks.isBookmarked( >+ PU._uri(MOZURISPEC)); >+ is(isBookmarked, true, "Mozilla.com is bookmarked"); use const >+ var bookmarkIds = PU.bookmarks.getBookmarkIdsForURI( >+ PU._uri(histNode.uri), {}); >+ ok(bookmarkIds > 0, "bookmark exists for the tagged history item: " + bookmarkIds); >+ }, >+ >+ checkForBookmarkInUI: function(){ >+ // is the bookmark visible in the UI? >+ PO.selectLeftPaneQuery("AllBookmarks"); >+ // we selected all bookmarks, lets get the node >+ var nodeList = PO._places.getSelectionNodes(); >+ // select the All Bookmarks node in the UI >+ PO._places.selectNode(nodeList[0]); >+ //get the Unsorted Bookmarks node >+ var node = nodeList[0].getChild(2); >+ // select Unsorted Bookmarks in the UI as above, this is fragile >+ PO._places.view.nodeForTreeIndex(node.viewIndex); >+ PO._places.selectNode(node); >+ // now we can see what is in the _content tree >+ var unsortedNode = PO._content.view.nodeForTreeIndex(0); >+ PO._content.selectNode(unsortedNode); >+ ok(unsortedNode, "unsortedNode is not null: " + unsortedNode.uri); >+ ok(unsortedNode.uri === this.histNode.uri, >+ "node uri's are the same"); probably you don't need to cache histNode if you checked above histNode.uri is == to your const (so you can directly use the const) >+ }, >+ >+ tagNode: null, >+ >+ queryForURI: function (){ this function looks unused and never called, what's the purpose of querying history since you already did a bunch of checks above to ensure visit has been added? overall looks cleaner than before
I think using PO._content.view.nodeForTreeIndex(0) is ok inside of tests where you know the item you are selecting is inside of _content and was just created by the script. I am posting a new patch right now.
Attached patch further clean up to tests (obsolete) — — Splinter Review
Attachment #368071 - Attachment is obsolete: true
Attachment #369303 - Flags: review?(mak77)
Attachment #369303 - Attachment is obsolete: true
Attachment #369310 - Flags: review?(mak77)
Attachment #369303 - Flags: review?(mak77)
Comment on attachment 369310 [details] [diff] [review] moved transactions.push outside of conditional block >diff --git a/browser/components/places/tests/browser/browser_410196.js b/browser/components/places/tests/browser/browser_410196.js >+ isnot(testURI, null, testURI + "is not null"); nit: when you merge strings be sure you add an empty space in front of the second one, here and all other places below.
Attachment #369310 - Flags: review?(mak77) → review+
uh one last thing, you're mixing up var and let in some function, use var everywhere unless you explicitely need let.
added a test function that removes the tags and history
Attachment #369310 - Attachment is obsolete: true
Attachment #369383 - Flags: review+
Perhaps we can create a head.js to add this cleanDatabase function to? I have no idea how to do that.
Attachment #369383 - Attachment is obsolete: true
Attachment #369386 - Flags: review?(mak77)
please for now restore your old cleanUp function (previous version) and simply add a bs.removeFolderChildren(bs.unfiledBookmarksFolder); this version clearing all the tables is too risky, so if we are going to provide a common header (that is something we can look into in your new bug and would make the env clean for every test) we need to ensure we don't remove too much from tables.
Attached patch restored the cleanUp test (obsolete) — — Splinter Review
Attachment #369386 - Attachment is obsolete: true
Attachment #369510 - Flags: review+
Attachment #369386 - Flags: review?(mak77)
Attached patch added Makefile to patch (obsolete) — — Splinter Review
Ran full test suite for browser-chrome-tests, 449 tests pass
Attachment #369510 - Attachment is obsolete: true
Attachment #369514 - Flags: review+
since this has automatic test, a litmus test should not be required.
Flags: in-litmus? → in-testsuite?
Keywords: checkin-needed
Target Milestone: --- → Firefox 3.6a1
Attached patch for checkin — — Splinter Review
i had to change this a bit for 2 reasons: 1. cmd_paste was wrongly hidden and disabled in context menus, so i special cased it in controller.js 2. test was failing on my VM, since most tinderboxes are VMs i think makes sense to make the test more reliable. Since putting data into clipboard is async i think makes sense to wait for those data.
Attachment #369514 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/84b3844bc3eb
Status: REOPENED → RESOLVED
Closed: 17 years ago → 16 years ago
Resolution: --- → FIXED
Flags: in-testsuite? → in-testsuite+
Depends on: 485765
Attachment #369663 - Flags: approval1.9.1?
Comment on attachment 369663 [details] [diff] [review] for checkin askin approval to make tagging behavior consistent. medium risk, has an automatic test.
Drivers: please note that landing this bug on 1.9.1 without bug 485765 breaks the Library window context menus, so these two patches need to be approved together.
Comment on attachment 369663 [details] [diff] [review] for checkin (In reply to comment #66) > Drivers: please note that landing this bug on 1.9.1 without bug 485765 breaks > the Library window context menus, so these two patches need to be approved > together. No, we need a branch-safe patch.
Attachment #369663 - Flags: approval1.9.1?
Attached patch merged patch, safe for branch (obsolete) — — Splinter Review
asking approval on this patch that includes the regression fix from ehsan. Landing this without bitrotting will also require bug 468233 before (that is a low-low risk patch though). Risk of this patch is medium, but has a browser-chrome test.
Attachment #370033 - Flags: approval1.9.1?
Verified fixed with builds on OS X and Windows: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090403 Minefield/3.6a1pre ID:20090403030700 The only issue I can see right now is filed as bug 486740 now.
Status: RESOLVED → VERIFIED
Comment on attachment 370033 [details] [diff] [review] merged patch, safe for branch Please post a seperate branch patch since we're not going to be taking bug 468233
Attachment #370033 - Flags: approval1.9.1? → approval1.9.1-
unbitrot and update to not rely on bug 468233
Attachment #370033 - Attachment is obsolete: true
Attachment #372987 - Flags: approval1.9.1?
Comment on attachment 372987 [details] [diff] [review] new patch for branch a191=beltzner
Attachment #372987 - Flags: approval1.9.1? → approval1.9.1+
Verified fixed on 1.9.1 with builds on OS X and Windows: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090420 Shiretoko/3.5b4pre ID:20090420031111
Flags: in-litmus-
Keywords: fixed1.9.1 → verified1.9.1
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
The test case for this bug fails with the HTML5 parser (bug 545661). Is the test case supposed to navigate to an HTML page at all?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: