Add a createAsyncStatement API on mozIStorageConnection

RESOLVED FIXED in mozilla1.9.3a3

Status

()

defect
P1
normal
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: sdwilsh, Assigned: asuth)

Tracking

Trunk
mozilla1.9.3a3
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

222.23 KB, patch
sdwilsh
: review+
Details | Diff | Splinter Review
1.27 KB, patch
Details | Diff | Splinter Review
Reporter

Description

10 years ago
This would allow consumers to create statements that are only able to be executed asynchronously.  This means we wouldn't have to clone the statement upon first use asynchronously, saving some work.
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
sdwilsh, some async connection implementation decision questions:

(Based on previous conversation, it is assumed that a new statement class is the proper thing to do to avoid increasing the complexity of the existing Statement class.)

1) Do you think the initialization of the asynchronous statement should take an (optional?) callback that would receive the result of initializing the statement or should the failure just be implicit in the failure of attempts to executeAsync it.

I lean towards: Yes, optional initialization callback.  My rationale is that it enables the friendliest error reporting, although I would likely just try and re-use mozIStorageStatementCallback rather than introduce a new callback type.


2) What to do about mozIStorageConnection::executeAsync and its dependency on mozIStorageStatement and the ability to assume a static cast to mozilla::storage::Statement for access to getAsynchronousStatementData?

I am currently introducing a new interface, mozIStorageAsyncStatement that does not share an inheritance hierarchy with mozIStorageStatement.  I could:

a) keep the single executeAsync on mozIStorageConnection but change its signature to consume nsISupports and use a QI to mozIStorageStatement/mozIStorageAsyncStatement to determine the proper static downcast.

b) keep the single executeAsync by introducing a new interface that both types of connection implement.  Given the signature and intent of getAsynchronousStatementData, some non-scriptable/non-xpcom trickery could be put into play.

c) just add a new executeAsyncBatch method that only takes async stuff.


3) How to support binding by name before the asynchronous statement has been created and we have the list of column names?

I would lean towards adding a speculative list of column names along the lines of mColumnNames that we use for all queries prior to having the real information available.  We perform a fix-up pass for all indices in the speculative range at execution bind time.

For example, when we do not have mColumnNames yet and are asked for the index of "foo" we would see it is not in the mSpeculativeColumnNames list, add it to the list and return SPECULATIVE | 1.  When we are asked for "bar" we see it is not in the list, add it, return SPECULATIVE | 2.  When we are asked for "foo" again it is in the list so we return SPECULATIVE | 1.

When it comes time to bind those parameters, we see that the column >= SPECULATIVE and use our special fix-up logic.  Our fix-up logic masks out SPECULATIVE and uses that index into mSpeculativeColumnNames to get back to the string "foo" and uses that to get the correct index from mColumnNames.

Locking/memory coherence issues would likely be dealt with by a) locking down the speculative column names after a single row and 2) making sure the sqlite3_stmt initializer dude runs on the async thread then the main thread again in order to define a clear barrier (erring on the side of extra use of the speculative columns), even if we don't generate a callback.
Reporter

Comment 2

10 years ago
(In reply to comment #1)
> 1) Do you think the initialization of the asynchronous statement should take an
> (optional?) callback that would receive the result of initializing the
> statement or should the failure just be implicit in the failure of attempts to
> executeAsync it.
I think we should go for a runtime failure when it is executed.  While it's true that it might be a bit more clear to the consumer if they pass an optional callback  I think it makes sense for the following reasons:
1) Already have runtime errors for binding issues (see below).
2) Already need to notify the mozIStorageStatementCallback if we don't get an optional callback.
3) Because of (2), it'll be simpler to code (and test) if we just have one error handling path.
Summary: while it'd be nice, I think the cost of supporting it greatly outweighs the benefit.

> 2) What to do about mozIStorageConnection::executeAsync and its dependency on
> mozIStorageStatement and the ability to assume a static cast to
> mozilla::storage::Statement for access to getAsynchronousStatementData?
I lean towards (b) and the method should be able to take a mix of both the pure async and sync-async statements.  However, if it ends up being too much work, we'll just go with (c).

> 3) How to support binding by name before the asynchronous statement has been
> created and we have the list of column names?
I think it'd be easier to just not store the column names, and just bind at run time on the background thread.  We could do some magic where on the first lookup we then know all the real column names, but that will add more locking, and I'd rather not do that (especially on a statement).  I think the easiest way to do this is to use nsTHashtable that inserts a class that inherits from nsCStringHashKey and stores a nsCOMPtr<mozilla::storage::Variant> in the data we pass over to the async executioner.  At least, I'm pretty sure we could do this.  Do you see any issue with it?

Also note that the .params magic properties should work on this statement too, although we'd have to disallow enumeration.  We want to to make it dirt simple to convert existing code to this (basically just changing how we create the statement).

> Locking/memory coherence issues would likely be dealt with by a) locking down
> the speculative column names after a single row and 2) making sure the
> sqlite3_stmt initializer dude runs on the async thread then the main thread
> again in order to define a clear barrier (erring on the side of extra use of
> the speculative columns), even if we don't generate a callback.
We'd have to have it run on the calling thread, not the main thread (although that almost always is the main thread).  We'd still have to use a lock here, which sucks a bunch.
(In reply to comment #2)
> > 3) How to support binding by name before the asynchronous statement has been
> > created and we have the list of column names?
> I think it'd be easier to just not store the column names, and just bind at run
> time on the background thread.  We could do some magic where on the first
> lookup we then know all the real column names, but that will add more locking,
> and I'd rather not do that (especially on a statement).  I think the easiest
> way to do this is to use nsTHashtable that inserts a class that inherits from
> nsCStringHashKey and stores a nsCOMPtr<mozilla::storage::Variant> in the data
> we pass over to the async executioner.  At least, I'm pretty sure we could do
> this.  Do you see any issue with it?

I am unclear on how this would impact BindingParams.  Would it:

a) have two internal storage mechanisms, where once you decide to use BindByIndex/BindByName we pick the storage and require you only use index/name binding for the rest of the use?

b) tunnel numeric keys when binding by index or use a more complex hybrid key?


> Also note that the .params magic properties should work on this statement too,
> although we'd have to disallow enumeration.  We want to to make it dirt simple
> to convert existing code to this (basically just changing how we create the
> statement).

Agreed.

 
> > Locking/memory coherence issues would likely be dealt with by a) locking down
> > the speculative column names after a single row and 2) making sure the
> > sqlite3_stmt initializer dude runs on the async thread then the main thread
> > again in order to define a clear barrier (erring on the side of extra use of
> > the speculative columns), even if we don't generate a callback.
> We'd have to have it run on the calling thread, not the main thread (although
> that almost always is the main thread).  We'd still have to use a lock here,
> which sucks a bunch.

As proposed, I don't think a synchronization primitive would be required in storage code because we would use a runnable (and its effective guarantees) to coordinate the calling thread and async thread.

I suggested this approach because it allows us to largely maintain the existing code paths and hypothetical higher efficiency of the list implementation.  It definitely has a higher conceptual difficulty, though.  I'm fine with the hash-table approach.  We can optimize things later should it ever become a major hot-spot.  (And the changes would likely need to be something more drastic anyway.)
Reporter

Comment 4

10 years ago
(In reply to comment #3)
> I am unclear on how this would impact BindingParams.  Would it:
We should be able to handle both, so we'd still have our nsCOMArray of Variants.  The bindByIndex stuff dumps it there, and the bindByName stuff goes into the hash table.  At run time, we should convert everything into indexes, and process that array like we do now.  Not much more in memory overhead here I do not think.

> As proposed, I don't think a synchronization primitive would be required in
> storage code because we would use a runnable (and its effective guarantees) to
> coordinate the calling thread and async thread.
It's possible I have the case of the Monday's, but I don't fully see how it works out still.  Could you maybe sketch the synchronization between the threads in some way (I've found this to be very useful lately)?

> I suggested this approach because it allows us to largely maintain the existing
> code paths and hypothetical higher efficiency of the list implementation.  It
> definitely has a higher conceptual difficulty, though.  I'm fine with the
> hash-table approach.  We can optimize things later should it ever become a
> major hot-spot.  (And the changes would likely need to be something more
> drastic anyway.)
I would prefer to do the simple thing first, and optimize if needed (our hash tables are pretty efficient as I understand things).
(In reply to comment #4)
> It's possible I have the case of the Monday's, but I don't fully see how it
> works out still.  Could you maybe sketch the synchronization between the
> threads in some way (I've found this to be very useful lately)?

pythonic pseudo-code blocks:

class AsyncStatementInitializer:
  def __init__(self, statement):
    self.statement = statement
    self.calling_thread = MAGIC.current_thread()
    async_thread = self.statement.get_connection().get_async_thread()
    async_thread.run(self.run_on_async_thread)

  def run_on_async_thread:
    self.statement.get_async_sqlite3_statement()
    self.calling_thread.run(sef.run_on_calling_thread_afterwards)

  def run_on_calling_thread_afterwards:
    self.statement.have_columns = True

class AsyncStatement:
  def __init__(self):
    self.have_columns = False
    self.column_names = None
    self.speculative_column_names = []
    AsyncStatementInitializer(self)

  # map name to index, populating/reusing speculative columns
  def get_parameter_index(self, name):
    if self.have_columns:
      return self.column_names.index(name)
    elif self.speculative_columns_latched:
      return SPECULATIVE | self.speculative_column_names.index(name)
    else:
      if name in self.column_names:
        return SPECULATIVE | self.speculative_column_names.index(name)
      self.speculative_column_names.append(name)
      return SPECULATIVE | (len(self.speculative_column_names) - 1)

  # called when a statement is asynchronously dispatched
  def latch_speculative_columns(self):
    self.speculative_columns_latched = True

  # called only by the async thread
  def get_async_sqlite_stmt(self):
    if self.async_stmt: return self._async_stmt
    self.async_stmt = sqlite3.make_statement(self.sql)
    self.column_names = sqlite3.tell_me_columns(self.async_stmt)

  # called only by the async thread
  def map_speculative_index_to_real_index(self, index):
    return self.column_names.index(self.speculative_column_names[index]))

  def get_column_count(self):
    if self.have_columns:
      return len(self.column_names)
    else
      return len(self.speculative_column_names)

class BindingParams:
  def __init__(self, statement):
    self.mode = MODE_UNKNOWN
    self.statement = statement
    # we are assuming a growing implementation...
    self.parameters = [None] * statement.get_column_count()

  def bind_by_name(self, name, value):
    self.bind_by_index(self.statement.get_parameter_index(name), value)

  def bind_by_index(self, index, value):
    if index & SPECULATIVE:
      assert(self.mode != MODE_KNOWNCOLUMNS)
      self.mode = MODE_SPECULATIVE
      index &= ~SPECULATIVE
    else:
      assert(self.mode != MODE_SPECULATIVE)
      self.mode = MODE_KNOWNCOLUMNS
    self.parameters[index] = value

  def bind_to_sqlite(self):
    sqlstmt = self.statement.get_async_sqlite3_statement()
    if self.mode == MODE_SPECULATIVE:
      for index, value in enumerate(self.parameters):
        sqlstmt.bind(self.statement.map_speculative_index_to_real_index(index),
                     value)
    else:
      for index, value in enumerate(self.parameters):
        sqlstmt.bind(index, value)



So the deal there is that:

1) The calling thread locks down speculative_column_names before there is ever any code dispatched to the async thread that accesses it.  This makes it safe assuming a sane list implementation.

