move markPageAsXXX methods to nsINavHistoryService

RESOLVED FIXED in mozilla22

Status

()

Toolkit
Places
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mak, Assigned: raymondlee)

Tracking

(Blocks: 1 bug, {addon-compat})

Trunk
mozilla22
x86_64
Windows 7
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
these methods are currently spread half in nsIBrowserHistory and half in nsINavHistoryService, we should just move all of them to nsINavHistoryService.
(Assignee)

Updated

5 years ago
Assignee: nobody → raymond
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 711723 [details] [diff] [review]
v1

https://tbpl.mozilla.org/?tree=Try&rev=27d729127f07
Attachment #711723 - Flags: review?(mak77)
(Reporter)

Comment 2

5 years ago
we need also an additional patch for Metro elm project branch, that removes nsIBrowserHistory from here:
http://mxr.mozilla.org/projects-central/source/elm/browser/metro/base/content/browser-scripts.js#57
(Reporter)

Comment 3

5 years ago
Comment on attachment 711723 [details] [diff] [review]
v1

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

once these comments are fixed we will need a superreview here, gavin may likely do it.
Luckily the ClassInfo on history and the PlacesUIUtils helpers help us with compatibility, I'm now going to check add-ons mxr for breaking usage.

::: toolkit/components/places/nsIBrowserHistory.idl
@@ -108,5 @@
> -     *
> -     * @param aURI
> -     *        URI of the page to be marked.
> -     */
> -    void markPageAsFollowedLink(in nsIURI aURI);

you must rev the UUID here

::: toolkit/components/places/nsINavHistoryService.idl
@@ +1280,5 @@
> +   *
> +   * @param aURI
> +   *        URI of the page to be marked.
> +   */
> +  void markPageAsFollowedLink(in nsIURI aURI);

as well as here
Attachment #711723 - Flags: review?(mak77) → review+
(Reporter)

Comment 4

5 years ago
As I suspected, the classInfo and PlacesUtils here makes so that most of the add-ons will keep working without changes, I found a couple that are getting a handle through globalhistory2 and while this bug won't break them, bug 838874 will regardless.
(Reporter)

Updated

5 years ago
Keywords: addon-compat
(Assignee)

Comment 5

5 years ago
Created attachment 711929 [details] [diff] [review]
v2

Rev the UUIDs as suggested in comment 3
Attachment #711723 - Attachment is obsolete: true
Attachment #711929 - Flags: superreview?(gavin.sharp)
(Reporter)

Comment 6

5 years ago
please see also comment 2
(Assignee)

Comment 7

5 years ago
Created attachment 711947 [details] [diff] [review]
Patch for Metro branch

(In reply to Marco Bonardo [:mak] from comment #2)
> we need also an additional patch for Metro elm project branch, that removes
> nsIBrowserHistory from here:
> http://mxr.mozilla.org/projects-central/source/elm/browser/metro/base/
> content/browser-scripts.js#57

Address this.
Attachment #711947 - Flags: review?(mak77)
(Reporter)

Updated

5 years ago
Attachment #711947 - Flags: review?(mak77) → review+
Attachment #711929 - Flags: superreview?(gavin.sharp) → superreview+
(Assignee)

Comment 8

5 years ago
Created attachment 718803 [details] [diff] [review]
Patch for check-in
Attachment #711929 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 718805 [details] [diff] [review]
Patch for check-in for Metro branch
Attachment #711947 - Attachment is obsolete: true
(Reporter)

Comment 10

5 years ago
the Metro branch has been merged to central some days ago, so both patches can land in inbound
(Reporter)

Comment 11

5 years ago
Ah I see you already fixed the usage in central, then probably we don't need the metro patch, from what I see they just merge the branch with central periodically.
(Assignee)

Comment 12

5 years ago
Passed try
https://tbpl.mozilla.org/?tree=Try&rev=db8b2eb00be8
Keywords: checkin-needed
Whiteboard: c
(Reporter)

Updated

5 years ago
Attachment #718805 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Blocks: 845895
(Reporter)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e73692553d1f
Keywords: checkin-needed
Whiteboard: c
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/e73692553d1f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Just FYI: I updated https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsINavHistoryService and https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIBrowserHistory to reflect the interface changes.
(Reporter)

Comment 16

4 years ago
thanks, I indeed forgot to set dev-doc-needed :(
You need to log in before you can comment on or make changes to this bug.