542941 - Better handling of EMBED visits

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

If a framed visit comes from user's action, we should mark it as TRANSITION_LINK, not TRANSITION_EMBED.

While here i'll also cleanup some addVisit comment and path, since there is a bit of confusion there, and that will help next work to build an async addVisit.

I'm also investigating some really strange behavior that i see in my history database, so could be i'll find more.

Comment 1

[Marco Bonardo [:mak]]

Attachment: Part1: AddVisitChain cleanup

first cleanup, this makes the code more readable and fixes comments.
I've also changed used times, i'm unsure why the code was not using the correct time for the redirects and passed in time...
btw, will ensure the full thing is right once i have all parts.

Comment 2

[Marco Bonardo [:mak]]

Attachment: Part2: markPageAsFollowedLink

we need to special mark pages if we want to better handle embed links

Comment 3

[Marco Bonardo [:mak]]

Attachment: Part3: handleLinkClick hook

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 424249 [details] [diff] [review]
Part3: handleLinkClick hook

bah, hg diff does not like browser.js

Comment 5

[Marco Bonardo [:mak]]

ohr, there was some CRLF in browser.js, pushed http://hg.mozilla.org/mozilla-central/rev/cce4b98a36a2

Comment 6

[Marco Bonardo [:mak]]

Attachment: Part1: cleanup AddVisitChain code

Comment 7

[Marco Bonardo [:mak]]

Attachment: Part2: markPageAsFollowedLink and TRANSITION_FRAMED_LINK

Comment 8

[Marco Bonardo [:mak]]

Attachment: Part3: Browser hook for markPageAsFollowedLink

Comment 9

[Marco Bonardo [:mak]]

i need to write a test for this, it will be a bit slow (have to wait LAZY_ADD), but we need some decent redirect test.

Comment 10

[Marco Bonardo [:mak]]

i need the patch in bug 544241, to create an httpd.jsm module, i can't use do_load_httpd_js() because it would conflict with our Cc, Ci and so on, and i can't change all Places test.

Comment 11

[Marco Bonardo [:mak]]

Attachment: Part1: cleanup AddVisitChain code

Comment 12

[Marco Bonardo [:mak]]

Attachment: Part2: markPageAsFollowedLink and TRANSITION_FRAMED_LINK

Comment 13

[Marco Bonardo [:mak]]

Attachment: Part3: Browser hook for markPageAsFollowedLink

Comment 14

[Marco Bonardo [:mak]]

Attachment: Part3: Browser hook for markPageAsFollowedLink

Comment 15

[Marco Bonardo [:mak]]

Attachment: Part4: httpd.jsm module for the test

This is a copy of httpd.js component, i cannot use do_load_httpd_js() because it will import Cc, Ci, and so on in our scope. I've tried to make xpcshell import that in a separate scope, but that requires quite deep changes.
The httpd.js part obviously does not need review.

Comment 16

[Marco Bonardo [:mak]]

Attachment: Part5: fix a sessionID bug, and introduce a new test for redirects.

Comment 17

[Marco Bonardo [:mak]]

Attachment: Part6: fix existing tests to check for the new transition type

Comment 18

[Marco Bonardo [:mak]]

Attachment: Part7: Fix CRLF newlines in test_multi_queries.js

this does not really need review.

Comment 19

[Marco Bonardo [:mak]]

Attachment: Part8 - fix test_multi_queries.js for the new transition

last one.

Comment 20

[Marco Bonardo [:mak]]

Brief description of the above patches.

- introduced a new TRANSITION_FRAMED_LINK that is set on a visit when it happens in a frame, but we have detected a click(or keyboard action) on the same uri
- these visits are synced to disk and persisted through sessions, while usual EMBED visits are not (automatic visits in frames or internal contents of pages).
- the marking is done in browser.js, through a new method in browserHistory.idl called markPageAsFollowedLink.

plus cleanup of the visits chain and a couple minor bugs fixes (tested).

Comment 21

[Marco Bonardo [:mak]]