2) have_column_names is only ever set or modified on the calling thread.

3) Although column_names is written (once) on the async thread and read on both threads, it is only ever read by the calling thread after have_column_names is set to true by the runnable which strictly occurs after column_names is set.  Under the memory models in play, the changes to column_names must be visible to the calling thread at that point.
Reporter

Comment 6

10 years ago
OK, got it and your logic all checks out OK.  My concern now is that we can have different behavior based on thread timings.  For example, a developer could easily write code that doesn't error except on runtime on his machine, but then it is suddenly getting a call time error (on binding).  Or the reverse (which I can see happening easily on tinderbox causing random orange).  Having to handle the error in two places seems less than ideal.
No worries, I'm down with the hash table and its simplicity.
(In reply to comment #2)
> > 2) What to do about mozIStorageConnection::executeAsync and its dependency on
> > mozIStorageStatement and the ability to assume a static cast to
> > mozilla::storage::Statement for access to getAsynchronousStatementData?
> I lean towards (b) and the method should be able to take a mix of both the pure
> async and sync-async statements.  However, if it ends up being too much work,
> we'll just go with (c).

[quoting that:]

(In reply to comment #1)
> b) keep the single executeAsync by introducing a new interface that both types
> of connection implement.  Given the signature and intent of
> getAsynchronousStatementData, some non-scriptable/non-xpcom trickery could be
> put into play.

I'm having trouble accomplishing 'b' without screwing existing C++ code:

* XPCOM does not support multiple inheritance. C++ does, but C++ code talking directly about interfaces is going to need to explicitly QueryInterface to the other inherited interface.  (For JS, XPConnect will automatically coerce to the type signature of method calls, which could solve problems.)

* mozIStorageStatement inherits from mozIStorageValueArray.

* mozIStorageValueArray is referenced elsewhere, most problematically by mozIStorageFunction/mozIStorageAggregateFunction.  (Less problematically by mozIStorageRow, at least thus far.)  If not for this, we could have mozIStorageStatement inherit from a common base interface along with his async brother; we would copy the contents of the mozIStorageValueArray into mozIStorageStatement (or an interposed interface) so that C++ code could compile cleanly.


I think we can accomplish an improved 'c' though.  Namely, we can implement a new executeFancyBatch method that takes both.  We introduce the new base statement interface primarily as a tagging interface and have mozilla::storage::statement also inherit from that too.

Unless I hear otherwise, I will name things like so:
* mozIStorageBaseStatement
* mozIStorageAsyncStatement
* mozIStorageConnection::executeFancyBatch

If you can think of a way to save 'b', or simply don't care about C++ consumers (unlikely, given the existence of places), let me know...
Reporter

Comment 9

10 years ago
(In reply to comment #8)
> * mozIStorageValueArray is referenced elsewhere, most problematically by
> mozIStorageFunction/mozIStorageAggregateFunction.  (Less problematically by
> mozIStorageRow, at least thus far.)  If not for this, we could have
> mozIStorageStatement inherit from a common base interface along with his async
> brother; we would copy the contents of the mozIStorageValueArray into
> mozIStorageStatement (or an interposed interface) so that C++ code could
> compile cleanly.
I've run into this before, and it is painful.  I fully support making mozIStorageBaseStatement (what is going to be on it exactly?) and having mozIStorageStatement and mozIStorageAsyncStatement inherit from it.  I'm OK with pulling the stuff off of mozIStorageValueArray.  We can still QI to it so we don't break folks, but we'll change our inheritance.

> * mozIStorageConnection::executeFancyBatch
Why do we need this?
(In reply to comment #9)
> > * mozIStorageConnection::executeFancyBatch
> Why do we need this?

We don't.  I had (happily! :) forgotten some C++ multiple-inheritance nuances that make things work out.
It's review payback time!

This is straight from my patch queue, so just 3 lines of context; let me know if you need more.  This is happy against mozilla-central trunk.  "Happy" is a relative term where the number of JS places tests that fail remains constant before and after the patch is applied.  (I am running straight trunk with my revised asyncClose patch on bug 496019 plus your places asyncClose patch on bug 526601.  It's a xulrunner debug build rather than a firefox build; maybe that explains things?)  Storage tests obviously pass.

At the end of the day, the only compatibility glitch seems to be that there is no way to escape the need to do:
-    mozIStorageStatement *stmts[] = {
+    mozIStorageBaseStatement *stmts[] = {

So there are a few of those in there for places.  The other places change required is where some code assumed that mozIStorageStatement inherited from mozIStorageValueArray.  Amusingly, one of the places I ended up changing actually was doing the right thing with an explicit QI but I took it out because it was easier to change the signature of the helper function involved to just consume mozIStorageStatement.

Test-wise, I introduced the promised C++ test that wraps the SQLite mutexes so we can finally be confident our async stuff is truly async.  I also made the statement executeAsync tests run a pass for both mozIStorageStatement and mozIStorageAsyncStatement (with some sync/async-specific tests).  The connection executeAsync tests were just modified to use a mixture of both types of statements.

I just pushed the required stack of patches to the try server; haven't done it previously.  Time-decaying link for the lazy but prompt:
http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry

I presume this will require SR but I think it makes sense to defer that until you are solid on all the major decisions and the way they turned out.
Attachment #426215 - Flags: review?(sdwilsh)
the try server suggests I should set up pymake on my fancy windows build box so I can actually use it.  I pushed-to-try this minor change to see if it gets any further.  If windows fails again I'll be sure to fix up windows locally tomorrow.
Attachment #426215 - Attachment is obsolete: true
Attachment #426219 - Flags: review?(sdwilsh)
Attachment #426215 - Flags: review?(sdwilsh)
Reporter

Comment 13

10 years ago
This is going to take a while to review, but here are some initial higher level comments:
1) mozIStorageValueArray functions added to mozIStorageStatement should probably be added at the end of the idl.  Doing so at the beginning feels strange to me.
2) All the idls you touch need to have their uuid reved.
3) You've added a bunch of noscript methods which are basically our private methods used to communicate bits of data between classes (like getStatementData).  I have a few concerns with this:
a) native code consumers could start to abuse this (I'm mostly worried about add-ons).
b) we were previously removing all instances sqlite3 data objects in our idls, which this sorta reverses.
c) at the very least, I think you meant most of them to be [notxpcom]
4) We should mark bindStringParameter and friends on mozIStorageBaseStatement as [deprecated], and have mozIStorageBaseStatement inherit from mozIStorageBindingParams.  Those binding functions are better from script and C++.
5) #defines around header files should be in the form of mozilla_storage_filename_h_

More to come later.
(In reply to comment #13)
> 3) You've added a bunch of noscript methods which are basically our private
> methods used to communicate bits of data between classes (like
> getStatementData).  I have a few concerns with this:
> a) native code consumers could start to abuse this (I'm mostly worried about
> add-ons).
> b) we were previously removing all instances sqlite3 data objects in our idls,
> which this sorta reverses.
> c) at the very least, I think you meant most of them to be [notxpcom]

Yes, I'm not crazy about these exposures either, and indeed notxpcom is more what I was thinking of, but it was unclear what the true semantics of notxpcom are given the suboptimal XPCOM documentation.

The other main option I considered was introducing a private IDL interface for the base statement that the sync/async statements would implement so that we could just QI across from the public interface.  Having a straight-up abstract C++ class was also considered to try and totally shield it from prying eyes, but I don't think we can use dynamic_cast and the static_cast would require us to identify the actual implementation class via QI to mozIStorageStatement/mozIStorageAsyncStatement which seemed extra ugly.

The overall goal was to try and avoid cluttering up the code with conditional logic based on QI'ing to find out the implementation type.  In some cases copy-paste-modify a sync-specific class to introduce an async-aware variant was also possible, but this also seemed undesirable in terms of creating duplicate code and generally further bloating the storage implementation.

If you have a specific course of action you would like to have me follow, please let me know.  I'm not wedded to things as they are, I just wanted to have something working to start from.
Reporter

Comment 15

