1368754 - Change nsINavHistoryResultObserver::NodeHistoryDetailsChanged/NodeURIChanged to be called with the old time/access count/uri rather than the new ones

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P2)

Description

[Milind L (:milindl)]

Current situation is that NodeHistoryDetailsChanged is called with the new node, the new time and the new access count. This doesn't make sense as the node contains info about time and accesscount, so extra info is redundant. 

Thus, we need to call it with old values rather than the new ones.

Comment 1

[Milind L (:milindl)]

Attachment: Bug 1368754 - Change `NodeHistoryDetailsChanged` and `NodeURIChanged` to be called with old time,access count and uri,

Update interface and all instances where the method is called
to be called with the old values, since the new values are already
there as a part of the node, and thus redundant.

Review commit: https://reviewboard.mozilla.org/r/144248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144248/

Comment 2

[Milind L (:milindl)]

Following above patch up with another changeset for updating any consumers using the new access count or new time.

Comment 3

[Milind L (:milindl)]

Attachment: Bug 1368754 - fix consumers which wait for `NodeHistoryDetailsChanged`/`NodeURIChanged` to be called to use node for new values,

Updated time and access count can be accessed using the node passed to the method.
There is no need to access the other two arguments, which contain the old time and
access count now.

Review commit: https://reviewboard.mozilla.org/r/144258/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144258/

Comment 4

[Milind L (:milindl)]

Comment on attachment 8872715 [details]
Bug 1368754 - Change `NodeHistoryDetailsChanged` and `NodeURIChanged` to be called with old time,access count and uri,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144248/diff/1-2/

Comment 5

[Milind L (:milindl)]

I noticed yesterday that the methods nodeXChanged always call with the node and the new value of X. Keeping consistent with our current idea, we should probably change all of them to call with the old values instead. Then again maybe I could just do url and time/accessCount and move on and keep the rest for later in another bug, since that may prove to be a longer task. 

Anyway, there is something different going on with nsLivemarkService.js and browserPlacesViews.js, since they seem to be using accessCount as a boolean instead of a number. I've fixed it the best I can (so it works like it currently is) but might be that I've missed something.

Thanks!

Comment 6

[Marco Bonardo [:mak]]

