546938 - Create an xpcom wrapper around async History API to allow for xpcshell tests

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect)

Description

[Frederic Plourde (:fred23)]

Currently, the History async API (places/src/history.cpp) is tested using compiled-code tests. Xpcshell testing is impossible because the object is not exposed.

However, bug 516728 remotes IHistory's visitedness information across parent/child process boundary. The associated History tests should then be able to run in content process, which is currently impossible with compiled-code tests.

The idea here is to write a new xpcom component "nsHistoryTest" encapsulating all the tests currently living in the compiled-code tests and then write an xpcshell test to drive them in the child using Jduell's run_test_in_child.

Comment 1

[Shawn Wilsher :sdwilsh]

So, the original idea of this was to use a native code API because we didn't want to expose this to script.  Creating an XPCOM interface for this just for tests seems wrong though.

Why is it impossible to spawn a child process from complied code tests?

Comment 2

[Frederic Plourde (:fred23)]

(In reply to comment #1)
> So, the original idea of this was to use a native code API because we didn't
> want to expose this to script.  Creating an XPCOM interface for this just for
> tests seems wrong though.

I've talked with ted about that and it seems there are ways we could avoid shipping this interface to end users. More news on that front coming up.
 
> Why is it impossible to spawn a child process from complied code tests?

It's not impossible, of course, but it's not currently done in any way. So IMO, making compiled-code tests spawn child processes would involve more work than just creating an xpcom wrapper.

Comment 3

[Frederic Plourde (:fred23)]

sdwilsh:
by the way, tests typelibs are already being used in the mozilla codebase. See this example :
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/ 

This gets defined in its own "crashreporter_test" module. You can see that its interface is not being shipped with the app because it's not added in any way to the package manifest. To make it available to xpcshell tests, however, there's some magic involved. see :
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/Makefile.in#75

I'll have a patch ready that uses this exact same technique in testing/xpcshell/Makefile.in.

Comment 4

[Frederic Plourde (:fred23)]

Attachment: test v.1

Here's an xpcom wrapper around former test_IHistory.cpp compiled-code test. It's really encapsulating the whole thing, with minor modifs for compatibility, so everything oughta be just fine. It's been tested on the actual IHistory code and it runs smoothly.

note1: Note however that the tests are leaking. I've filed Bug 551020 for that.

note2: This patch only tests the IHistory API in the MOZ_IPC case (on the content process side). To test from the parent, we still need former test_IHistory.cpp compiled-code test. I've filed another bug (Bug 551014) to track future work on that issue.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 431356 [details] [diff] [review]
test v.1

>+# Portions created by the Initial Developer are Copyright (C) 2007
I suspect you copied this from something else and pasted it.  You should use the http://www.mozilla.org/MPL/boilerplate-1.1/ instead

>+DEPTH		= ../../../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
>+VPATH		= @srcdir@
weird spacing (tabs?)

>+DIRS = \
>+	public \
>+	src \
>+	$(NULL)
only indent two spaces

>+const spec = 
nit consts should be all caps or have the k prefix
>+[ 
>+  "http://mozilla.org/test_visited_notifies",
>+  "http://mozilla.org/test_same_uri_notifies_both",
>+  "http://mozilla.org/test_unregistered_visited_does_not_notify",
>+  "http://mozilla.org/test_new_visit_notifies_waiting_Link",
>+  "http://mozilla.org/test_RegisterVisitedCallback_returns_before_notifying",
>+  "http://mozilla.org/test_observer_topic_dispatched_visitedURI"
nit: add trailing comma please

>+]
nit: missing semicolon

>+
>+var profDir = do_get_profile();
>+
>+let dirSvc = Cc["@mozilla.org/file/directory_service;1"].
>+      getService(Ci.nsIProperties);
why the weird combination of var and let?

>+  QueryInterface: function(iid) {
>+    if (iid.equals(Ci.nsIDirectoryServiceProvider) ||
>+        iid.equals(Ci.nsISupports)) {
>+      return this;
>+    }
>+    throw Cr.NS_ERROR_NO_INTERFACE;
use XPCOMUtils.generateQI here

>+//
>+// Make sure we're adding test visits only on the chrome process side!
>+//
>+var runtime = Components.classes["@mozilla.org/xre/app-info;1"].
>+          getService(Components.interfaces.nsIXULRuntime);
weird spacing again, and you defined Cc but then don't use it?  Same with Ci

>+if (runtime.processType == 
>+          Components.interfaces.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {
ditto

>+  try {
>+    var hs = Cc["@mozilla.org/browser/nav-history-service;1"]
>+      .getService(Ci.nsINavHistoryService);
>+  } catch(ex) {
>+    do_throw("Could not get history service\n");
>+  } 
>+
>+  try {
>+    var ioSrv = Cc["@mozilla.org/network/io-service;1"]
>+      .getService(Ci.nsIIOService);
>+  } catch(ex) {
>+    do_throw("Could not get io service\n");
>+  }
Do not wrap these in try-catch

>+  // add the various visits that we're going to need in History
>+  for(i=0; i<spec.length; i++) { 
nit: space before (; i undeclared

>+    var newURI = ioSrv.newURI(spec[i], null, null);
NetUtil.newURI

>+    var visitID = hs.addVisit(newURI, Date.now()*1000, 
>+       null, Ci.nsINavHistoryService.TRANSITION_LINK, 0, 0, id);
style is odd here; line up new lines with (

>\ No newline at end of file
fix please


>+++ b/toolkit/components/places/tests/unit_ipc/public/Makefile.in
ditto re: previous makefile

>+[scriptable, uuid(02CB2DE5-5EAA-4adb-8AA8-D91CC1F46C57)]
nit: we don't use all caps in these AFAIK; looks weird

>+   /**
>+    * XPCOM wrapper for testing the History Component from content process.
>+    *    
>+    */
I think you want this above the idl

>+  // this is the only scriptable entry point to the internal test
>+  void runTest();
proper java-doc style please, and add more details

>\ No newline at end of file
fix please

>+++ b/toolkit/components/places/tests/unit_ipc/real_test_IHistory.js
no license

>+function run_test() { 
nit: brace on new line

>+  ////////////////////////////////////////////////////////////////////////////
>+  //// get the IHistory test interface
this style of comment seems odd - just do // and then the comment with proper sentences.

>+  try {
>+    var test = Cc["@mozilla.org/places/historytest;1"].
>+      createInstance(Ci.nsIHistoryTest);
>+  } catch(ex) {
>+    do_throw("Could not instanciate historytest\n");
>+  }
>+
>+  test.runTest();
You should just do this though:

let test = Cc["@mozilla.org/places/historytest;1"].
           createInstance(Ci.nsIHistoryTest);
function run_test() test.runTest();

That is all you need for this file :)

>+++ b/toolkit/components/places/tests/unit_ipc/src/Makefile.in
ditto about license block

>+CPPSRCS = \
>+          nsHistoryTest.cpp \
>+          $(NULL)
only two space indent please

>+++ b/toolkit/components/places/tests/unit_ipc/src/nsHistoryTest.cpp
so, this is basically a copy of toolkit/components.places/tests/cpp/test_IHistory.cpp, right?  Can you do and hg cp so I only get the diff of what you changed please?  Also, we don't use the ns prefix for places stuff anymore, so you should come up with something different.

>+++ b/toolkit/components/places/tests/unit_ipc/src/nsHistoryTestHarness.h
This is a copy too, right?  Please hg cp this as well.

>+++ b/toolkit/components/places/tests/unit_ipc/src/nsHistoryTestHarness_tail.h
and this

>+++ b/toolkit/components/places/tests/unit_ipc/src/nsHistoryTestMockLink.h
and this

>+++ b/toolkit/components/places/tests/unit_ipc/test_IHistory_wrap.js
missing license

>@@ -0,0 +1,8 @@
>+//
>+// Run test script in content process instead of chrome (xpcshell's default)
>+// This code runs in the parent (chrome process)
>+// 
This style comment doesn't match anything in places.  Multiline like this are
/**
 * Comment here
 */
and nix the trailing white space

>+function run_test() {
nit: brace on new line
>+  run_test_in_child("./real_test_IHistory.js");
>+}

ted should review the build changes too; and :mak should also review the places changes

Comment 6

[Frederic Plourde (:fred23)]

Attachment: patch v.2

Patch corrected per sdwilsh's comments.

Ted: Could you review the "build" part of the build, please ? Remember, we talked about that on IRC, This is an XPCOM wrapper for tests in places that I don't want to ship with the product. I basically did what you did in http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/Makefile.in#74

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 434869 [details] [diff] [review]
patch v.2

>diff --git a/toolkit/components/places/tests/Makefile.in b/toolkit/components/places/tests/Makefile.in

>+	
>+ifdef MOZ_IPC
>+XPCSHELL_TESTS += unit_ipc
>+endif
>+
> 
> # Simple MochiTests
> MOCHI_TESTS = \
>@@ -63,6 +69,7 @@ MOCHI_TESTS = \
> 	$(NULL)
> 
> DIRS = \
>+	unit_ipc \

any reason to not move the ifdef after this, and make instead:

ifdef MOZ_IPC
XPCSHELL_TESTS += unit_ipc
DIRS += unit_ipc
endif

btw, for how xpcshell tests are working, won't this copy all cpp and other files to the xpcshell tests folders? I guess if it would not be better to split ipc_test_wrapper/ and unit_ipc/, put the wrapper in the former and unit tests in the latter. maybe ted has hints about this

>diff --git a/toolkit/components/places/tests/unit_ipc/head_test_IHistory_wrap.js b/toolkit/components/places/tests/unit_ipc/head_test_IHistory_wrap.js

can we call this just head_ipc.js?

>+// This code sets up the test environment for all content-side tests in 
>+// real_test_IHistory.js
>+
>+const NS_APP_USER_PROFILE_50_DIR = "ProfD";
>+const NS_APP_HISTORY_50_FILE = "UHist";
>+
>+const Ci = Components.interfaces;
>+const Cc = Components.classes;
>+const Cr = Components.results;
>+const Cu = Components.utils;
>+
>+const SPECS = 
>+[ 
>+  "http://mozilla.org/test_visited_notifies",
>+  "http://mozilla.org/test_same_uri_notifies_both",
>+  "http://mozilla.org/test_unregistered_visited_does_not_notify",
>+  "http://mozilla.org/test_new_visit_notifies_waiting_Link",
>+  "http://mozilla.org/test_RegisterVisitedCallback_returns_before_notifying",
>+  "http://mozilla.org/test_observer_topic_dispatched_visitedURI",
>+];
>+
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+
>+__defineGetter__("NetUtil", function() {
>+  delete this.NetUtil;
>+  Cu.import("resource://gre/modules/NetUtil.jsm");
>+  return NetUtil;
>+});

can you use XPCOMUtils.defineLazyGetter?

>+//
>+// Make sure we're adding test visits only on the chrome process side!
>+//

nit: this comment style does not look standard, just one line is enough

>+let runtime = Cc["@mozilla.org/xre/app-info;1"].
>+              getService(Components.interfaces.nsIXULRuntime);
>+if (runtime.processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {
>+  let hs = Cc["@mozilla.org/browser/nav-history-service;1"].
>+           getService(Ci.nsINavHistoryService);
>+  if (!hs)
>+    do_throw("Could not get history service\n");

i would bet getting hs would already throw by itself in such a case :)

>+  // add the various visits that we're going to need in History
>+  for (let i = 0; i < SPECS.length; i++) { 
>+    let id = {};

what is this for? there is not need to pass in an object to addVisit

>+    let newURI = NetUtil.newURI(SPECS[i], null, null);
>+    let visitID = hs.addVisit(newURI, Date.now()*1000, 
>+                              null, Ci.nsINavHistoryService.TRANSITION_LINK, 
>+                              0, 0, id);

"0, 0, id);" should be "false, 0);"

>+    do_check_true(visitID > 0);

do_check_true is not really useful, method would throw, thus it is not useful to store the visit id at all

>diff --git a/toolkit/components/places/tests/unit_ipc/real_test_IHistory.js b/toolkit/components/places/tests/unit_ipc/real_test_IHistory.js

why "real"? is not the harness skipping files not called test_something.js?

>+++ b/toolkit/components/places/tests/unit_ipc/real_test_IHistory.js

>+let test = Cc["@mozilla.org/places/historytest;1"].
>+           createInstance(Ci.nsIHistoryTest);
>+
>+function run_test() test.runTest();
>\ No newline at end of file

your editor can do better, add newline please :)

>diff --git a/toolkit/components/places/tests/cpp/test_IHistory.cpp b/toolkit/components/places/tests/unit_ipc/src/historyTest.cpp

>+void 
>+Test_unvisted_does_not_notify_part1()
> {
>+  nsresult rv; 

>   // Now, register our Link to be notified.
>   nsCOMPtr<IHistory> history(do_get_IHistory());
>-  nsresult rv = history->RegisterVisitedCallback(testURI, testLink);
>+  rv = history->RegisterVisitedCallback(testURI, testLink);

why did you move this, define at first use unless you can't.
also, trailing spaces

> void
>-test_visited_notifies()
>+Test_visited_notifies()
> {
>+  nsresult rv;
>+

ditto

> void
>-test_unvisted_does_not_notify_part2()
>+Test_unvisted_does_not_notify_part2()
> {
>+  nsresult rv;

same (and so on, won't comment further)

> void
>-test_unregistered_visited_does_not_notify()
>+Test_unregistered_visited_does_not_notify()
> {

>   // And finally add a visit for the URI.
>-  addURI(testURI);
>+  //addURI(testURI);

why is this commented out? if it's useless remove it

> void
>-test_new_visit_notifies_waiting_Link()
>+Test_new_visit_notifies_waiting_Link()
> {

>   // Add ourselves to history.
>-  addURI(testURI);
>+  //addURI(testURI);

ditto, and so on.

> 
>+  // Run the next test.
>   run_next_test();

useless comment, the function name is self documenting
same in other points

>diff --git a/toolkit/components/places/tests/cpp/mock_Link.h b/toolkit/components/places/tests/unit_ipc/src/historyTestMockLink.h

>+/**
>+ * Run test script in content process instead of chrome (xpcshell's default)
>+ * This code runs in the parent (chrome process)
>+ */
>+function run_test()
>+{
>+  run_test_in_child("./real_test_IHistory.js");
>+}

oh i see test is ran here, if it's not a problem due to conflicts with xpcshell usual tests, i'd prefer these be just called test_something.js and drop this "real" prefix that is meaningless

I'd like to do a second pass on next iteration, after ted did his pass.
thanks

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 434869 [details] [diff] [review]
patch v.2

Flag me for review after you've addressed mak's comments please :)

Comment 9

[Frederic Plourde (:fred23)]

Attachment: patch v.3

(In reply to comment #7)
> any reason to not move the ifdef after this, and make instead:
> 
> ifdef MOZ_IPC
> XPCSHELL_TESTS += unit_ipc
> DIRS += unit_ipc
> endif

done.


> btw, for how xpcshell tests are working, won't this copy all cpp and other
> files to the xpcshell tests folders? I guess if it would not be better to split ipc_test_wrapper/ and unit_ipc/, put the wrapper in the former and unit tests in the latter. maybe ted has hints about this

You're right. I tested and all the .h and .cpp files get copied into _tests xpcshell directory. Not good. So I just created an ipc_test_wrapper/ directory as you suggested.

> > diff --git a/toolkit/components/places/tests/unit_ipc
> 
> can we call this just head_ipc.js?

I'm afraid not, because this file is really about IHistory stuff (look at the SPECS const). leaving unchanged for now.


> >+__defineGetter__("NetUtil", function() {
> >+  delete this.NetUtil;
> >+  Cu.import("resource://gre/modules/NetUtil.jsm");
> >+  return NetUtil;
> >+});
> 
> can you use XPCOMUtils.defineLazyGetter?

done. 


> >+//
> >+// Make sure we're adding test visits only on the chrome process side!
> >+//
> 
> nit: this comment style does not look standard, just one line is enough

done.
 
> >+  if (!hs)
> >+    do_throw("Could not get history service\n");
>
> i would bet getting hs would already throw by itself in such a case :)

true. I've removed this check.


>l, method would throw, thus it is not useful
> to store the visit id at all

done. dropped the need for id and visitId.
 

> >diff --git a/toolkit/components/places/tests/unit_ipc/real_test_IHistory.js b/toolkit/components/places/tests/unit_ipc/real_test_IHistory.js
> 
> why "real"? is not the harness skipping files not called test_something.js?

The test file must *not* start with "test_" because it's going to be run by runxpcshell on the *main chrome process*, which we want to avoid here, because it wouldn't make sense. The wrapper, though, will be run be the chrome process and must start with "test_".
Instead, here, I renamed the test file "IHistory_test.js", simply.


> >\ No newline at end of file
> 
> your editor can do better, add newline please :)

done.


> >+  nsresult rv; 
> 
> >   // Now, register our Link to be notified.
> >   nsCOMPtr<IHistory> history(do_get_IHistory());
> >-  nsresult rv = history->RegisterVisitedCallback(testURI, testLink);
> >+  rv = history->RegisterVisitedCallback(testURI, testLink);
> 
> why did you move this, define at first use unless you can't.
> also, trailing spaces

done.


> >+  nsresult rv;
> >+
> 
> ditto

done for all of them.


> >   // And finally add a visit for the URI.
> >-  addURI(testURI);
> >+  //addURI(testURI);
> 
> why is this commented out? if it's useless remove it

done. 


> >+  // Run the next test.
> >   run_next_test();
> 
> useless comment, the function name is self documenting
> same in other points

done.


> >+/**
> >+ * Run test script in content process instead of chrome (xpcshell's default)
> >+ * This code runs in the parent (chrome process)
> >+ */
> >+function run_test()
> >+{
> >+  run_test_in_child("./real_test_IHistory.js");
> >+}
> 
> oh i see test is ran here, if it's not a problem due to conflicts with xpcshell usual tests, i'd prefer these be just called test_something.js and drop this "real" prefix that is meaningless.

same objection as explained above.

Comment 10

[Frederic Plourde (:fred23)]

I made a small nit in my last comment. Just to make sure we understand each other, this should have read : 

> >diff --git a/toolkit/components/places/tests/unit_ipc/real_test_IHistory.js b/toolkit/components/places/tests/unit_ipc/real_test_IHistory.js
> 
> why "real"? is not the harness skipping files not called test_something.js?

The test file must *not* start with "test_" because it's going to be run by
runxpcshell on the *main chrome process*, which we want to avoid here, because
it wouldn't make sense. The wrapper, though, will be run be the *CONTENT* process and must start with "test_".
Instead, here, I renamed the test file "IHistory_test.js", simply.

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 436534 [details] [diff] [review]
patch v.3

For more context with comments, please see http://reviews.visophyte.org/r/436534/

on file: toolkit/components/places/tests/ipc_test_wrappers/IHistory/historyTestHarness_tail.h line 51
> #ifdef XP_WIN
> #define SIZE_T "%Iu"
> #else
> #define SIZE_T "%zu"
> #endif
> 

don't think you want this anymore


on file: toolkit/components/places/tests/ipc_test_wrappers/IHistory/historyTest.cpp line 25
>  *   Frederic  Plourde <[e-mail]>

nit: looks like you have two spaces in between your first and last names


on file: toolkit/components/places/tests/ipc_test_wrappers/IHistory/historyTest.cpp line 43
> #include "historyTestMockLink.h"

I think we can save our copying of the file here and just do a relative
include:
#inlcude "../../cpp/mock_link.h"


on file: toolkit/components/places/tests/ipc_test_wrappers/IHistory/historyTest.cpp line 47
> NS_IMPL_ISUPPORTS1(HistoryTest,
>                    nsIHistoryTest)

NS_IMPL_ISUPPORTS1(
  HistoryTest
, nsIHistoryTest
)


on file: toolkit/components/places/tests/ipc_test_wrappers/IHistory/historyTest.cpp line 50
> HistoryTest::HistoryTest()
> {
> }
> 
> HistoryTest::~HistoryTest()
> {
> }

Use the defaults if the are empty.


on file: toolkit/components/places/tests/ipc_test_wrappers/IHistory/historyTest.cpp line 96
>   testURI = new_test_uri(nsCString("http://mozilla.org/test_unvisted_does_not_notify_part1"));

nit: you want to use NS_LITERAL_STRING, but I think this is all going to have
to revert back to the old way to address my other comments.


on file: toolkit/components/places/tests/ipc_test_wrappers/IHistory/historyTest.cpp line 110
> Test_visited_notifies()

Don't understand why you are upper casing all of these.  It makes it a lot
harder to apply a change from one file to both.


on file: toolkit/components/places/tests/ipc_test_wrappers/IHistory/historyTest.cpp line 172
> void
> Test_unregistered_visited_does_not_notify()
> {
>   // This test must have a test that has a successful notification after it.
>   // The Link would have been notified by now if we were buggy and notified
>   // unregistered Links (due to request serialization).
> 
>   // First, we add our test URI to history.
>   // This spec *HAS* to match the one defined in head_test_IHistory_wrap.js
>   nsCOMPtr<nsIURI> testURI(new_test_uri(nsCString("http://mozilla.org/test_unregistered_visited_does_not_notify")));
>   nsCOMPtr<Link> link(new mock_Link(expect_no_visit));
> 
>   // Now, register our Link to be notified.
>   nsCOMPtr<IHistory> history(do_get_IHistory());
>   nsresult rv = history->RegisterVisitedCallback(testURI, link);
>   do_check_success(rv);
> 
>   // Unregister the Link.
>   rv = history->UnregisterVisitedCallback(testURI, link);
>   do_check_success(rv);
> 
>   // If history tries to notify us, we'll either crash because the Link will
>   // have been deleted (we are the only thing holding a reference to it), or our
>   // expect_no_visit call back will produce a failure.  Either way, the test
>   // will be reported as a failure.
> 
>   run_next_test();
> }

Removing this invalidates this test.  We need to have a way to tell the chrome
process to add a visit for us at certain times and then wait for it to
complete (by spinning the event loop).


on file: toolkit/components/places/tests/ipc_test_wrappers/IHistory/historyTest.cpp line None

the ordering of this call is also important.


on file: toolkit/components/places/tests/ipc_test_wrappers/IHistory/historyTest.h line 63
> const Test gTests[] = {
>   TEST(Test_unvisted_does_not_notify_part1), // Order Important!
>   TEST(Test_visited_notifies),
>   TEST(Test_unvisted_does_not_notify_part2), // Order Important!
>   TEST(Test_same_uri_notifies_both),
>   TEST(Test_unregistered_visited_does_not_notify), // Order Important!
>   TEST(Test_new_visit_notifies_waiting_Link),
>   TEST(Test_RegisterVisitedCallback_returns_before_notifying),
>   TEST(Test_observer_topic_dispatched),
> };

I think it is better to declare this array in the other file so we don't have
to write out our method names more than needed.  Having to update two places +
implement the code each time is annoying when you want to add a new test.


on file: toolkit/components/places/tests/ipc_test_wrappers/IHistory/historyTest.h line 79
>   NS_DECL_ISUPPORTS
>     NS_DECL_NSIHISTORYTEST

weird indentation


on file: toolkit/components/places/tests/ipc_test_wrappers/IHistory/historyTest.h line 87
> #define TEST_NAME "IHistory"
> #define TEST_FILE file

I don't think these are needed anymore wit this setup.


on file: toolkit/components/places/tests/ipc_test_wrappers/Makefile.in line 1
> #
> # ***** BEGIN LICENSE BLOCK *****

Please use the right boilerplate here by copying the right one from here:
http://www.mozilla.org/MPL/boilerplate-1.1/


on file: toolkit/components/places/tests/unit_ipc/head_test_IHistory_wrap.js line 63
> XPCOMUtils.defineLazyGetter(this, "NetUtil", function() {
>   Cu.import("resource://gre/modules/NetUtil.jsm");
>   return NetUtil;
> });

any reason why we can't just do the Cu.import?  It's a test, so perf is not
important here.


on file: toolkit/components/places/tests/unit_ipc/head_test_IHistory_wrap.js line 82
>   QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
>                                                Ci.nsISupportsWeakReference])

weird indentation.  Please follow
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsPlacesDBFlush.js#403
as a guide


on file: toolkit/components/places/tests/unit_ipc/head_test_IHistory_wrap.js line 94
>   // add the various visits that we're going to need in History
>   for (let i = 0; i < SPECS.length; i++) { 
>     let newURI = NetUtil.newURI(SPECS[i], null, null);
>     hs.addVisit(newURI, Date.now()*1000, 
>                               null, Ci.nsINavHistoryService.TRANSITION_LINK, 
>                               0, false, 0);

we cannot do this all ahead of time per previous comments.

Comment 12

[Marco Bonardo [:mak]]

(In reply to comment #11)
> > XPCOMUtils.defineLazyGetter(this, "NetUtil", function() {
> >   Cu.import("resource://gre/modules/NetUtil.jsm");
> >   return NetUtil;
> > });
> 
> any reason why we can't just do the Cu.import?  It's a test, so perf is not
> important here.

Unless this is used for sure (thus the lazy getter is useless) i think we should always care to perf in tests, just because if nobody cares, running tests can take longer. Also extensions developers and new developers use existing code as examples of good practice :)
Sure, this is an edge case not providing a large benefit, but just to say not caring about how much tests take is not the rule.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> Unless this is used for sure (thus the lazy getter is useless) i think we
In this case, we always use it.

Comment 14

[Frederic Plourde (:fred23)]

> Removing this invalidates this test.  We need to have a way to tell the chrome
> process to add a visit for us at certain times and then wait for it to
> complete (by spinning the event loop).


> on file: toolkit/components/places/tests/unit_ipc/head_test_IHistory_wrap.js
> line 94
> >   // add the various visits that we're going to need in History
> >   for (let i = 0; i < SPECS.length; i++) { 
> >     let newURI = NetUtil.newURI(SPECS[i], null, null);
> >     hs.addVisit(newURI, Date.now()*1000, 
> >                               null, Ci.nsINavHistoryService.TRANSITION_LINK, 
> >                               0, false, 0);
> 
> we cannot do this all ahead of time per previous comments.


Ok Shawn, here's what I understand from all this.

Addind all the required test URIs at the beginning busts some test invariants. I agree. We need to be able to add visits when needed. Problem is, currently, this is done through <nsINavHistoryService> which is unfortunately *not* accessible from content... that's why I initially chose to add all the required URIs from head_test_IHistorr_wrap.js running on the chrome process. We're kind of stuck.

But... Ben Stover is leading the ongoing "Asynchronous Add Visit" project
https://wiki.mozilla.org/Firefox/Projects/Asynchronous_Add_Visit
His work is going to let us add an async visit from IHistory directly (on either chrome or content process)

So I suggest we hold this bug's progress until he patches/lands his work.
What do you think ?

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> Addind all the required test URIs at the beginning busts some test invariants.
> I agree. We need to be able to add visits when needed. Problem is, currently,
> this is done through <nsINavHistoryService> which is unfortunately *not*
> accessible from content... that's why I initially chose to add all the required
> URIs from head_test_IHistorr_wrap.js running on the chrome process. We're kind
> of stuck.
It's not that it busts some test invariants, but rather it makes the test not test what it is supposed to test.

> So I suggest we hold this bug's progress until he patches/lands his work.
> What do you think ?
Yeah.  Sadly, that means we'll have to refactor these tests to add those visits asynchronously and then continue upon success (which the API doesn't make easy to do).  It might end up being that it's OK though.  We should wait, and re-evaluate at that time.  It may end up being that we need a test-only API that is remoted for this purpose.