10 years ago
The abstract C++ class can work, and we can hook it up to work with QI then as well instead of static_cast.  I'm not totally clear on how this would mean we'd have to worry about if it's async or not though.  Could you elaborate?
(In reply to comment #15)
> The abstract C++ class can work, and we can hook it up to work with QI then as
> well instead of static_cast.  I'm not totally clear on how this would mean we'd
> have to worry about if it's async or not though.  Could you elaborate?

I was assuming it was verboten to add random things to the QI mapper or things not born of the xpidl compiler.  Is there an in-tree example or documentation you can point me at?  I can read the XPCOM implementation stuff and figure it out, but since this is hopefully my only foray into deep XPCOM for the next year it'd be more time efficient if I didn't.
Reporter

Comment 17

10 years ago
With two macros, it's actually really easy.  See bug 461199 part 2, and look for IHistory.h for an example.  When you want to use that, you just place it in an nsCOMPtr<IHistory>.  We also do this for nsIContent and a few other things.
Duplicate of this bug: 528802
Ugh, so you may have noticed a red flag in the patch where I mention XPCVariant triggering asserts because it's not thread-safe (and my avoidance of that specific case).  It turns out that it is indeed a more comprehensive problem.

Right now (prior to this patch), the only way to get an XPCVariant into the system is on a BindingParams instance.  Since that BindingParams (and its owning array) instance must be surfaced to XPConnect in order to use it, there is a high probability that the main thread will still have a reference to the BindingParams (or its owning array) when StatementData::finalize ditches its reference to the BindingParamsArray.

When I made mozIStorageBaseStatement inherit from mozIStorageBindingParams I introduced a way for JS to create an XPCVariant without gaining any references to the BindingParams/BindingParamsArray.  So now we have reliable XPCVariant assertion anger.

While I don't think the cycle collector itself is in any great danger from the problem since the storage stuff isn't cycle-collector aware, it seems like there could be reference count danger (on platforms where the non-PR_Atomic stuff doesn't net out to the same thing) resulting in a memory leak.  On the upside, there are no in-tree users of the API.

Having said that, I think we still need to stop messing with XPCVariant reference counts off the calling thread because the assertions are troublesome.

The straightforward solution is to only release the StatementDataArray from the calling thread which implies having the completion notification hold onto a reference to the AsyncExecuteStatements and always be issued (or having AsyncExecuteStatements just re-marshal itself back to the calling thread).

The upside to this is that it could provide us with an invariant that we only ever release things on the calling thread, allowing us to discard some NS_ProxyRelease contortions we go through.  The downside is a potential for an increased memory footprint in some situations (although less chance of thread-local allocator cache bloat on the async thread).
I believe this addresses all raised concerns so far.  I did what I proposed in the last comment and always send the completion notifier and have him hold onto the async exec object.  I have not followed that decision to its logical end (removing lots of proxied releases and comments) since I think feedback is required before committing to that decision.

I built a firefox debug build this time and the places xpcshell tests all pass.  I also just pushed to the try server.  Hopefully that fares better this time since there should no longer be any stale UUIDs and maybe the trunk is happier.
Attachment #426219 - Attachment is obsolete: true
Attachment #426475 - Flags: review?(sdwilsh)
Attachment #426219 - Flags: review?(sdwilsh)
Depends on: 496019
Blocks: 545621
Blocks: 545625
Try server builds failed with a crash on StatementData::finalize because mStatement was being used when null because the test machines were slow enough that the 'cancel' actually got a chance to cancel.  I resolved this problem and added a new unit test that tests bad SQL with createAsyncStatement which would reliably cause the same underlying problem.

I've pushed a new try server build.

In order to attempt to address the problem that our cancellation tests are unreliable I am going to add another C++ test case that addresses the problem by wedging the async thread prior to dispatching.  I can't think of an easy way to accomplish the same thing from an xpcshell test without having to build and surface a new XPCOM helper class that would also be available from non-test situations (bad).
Attachment #426475 - Attachment is obsolete: true
Attachment #426622 - Flags: review?(sdwilsh)
Attachment #426475 - Flags: review?(sdwilsh)
Add
- real cancellation test (cancel before the statement executes) in C++
- definite cancellation after the statement has executed test in JS

We still do not test cancellation while a statement is in progress.  We could test this by implementing a custom SQL function and sending a SELECT returning 2 rows with the function used in a style so that it is invoked once for each result.  We add wedging logic to the function and we're good to go.

However, I think that would be gratuitous given that the code involved in the mid-execution cancellation case is fundamentally the same as the error case in the same loop.
Attachment #426622 - Attachment is obsolete: true
Attachment #426634 - Flags: review?(sdwilsh)
Attachment #426622 - Flags: review?(sdwilsh)
Windows is having build problems related to:
test_true_async.cpp(228) : error C3083: '{ctor}': the symbol to the left of a '::' must be a type

Where that line is presumably about the attempted constant access off of nsIEventTarget::NS_DISPATCH_NORMAL.

Otherwise things are believed to be happy.  I'll resolve the windows issue over the weekend.
Attachment #426634 - Attachment is obsolete: true
Attachment #426673 - Flags: review?(sdwilsh)
Attachment #426634 - Flags: review?(sdwilsh)
Reporter

Comment 24

10 years ago
Been going through this review a bit today.  I'll note that you did some hg copys in some places, and it actually made the diff a lot harder to read than just doing a brand new hg add.  If you could fix that in subsequent patches, that'd be awesome.
Reporter

Comment 25

10 years ago
Another question:  what about removing the base statement interface, and just have the sync statement inherit from the async one?  It has all the same methods, just some more for synchronous operation, right?  That may make doing different things based on different types of statements harder though (I have not yet gotten that far in the patch).
Reporter

Comment 26

10 years ago
I didn't get although through page one, but the website was starting to act weird (JS errors) and I need to start to prep for an interview.  Nevertheless, these comments should be useful to you:
http://reviews.visophyte.org/r/426673/

on file: storage/public/Makefile.in line 57
> 	mozIStorageBaseStatement.idl \
> 	mozIStorageStatement.idl \
> 	mozIStorageStatementWrapper.idl \
> 	mozIStorageAsyncStatement.idl \

These were not really alphabetized before, so I'm not sure about your ordering
choices here.


on file: storage/public/mozIStorageBaseStatement.idl line 16
>  * The Original Code is Oracle Corporation code.
>  *
>  * The Initial Developer of the Original Code is
>  *  Oracle Corporation
>  * Portions created by the Initial Developer are Copyright (C) 2004
>  * the Initial Developer. All Rights Reserved.

I think that this is all really new.  While it is true that the API is being
pulled off of bits of things that Oracle did, it's a new interface (and I'm
going to have comment changes for you) such that this is now Mozilla code.


on file: storage/public/mozIStorageBaseStatement.idl line None

nit: s/null/NULL/

Elaborating why it is useful to JS consumers would be useful too:
1) if you want to close the database
2) if you use async statements


on file: storage/public/mozIStorageBaseStatement.idl line None

Can you convert this to the proper javadoc style comment please?


on file: storage/public/mozIStorageBaseStatement.idl line None

nit: I assume you formatted this like this due to line length constraints. 
However, I'd prefer it to be formatted like executeAsync.


on file: storage/public/mozIStorageBaseStatement.idl line None

This is probably my bad, but we need to be using @return instead of @returns
(no s).  Only going to point this out once.


on file: storage/public/mozIStorageBaseStatement.idl line None

nit: @note (bet this is my fault again too!)


on file: storage/public/mozIStorageBaseStatement.idl line None

You should also state that it is only possible for *synchronous*
mozIStorageStatements.


on file: storage/public/mozIStorageBaseStatement.idl line None

So this is new.  Can you explain why it has been added?  We don't really have
a way to compare mozIStorageConnection instances other than pointers in c++.


on file: storage/public/mozIStorageFunction.idl line 42
> // At least one includer of this file practiced bad header hygiene, so we
> // need to import the idl.

Care to elaborate?


on file: storage/public/mozIStorageAsyncStatement.idl line 19
>  * Mozilla Messaging, Inc.

At MoCo we are supposed to attribute copyright to the Mozilla Foundation, so I
imagine it's the same for you.


on file: storage/public/mozIStorageStatement.idl line None

I'm not sure this bit is useful to keep here.


on file: storage/public/mozIStorageStatement.idl line None

I'd rather have it formatted like we do for interface sections in cpp files:
///////////////////
//// mozIStorageValueArray


on file: storage/public/mozIStorageStatement.idl line None

... and then you don't need this.


on file: storage/public/mozIStorageValueArray.idl line 47
>  * IMPORTANT! The introduction of the pure-async mozIStorageAsyncStatement
>  * interface has required us (to address backwards compatibility concerns)
>  * to copy the contents of this interface into mozIStorageStatement. If you are
>  * thinking about making changes to this interface, you probably also want to
>  * make the changes on mozIStorageStatement too. The concrete Statement
>  * implementation implements both that interface and this one, so it makes sense
>  * to try and keep things lined up.

As long as mozilla::storage::Statement inherits from (and QIs to)
mozIStorageValueArray, which is should still, it'll be a compiler error and we
won't need this comment.


on file: storage/src/IStorageBaseStatementInternal.h line 83
>   NS_IMETHOD GetAsyncStatement(PRInt32 *_rc, sqlite3_stmt **_stmt) = 0;

I think we want this instead:
virtual int GetAsyncStatement(sqlite3_stmt **_stmt) = 0;


on file: storage/src/IStorageBaseStatementInternal.h line None

virtual StatementData &GetAsynchronousStatementData() = 0;


on file: storage/src/IStorageBaseStatementInternal.h line None