(In reply to Milind L (:milindl) from comment #5)
> I noticed yesterday that the methods nodeXChanged always call with the node
> and the new value of X. Keeping consistent with our current idea, we should
> probably change all of them to call with the old values instead. Then again
> maybe I could just do url and time/accessCount and move on and keep the rest
> for later in another bug, since that may prove to be a longer task.

Right, let's do what we need first, niceties later.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8872715 [details]
Bug 1368754 - Change `NodeHistoryDetailsChanged` and `NodeURIChanged` to be called with old time,access count and uri,

https://reviewboard.mozilla.org/r/144248/#review149756

::: toolkit/components/places/nsLivemarkService.js:683
(Diff revision 2)
> +            if (aVisitedStatus && node.accessCount === 0) {
> +              // This is the first time we are visiting it.
> +              node.accessCount = 1;
> +            }
>              Services.tm.dispatchToMainThread(() => {
> -              observer.nodeHistoryDetailsChanged(node, 0, aVisitedStatus);
> +              observer.nodeHistoryDetailsChanged(node, 0, oldAccessCount);

The reason for the special code here is that these nodes are not real (in the sense of being children of an nsNavHistoryResult), they are made-up and passed to an existing result. So there's no real accessCount to increase/decrease, we just return 0 or 1.

What you are doing looks conceptually correct, but it's not correct.
The problem is that accessCount is a readonly attribute:
http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/toolkit/components/places/nsLivemarkService.js#606 so you can't just overwrite it.
I actually think not overwriting accessCount will just do the right thing.
The problem may be oldAccessCount, to have it correct you should fetch the previous value of the visited status in the previous for loop.. you could probably do child.wasVisited = child.visited; child.visited = ... and here you'd have the previous status in .wasVisited

::: toolkit/components/places/nsNavHistoryResult.cpp:1579
(Diff revision 2)
>      uint32_t oldAccessCount = node->mAccessCount;
>      PRTime oldTime = node->mTime;
> +
> +    uint32_t parentOldAccessCount = parent->mAccessCount;
> +    PRTime parentOldTime = parent->mTime;
>      aCallback(node, aClosure, result);

nit: remove newline before parentOldAccessCount and add a newline before aCallback

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8872726 [details]
Bug 1368754 - fix consumers which wait for `NodeHistoryDetailsChanged`/`NodeURIChanged` to be called to use node for new values,

https://reviewboard.mozilla.org/r/144258/#review149760

test_nsINavHistoryViewer.js is also a consumer. that test is currently disabled due to being intermittent, but you could temporarily enable it and fix it (but still land with it disabled).

Comment 9

[Milind L (:milindl)]

(Seems right to rescope the bug since we need both for the bug 1107364, and we're not doing all the nodeXChanged right now)

Ok, I will apply the comments and the new changes for URI as well. Is this unallowed to land till after thursday as well, or is this safe to land?

Thanks!

Comment 10

[Marco Bonardo [:mak]]

it looks safe to me, nothing that may affect the product stability.

Comment 11

[Milind L (:milindl)]

I also noticed that we're calling nodeHistoryDetailsChanged inside http://searchfox.org/mozilla-central/source/browser/components/places/tests/chrome/test_bug549491.xul, but wasn't sure how to deal with it, since I am not sure how to get the old values for the rootNode. 

I read the related bug 549491 but from what I can gather the values passed should not matter. Is it ok then to just leave it there?

Comment 12

[Marco Bonardo [:mak]]

(In reply to Milind L (:milindl) from comment #11)
> I also noticed that we're calling nodeHistoryDetailsChanged inside
> http://searchfox.org/mozilla-central/source/browser/components/places/tests/
> chrome/test_bug549491.xul, but wasn't sure how to deal with it, since I am
> not sure how to get the old values for the rootNode. 

looks like that test is just invoking the notifications manually and checking the consumer doesn't throw... nothing serious off-hand.

Comment 13

[Milind L (:milindl)]

Comment on attachment 8872715 [details]
Bug 1368754 - Change `NodeHistoryDetailsChanged` and `NodeURIChanged` to be called with old time,access count and uri,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144248/diff/2-3/

Comment 14

[Milind L (:milindl)]

Comment on attachment 8872726 [details]
Bug 1368754 - fix consumers which wait for `NodeHistoryDetailsChanged`/`NodeURIChanged` to be called to use node for new values,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144258/diff/1-2/

Comment 15

[Milind L (:milindl)]

Side Note: that while I have modified the intermittent-failure test, I cannot check it. It fails locally both after my changesets are applied and when I'm on central.

it fails when it calls `clearHistoryEnabled` - I get an error saying that PlacesTestUtils.clearHistoryEnabled is not a function, and indeed it's not there in PlacesTestUtils.jsm.

Comment 16

[Marco Bonardo [:mak]]

looks like a search&replace mistake someone did (maybe even me), clearHistoryEnabled is in head_common.js, so you should just remove "PlacesTestUtils." from in front of it.

Comment 17

[Marco Bonardo [:mak]]

ah no sorry. it's in head_expiration.js, so it just can't work... That code doesn't make any sense... let me check again.

Comment 18

[Marco Bonardo [:mak]]

Ah ok, it should be a PlacesTestUtils.clearHistory().then...

Comment 19

[Milind L (:milindl)]

Comment on attachment 8872726 [details]
Bug 1368754 - fix consumers which wait for `NodeHistoryDetailsChanged`/`NodeURIChanged` to be called to use node for new values,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144258/diff/2-3/

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8872715 [details]
Bug 1368754 - Change `NodeHistoryDetailsChanged` and `NodeURIChanged` to be called with old time,access count and uri,

https://reviewboard.mozilla.org/r/144248/#review150076

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 8872726 [details]
Bug 1368754 - fix consumers which wait for `NodeHistoryDetailsChanged`/`NodeURIChanged` to be called to use node for new values,

https://reviewboard.mozilla.org/r/144258/#review150082

LGTM, thank you. Please check xpcshell and mochitests on Try, doing just linux and windows, both debug and opt, should be fine.

Comment 22

[Milind L (:milindl)]

Here is the try run - https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c8b61a983a687612454345ee8eb5d4894f4b858

Comment 23

[Marco Bonardo [:mak]]

it looks good. I forgot to tell you that you can use the "Automation / Trigger a Try Build" option in mozreview.

Comment 24

[Milind L (:milindl)]

(In reply to Marco Bonardo [::mak] from comment #23)
> it looks good. I forgot to tell you that you can use the "Automation /
> Trigger a Try Build" option in mozreview.

I noticed this after pushing to try, when it didn't show up automatically in review board. Hovering on the Trigger Try Build gives me a tooltip saying that I don't have the scm rights or whatever. In any case don't worry, I'll fix it the next time, so it is easy to see. Main thing is that I can actually push to try.

Thanks!

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9cab45e72b30
Change `NodeHistoryDetailsChanged` and `NodeURIChanged` to be called with old time,access count and uri, r=mak
https://hg.mozilla.org/integration/autoland/rev/aa36ffe32ddc
fix consumers which wait for `NodeHistoryDetailsChanged`/`NodeURIChanged` to be called to use node for new values, r=mak

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9cab45e72b30
https://hg.mozilla.org/mozilla-central/rev/aa36ffe32ddc