Closed Bug 1101553 Opened 5 years ago Closed 5 years ago

remove nsPIPlacesHistoryListenersNotifier

Categories

(Toolkit :: Places, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
mozilla38
Iteration:
38.1 - 26 Jan

People

(Reporter: mak, Assigned: ttaubert, Mentored)

References

Details

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

Attachments

(1 file, 1 obsolete file)

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+
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
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)
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
I'm on mozilla-central. trunc is other repository?
no, mozilla-central is fine, ensure you have pulled the latest code out of it
Sorry, forgot to rebuild after pulling out.
Assignee: w32 → mak77
Hi, did you decide to leave the bug or was reassigning just a mistake?
Flags: needinfo?(w32)
Sorry, I've joined servo community. Now, I'm fully busy in this project.
Flags: needinfo?(w32)
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)
Blocks: 1113175
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.
Attachment #8548086 - Attachment is obsolete: true
Attachment #8548086 - Flags: review?(mak77)
Attachment #8548116 - Flags: review?(mak77)
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.