Closed Bug 409301 Opened 17 years ago Closed 17 years ago

Visiting bookmark is recorded as link transition instead of bookmark transition.

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta3

People

(Reporter: Mardak, Assigned: moco)

References

Details

Attachments

(1 file, 4 obsolete files)

It seems like clicking bookmarks results in visit_type of TRANSITION_LINK (1) instead of TRANSITION_BOOKMARK (3)

http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/public/nsINavHistoryService.idl#1166

http://mxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavHistory.cpp#3246

Bug 394066 won't really do anything meaningful until this is fixed.
SELECT COUNT(*) FROM moz_historyvisits WHERE visit_type=3

gives me 0 for my profiles, and I've used bookmarks from time to time.
Depends on: 409327
No longer depends on: 409327
not recording bookmark visits as nsINavHistoryService::TRANSITION_BOOKMARK

We need to start call MarkPageAsFollowedBookmark(), like we do for
MarkPageAsTyped()

we are depending on this for our global frecency algo, see bug #394038

note, we need to fix this typo in MarkPageAsFollowedBookmark(), which I have
fixed locally:

-  mRecentTyped.Put(uriString, GetNow());
+  mRecentBookmark.Put(uriString, GetNow());
Assignee: nobody → sspitzer
Blocks: 394038
Flags: blocking-firefox3?
I think we want to call MarkPageAsFollowedBookmark() from _openTabset() in utils.js and openSelectedNodeIn() in controller.js

but, we need to be careful that when clicking on history items in the places organizer, sidebar, or history menu, we don't record those as bookmarked visits, right?

additionally, we need to decide what clicking the home button should do (act like a bookmark visit?)
Status: NEW → ASSIGNED
the history menu might already work fine, as it calls openUILink().

as for the history sidebar and history item in the organizer, we need to check before calling MarkPageAsFollowedBookmark()

working on a patch...
Attached patch wip patch (obsolete) — Splinter Review
for history, we should just not call markPageAsFollowedBookmark().

I think we might consider calling markPageAsTyped.

If we don't call anything, it will act as link.  I think choosing it from history sidebar or in the places organizer (or the History menu, which needs to be fixed) should recorded the visit the same way as when you type a url in the url bar or if you select a url in the autocomplete drop down (which is markPageAsTyped.)

I think of "link" as a link in a page.

Dietrich, do you agree?

When I fix this, I'll also add a unit test and fix this unit test:

http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/tests/unit/test_browserhistory.js#128

122   /**
123    * markPageAsTyped
124    * Designate the url as having been explicitly typed in by
125    * the user, so it's okay to be an autocomplete result.
126    */
127   //XXX how to test this?
128   //bhist.markPageAsTyped(testURI);
Attached patch updated wip patch (obsolete) — Splinter Review
Attachment #294308 - Attachment is obsolete: true
Attached patch patch, with tests (obsolete) — Splinter Review
Attachment #294389 - Attachment is obsolete: true
Attached patch updated patch (improve comments) (obsolete) — Splinter Review
Attachment #294446 - Attachment is obsolete: true
here are the scenarios to test:

should be TRANSITION_TYPED