This probably doesn't even need to be virtual, and you can add the
implementation to this if you wanted (but I'm not going to require it). 
However, I'd suggest this:
virtual already_AddRefed<mozIStorageBindingParams>
NewBindingParams(mozIStorageBindingParamsArray *aOwner) = 0;


on file: storage/src/IStorageBindingParamsInternal.h line 73
>   NS_IMETHOD Bind(sqlite3_stmt *aStatement, mozIStorageError **_err) = 0;

virtual already_AddRefed<mozIStorageError> Bind(sqlite3_stmt *aStatement) = 0;


on file: storage/src/mozStorageAsyncStatementExecution.h line 132
>   bool bindExecuteAndProcessStatement(sqlite3_stmt *aStatement,
>                                       StatementData &aData,
>                                       bool aLastStatement);

Update the documentation please.


on file: storage/src/mozStorageAsyncStatementExecution.cpp line 563
>       // And notify.
>       (void)notifyError(rc, ::sqlite3_errmsg(mStatements[i]));
>       break;

In order to not make bug 490881 any worse than it currently is, you want to
hold the database lock for a duration of this time.


on file: storage/src/mozStorageBindingParams.h line 112
>  * Adds late resolution of named parameters so they don't get resolved until we
>  * we try and bind the parameters on the async thread.  We also stop checking

nit: we at both end of line and beginning of next


on file: storage/src/mozStorageBindingParams.h line 134
>   nsInterfaceHashtable<nsCStringHashKey, nsIVariant> mNamedParameters;

Generally speaking, it's better to use nsTHashtable directly for efficiency. 
You'd have to make your own hashing function for that in order to store the
nsIVariant with it though.  You can see attachment 423641 [details] [diff] [review] for an example on
how to do this.


on file: storage/src/mozStorageBindingParams.cpp line 151
> AsyncBindingParams::AsyncBindingParams(
>     mozIStorageBindingParamsArray *aOwningArray)

nit: closing paren on next line


on file: storage/src/mozStorageBindingParams.cpp line 201
>       new Error(SQLITE_RANGE,
>                 PromiseFlatCString(aName +
>                                    NS_LITERAL_CSTRING(
>                                      " is not a valid named parameter."))
>                   .get());

I think this would be clearer if you used a temporary auto string to build
your string, then wrap that variable in PromiseFlatCString.  It's a shame we
have to use nsACString here since nsCString is always a flat string.
(In reply to comment #25)
> Another question:  what about removing the base statement interface, and just
> have the sync statement inherit from the async one?  It has all the same
> methods, just some more for synchronous operation, right?  That may make doing
> different things based on different types of statements harder though (I have
> not yet gotten that far in the patch).

My motivation wasn't to make it easier to tell what type of statement it is, just to allow each fundamental type of statement to have methods specialized to it.  Since mozIStorageAsyncStatement is empty right now except for the 'params' comment, that's currently not an issue.  And indeed I have a hard time imagining such a thing in the future.  It'll just be annoying and potentially dangerous to split out later if such a need does arise.
(In reply to comment #26)
> on file: storage/public/mozIStorageBaseStatement.idl line None
> 
> So this is new.  Can you explain why it has been added?  We don't really have
> a way to compare mozIStorageConnection instances other than pointers in c++.

I exposed the statement's owner because I couldn't think of a good reason why we weren't exposing it.  My hypothetical use case was debugging code; why pass around a statement and a connection when you can just pass around a statement?  I'm not sure statements or connections are introspectable enough that such a debugging support library is likely to ever crop up, though.

Except for the internal use (which does not require XPCOM) which is clearly required, I don't think it's necessary and am fine with taking it out again.
Reporter

Comment 29

10 years ago
Are you building on top of bug 496019?  (if so, do you want to land it?)

(In reply to comment #27)
> My motivation wasn't to make it easier to tell what type of statement it is,
> just to allow each fundamental type of statement to have methods specialized to
> it.  Since mozIStorageAsyncStatement is empty right now except for the 'params'
> comment, that's currently not an issue.  And indeed I have a hard time
> imagining such a thing in the future.  It'll just be annoying and potentially
> dangerous to split out later if such a need does arise.
OK, let's keep it.

(In reply to comment #28)
> Except for the internal use (which does not require XPCOM) which is clearly
> required, I don't think it's necessary and am fine with taking it out again.
I think I'd prefer it removed.
(In reply to comment #29)
> Are you building on top of bug 496019?  (if so, do you want to land it?)

Yes, I am building on top of it.  I was planning to land that fix when we land this fix.  (I realize this has reviewboard fallout and I'll make sure to fix-up the reviewboard diff next time to use the basis patches next time if it's still an issue.  It can be done manually.)

I am willing to land it earlier once I absolve it of blame... my try server pushes have resulted in weird consistent mochitest fallout in places I would not expect it (layout tests?) and where none of the other people's try pushes in the same time period seemed to trigger.  I need to make sure that it wasn't 496019 (and its places follow-on) causing the problem but instead my changes.

I should be able to do another iteration and look into the mochitest oddness Monday or Tuesday.
Reporter

Comment 31

10 years ago
And the rest.  I'd like to see an updated patch with the comments so far addressed, and then I'll do another review.

http://reviews.visophyte.org/r/426673/

on file: storage/src/mozStorageConnection.cpp line 647
> Connection::ExecuteAsync(mozIStorageBaseStatement **aStatements,
>                          PRUint32 aNumStatements,
>                          mozIStorageStatementCallback *aCallback,
>                          mozIStoragePendingStatement **_handle)
> {
>   nsTArray<StatementData> stmts(aNumStatements);
>   for (PRUint32 i = 0; i < aNumStatements; i++) {
>     nsCOMPtr<IStorageBaseStatementInternal> stmt = 
>       do_QueryInterface(aStatements[i]);

Can't we just pass in a IStorageBaseStatementInternal here?


on file: storage/src/mozStorageAsyncStatement.h line 120
>   /**
>    * The SQL string as passed by the user.  We store it because we create the
>    * async statement on-demand on the async thread.  We could throw it away
>    * if we used a dedicated initialization helper; we are avoiding that extra
>    * complexity.

Drop the bit about throwing it away.


on file: storage/src/mozStorageStatement.h line None

nit: commas inline with the colon on the next line.  Easy to add new
interfaces without changing blame


on file: storage/src/mozStorageAsyncStatement.cpp line 103
>     static_cast<AsyncStatement *>(mStatement.get())->internalFinalize();

Can you split this line into two lines please?  I'm finding it being a bit
confusing (might be a case of the fridays, but nevertheless...), so I'd like a
temp variable please.


on file: storage/src/mozStorageAsyncStatement.cpp line 315
>   mDBConnection->threadOpenedOn->IsOnCurrentThread(&onCallingThread);

This returns something.  (void) it please


on file: storage/src/mozStorageAsyncStatement.cpp line 374
> AsyncStatement::GetAsyncStatement(PRInt32 *_rc, sqlite3_stmt **_stmt)

Can we add debug-only code here to make sure we are running on the async
thread please.


on file: storage/src/mozStorageAsyncStatement.cpp line 588
> NS_IMETHODIMP
> AsyncStatement::NewBindingParamsArray(mozIStorageBindingParamsArray **_array)

It actually seems like this method could be implemented on the common base
class since it is the same, right? (The internal base class, not the external
one).


on file: storage/src/mozStorageAsyncStatement.cpp line 601
> nsresult
> AsyncStatement::ExecuteAsync(mozIStorageStatementCallback *aCallback,

This method can also go to the base class.


on file: storage/src/mozStorageAsyncStatement.cpp line 623
> NS_IMETHODIMP
> AsyncStatement::GetOwner(mozIStorageConnection **_owner)
> {
>   NS_ADDREF(*_owner = mDBConnection);
>   return NS_OK;
> }

When you deCOMtaminate this, it can probably just return a raw pointer
actually.


on file: storage/src/mozStorageAsyncStatement.cpp line None

To the common base class!


on file: storage/src/mozStorageStatement.cpp line 805
>   return BindByIndex(aIndex, value);
> }
> 
> NS_IMETHODIMP
> AsyncStatement::BindNullByIndex(PRUint32 aIndex)

I think this can probably live on the base class too
(IStorageBaseStatementInternal).

You can probably make it just StorageBaseStatementInternal too since it is no
longer just an interface.  The code sharing in the common cases will be nice
to have I think.


on file: storage/src/mozStorageAsyncStatementJSHelper.cpp line 126
>   AsyncStatement *stmt = static_cast<AsyncStatement *>(
>                            static_cast<mozIStorageAsyncStatement *>(
>                              aWrapper->Native()));

Use a temporary here please.


on file: storage/src/mozStorageAsyncStatementJSHelper.cpp line 132
>     nsCOMPtr<mozIStorageAsyncStatement> isStatement(
>                                          do_QueryInterface(aWrapper->Native()));

Use a temporary pointer here please.


on file: storage/test/test_true_async.cpp line 19
>  * Mozilla Messaging, Inc.

nit: Mozilla Foundation


on file: storage/test/test_true_async.cpp line None

nit: /**


on file: storage/test/test_true_async.cpp line None

<3


on file: storage/test/test_true_async.cpp line None

Note that we could also just register a SQL function and get the thread there
for sure (as long as our SQL statement calls it).  Your call if you want to
change your logic.


on file: storage/test/test_true_async.cpp line None

how about do_check_ok?


on file: storage/test/test_true_async.cpp line None

The two comments here seem inconsistent.


on file: storage/test/test_true_async.cpp line None

See previous comment about a non-hacky way to do this.


on file: storage/test/test_true_async.cpp line None

//////////...
//// Tests
Reporter

Updated

10 years ago
Attachment #426673 - Flags: review?(sdwilsh) → review-
In regards to turning IStatementBaseInternal to StatementBaseInternal and pushing duplicate code from Statement and AsyncStatement into that common class, how do you want to deal with the inheritance issues?

Namely, (I)StatementBaseInternal only currently inherits from nsISupports and so implementations of things from mozIStorageBaseStatement would not technically be implementing the same signature.

Options would seem to be:

1) Use some C++ magic that I have either forgotten or never learned to place the StatementBaseInternal implementations of the methods in the implementations' vtable for mozIStorageBaseStatement without any intermediary.  (Sadly, all my good C++ reference books like the standard are in the US.)

2) Use StatementBaseInternal as a mix-in. Have StatementBaseInternal implement the methods and define helper #defines that just proxy the calls over to its implementations.

3) Do what you previously proposed and make mozIStorageBaseStatement mozIStorageAsyncStatement (losing a purely async interface), then do the following to avoid a diamond:
a) StatementBaseInternal C++ subclasses mozIStorageStatement, implements only shared methods and remains abstract.
b) Statement subclasses StatementBaseInternal and has its QI admit the mozIStorageStatement support.
c) AsyncStatement subclasses StatementBaseInternal but has its QI claim that it does not support mozIStorageStatement.  Fill all the sync methods with NOPs (or have the superclass implement the NOPs).

I can't currently think of any other ways to avoid the diamond situation and still have mozIStorageStatement inherit from a common base interface implemented by the async statement.
(In reply to comment #30)
> I am willing to land it earlier once I absolve it of blame... my try server
> pushes have resulted in weird consistent mochitest fallout in places I would
> not expect it (layout tests?) and where none of the other people's try pushes
> in the same time period seemed to trigger.  I need to make sure that it wasn't
> 496019 (and its places follow-on) causing the problem but instead my changes.

For reference, I was seeing what is described here:
https://bugzilla.mozilla.org/show_bug.cgi?id=539904#c3

And that is the case just with the asyncClose patches.
I've either addressed your concerns or commented on them on the review:
http://reviews.visophyte.org/r/426673/

This patch builds on the asyncClose patch but the patch is not landed; I am going to try and fix up the review by telling it about the base patch...
Attachment #426673 - Attachment is obsolete: true
Attachment #429037 - Flags: review?(sdwilsh)
Okay, I think I convinced it to play ball; the review should be assuming the asyncClose patch but only showing our changes on top of that.  Magic review links / review queues should work, but here it is anyways:

http://reviews.visophyte.org/r/429037/diff/#index_header
Reporter

Comment 36

9 years ago
(In reply to comment #32)
Let's go with option 2.  That's what the HTML DOM stuff does:
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLAnchorElement.cpp#342
Attachment #429037 - Attachment description: v3 make requested changes except for shared base logic pending comment 23 response → v3 make requested changes except for shared base logic pending comment 23 response http://hg.mozilla.org/try/rev/db44196e41c4
Reporter

Comment 37

9 years ago
Comment on attachment 429037 [details] [diff] [review]
v3 make requested changes except for shared base logic pending comment 23 response http://hg.mozilla.org/try/rev/db44196e41c4

http://reviews.visophyte.org/r/429037/

on file: storage/public/mozIStorageBaseStatement.idl line 80
>    * @param aParamIndex 0-based index, 0 corresponding to the first numbered
>    *     argument or "?1".
>    * @param aValue Argument value.

nit: description on a new line after the argument name please


on file: storage/public/mozIStorageBaseStatement.idl line None

nit: indentation is off here


on file: storage/public/mozIStorageBaseStatement.idl line None

this is sorta weird since we don't format it like this anywhere else


on file: storage/public/mozIStorageConnection.idl line 156
>    * @param aSQLStatement  The SQL statement to execute
>    *

nit: desc on new line


on file: storage/public/mozIStorageStatement.idl line 127
>    * @return a boolean indicating whether there are more rows or not;
>    * row data may be accessed using mozIStorageValueArray methods on
>    * the statement.

nit: since you are touching this, have the new lines line up with "a" after
"@return"


on file: storage/public/mozIStorageStatement.idl line 190
>    * Obtain a value for the given entry (column) index.
>    * Due to SQLite's type conversion rules, any of these are valid
>    * for any column regardless of the column's data type.  However,
>    * if the specific type matters, getTypeOfIndex should be used
>    * first to identify the column type, and then the appropriate
>    * get method should be called.
>    *
>    * If you ask for a string value for a NULL column, you will get an empty
>    * string with IsVoid set to distinguish it from an explicitly set empty
>    * string.

nit: not proper java doc style


on file: storage/public/mozIStorageStatement.idl line None

nit: should be @note


on file: storage/src/IStorageBaseStatementInternal.h line 82
>    * @return The sqlite result code for creating the statement if created,
>    *             SQLITE_OK if creation was not required.

nit: s/sqlite/SQLite/ and second row is indented wrong


on file: storage/src/IStorageBaseStatementInternal.h line None

nit: ) = 0; on new line


on file: storage/src/IStorageBindingParamsInternal.h line 66
>    * @param[in] aStatement
>    *            The statement to bind our data to.
>    * @param[out] _err
>    *             nsnull on success, or a mozIStorageError object if an error
>    *             occurred.

these were not updated with the method change


on file: storage/src/mozStorageAsyncStatement.h line 149
>   friend class AsyncStatementJSHelper;
>   friend class AsyncStatementParams;

Do we need these two if we make cleanupJSHelpers public?


on file: storage/src/mozStorageAsyncStatement.h line None

nit: space after //


on file: storage/src/mozStorageAsyncStatement.cpp line 122
> NS_IMPL_CI_INTERFACE_GETTER4(
>   AsyncStatement,
>   mozIStorageAsyncStatement,
>   mozIStorageBaseStatement,
>   mozIStorageBindingParams,
>   mozilla::storage::IStorageBaseStatementInternal
> )

We should probably put the commas up front to preserve blame (like we do in
constructors)


on file: storage/src/mozStorageAsyncStatement.cpp line None

nit: indentation is off


on file: storage/src/mozStorageAsyncStatement.cpp line None

nit: wrong indentation here


on file: storage/src/mozStorageAsyncStatement.cpp line None

The more I look at this, the more I think we don't need to return an
already_AddRefed, and maybe should just do a raw pointer.


on file: storage/src/mozStorageAsyncStatement.cpp line None

nit: brace please


on file: storage/src/mozStorageAsyncStatementExecution.h line 250
>    *
>    * In order for the deadlock detector to work, this must be a reference to the
>    * exact SQLiteMutex used by the Connection instance.

I'm not sure this is needed.


on file: storage/src/mozStorageAsyncStatementExecution.cpp line 353
>       // Even though we have the connection lock it is possible for other
>       // connections or same connection statements to be fighting us.  Since we
>       // cannot advance we must yield so that others can do so.  (Even if it's
>       // another connection that is the problem, our main thread can perform
>       // operations that will succeed despite requiring the mutex.)

Not sure how other connections could be fighting us.  Is this actually true?


on file: storage/src/mozStorageAsyncStatementExecution.cpp line 372
>     // The 'flapping' on the way out is okay given this path is uncommon.

I don't understand this comment at all.


on file: storage/src/mozStorageAsyncStatementExecution.cpp line 574
>     {
>       // lock the sqlite mutex so sqlite3_errmsg cannot change

nit: move these onto the same line please


on file: storage/src/mozStorageAsyncStatementExecution.cpp line None

I think I know what you mean this time around, but a more clear comment would
be ideal.


on file: storage/src/mozStorageBindingParams.h line 58
> class BindingParams : public mozIStorageBindingParams,
>                       public IStorageBindingParamsInternal

nit: common on second line, in line with ":"


on file: storage/src/mozStorageBindingParams.cpp line 158
> AsyncBindingParams::AsyncBindingParams(
>     mozIStorageBindingParamsArray *aOwningArray
>     )

nit: indent is too far, and do not indent closing paren


on file: storage/src/mozStorageBindingParams.cpp line 239
> NS_IMPL_THREADSAFE_ISUPPORTS2(
>   BindingParams,
>   mozIStorageBindingParams,
>   IStorageBindingParamsInternal

nit: commas at col 0 please


on file: storage/src/mozStorageBindingParams.cpp line 250
> BindingParams::bind(sqlite3_stmt * aStatement)

nit: don't think you actually want this change


on file: storage/src/mozStorageBindingParams.cpp line 411
>   // we do not check the index value because we do not know

More descriptive comment with better grammar would be useful here.


on file: storage/src/mozStoragePrivateHelpers.h line 122
> bool
> bindJSValue(JSContext *aCtx,
>             mozIStorageBindingParams *aParams,
>             const nsACString &aName,
>             jsval aValue);

nit: for headers, return type is same line as declaration (like above
methods).


on file: storage/src/mozStoragePrivateHelpers.cpp line 190
>     (void)aParams->BindInt32ByName(aName, v);

I know we didn't in the old function, but maybe we should check the return
value of Bind* functions and return false if it failed?

Also, what about making a new method that converts a jsval to an nsIVariant
(you may be able to steal/reuse code here from xpconnect) and then just use
BindByName/BindByIndex for bindJSValue?


on file: storage/src/mozStorageStatement.h line 71
>   // we do not NS_DECL_MOZISTORAGEVALUEARRAY because its interface is a subset
>   // of mozIStorageStatement and so we already get all those prototypes.

What if we do this:
// NS_DECL_MOZISTORAGEVALUEARRAY (methods in mozIStorageStatement)


on file: storage/src/mozStorageStatementData.h line 66
>   , mStatementOwner(aStatementOwner)

mStatementOwner(do_QueryInterface(aStatementOwner)) and assert that it is not
null in the constructor.


on file: storage/src/mozStorageStatementData.h line 86
>       nsCOMPtr<IStorageBaseStatementInternal> internalOwner =
>         do_QueryInterface(mStatementOwner);

...and then you don't need to QI here.


on file: storage/src/mozStorageStatementData.h line 103
>     nsCOMPtr<IStorageBaseStatementInternal> internalOwner =
>       do_QueryInterface(mStatementOwner);

...or here.


on file: storage/src/mozStorageStatementData.h line 155
>   nsCOMPtr<mozIStorageBaseStatement> mStatementOwner;

...and change this to IStorageBaseStatementInternal


on file: storage/src/mozStorageStatementJSHelper.cpp line 78
>                       static_cast<mozIStorageStatement *>(wrapper->Native()));

nit: indent only one level, and put closing parent on new line please


on file: storage/src/mozStorageStatementJSHelper.cpp line 83
>                                      do_QueryInterface(wrapper->Native()));

ditto here


on file: storage/src/mozStorageStatementJSHelper.cpp line 214
>                       static_cast<mozIStorageStatement *>(aWrapper->Native()));

and here


on file: storage/test/unit/head_storage.js line 151
>  * Invoke the given function and assert that it throws an exception expressing
>  * the provided error code in its 'result' attribute.  JS function expressions
>  * can be used to do this concisely.
>  *
>  * Example:
>  *  expectError(Cr.NS_ERROR_INVALID_ARG, function() explodingFunction());

nit: not proper java doc style comments


on file: storage/test/unit/head_storage.js line None

nit: brace on new line


on file: storage/test/unit/head_storage.js line None

nit: brace please due to multiple lines


on file: storage/test/unit/head_storage.js line None

nit: does not follow style guidelines for java doc comments


on file: storage/test/unit/head_storage.js line None

nit: brace on new line


on file: storage/test/unit/head_storage.js line None

nit: does not follow style guide


on file: storage/test/unit/test_statement_executeAsync.js line 49
> /**
>  * Execute the given statement asynchronously, spinning an event loop until the
>  * async statement completes.
>  *
>  * @param aStmt The statement to execute.
>  * @param [aOptions={}]
>  * @param [aOptions.error=false] If true we should expect an error whose code
>  *     we do not care about.  If a numeric value, that's the error code we
>  *     expect and require.  If we are expecting an error, we expect a completion
>  *     reason of REASON_ERROR.  Otherwise we expect no error notification and a
>  *     completion reason of REASON_FINISHED.
>  * @param [aOptions.cancel] If true we cancel the pending statement and
>  *     additionally return the pending statement in case you want to further
>  *     manipulate it.
>  * @param [aOptions.returnPending=false] If true we keep the pending statement
>  *     around and return it to you.  We normally avoid doing this to try and
>  *     minimize the amount of time a reference is held to the returned
>  *     pending statement.
>  * @param [aResults] If omitted, we assume no results rows are expected.  If
>  *     it is a number, we assume it is the number of results rows expected.
>  *     If it is a function, we assume it is a function that takes the 1) result
>  *     row number, 2) result tuple, 3) call stack for the original call to
>  *     execAsync as arguments.
>  *     If it is a list, we currently assume it is a list of functions where
>  *     each function corresponds to
>  *
>  * This helper function to improve test readability and cut down on the length
>  * of the file (in order to avoid people's eyes glazing over).

nit: doesn't follow style guide for java-doc comments.


on file: storage/test/unit/test_statement_executeAsync.js line None

nit: brace on newline


on file: storage/test/unit/test_statement_executeAsync.js line None

nit: brace please


on file: storage/test/unit/test_statement_executeAsync.js line None

nit: brace on newline


on file: storage/test/unit/test_statement_executeAsync.js line 509
> function test_bind_direct_binding_params_by_index() {

nit: brace on newline


on file: storage/test/unit/test_statement_executeAsync.js line None

nit: brace on new line


on file: storage/test/unit/test_statement_executeAsync.js line None

nit: brace on new line please


on file: storage/test/unit/test_statement_executeAsync.js line None

nit: brace on new line


on file: storage/test/unit/test_statement_executeAsync.js line 915
> /**
>  * Create a statement of the type under test per (testPass).
>  */
> function makeTestStatement(aSQL) {

nit: proper java doc comments and brace on new line 


on file: storage/test/unit/test_statement_executeAsync.js line 1005
>   if (!_quit)
>     // For saner stacks, we execute this code RSN.
>     do_execute_soon(_run_next_test);

nit: brace please
Attachment #429037 - Flags: review?(sdwilsh) → review-
I think I generally did everything requested.  Unfortunately, in an attempt to be a nice guy and get you a review board interdiff (and likely as a cascading result of a previous attempt to be a nice guy and get the base diff applied where I did an ill-advised thing and directly manipulated the production database), it looks like I may have detached your comments from the most recent review into oblivion.  Likewise, my very few comments on your comments also went into oblivion.  Amusingly, I think I did manage to create an interdiff-capable linkage, but I think it may be v4 (masquerading as v3) against the previous review you left comments on.

I solemnly swear from here on out to never do that again for this production reviewboard install.  The good news is that I've enhanced reviewboard so that I won't have to mess around with base patches again.  Unfortunately, interdiffs are not a solved problem because of the problem where our reviews correspond to attachment id's rather than bug id's.

I will re-address the one comment where I recall having something meaningful to say:

>on file: storage/src/mozStorageAsyncStatementExecution.cpp line 353
>>       // Even though we have the connection lock it is possible for other
>>       // connections or same connection statements to be fighting us.  Since we
>>       // cannot advance we must yield so that others can do so.  (Even if it's
>>       // another connection that is the problem, our main thread can perform
>>       // operations that will succeed despite requiring the mutex.)
>
>Not sure how other connections could be fighting us.  Is this actually true?

Another connection (in-process or in another process) can acquire a file lock on the database manifesting in the defined reason for a SQLITE_BUSY being returned.  In the places case I know it is official decree that no one else is allowed to touch it, but maybe other people are cool with it and at the very least it can happen.

I think my claim about same-connection statements fighting us is probably a misleading recollection of that one bug (now long fixed) where SQLite would in fact return a SQLite busy in a weird case involving same-connection statements... that specific investigation having been way back over when we were debating the (correctness violating) thread pool.

My goal there was just to try and explain to people reading the code why we most definitely needed to yield the lock.  I may have been overselling it given that it's generally a bad idea to hold onto locks longer than you need anyways.  (But given the connection lock situation I mention could result in us locking up the main thread until an external process releases its file lock, we definitely do want to give it up.)

I can make changes to/removal of the comment as you desire.


The two changes not explicitly requested were:

- I cleaned up some of the doxygen blocks in existing interfaces that had led me astray.

- I added some additional commentary to the style guide and some examples in an attempt to at least help myself.  You may want to polish.  (In my defense, I don't think there actually was any javadoc style guidance in terms of indentation in there, and the sun javadoc style is most definitely not what we are doing :)
Attachment #429037 - Attachment is obsolete: true
Attachment #430009 - Flags: review?(sdwilsh)
(In reply to comment #38)
> The good news is that I've enhanced reviewboard so that I
> won't have to mess around with base patches again.

Woo, this claim did not jinx itself.  The diff got pulled across from the try server correctly (if ridiculously slowly)!

http://reviews.visophyte.org/r/430009/diff/#index_header
Reporter

Comment 40

9 years ago
(In reply to comment #38)
> My goal there was just to try and explain to people reading the code why we
> most definitely needed to yield the lock.  I may have been overselling it given
> that it's generally a bad idea to hold onto locks longer than you need anyways.
>  (But given the connection lock situation I mention could result in us locking
> up the main thread until an external process releases its file lock, we
> definitely do want to give it up.)
OK, so it sounds to me like you are talking about two locks at the same time (the file lock, and the thread synchronization lock).  Since this comment is right above releasing the synchronization lock, I think it's best to just talk about it (something as simple as "don't hold the lock while we call outside our module" which is a mozilla rule we are supposed to follow anyway.
Reporter

Updated

9 years ago
Attachment #430009 - Flags: review?(sdwilsh) → review-
Reporter

Comment 41

9 years ago
Comment on attachment 430009 [details] [diff] [review]
v4 StatementBaseInternal change from v2.3 review round plus v3 review round fixes http://hg.mozilla.org/try/rev/13b1757514f7

This is really close.  http://reviews.visophyte.org/r/430009/

on file: storage/public/mozIStorageBaseStatement.idl line 16
>  * The Original Code is Oracle Corporation code.
>  *
>  * The Initial Developer of the Original Code is the Mozilla Foundation.
>  * Portions created by the Initial Developer are Copyright (C) 2010
>  * the Initial Developer. All Rights Reserved.

Something here seems to contradict...


on file: storage/public/mozIStorageBaseStatement.idl line None

Add an @note here about this only working for statements that are executed
asynchronously please.


on file: storage/public/mozIStorageBaseStatement.idl line None

Add an @note here about this only being useful for statements that are
executed asynchronously please.


on file: storage/public/mozIStorageBaseStatement.idl line None

used to bind multiple sets of data to a statement with bindParameters.


on file: storage/public/mozIStorageBaseStatement.idl line None

the grammar here seems funny.


on file: storage/src/StorageBaseStatementInternal.h line 116
>    * It is not scriptable because this is really more of an internal thing.

couldn't be scriptable anyway seeing as how it's a binary only interface :P


on file: storage/src/StorageBaseStatementInternal.cpp line 50
>   mozIStorageBindingParamsArray **_array)

I think, per our style, we want the closing paren on the next line at col 0
since we reduced indentation (just like we do when calling methods).  This is
not explicitly called out in the style guide.  This is true in a few other
places as well.


on file: storage/src/StorageBaseStatementInternal.cpp line None

on mozilla-central we don't need to check the result of new anymore, but
you'll want this for your branch version so we should fix this in a follow-up
(this comment is more informative than asking you to fix anything).


on file: storage/src/mozStorageAsyncStatement.h line 63
> class AsyncStatement : public mozIStorageAsyncStatement,
>                        public StorageBaseStatementInternal

nit: comma placement


on file: storage/src/mozStorageAsyncStatement.h line None

I don't think these are needed anymore.


on file: storage/src/mozStorageAsyncStatement.cpp line 16
>  * The Original Code is Oracle Corporation code.
>  *
>  * The Initial Developer of the Original Code is
>  *  Oracle Corporation

I think, for everything you've grabbed here in this file, Oracle didn't write
so this should probably go to the Foundation at this point.


on file: storage/src/mozStorageAsyncStatement.cpp line None

nit: wrong pointer style (even though we should update to the rest of the
tree's style eventually, we won't do that here).


on file: storage/src/mozStorageAsyncStatement.cpp line None

Realistically speaking, this class is the same for both sync and async
statements, right?  We should make a common one both of them use (and maybe
make a helper method?).


on file: storage/src/mozStorageAsyncStatement.cpp line None

Huh - this comment is wrong (but I think you just copied it).  Things do have
a reference (the caller), but we do want to make sure they don't touch it
anymore.


on file: storage/src/mozStorageAsyncStatement.cpp line None

mSQLString is an nsCString which is always flat, so just do mSQLString.get()


on file: storage/src/mozStorageAsyncStatement.cpp line None

same thing about flat strings here.


on file: storage/src/mozStorageAsyncStatement.cpp line None

and here about flat strings


on file: storage/src/mozStorageAsyncStatement.cpp line None

not sure this comment is needed


on file: storage/src/mozStorageAsyncStatement.cpp line None

do not need to promiseflatcstring - just call get on mSQLString


on file: storage/src/mozStorageAsyncStatement.cpp line None

I don't see in the bug why we couldn't implement these on
StatementBaseInternal.  The methods look to be identical on both, and
getParams is a pure virtual method so it will call the proper class.  Also,
codesize win.


on file: storage/src/mozStorageAsyncStatementExecution.cpp line 241
> bool
> AsyncExecuteStatements::bindExecuteAndProcessStatement(sqlite3_stmt *aStatement,
>                                                        StatementData &aData,
>                                                        bool aLastStatement)

It is not clear to me why this change is needed.  The StatementData still
references a sqlite3_statement, and will get it properly if needed.  Sure,
it's a method invocation, but it should be pretty cheap and passing less
arguments to a method is a mental win.


on file: storage/src/mozStorageAsyncStatementExecution.cpp line 353
>       // Even though we have the connection lock it is possible for other
>       // connections or same connection statements to be fighting us.  Since we
>       // cannot advance we must yield so that others can do so.  (Even if it's
>       // another connection that is the problem, our main thread can perform
>       // operations that will succeed despite requiring the mutex.)

Update this per discussion in the bug please.


on file: storage/src/mozStorageAsyncStatementExecution.cpp line 584
>         nsCOMPtr<mozIStorageError> errorObj(new Error(rc,
>                                                       ::sqlite3_errmsg(db)));

this looks really awkward


on file: storage/src/mozStorageAsyncStatementJSHelper.h line 19
>  * Mozilla Corporation

Foundation


on file: storage/src/mozStorageAsyncStatementJSHelper.h line None

indent one more space please


on file: storage/src/mozStorageAsyncStatementJSHelper.cpp line 19
>  * Mozilla Corporation.

Foundation


on file: storage/src/mozStorageAsyncStatementJSHelper.cpp line None

You contributed!


on file: storage/src/mozStorageBindingParams.h line 78
>    * @returns the pointer to the owning BindingParamsArray.  Used by
>    * a BindingParamsArray to verify that we belong to it when added.

nit: style violation


on file: storage/src/mozStorageBindingParams.h line 135
>   struct enumClosureThunk

I missed this before, but I keep thinking this is an enum by it's name.  Can
you name it something more clear please?


on file: storage/src/mozStorageBindingParams.cpp line 153
> , mOwningStatement(nsnull)

We should be setting this to the right value.  Not doing so could make some of
our assertion checks to not be effective.


on file: storage/src/mozStorageBindingParams.cpp line 193
> AsyncBindingParams::enumNamedParameters(const nsACString &aName,

sad that nsCStringHash stores an nsACString :(


on file: storage/src/mozStorageBindingParams.cpp line None

style violation (comma at front)


on file: storage/src/mozStorageBindingParamsArray.cpp line 51
>     StorageBaseStatementInternal *aOwningStatement)

nit: paren on next line


on file: storage/src/mozStoragePrivateHelpers.cpp line 127
> nsIVariant *
> convertJSValToVariant(JSContext *aCtx, jsval aValue)

nit: style violation (each parameter on its own line)


on file: storage/src/mozStorageStatement.cpp line 410
> MIXIN_IMPL_STORAGEBASESTATEMENTINTERNAL(Statement, (void)0;)

lol @ guard


on file: storage/src/mozStorageStatement.cpp line 1031
> ////////////////////////////////////////////////////////////////////////////////
> //// mozIStorageBindingParams

-> base class!


on file: storage/src/mozStorageStatementData.h line 100
>   operator sqlite3 *() const {

nit: style violation


on file: storage/src/mozStorageAsyncStatementParams.cpp line 79
> AsyncStatementParams::SetProperty(nsIXPConnectWrappedNative *aWrapper,
>                              JSContext *aCtx,
>                              JSObject *aScopeObj,
>                              jsval aId,
>                              jsval *_vp,
>                              PRBool *_retval)

nit: indentation error


on file: storage/src/mozStorageAsyncStatementParams.cpp line 117
>                                  JSContext *aCtx,
>                                  JSObject *aScopeObj,
>                                  jsval aId,
>                                  PRUint32 aFlags,
>                                  JSObject **_objp,

nit: indentation issue


on file: storage/style.txt line 28
> * Javadoc @param tags should have the parameter description start on a new line
>   aligned with the variable name.  Example:
>  * @param aArgument
>  *        Description description description description description etc etc
>  *        next line of description.
> 
> * Javadoc @return (note: non-plural) continuation lines should be lined up with
>   the initial comment.  Example:
>  * @return Description description description description description etc etc
>  *         next line of description.
> 

It is probably best to just reference the big example here.


on file: storage/style.txt line 60
> BIG EXAMPLE:

+1 for awesome


on file: storage/test/unit/head_storage.js line 234
>     stmt.reset();
>     stmt.finalize();

just need to finalize here, not both
I commented on your comments; I made a new export thing for archival purposes and so you don't have to hunt if you don't want.  Note that the try server infrastructure is being revived so this try build might fail for reasons other than the patch.
http://reviews.visophyte.org/r/430009/


on file: storage/src/mozStorageAsyncStatement.h line 146
>>   // Required for access to private mStatementParamsHolder field.
>>   friend class AsyncStatementJSHelper;
>>   // Required for access to private getParams method.
>>   friend class AsyncStatementParams;

> I don't think these are needed anymore.

The former is still required, the latter is not.  I have updated the comment
for the former to be slightly more specific.


on file: storage/src/mozStorageAsyncStatement.cpp line 80
>> class AsyncStatementFinalizer : public nsRunnable
>> {

> Realistically speaking, this class is the same for both sync and async
> statements, right?  We should make a common one both of them use (and maybe
> make a helper method?).

Sufficiently the same, yes.  The AsyncStatement variant held a reference to
the AsyncStatement whereas the Statement variant just held onto the
sqlite3_stmt, as was appropriate to how the sqlite3_stmt comes into being.

I have refactored asyncFinalize/internalAsyncFinalize onto
StorageBaseStatementInternal.  Statement grew an internalFinalize so that it
could know whether the destructor was calling or not, as that determines
whether we attempt to dispatch to the async thread or not. 
AsyncStatementFinalizer moved out of an anonymous namespace so that it could
be a friend of StorageBaseStatementInternal so that internalAsyncFinalize
could be protected.


on file: storage/src/mozStorageAsyncStatement.cpp line 496
>> NS_IMETHODIMP
>> AsyncStatement::BindUTF8StringParameter(PRUint32 aParamIndex,
>>                                         const nsACString &aValue)
>> {
>>   if (mFinalized)
>>     return NS_ERROR_UNEXPECTED;
>> 
>>   mozIStorageBindingParams *params = getParams();
>>   NS_ENSURE_TRUE(params, NS_ERROR_OUT_OF_MEMORY);
>> 
>>   return params->BindUTF8StringByIndex(aParamIndex, aValue);
>> }

> I don't see in the bug why we couldn't implement these on
> StatementBaseInternal.  The methods look to be identical on both, and
> getParams is a pure virtual method so it will call the proper class.  Also,
> codesize win.

Done.  Because 1) anything we put on StatementBaseInternal that needs to
actually end up in a public interface needs to be proxied and 2) we basically
end up proxying everything to the params instance anyways, I have just
implemented macros that proxy directly to the params instance.  The macros
live in StorageBaseStatementInternal.h.

The good news is this ends up roughly just as efficient as pre-refactoring of
this and definitely more efficient that migrating the existing implementations
to StorageBaseStatementInternal and proxying as #1 requires.  (It is 'roughly'
because in the synchronous statement case, the name resolution loses the
ability to use an inlined GetParameterIndex but we at least are not going
through a vtable because we know it's a Statement.)


on file: storage/src/mozStorageBindingParams.cpp line 153
>> , mOwningStatement(nsnull)

> We should be setting this to the right value.  Not doing so could make some of
> our assertion checks to not be effective.

I think you are thinking of mOwningArray, which we do set.  mOwningStatement
is never used in an assertion in this file and is never made accessible to
anything outside of the file, not even via a friend declaration.  It is only
used to translate parameter names to parameter indices and only in the
synchronous case.  The type of mOwningStatement is Statement in order to
accomplish that.

PS: In related news, I had to change unlock to take a Statement so we can
restore mOwningStatement.  Now that we proxy all the calls to the
BindingParams it's really important to have mOwningStatement have a value. 
(This was not a bug under the previous regime because no one ever got to see
the BindingParams and so could not call the exploding name variants.)
Attachment #430009 - Attachment is obsolete: true
Attachment #431470 - Flags: review?(sdwilsh)
Reporter

Comment 43

9 years ago
Comment on attachment 431470 [details] [diff] [review]
v5 address v4 round of review comments. http://hg.mozilla.org/try/rev/d132e3c54121

http://reviews.visophyte.org/r/431470/

on file: storage/public/mozIStorageBaseStatement.idl line 83
>    * @param aValue Argument value.

nit: style violation


on file: storage/public/mozIStorageBaseStatement.idl line 97
>   [deprecated] void bindBlobParameter(
>     in unsigned long aParamIndex,
>     [array,const,size_is(aValueSize)] in octet aValue,
>     in unsigned long aValueSize);

should probably also include an @param aValueSize up where the docs are


on file: storage/public/mozIStorageConnection.idl line 260
>    * @throws NS_ERROR_FAILURE if the table already exists or could not be
>    *         created for any other reason.

hmm - do we want the text on a new line for @throws?


on file: storage/public/mozIStorageStatement.idl line 236
>    * @param[out] aDataSize
>    *             The number of bytes in the blob.
>    * @param[out] aData
>    *             The contents of the BLOB.  This will be NULL if aDataSize == 0.

seems like we make other flags after the name, not before.  should probably
try to be consistent here.


on file: storage/src/StorageBaseStatementInternal.h line 240
>  * @param _class The class name.
>  * @param _guard The guard clause to inject.
>  * @param _declName The argument list (with parens) for the ByName variants.
>  * @param _declIndex The argument list (with parens) for the index variants.
>  * @param _invArgs The invocation argumment list.

these violate our style guide (I think they all do in this file)


on file: storage/src/mozStorageAsyncStatement.cpp line 447
> BOILERPLATE_BIND_PROXIES(
>   AsyncStatement, 
>   if (mFinalized) return NS_ERROR_UNEXPECTED;)

per style, closing paren should be on a new line.  Also, WIN.


on file: storage/src/mozStorageAsyncStatementExecution.h line 242
>    * The wrapped SQLite recursive connection mutex used by sqlite3_step.  We use

I think it is used by more than sqlite3_step, but we should just omit that
part.  "recursive connection mutex" is a sufficient description.


on file: storage/src/mozStorageAsyncStatementExecution.cpp line 248
>   // This cannot fail; we are only called if it's available.
>   (void)aData.getSqliteStatement(&aStatement);

Why don't we assert so we catch it in debug builds if it starts to do so.


on file: storage/src/mozStorageAsyncStatementJSHelper.cpp line 19
>  * Mozilla Corporation.

Mozilla Foundation


on file: storage/src/mozStorageBindingParams.h line 75
>    * @param aOwningStatement The statement that owns us.  We cleared this when
>    *        we were locked, and our invariant requires us to have this, so you
>    *        need to tell us again.

nit: style violation


on file: storage/src/mozStorageBindingParams.cpp line 205
>   int oneIdx = ::sqlite3_bind_parameter_index(closureThunk->statement,
>                                               PromiseFlatCString(name).get());

no need to call PromiseFlatCString; just call get() on name.


on file: storage/src/mozStorageBindingParams.cpp line 212
>                                   PromiseFlatCString(errMsg).get());

do not need to PromiseFlatCString; just do errMsg.get()


on file: storage/src/mozStorageStatement.h line 71
>   // NS_DECL_MOZISTORAGEVALUEARRAY (methods in mozIStorageStatement)

it's in the base right?


on file: storage/src/mozStorageStatement.h line 136
>    * @param destructing Is the destructor calling?

nit: style violation


on file: storage/src/mozStorageStatement.cpp line 388
> nsresult
> Statement::internalFinalize(bool destructing)

style violation (aDestructing)


on file: storage/src/mozStorageStatement.cpp line 892
> BOILERPLATE_BIND_PROXIES(
>   Statement, 
>   if (!mDBStatement) return NS_ERROR_NOT_INITIALIZED;)

trailing paren on new line per style guide


on file: storage/src/mozStorageStatementData.h line 87
>       if (rc != SQLITE_OK)
>         return rc;

why don't we use NS_ENSURE_TRUE here so we get more useful debugging output if
this fails.


on file: storage/src/mozStorageStatementJSHelper.cpp line 81
> #ifdef DEBUG
>   {
>     nsCOMPtr<mozIStorageStatement> isStatement(
>       do_QueryInterface(wrapper->Native())
>     );
>     NS_ASSERTION(isStatement, "How is this not a statement?!");
>   }
> #endif

this actually needs to be moved up since we assume the cast is OK just above
it (we want to check first, then do the cast).


on file: storage/src/mozStorageStatementJSHelper.cpp line 219
> #ifdef DEBUG
>   {
>     nsCOMPtr<mozIStorageStatement> isStatement(
>                                      do_QueryInterface(aWrapper->Native()));
>     NS_ASSERTION(isStatement, "How is this not a statement?!");
>   }
> #endif

move this check above the block before it too.


on file: storage/style.txt line 71
> class Foo : Bar

nit: use a public/private/protected/virtual keyword here since we'd want that.


on file: storage/style.txt line 109
> Foo::Foo(
>   LongArgumentLineThatWouldOtherwiseOverflow *aArgument1
> )

nit: add calling a method with long/lots of params that gets formatted like
this too please.


on file: storage/test/test_true_async.cpp line 282
> already_AddRefed<nsIThread> get_conn_async_thread(mozIStorageConnection *db)

style violation


on file: storage/test/test_true_async.cpp line 327
>   db->CreateAsyncStatement(
>     NS_LITERAL_CSTRING("INSERT INTO test (id) VALUES (?)"),
>     getter_AddRefs(stmt));

style violation (trailing parent on new line)


on file: storage/test/test_true_async.cpp line 336
>   db->CreateAsyncStatement(
>     NS_LITERAL_CSTRING("INSERT INTO test (id) VALUES (:id)"),
>     getter_AddRefs(stmt));

style violation (trailing parent on new line)


on file: storage/test/test_true_async.cpp line 380
>   db->CreateAsyncStatement(
>     NS_LITERAL_CSTRING("CREATE TABLE asyncTable (id INTEGER PRIMARY KEY)"),
>     getter_AddRefs(asyncStmt));

style violation (trailing parent on new line)


on file: storage/test/test_true_async.cpp line 392
>   db->CreateStatement(
>     NS_LITERAL_CSTRING("CREATE TABLE syncTable (id INTEGER PRIMARY KEY)"),
>     getter_AddRefs(syncStmt));


r=sdwilsh
Attachment #431470 - Flags: review?(sdwilsh) → review+
Reporter

Updated

9 years ago
Flags: in-testsuite?
Flags: in-litmus-
Priority: -- → P1
Target Milestone: --- → mozilla1.9.3a3
Comment on attachment 431470 [details] [diff] [review]
v5 address v4 round of review comments. http://hg.mozilla.org/try/rev/d132e3c54121

oh right, super review.

sdwilsh, please redirect sr as appropriate per your preference and understanding of availability of sr's.
Attachment #431470 - Flags: superreview?(vladimir)
Blocks: 519543
Comment on attachment 431470 [details] [diff] [review]
v5 address v4 round of review comments. http://hg.mozilla.org/try/rev/d132e3c54121

API looks fine, sorry this took a while!  Skimmed over the rest of the code as well, though I trust sdwilsh's review.
Attachment #431470 - Flags: superreview?(vladimir) → superreview+
pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/9f3391b5ca0c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Just a comment, these changes make interoperating with mozIStorageStatement (in the sync case) and mozIStorageRow (in the async case) more annoying.  As part of bug 536893 I'll now have to revert some of the Places changes made by this patch.  (Helper funcs that are designed to work in either case have to take mozIStorageValueArrays; in the async case mozIStorageRows can be passed directly but now in the sync case mozIStorageStatements first have to be QI'ed to mozIStorageValueArrays.)
Reporter

Comment 49

9 years ago
(In reply to comment #48)
> Just a comment, these changes make interoperating with mozIStorageStatement (in
> the sync case) and mozIStorageRow (in the async case) more annoying.  As part
> of bug 536893 I'll now have to revert some of the Places changes made by this
> patch.  (Helper funcs that are designed to work in either case have to take
> mozIStorageValueArrays; in the async case mozIStorageRows can be passed
> directly but now in the sync case mozIStorageStatements first have to be QI'ed
> to mozIStorageValueArrays.)
We can make this better for JS consumers at least by providing nsIClassInfo, but you are native, aren't you?
Yeah.  What I mean is that the annoyance derives from the fact that the two classes no longer share a common base class that provides access to row values.  Sure, they can both QI to mozIStorageValueArray, but now for any site that wants to act on a result row -- regardless of whether that row was gotten asyncly or syncly, it's just a bag of values -- some entries to that site now have the extra step of the QI.
Reporter

Comment 51

9 years ago
(In reply to comment #50)
> Yeah.  What I mean is that the annoyance derives from the fact that the two
> classes no longer share a common base class that provides access to row values.
>  Sure, they can both QI to mozIStorageValueArray, but now for any site that
> wants to act on a result row -- regardless of whether that row was gotten
> asyncly or syncly, it's just a bag of values -- some entries to that site now
> have the extra step of the QI.
The problem here is that idl does not have multiple inheritance, sadly.  We should be purging all callers of the sync API anyway though ;)
Depends on: 555124
Blocks: 555218
Depends on: 564090
Reporter

Updated

9 years ago
Blocks: 490881

Updated

9 years ago
Blocks: 584315

Updated

9 years ago
Blocks: 584316
You need to log in before you can comment on or make changes to this bug.