Last Comment Bug 568971 - Nuke nsIGlobalHistory3
: Nuke nsIGlobalHistory3
Status: RESOLVED FIXED
[inbound]
: addon-compat, dev-doc-complete
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Matheus Kerschbaum
:
: Marco Bonardo [::mak]
Mentors:
Depends on: 556400 568612
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-28 15:15 PDT by Benjamin Stover (:stechz)
Modified: 2011-11-02 10:44 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (28.82 KB, patch)
2011-06-23 23:55 PDT, Matheus Kerschbaum
no flags Details | Diff | Splinter Review
patch (28.82 KB, patch)
2011-07-31 12:21 PDT, Matheus Kerschbaum
mak77: review+
Details | Diff | Splinter Review
patch (28.73 KB, patch)
2011-08-05 19:45 PDT, Matheus Kerschbaum
matjk7: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
patch for checkin [r=mak sr=bz] (28.73 KB, patch)
2011-08-12 21:46 PDT, Matheus Kerschbaum
no flags Details | Diff | Splinter Review

Description Benjamin Stover (:stechz) 2010-05-28 15:15:09 PDT
Since docshell will no longer be requiring the use of GH3, and it was included for places use, we can get rid of the interface now.
Comment 1 Doug Turner (:dougt) 2010-06-02 12:14:37 PDT
Reset Assignee to default.
Comment 2 Matheus Kerschbaum 2011-06-23 23:55:06 PDT
Created attachment 541619 [details] [diff] [review]
patch
Comment 3 Matheus Kerschbaum 2011-07-31 12:21:27 PDT
Created attachment 549678 [details] [diff] [review]
patch

Updated to tip.
Comment 4 Marco Bonardo [::mak] 2011-08-01 17:14:28 PDT
Will check this soon, as a first feedback I think we should wait to land this after next merge, since it's pretty near.
Comment 5 Marco Bonardo [::mak] 2011-08-04 07:10:40 PDT
Stupid question, but due to the nature of the patch I have to ask it: do the 2 tests you are modifying still pass correctly? test_history_redirects.js may probably work, I'm surprised of test_bug_411966.html. maybe it's already going through the new code, or was just plain wrong.
Comment 6 Matheus Kerschbaum 2011-08-04 09:46:01 PDT
(In reply to comment #5)
> Stupid question, but due to the nature of the patch I have to ask it: do the
> 2 tests you are modifying still pass correctly? test_history_redirects.js
> may probably work, I'm surprised of test_bug_411966.html. maybe it's already
> going through the new code, or was just plain wrong.

I don't know if the tests are wrong but they do pass: http://tbpl.mozilla.org/?tree=Try&rev=e6ea48e2cb2b
Comment 7 Marco Bonardo [::mak] 2011-08-05 03:56:52 PDT
Comment on attachment 549678 [details] [diff] [review]
patch

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

r=me with the following comment addressed.
I suppose you also need a superreview since you are removing an interface.

For added safety, please land this after we have merged for Firefox 9.

::: toolkit/components/places/nsNavHistory.cpp
@@ +2736,5 @@
>    // Normally docshell sends the link visited observer notification for us (this
>    // will tell all the documents to update their visited link coloring).
> +  // However, for downloads (since we implement nsIDownloadHistory)
> +  // this will not happen and we need to send it ourselves.
> +  if (newItem && aTransitionType == TRANSITION_DOWNLOAD) {

changing the comment to remove the reference to nsIGlobalHistory is fine, but you should not remove the aIsRedirect check from here, since addVisit is still an exposed API (even if we should try to deprecate it soonish). Also addURI may call into this.
Comment 8 Matheus Kerschbaum 2011-08-05 19:45:57 PDT
Created attachment 551222 [details] [diff] [review]
patch

Review comment addressed.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-08-11 10:34:26 PDT
Comment on attachment 551222 [details] [diff] [review]
patch

sr=me
Comment 10 Matheus Kerschbaum 2011-08-12 21:46:04 PDT
Created attachment 552831 [details] [diff] [review]
patch for checkin [r=mak sr=bz]

Updated to tip.
Comment 12 Marco Bonardo [::mak] 2011-08-19 03:07:18 PDT
http://hg.mozilla.org/mozilla-central/rev/0f8e88bb5bc8
Comment 13 Eric Shepherd [:sheppy] 2011-11-02 10:44:15 PDT
This was already updated:

https://developer.mozilla.org/en/NsIGlobalHistory3

Removal mentioned on Firefox 9 for developers.

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