remove nsPIPlacesHistoryListenersNotifier

RESOLVED FIXED in mozilla38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mak, Assigned: ttaubert, Mentored)

Tracking

Trunk
mozilla38
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good second bug][lang=js][lang=cpp])

Attachments

(1 attachment, 1 obsolete attachment)

we can use PlacesUtils.history.getObservers to issue notifications instead of this private API.

we can remove this API, its implementation, and directly invoke the observers where needed.
Flags: qe-verify-
Flags: firefox-backlog+

Comment 1

4 years ago
I'm working on this bug. Please, assign to me.
Hi, thank you for your contribution.
There shouldn't be many consumer of this API, it is basically
nsPlacesExpiration.js and History.jsm
they should directly use getObservers() and then notify with the right arguments.
Then the idl should be removed, and the implementation in nsNavHistory.cpp should stop implementing the API (add declaration to the header and change NS_IMETHODIMP with nsresult) since there is still an internal consumer in nsNavHistory.cpp but that's not important to change here.
Assignee: nobody → w32

Comment 3

4 years ago
I have some problems with debugging. I can't initialize this module in web console, because utils in Components doesn't exist. What a reason of this? Few days ago I experimenting with ctypes.jsm and all was fine.
It is unlikely to work in the Web console, you need to use the Browser console or Scratchpad in Browser environment (after enabling chrome debugging in devtools options and then choosing browser in the environment menu)

Comment 5

4 years ago
It seems PlacesUtils.history.getObservers is not implemented. This is also my task?
are you on the latest trunk? it works for me... it is .getObservers() fwiw

Comment 7

4 years ago
I'm on mozilla-central. trunc is other repository?
no, mozilla-central is fine, ensure you have pulled the latest code out of it

Comment 9

4 years ago
Sorry, forgot to rebuild after pulling out.

Updated

4 years ago
Assignee: w32 → mak77
Hi, did you decide to leave the bug or was reassigning just a mistake?
Flags: needinfo?(w32)

Comment 11

4 years ago
Sorry, I've joined servo community. Now, I'm fully busy in this project.
Flags: needinfo?(w32)
(Assignee)

Comment 12

4 years ago
When History.jsm and nsPlacesExpiration.js don't call NotifyOnPageExpired() anymore, how can we set mDaysOfHistory = -1?

Does it also mean that now both of the callers have to duplicate the logic in NotifyOnPageExpired() to decide whether to call onDeleteURI() and onDeleteVisits()?
Flags: needinfo?(mak77)
(Assignee)

Updated

4 years ago
Blocks: 1113175
(Assignee)

Comment 13

4 years ago
Stealing. I'm sure Marco doesn't mind :)
Assignee: mak77 → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 37.3 - 12 Jan
(In reply to Tim Taubert [:ttaubert] from comment #12)
> When History.jsm and nsPlacesExpiration.js don't call NotifyOnPageExpired()
> anymore, how can we set mDaysOfHistory = -1?

That problem has already been solved, when you call .getObservers() out of history, mDaysOfHistory gets set to -1.

> Does it also mean that now both of the callers have to duplicate the logic
> in NotifyOnPageExpired() to decide whether to call onDeleteURI() and
> onDeleteVisits()?

yes, for now.
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #14)
> > Does it also mean that now both of the callers have to duplicate the logic
> > in NotifyOnPageExpired() to decide whether to call onDeleteURI() and
> > onDeleteVisits()?
> 
> yes, for now.

well, provided they could take both branches, if they can only take one branch, there's no need for any logic.
(Assignee)

Comment 16

4 years ago
Created attachment 8548086 [details] [diff] [review]
0001-Bug-1101553-Remove-nsPIPlacesHistoryListenersNotifie.patch
Attachment #8548086 - Flags: review?(mak77)
(Assignee)

Comment 17

4 years ago
Created attachment 8548116 [details] [diff] [review]
0001-Bug-1101553-Remove-nsPIPlacesHistoryListenersNotifie.patch, v2
Attachment #8548086 - Attachment is obsolete: true
Attachment #8548086 - Flags: review?(mak77)
Attachment #8548116 - Flags: review?(mak77)

Updated

4 years ago
Iteration: 37.3 - 12 Jan → 38.1 - 26 Jan
Comment on attachment 8548116 [details] [diff] [review]
0001-Bug-1101553-Remove-nsPIPlacesHistoryListenersNotifie.patch, v2

Review of attachment 8548116 [details] [diff] [review]:
-----------------------------------------------------------------

glad to see more xpidl going away...

::: toolkit/components/places/nsPlacesExpiration.js
@@ +437,5 @@
>                                       "@mozilla.org/widget/idleservice;1",
>                                       "nsIIdleService");
> +  XPCOMUtils.defineLazyModuleGetter(this, "_utils",
> +                                    "resource://gre/modules/PlacesUtils.jsm",
> +                                    "PlacesUtils");

let's just define this at the top of the component as PlacesUtils...

I think we went a bit too far on the abstraction side in this component :)

@@ +635,5 @@
> +            observer.onDeleteURI(uri, guid, reason);
> +          } else {
> +            observer.onDeleteVisits(uri, visitDate, guid, reason, 0);
> +          }
> +        } catch (ex) {}

let's just copy the notify() util here and uniform the notation

This is using 0 for the transition, while History.jsm is using -1... let's be consistent and just pass 0 in both places.
Attachment #8548116 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/33b4ad1f2fc0
https://hg.mozilla.org/mozilla-central/rev/b0ee8d61158a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.