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

RESOLVED FIXED in Firefox 55

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: milindl, Assigned: milindl)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1107364
Assignee: nobody → i.milind.luthra
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
Following above patch up with another changeset for updating any consumers using the new access count or new time.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
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!
Priority: -- → P2
(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

2 years ago
mozreview-review
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
Attachment #8872715 - Flags: review?(mak77)

Comment 8

2 years ago
mozreview-review
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).
Attachment #8872726 - Flags: review?(mak77)
(Assignee)

Updated

2 years ago
Summary: Change nsINavHistoryResultObserver::NodeHistoryDetailsChanged to be called with the old time/access count rather than the new ones → Change nsINavHistoryResultObserver::NodeHistoryDetailsChanged/NodeURIChanged to be called with the old time/access count/uri rather than the new ones
(Assignee)

Comment 9

2 years ago
(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!
it looks safe to me, nothing that may affect the product stability.
(Assignee)

Comment 11

2 years ago
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?
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
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 hidden (obsolete)
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.
Ah ok, it should be a PlacesTestUtils.clearHistory().then...
Comment hidden (mozreview-request)

Comment 20

2 years ago
mozreview-review
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
Attachment #8872715 - Flags: review?(mak77) → review+

Comment 21

2 years ago
mozreview-review
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.
Attachment #8872726 - Flags: review?(mak77) → review+
it looks good. I forgot to tell you that you can use the "Automation / Trigger a Try Build" option in mozreview.
(Assignee)

Comment 24

2 years ago
(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

2 years ago
Pushed by mak77@bonardo.net:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9cab45e72b30
https://hg.mozilla.org/mozilla-central/rev/aa36ffe32ddc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.