from history menu
from the url bar
from url autocomplete results
from from history sidebar, single and host container, open in tabs (for date container, see bug #409661)
from places organizer, history (single and select multiple open in tabs)

should be TRANSITION_BOOKMARK

from bm menu (single item and folder, open in tabs)
from bm toolbar (single item and folder, open in tabs)
from bm sidebar (single item and folder, open in tabs)
from places organizer, single, select multiple open in tabs, container open in tabs
I've passed the test cases in comment #11, but I cheated by using a printf in nsNavHistory::AddVisitChain().

Someone without that could still verify by doing those tasks and then inspecting places.sqlite.

The unit test covers some of the basics, but we could do more with browser chrome tests + some sql queries (or doing RESULTS_AS_FULL_VISITS queries, once they are supported, see bug #409662)
Flags: in-litmus+
Target Milestone: --- → Firefox 3 M11
Comment on attachment 294447 [details] [diff] [review]
updated patch (improve comments)

seeking review from dietrich
Attachment #294447 - Flags: review?(dietrich)
Flags: in-litmus+ → in-testsuite+
The tests haven't been checked-in yet, right? Please only grant in-testsuite once the tests have landed.
Flags: in-testsuite+ → in-testsuite?
Comment on attachment 294447 [details] [diff] [review]
updated patch (improve comments)


>+  markPageAsTyped: function PU_markPageAsTyped(aURL) {
>+    try {
>+      this.globalHistory.markPageAsTyped(this.createFixedURI(aURL));
>+    }
>+    catch (ex) {
>+    }

what do you expect to fail here? should at least dump so the failure is visible somehow?

>+  },
>+
>+  /**
>+   * By calling this before we visit a URL, we will use TRANSITION_BOOKMARK
>+   * as the transition for the visit to that URL (if we don't have a referrer).
>+   * This is used when visiting pages from the bookmarks menu, 
>+   * personal toolbar, and bookmarks from within the places organizer.
>+   * If we don't call this, we'll treat those visits as TRANSITION_LINK.
>+   */
>+  markPageAsFollowedBookmark: function PU_markPageAsFollowedBookmark(aURL) {
>+    try {
>+      this.history.markPageAsFollowedBookmark(this.createFixedURI(aURL));
>+    }
>+    catch (ex) {
>+    }
>+  },

ditto


>@@ -1640,26 +1695,32 @@ var PlacesUtils = {
>     browserWindow.getBrowser().loadTabs(aURLs, loadInBackground,
>                                         replaceCurrentTab);
>   },
> 
>   openContainerNodeInTabs: function PU_openContainerInTabs(aNode, aEvent) {
>     var urlsToOpen = this.getURLsForContainerNode(aNode);
>     if (!this._confirmOpenInTabs(urlsToOpen.length))
>       return;
>-    this._openTabset(urlsToOpen, aEvent);
>+    this._openTabset(urlsToOpen, aEvent, 
>+                     !this.nodeIsHost(aNode) && !this.nodeIsDay(aNode));

couldn't you check node.itemId? (non-static containers would have -1), or maybe add a nodeIsBookmarkFolder() which checks itemId != -1 and nodeIsFolder().

>   },
> 
>   openURINodesInTabs: function PU_openURINodesInTabs(aNodes, aEvent) {
>     var urlsToOpen = [];
>+    var nodesAreBookmarks = false;
>     for (var i=0; i < aNodes.length; i++) {
>-      if (this.nodeIsURI(aNodes[i]))
>+      // skip over separators and folders
>+      if (this.nodeIsURI(aNodes[i])) {
>         urlsToOpen.push(aNodes[i].uri);
>+        if (!nodesAreBookmarks)
>+          nodesAreBookmarks = this.nodeIsBookmark(aNodes[i]);
>+      }
>     }
>-    this._openTabset(urlsToOpen, aEvent);
>+    this._openTabset(urlsToOpen, aEvent, nodesAreBookmarks);

is it possible that we'll get a mixed set where some nodes are bookmarks and other not? if so, this seems wrong, since later in openTabset all will get marked as followed-bookmarks.

it might be better to either check each URI in openTabset, or always do the marking (as bookmark or typed) outside of it where we're better able to make the determination.
Attachment #294447 - Flags: review?(dietrich)
1)

> what do you expect to fail here? should at least dump so the failure is visible
> somehow?

I'm not expecting this to fail, and both nsNavHistory::MarkPageAsTyped() and nsNavHistory::MarkPageAsFollowedBookmark() always returns NS_OK, so I'll remove the try / catch from both.

I was being (overly) defensive, as we would not want markPageAsTyped() to throw.

2)

> >   openContainerNodeInTabs: function PU_openContainerInTabs(aNode, aEvent) {
> >     var urlsToOpen = this.getURLsForContainerNode(aNode);
> >     if (!this._confirmOpenInTabs(urlsToOpen.length))
> >	return;
> >-    this._openTabset(urlsToOpen, aEvent);
> >+    this._openTabset(urlsToOpen, aEvent, 
> >+		       !this.nodeIsHost(aNode) && !this.nodeIsDay(aNode));

> couldn't you check node.itemId? (non-static containers would have -1), or maybe
> add a nodeIsBookmarkFolder() which checks itemId != -1 and nodeIsFolder().

Actually, I can't do what I'm doing, or what you suggest, because for queries I would have itemId != -1, but nodeIsFolder() would be false.

I think for both this review comment (and the next), I need to determine for each uri if it is a bookmark or not inside openTabset()

Note, for some queries the results are history items and for others they are bookmarks.

3)

> >+    this._openTabset(urlsToOpen, aEvent, nodesAreBookmarks);

> is it possible that we'll get a mixed set where some nodes are bookmarks and
> other not?

It isn't currently possible, but if we have a unified query in the organizer, it could be.

I'll fix this along with #2 above and attach a new patch.
Attachment #294686 - Flags: review?(dietrich)
Comment on attachment 294686 [details] [diff] [review]
updated patch, per dietrich's review comments

r=me, thanks for revising.
Attachment #294686 - Flags: review?(dietrich) → review+
Comment on attachment 294686 [details] [diff] [review]
updated patch, per dietrich's review comments

seeking approval.

we want this as it is part of the global frecency work (a p1 / firefox 3 + blocker).

we will be using the transition state (bookmark, typed, link) of visits to weight them.
Attachment #294686 - Flags: approval1.9?
fixed.

note, because this patch includes changes to make it so all history visits get TRANSITION_TYPED, I updated the comment in nsINavHistoryService.idl before checking in:

   /**
    * This transition type means that the user typed the page's URL in the
-   * URL bar.
+   * URL bar or selected it from URL bar autocomplete results, clicked on
+   * it from a history query (from the History sidebar, History menu, 
+   * or history query in the personal toolbar or Places organizer.
    */
   const unsigned long TRANSITION_TYPED = 2;

Checking in browser/base/content/browser-menubar.inc;
/cvsroot/mozilla/browser/base/content/browser-menubar.inc,v  <--  browser-menubar.inc
new revision: 1.124; previous revision: 1.123
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.920; previous revision: 1.919
done
Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.191; previous revision: 1.190
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.91; previous revision: 1.90
done
Checking in toolkit/components/places/public/nsINavHistoryService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v  <--  nsINavHistoryService.idl
new revision: 1.75; previous revision: 1.74
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.221; previous revision: 1.220
done
Checking in toolkit/components/places/tests/unit/test_browserhistory.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_browserhistory.js,v  <--  test_browserhistory.js
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_markpageas.js,v
done
Checking in toolkit/components/places/tests/unit/test_markpageas.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_markpageas.js,v  <--  test_markpageas.js
initial revision: 1.1
done
Checking in toolkit/components/history/public/nsIBrowserHistory.idl;
/cvsroot/mozilla/toolkit/components/history/public/nsIBrowserHistory.idl,v  <--  nsIBrowserHistory.idl
new revision: 1.11; previous revision: 1.10
done

Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Summary: Visiting bookmark is recorded as link transition → Visiting bookmark is recorded as link transition instead of bookmark transition.
???

Uh, this bug doesn't have approval nor is it a blocker. Why did you check-in the patch?
> Uh, this bug doesn't have approval nor is it a blocker. Why did you check-in
> the patch?

it is part of and blocks the firefox-3-blocker+ / p1 bug for global frecency.
(In reply to comment #22)
> > Uh, this bug doesn't have approval nor is it a blocker. Why did you check-in
> > the patch?
> 
> it is part of and blocks the firefox-3-blocker+ / p1 bug for global frecency.

That has never been a valid reason to check-in during an approval period. http://wiki.mozilla.org/TreeStatus clearly states that the bug itself must be a blocker or else the patch must have approval1.9+, of which neither of those things is true for this bug/patch.

I'm not sure why you requested approval in comment #19 and then turned right around and checked it in anyway... That doesn't make sense.
Comment on attachment 294686 [details] [diff] [review]
updated patch, per dietrich's review comments

ho ho ho, it's a winter vacation approval.
Attachment #294686 - Flags: approval1.9? → approval1.9+
Flags: in-testsuite? → in-testsuite+
Priority: -- → P2
Flags: blocking-firefox3?
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
baked in-testsuite long enough
Status: RESOLVED → VERIFIED
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
You need to log in before you can comment on or make changes to this bug.