Actually there is an alternative to the httpd.jsm module, that i just thought. i could just create a new tests/network/ folder and avoid Cc, Ci, and so on in the head file. Tests willing to use httpd.js should then be moved there. this would save duplicating 150KB of code, that should be fine.

Comment 22

[Marco Bonardo [:mak]]

Attachment: Part5: fix a sessionID bug, and introduce a new test for redirects.

ok, killed the httpd.js copy.

Comment 23

[Shawn Wilsher :sdwilsh]

What's wrong with including the http server in all of our tests and just undefine Cc and Ci there?  Copying httpd.js is a *bad* idea since it still gets fixes frequently.

Comment 24

[Marco Bonardo [:mak]]

(In reply to comment #23)
> What's wrong with including the http server in all of our tests and just
> undefine Cc and Ci there?

it is also importing a bunch of other stuff in the global scope, that means slower tests, and more risk of var conflicts. I think the solution i took is better, if you need it just put the test in our unit/network folder, and that's it :)

> Copying httpd.js is a *bad* idea since it still gets
> fixes frequently.

yes, indeed i gave up and removed that.

Comment 25

[Dietrich Ayala (:dietrich)]

Comment on attachment 425293 [details] [diff] [review]
Part1: cleanup AddVisitChain code


>@@ -2756,8 +2762,9 @@ nsNavHistory::AddVisit(nsIURI* aURI, PRT
>   // Get the place id for the referrer, if we have one
>   PRInt64 referringVisitID = 0;
>   PRInt64 referringSessionID;
>+  PRTime referringTime;
>   if (aReferringURI &&
>-      !FindLastVisit(aReferringURI, &referringVisitID, &referringSessionID)) {
>+      !FindLastVisit(aReferringURI, &referringVisitID, &referringTime, &referringSessionID)) {

is referringTime is used in a later patch in the stack?

>-    // for redirects in frames, we don't want to see those items in history
>+    // Recusively call addVisitChain to walk up the chain till we find a not
>+    // redirected URI.
>+    // Ensure that the sources have a visit time smaller than aTime, otherwise
>+    // we would end up with bad ordered visits.

s/bad/incorrectly/

>+    PRTime sourceTime = NS_MIN(redirectTime, aTime - 1);
>+    PRInt64 sourceVisitId = 0;
>+    rv = AddVisitChain(redirectSourceURI, sourceTime, aToplevel,
>+                       PR_TRUE, // Is a redirect.
>+                       aReferrerURI, // This one is the originating source.
>+                       &sourceVisitId, // Get back the visit id of the source.
>+                       aSessionID,
>+                       aRedirectBookmark);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // We have added all the chain before us, wow we only have to add the visit
>+    // to aURI.

s/wow/now/ :)

>+    // We came to this page through a redirect and we are in a frame, we don't
>+    // want to see this page in history so mark the visit as EMBED.
>     // see bug #381453 for more details
>     if (!aToplevel) {
>       transitionType = nsINavHistoryService::TRANSITION_EMBED;
>     }
> 
>-    // We have been redirected, if the previous site was not a redirect
>-    // update the referrer so we can walk up the redirect chain.
>+    // We have been redirected to this page, thus we will save the page that
>+    // brought to us in from_visit, to be able to walk up the chain.

s/to//

>     // See bug 411966 and Bug 428690 for details.
>-    fromVisitURI = redirectURI;
>-  } else if (aReferrerURI) {
>+    // TODO: We need to add a chain id to easily reconstruct chains without
>+    // having to recurse through the table.  We could reuse session eventually
>+    // since it's usage is not well defined.

file a bug please

>+    fromVisitURI = redirectSourceURI;
>+  }
>+  else if (aReferrerURI) {
>+    // We have not been redirected to this page, but we have a referrer.

s/but/and/

>+    // Since we have a referrer, we know this visit came from a page.
>+    // For toplevel windows, we assume this is a manual visit and the user wants
>+    // to see it in history.

s/toplevel/top-level/

s/manual/user-initiated/

>+    // If instead we are in a frame, we should distinguish between content
>+    // visited as result of a user action (he clicked a link), or content loaded
>+    // automatically in frames.
>+    // We would like to show the former while hide the latter.
>+    // Unfortunately, the docshell does not pass us enough information to
>+    // distinguish them, so we mark both as EMBED visits.
>+    // This is bad since we decorate embed links only for the session,
>+    // and user wants the visited decoration to persistfor clicked links.

persistfor

and file a bug to fix docshell?

>-    // Try to turn the referrer into a visit.
>+    // Check if the referrer has a visit,
>     // This also populates the session id.
>-    if (!FindLastVisit(aReferrerURI, &referringVisit, aSessionID)) {
>-      // we couldn't find a visit for the referrer, don't set it
>+    PRTime lastVisitTime;
>+    PRInt64 referringVisitId;
>+    if (!FindLastVisit(aReferrerURI, &referringVisitId, &lastVisitTime, aSessionID) ||
>+        aTime - lastVisitTime > RECENT_EVENT_THRESHOLD) {
>+      // Either we did not find a visit for the referrer, or that visit was to

s/to/too/

>+  else {
>+    // When there is no referrer:
>+    // - Check recent events for a typed-in uri.
>+    // - Check recent events for a bookmark selection.
>+    // - Otherwise mark as TRANSITION_LINK or TRANSITION_EMBED depending on
>+    //   whether we are in a frame (see above for reasoning about this).
>+    // Notice we don't handle drag-and-drop operations so they will most likely
>+    // be marked as links.
>     if (CheckIsRecentEvent(&mRecentTyped, spec))
>       transitionType = nsINavHistoryService::TRANSITION_TYPED;
>     else if (CheckIsRecentEvent(&mRecentBookmark, spec))
>-      transitionType = nsINavHistoryService::TRANSITION_BOOKMARK;
>+     transitionType = nsINavHistoryService::TRANSITION_BOOKMARK;

indent mistake?

>@@ -6663,11 +6689,13 @@ nsNavHistory::ExpireNonrecentEvents(Rece
> 
> PRBool
> nsNavHistory::GetRedirectFor(const nsACString& aDestination,
>-                             nsACString& aSource, PRTime* aTime,
>+                             nsACString& aSource,
>+                             PRTime* aTime,
>                              PRUint32* aRedirectType)
> {
>   RedirectInfo info;
>   if (mRecentRedirects.Get(aDestination, &info)) {
>+    // Consume the redirect entry, it's no more useful.

s/no more/no longer/

r=me w/ these nits fixed

Comment 26

[Marco Bonardo [:mak]]

(In reply to comment #25)
> >   if (aReferringURI &&
> >-      !FindLastVisit(aReferringURI, &referringVisitID, &referringSessionID)) {
> >+      !FindLastVisit(aReferringURI, &referringVisitID, &referringTime, &referringSessionID)) {
> 
> is referringTime is used in a later patch in the stack?

it is used later (see lastVisitTime), so i've added it to the method, and i have to pass in a pointer, even if i don't use it since it will be assigned to.

> >     // See bug 411966 and Bug 428690 for details.
> >-    fromVisitURI = redirectURI;
> >-  } else if (aReferrerURI) {
> >+    // TODO: We need to add a chain id to easily reconstruct chains without
> >+    // having to recurse through the table.  We could reuse session eventually
> >+    // since it's usage is not well defined.
> 
> file a bug please

I think there is already one, could have another face though, so i will search for it and make it clearer.

> and file a bug to fix docshell?

No reason, the next patches fix the problem using browserHistory.markPageAsFollowedLink, in an easier way, consistent to the rest of our code. the docshell is harder to make right and less "adaptable" to other implementers, this way they can mark what they prefer as a followed link.

Comment 27

[Dietrich Ayala (:dietrich)]

Comment on attachment 425294 [details] [diff] [review]
Part2: markPageAsFollowedLink and TRANSITION_FRAMED_LINK

Why are we treating embedded links specially? Why are they not just TRANSITION_LINK? From the average user's perspective, a framed link is just another link.

Comment 28

[Dietrich Ayala (:dietrich)]

Comment on attachment 425294 [details] [diff] [review]
Part2: markPageAsFollowedLink and TRANSITION_FRAMED_LINK


>   /**
>+   * By calling this before we visit a URL, we mark the page as being opened
>+   * manually by user.  This is actually used to distinguish visits in frames
>+   * so we correctly ignore only automatic visits.
>+   * Thus a page decorated this way and visited in frames will have a
>+   * TRANSITION_FRAMED_LINK visit.
>+   */
>+  markPageAsFollowedLink: function PU_markPageAsUserClicked(aURL) {
>+    PlacesUtils.history.QueryInterface(Ci.nsIBrowserHistory)
>+               .markPageAsFollowedLink(this.createFixedURI(aURL));
>+  },
>+

is dot-at-beginning-of-next-line the right style here?

>@@ -1263,8 +1263,8 @@ interface nsINavHistoryService : nsISupp
>   /**
>    * This transition type is set when some inner content is loaded. This is
>    * true of all images on a page, and the contents of the iframe. It is also
>-   * true of any content in a frame, regardless if whether or not the user
>-   * clicked something to get there.
>+   * true of any content in a frame if the user did not explicitly followed
>+   * a link to get there.
>    */
>   const unsigned long TRANSITION_EMBED = 4;

s/followed/follow/

>+  // if URL is already in the typed queue, then we need to remove the old one
>+  PRInt64 unusedEventTime;
>+  if (mRecentLink.Get(uriString, &unusedEventTime))
>+    mRecentLink.Remove(uriString);

typed?

>@@ -5208,18 +5266,15 @@ nsNavHistory::AddVisitChain(nsIURI* aURI
>     // Since we have a referrer, we know this visit came from a page.
>     // For toplevel windows, we assume this is a manual visit and the user wants
>     // to see it in history.
>-    // If instead we are in a frame, we should distinguish between content
>-    // visited as result of a user action (he clicked a link), or content loaded
>+    // If instead we are in a frame, we distinguish between content visited
>+    // as result of a user action (he clicked a link), or content loaded
>     // automatically in frames.

s/he/they/

r=me

Comment 29

[Marco Bonardo [:mak]]

(In reply to comment #27)
> (From update of attachment 425294 [details] [diff] [review])
> Why are we treating embedded links specially? Why are they not just
> TRANSITION_LINK? From the average user's perspective, a framed link is just
> another link.

we don't want uris of sub frames in autocomplete.

Comment 30

[Dietrich Ayala (:dietrich)]

Comment on attachment 425312 [details] [diff] [review]
Part5: fix a sessionID bug, and introduce a new test for redirects.


>     // We do not want to add a new visit if the referring site is the same as
>     // the new site.  This happens when a page refreshes itself.
>+    // We still want to add the visit if the page has never been added though.
>     PRBool referrerIsSame;
>     if (NS_SUCCEEDED(aURI->Equals(aReferrerURI, &referrerIsSame)) &&
>-        referrerIsSame)
>+        referrerIsSame && referrerHasPreviousVisit) {
>+      // We must still ensure a valid session id to the chain.
>+      if (aIsRedirect)
>+        *aSessionID = GetNewSessionID();
>       return NS_OK;
>-
>+    }

IMO, should take out the group narrative and passive voice in comments, eg:

s/We do not want to add/Do not add/

>+    if (!referrerHasPreviousVisit ||
>+        aTime - lastVisitTime > RECENT_EVENT_THRESHOLD) {
>+      // Either we did not find a visit for the referrer, or that visit was to
>+      // old to be part of this session.  Thus we want to start a new session.
>+      *aSessionID = GetNewSessionID();
>+    }
>+    

s/to/too/

extraneous whitespace

>diff --git a/toolkit/components/places/tests/network/head_bookmarks.js b/toolkit/components/places/tests/network/head_bookmarks.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/places/tests/network/head_bookmarks.js

how about s/head_bookmarks.js/head_.js/ ?

r=me otherwise.

Comment 31

[Dietrich Ayala (:dietrich)]

Comment on attachment 425304 [details] [diff] [review]
Part6: fix existing tests to check for the new transition type

>diff --git a/toolkit/components/places/tests/unit/test_000_frecency.js b/toolkit/components/places/tests/unit/test_000_frecency.js
>--- a/toolkit/components/places/tests/unit/test_000_frecency.js
>+++ b/toolkit/components/places/tests/unit/test_000_frecency.js
>@@ -82,6 +82,7 @@ var bucketPrefs = [
> 
> var bonusPrefs = {
>   embedVisitBonus: Ci.nsINavHistoryService.TRANSITION_EMBED,
>+  framedLinkVisitBonus: Ci.nsINavHistoryService.TRANSITION_FRAMED_LINK,
>   linkVisitBonus: Ci.nsINavHistoryService.TRANSITION_LINK,
>   typedVisitBonus: Ci.nsINavHistoryService.TRANSITION_TYPED,
>   bookmarkVisitBonus: Ci.nsINavHistoryService.TRANSITION_BOOKMARK,
>@@ -152,6 +153,7 @@ bucketPrefs.every(function(bucket) {
>       if (!points) {
>         if (!visitType ||
>             visitType == Ci.nsINavHistoryService.TRANSITION_EMBED ||
>+            visitType == Ci.nsINavHistoryService.TRANSITION_FRAMED_LINK ||
>             visitType == Ci.nsINavHistoryService.TRANSITION_DOWNLOAD ||
>             bonusName == "defaultVisitBonus")
>           frecency = 0;

add a test that framed link visits have zero frecency here?

>diff --git a/toolkit/components/places/tests/unit/test_385397.js b/toolkit/components/places/tests/unit/test_385397.js
>--- a/toolkit/components/places/tests/unit/test_385397.js
>+++ b/toolkit/components/places/tests/unit/test_385397.js
>@@ -93,16 +93,19 @@ function run_test() {
>   var root = result.root;
>   root.containerOpen = true;
>   var cc = root.childCount;
>-  do_check_eq(cc, 3 * TOTAL_SITES); 
>+  do_check_eq(cc, 4 * TOTAL_SITES);
>   for (var i=0; i < TOTAL_SITES; i++) {
>-    var node = root.getChild(i*3);
>+    var node = root.getChild(i*4);
>     var site = "http://www.test-" + (TOTAL_SITES - 1 - i) + ".com/";
>     do_check_eq(node.uri, site);
>     do_check_eq(node.type, options.RESULTS_AS_VISIT);
>-    node = root.getChild(i*3+1);
>+    node = root.getChild(i*4+1);
>     do_check_eq(node.uri, site + "blank.gif");
>     do_check_eq(node.type, options.RESULTS_AS_VISIT);
>-    node = root.getChild(i*3+2);
>+    node = root.getChild(i*4+2);
>+    do_check_eq(node.uri, site + "blank.gif");
>+    do_check_eq(node.type, options.RESULTS_AS_VISIT);
>+    node = root.getChild(i*4+3);
>     do_check_eq(node.uri, site);
>     do_check_eq(node.type, options.RESULTS_AS_VISIT);
>   }

maybe just declare a var for that up above?

r=me

Comment 32

[Marco Bonardo [:mak]]

Attachment: Part1: cleanup and comment AddVisitChain code

addressed comments, made comments still less "narrative".

Comment 33

[Marco Bonardo [:mak]]

(In reply to comment #31)
b/toolkit/components/places/tests/unit/test_000_frecency.js
> >       if (!points) {
> >         if (!visitType ||
> >             visitType == Ci.nsINavHistoryService.TRANSITION_EMBED ||
> >+            visitType == Ci.nsINavHistoryService.TRANSITION_FRAMED_LINK ||
> >             visitType == Ci.nsINavHistoryService.TRANSITION_DOWNLOAD ||
> >             bonusName == "defaultVisitBonus")
> >           frecency = 0;
> 
> add a test that framed link visits have zero frecency here?

if they would have frecency != 0, they would appear in autocomplete results, and thus we would still catch that in other ac tests. i don't feel comfortable touching this test too much, it gave me some headache in the past :)

Comment 34

[Marco Bonardo [:mak]]

Attachment: Part2: markPageAsFollowedLink and TRANSITION_FRAMED_LINK

Comment 35

[Marco Bonardo [:mak]]

Attachment: Part4 and 5: fix a sessionID bug, and introduce a new test for redirects.

Comment 36

[Marco Bonardo [:mak]]

Attachment: Part6: fix existing tests to check for the new transition type

Comment 37

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 425296 [details] [diff] [review]
Part3: Browser hook for markPageAsFollowedLink

(In reply to comment #29)
> > Why are we treating embedded links specially? Why are they not just
> > TRANSITION_LINK? From the average user's perspective, a framed link is just
> > another link.
> 
> we don't want uris of sub frames in autocomplete.

This seems like a lot of trouble to go through just for that seemingly relatively minor benefit... but maybe these patches just look scarier than they are?

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+     if (event.button == 0 || event.button == 1 && wrapper.href) {

&& binds tighter than ||, so you'd need parentheses here to get the behavior you want:

((event.button == 0 || event.button == 1) && wrapper.href)

I think this code may be better placed at the end of handleLinkClick, though. That wouldn't handle the sidebar-redirected link clicks, but I don't think that's worth worrying about.

Comment 38

[Marco Bonardo [:mak]]

(In reply to comment #37)
> This seems like a lot of trouble to go through just for that seemingly
> relatively minor benefit... but maybe these patches just look scarier than
> they are?

This is a bunch of patches, sure, but their purpose is not just doing this embed change. I've cleaned up most of the visits chain because soon someone (can't tell if me, Shawn or someone other) will start converting visits addition to async, and we need some clean and clear code path.

The embed change itself is not really scary, it boils down doing the same thing we do for typed visits or bookmarks visits, we just add uris to a local capped hash and when getting the visit we check if it's in one of these hashes.

And i think makes sense to distinguish this new visit type, the main Places advantage is having nice data to build whatever you want, so we should target:
- having more data
- having separated data
- showing pertinent data
Thus i think it's better distinguish visits better we can, and it's also better showing the more restricted set of results we can in the awesomebar. Because there is so much data we should do our best to filter.

> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >+     if (event.button == 0 || event.button == 1 && wrapper.href) {
> 
> && binds tighter than ||

uargh! what a stupid error, me ashamed. (lesson is: don't code at late night)

> I think this code may be better placed at the end of handleLinkClick, though.
> That wouldn't handle the sidebar-redirected link clicks, but I don't think
> that's worth worrying about.

ok, discussion on IRC satisfied my curiosity, i'll attach an updated patch.

Comment 39

[Marco Bonardo [:mak]]

Attachment: Part3: Browser hook for markPageAsFollowedLink

Moved to handleLinkClick per IRC discussion, removed return value since it's unused, made a single exit point.

I'll file bug about the code that is skipping feed uri sniffing, since looks like a really hidden and unused feature.

Comment 40

[:Gavin Sharp [email: [e-mail]]]

I found some extensions that seem to use the return value of handleLinkClick:
https://mxr.mozilla.org/addons/source/1614/chrome/content/overlay.js#292
jar:https://mxr.mozilla.org/addons/source/2933/linkwidget.jar?raw=1&ctype=application/x-java-archive!/content/code.js

and that wasn't an exhaustive search. I think that means it's best to leave the return value in - it's not particularly burdensome to keep supporting.

Comment 41

[Marco Bonardo [:mak]]

Attachment: Part3: Browser hook for markPageAsFollowedLink

added back return value.

Comment 42

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 428685 [details] [diff] [review]
Part3: Browser hook for markPageAsFollowedLink

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

> function handleLinkClick(event, href, linkNode)

>+  if (href && (event.button == 0 || event.button == 1)) {

This can be just if (handled), right? Don't need to check href, either way.

Comment 43

[Marco Bonardo [:mak]]

yes, but i have some doubt about contentAreaClick

if (event.button == 0 && !event.ctrlKey && !event.shiftKey &&
   !event.altKey && !event.metaKey) {
...
}
else {
  handleLinkClick(event, wrapper.href, linkNode);
}

is this not dismissing all simple left-clicks i'm interested in? and this is the reason in the first patch i hooked into contentAreaClick rather than in handleLinkClick
Can i say this method is confusing as it is? :)

Comment 44

[Marco Bonardo [:mak]]

also, the xlink part of contentAreaClick checks href, but the else call to handleLinkClick does not check it, so my href check was for that.

Comment 45

[Marco Bonardo [:mak]]

Comment on attachment 428685 [details] [diff] [review]
Part3: Browser hook for markPageAsFollowedLink

indeed this patch does not work.
you will kill me but i want to cleanup this code a bit. i won't be able to land till next week, thus we can look at the patch personally if needed.

Comment 46

[Marco Bonardo [:mak]]

Attachment: Part3: Browser hook for markPageAsFollowedLink

Unfortunatly, when i had finished doing this patch i did a wrong qrefresh and then qdeleted the right one :\ So i had to rewrite it by memory.
Thus i will have to double-check it again before review, but you're welcome for a first look (or forward the first pass to someone other).

Before you hit me saying i'm crazy, i will explain why i think i'm not (at least bssed on what i've learned from this code):
- these methods were inteded to be used in early handlers (onclick=""), thus the return value should be the preventDefault expectation.  That said, sounds like various next patches added things with random return values.  And i can guess why: the method documentation sucks.  Myself initially thought that was "return true if handled", but what it eas intended to be is "return false if the caller should prevent default action".
- Due to the way the code was written, there were some paths with error checking and some without. for example href was not always checked for null.
- There were a bunch of copy vars, making hard to follow where values were finishing.
- Some of the paths were probably bogus or broken, since it was really hard to follow this code as it ended up in years.

I've still tried to preserve the original form to avoid a huge patch, but i tried to make it readable.  I changed some return value where we handle the link, but we still tell the caller to open it as if we forgot to handle it. If we force saving an url, why should we try to load it?

The idea (that i think was also the original one) is: we first handle very special click cases, not necessarily links. Then if we have a valid href we register it with history (the new part), and finally we handle special link cases in handleLinkClick.

Comment 47

[:Gavin Sharp [email: [e-mail]]]

If you're going to be doing this kind of cleanup, can you do it in a separate bug? Helps future VCS archeologists, and just makes it easier to track things.

Comment 48

[Marco Bonardo [:mak]]

(In reply to comment #47)
> If you're going to be doing this kind of cleanup, can you do it in a separate
> bug? Helps future VCS archeologists, and just makes it easier to track things.

i'll do once i go down the plane, on saturday. thanks.

Comment 49

[Marco Bonardo [:mak]]

Attachment: Part3: hook after contentAreaclick cleanup

filed bug 549340 for the cleanup, need to wait on that one.

Comment 50

[Marco Bonardo [:mak]]

Comment on attachment 429539 [details] [diff] [review]
Part3: hook after contentAreaclick cleanup

i can push all backend changes, and then just wait for the contentAreaClick changes to enabled them with a frontend hook. ContentAreaClick bug still requires a further cleanup and a test.

Ideally i can use this bug for the toolkit part and bug 477882 for the browser hook, and that is what i'm going to do.l.

Comment 51

[Marco Bonardo [:mak]]

part1: http://hg.mozilla.org/mozilla-central/rev/c9262d953b60
part2: http://hg.mozilla.org/mozilla-central/rev/b6ed4adcd542
(part3 is the frontend that will be done in bug 477882)
part4-5: http://hg.mozilla.org/mozilla-central/rev/cb840bbc331e
part6: http://hg.mozilla.org/mozilla-central/rev/5fdf6045c495
part7: http://hg.mozilla.org/mozilla-central/rev/04f377d6dada
part8: http://hg.mozilla.org/mozilla-central/rev/0b9aa7272cae