Last Comment Bug 839034 - move markPageAsXXX methods to nsINavHistoryService
: move markPageAsXXX methods to nsINavHistoryService
Status: RESOLVED FIXED
: addon-compat
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla22
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks: 739219 845895
  Show dependency treegraph
 
Reported: 2013-02-07 05:22 PST by Marco Bonardo [::mak] (Away 6-20 Aug)
Modified: 2013-04-03 03:48 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (11.43 KB, patch)
2013-02-08 00:16 PST, Raymond Lee [:raymondlee]
mak77: review+
Details | Diff | Splinter Review
v2 (12.32 KB, patch)
2013-02-08 11:58 PST, Raymond Lee [:raymondlee]
gavin.sharp: superreview+
Details | Diff | Splinter Review
Patch for Metro branch (1.28 KB, patch)
2013-02-08 12:42 PST, Raymond Lee [:raymondlee]
mak77: review+
Details | Diff | Splinter Review
Patch for check-in (12.58 KB, patch)
2013-02-26 19:20 PST, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
Patch for check-in for Metro branch (1.54 KB, patch)
2013-02-26 19:20 PST, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Marco Bonardo [::mak] (Away 6-20 Aug) 2013-02-07 05:22:37 PST
these methods are currently spread half in nsIBrowserHistory and half in nsINavHistoryService, we should just move all of them to nsINavHistoryService.
Comment 2 Marco Bonardo [::mak] (Away 6-20 Aug) 2013-02-08 06:10:05 PST
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
Comment 3 Marco Bonardo [::mak] (Away 6-20 Aug) 2013-02-08 06:14:01 PST
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
Comment 4 Marco Bonardo [::mak] (Away 6-20 Aug) 2013-02-08 06:19:03 PST
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.
Comment 5 Raymond Lee [:raymondlee] 2013-02-08 11:58:06 PST
Created attachment 711929 [details] [diff] [review]
v2

Rev the UUIDs as suggested in comment 3
Comment 6 Marco Bonardo [::mak] (Away 6-20 Aug) 2013-02-08 11:59:51 PST
please see also comment 2
Comment 7 Raymond Lee [:raymondlee] 2013-02-08 12:42:41 PST
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.
Comment 8 Raymond Lee [:raymondlee] 2013-02-26 19:20:05 PST
Created attachment 718803 [details] [diff] [review]
Patch for check-in
Comment 9 Raymond Lee [:raymondlee] 2013-02-26 19:20:55 PST
Created attachment 718805 [details] [diff] [review]
Patch for check-in for Metro branch
Comment 10 Marco Bonardo [::mak] (Away 6-20 Aug) 2013-02-27 01:33:57 PST
the Metro branch has been merged to central some days ago, so both patches can land in inbound
Comment 11 Marco Bonardo [::mak] (Away 6-20 Aug) 2013-02-27 01:36:01 PST
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.
Comment 12 Raymond Lee [:raymondlee] 2013-02-27 07:28:51 PST
Passed try
https://tbpl.mozilla.org/?tree=Try&rev=db8b2eb00be8
Comment 13 Marco Bonardo [::mak] (Away 6-20 Aug) 2013-02-27 10:09:43 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/e73692553d1f
Comment 14 Ryan VanderMeulen [:RyanVM] 2013-02-27 17:52:40 PST
https://hg.mozilla.org/mozilla-central/rev/e73692553d1f
Comment 16 Marco Bonardo [::mak] (Away 6-20 Aug) 2013-04-03 03:48:52 PDT
thanks, I indeed forgot to set dev-doc-needed :(

Note You need to log in before you can comment on or make changes to this bug.