User:Andrew Sutherland/MailNews/GlobalDatabase/Review: Difference between revisions

Line 64: Line 64:
<b>(asuth)</b> This was intentional to allow the database name to differ from the binding name.  I think the idea was that the database names would make more sense standalone, whereas the binding name might be less intuitive in some ways.  But I think in retrospect, the ability for them to deviate just adds potential confusion... so yes.
<b>(asuth)</b> This was intentional to allow the database name to differ from the binding name.  I think the idea was that the database names would make more sense standalone, whereas the binding name might be less intuitive in some ways.  But I think in retrospect, the ability for them to deviate just adds potential confusion... so yes.


=== Standard8 ===


==== test directory server debug output ====
<b>(Standard8 [https://bugzilla.mozilla.org/show_bug.cgi?id=450494#c13 c13])</b>
-          } else {
-            dump("Wants directory: "+prop+"\n");
<i>Andrew I don't see why you feel the need to delete this. If you're getting it a
lot, then maybe something is requesting a directory more than it should be (or
we need to fix it). However I see it as a useful tool because if obtaining the
directory is buried in code somewhere, it can be very hard to realise that the
directory isn't being obtained because we haven't provided a test for it at
xpcshell level.</i>
<b>(asuth [https://bugzilla.mozilla.org/show_bug.cgi?id=450494#c14 c14]</b>So, I thought I was the one who added it in a fit of debugging.  I realized
this was not the case when it showed up in the patch, but I left it in because
it seemed like it might be more confusing to someone who didn't understand how
nsIDirectoryServiceProvider and family work.
For example, I was experiencing a problem where popstate.dat could not be
found.  I saw that output, saw the source of the dump, saw that it was throwing
an exception, and assumed that I needed to be providing another directory path
in the handler.  Of course, it turns out that the exception is actually a
normal thing to do and allows the built-in logic to just build off the single
directory path we do provide.  So the debug there led me down the wrong path.
My concern can easily be mitigated by adding a comment to the code with the
reinstatement of the debug.  Does that sound good?
<b>(Standard8 [https://bugzilla.mozilla.org/show_bug.cgi?id=450494#c16 c16])</b>
Yes, maybe a clearer debug statement as well - I think it'd just be good to
highlight if we are getting things wrong.
<b>(asuth status 9/18)</b> I will integrate this into the next test patch update or at commit preparations if that doesn't happen.


== Being Addressed ==
== Being Addressed ==
Confirmed users
360

edits