Merge IndexCountRequestOp::DoDatabaseWork and ObjectStoreCountRequestOp::DoDatabaseWork
Categories
(Core :: Storage: IndexedDB, task)
Tracking
()
People
(Reporter: sg, Unassigned)
Details
(Keywords: good-first-bug)
Attachments
(2 files)
3.96 KB,
patch
|
Details | Diff | Splinter Review | |
3.72 KB,
patch
|
Details | Diff | Splinter Review |
IndexCountRequestOp::DoDatabaseWork
and ObjectStoreCountRequestOp::DoDatabaseWork
do almost the same, just the query is different. A common function could be extracted from the two functions.
It might even possible to extract a common class template from the whole classes, but maybe that introduces too much complexity.
Comment 1•3 years ago
|
||
I'd like to take up this task if ya'll don't mind.
Comment 2•3 years ago
|
||
Also for this common function, to what class/object would it belong to? I'm new here so I'm not yet very familiar with the code base.
Comment 3•3 years ago
|
||
You can add the function to NormalTransactionOp
class besides the ObjectStoreHasIndexes
function.
Comment 4•3 years ago
|
||
Is there anything specific ya'll looking for in the method? Like should it take an input parameter as to what type of object is being used to execute DoDatabaseWork, if so I was thinking maybe using a String that can either be "IndexCountRequestOp::DoDatabaseWork" or ("ObjectStoreCountRequestOp::DoDatabaseWork" to be used to return either one of the two nsresults. However the problem I've been having with this approach is that I would need to create an instance of IndexCountRequestOp or ObjectStoreCountRequestOp, which I still don't know how to do due to still being new to this.
Comment 5•3 years ago
|
||
So we have two methods:
nsresult ObjectStoreCountRequestOp::DoDatabaseWork(
DatabaseConnection* aConnection) {
MOZ_ASSERT(aConnection);
aConnection->AssertIsOnConnectionThread();
AUTO_PROFILER_LABEL("ObjectStoreCountRequestOp::DoDatabaseWork", DOM);
const auto keyRangeClause = MaybeGetBindingClauseForKeyRange(
mParams.optionalKeyRange(), kColumnNameKey);
IDB_TRY_INSPECT(
const auto& maybeStmt,
aConnection->BorrowAndExecuteSingleStepStatement(
"SELECT count(*) "
"FROM object_data "
"WHERE object_store_id = :"_ns +
kStmtParamNameObjectStoreId + keyRangeClause,
[¶ms = mParams](auto& stmt) -> mozilla::Result<Ok, nsresult> {
IDB_TRY(stmt.BindInt64ByName(kStmtParamNameObjectStoreId,
params.objectStoreId()));
if (params.optionalKeyRange().isSome()) {
IDB_TRY(BindKeyRangeToStatement(params.optionalKeyRange().ref(),
&stmt));
}
return Ok{};
}));
IDB_TRY(OkIf(maybeStmt.isSome()), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR,
[](const auto) {
// XXX Why do we have an assertion here, but not at most other
// places using IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
MOZ_ASSERT(false, "This should never be possible!");
IDB_REPORT_INTERNAL_ERR();
});
const auto& stmt = *maybeStmt;
const int64_t count = stmt->AsInt64(0);
IDB_TRY(OkIf(count >= 0), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR, [](const auto) {
// XXX Why do we have an assertion here, but not at most other places using
// IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
MOZ_ASSERT(false, "This should never be possible!");
IDB_REPORT_INTERNAL_ERR();
});
mResponse.count() = count;
return NS_OK;
}
nsresult IndexCountRequestOp::DoDatabaseWork(DatabaseConnection* aConnection) {
MOZ_ASSERT(aConnection);
aConnection->AssertIsOnConnectionThread();
AUTO_PROFILER_LABEL("IndexCountRequestOp::DoDatabaseWork", DOM);
const auto indexTable = mMetadata->mCommonMetadata.unique()
? "unique_index_data "_ns
: "index_data "_ns;
const auto keyRangeClause = MaybeGetBindingClauseForKeyRange(
mParams.optionalKeyRange(), kColumnNameValue);
IDB_TRY_INSPECT(
const auto& maybeStmt,
aConnection->BorrowAndExecuteSingleStepStatement(
"SELECT count(*) "
"FROM "_ns +
indexTable + "WHERE index_id = :"_ns + kStmtParamNameIndexId +
keyRangeClause,
[&self = *this](auto& stmt) -> mozilla::Result<Ok, nsresult> {
IDB_TRY(stmt.BindInt64ByName(kStmtParamNameIndexId,
self.mMetadata->mCommonMetadata.id()));
if (self.mParams.optionalKeyRange().isSome()) {
IDB_TRY(BindKeyRangeToStatement(
self.mParams.optionalKeyRange().ref(), &stmt));
}
return Ok{};
}));
IDB_TRY(OkIf(maybeStmt.isSome()), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR,
[](const auto) {
// XXX Why do we have an assertion here, but not at most other
// places using IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
MOZ_ASSERT(false, "This should never be possible!");
IDB_REPORT_INTERNAL_ERR();
});
const auto& stmt = *maybeStmt;
const int64_t count = stmt->AsInt64(0);
IDB_TRY(OkIf(count >= 0), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR, [](const auto) {
// XXX Why do we have an assertion here, but not at most other places using
// IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
MOZ_ASSERT(false, "This should never be possible!");
IDB_REPORT_INTERNAL_ERR();
});
mResponse.count() = count;
return NS_OK;
}
These parts are exactly the same:
const auto keyRangeClause = MaybeGetBindingClauseForKeyRange(
mParams.optionalKeyRange(), kColumnNameValue);
IDB_TRY(OkIf(maybeStmt.isSome()), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR,
[](const auto) {
// XXX Why do we have an assertion here, but not at most other
// places using IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
MOZ_ASSERT(false, "This should never be possible!");
IDB_REPORT_INTERNAL_ERR();
});
const auto& stmt = *maybeStmt;
const int64_t count = stmt->AsInt64(0);
IDB_TRY(OkIf(count >= 0), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR, [](const auto) {
// XXX Why do we have an assertion here, but not at most other places using
// IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
MOZ_ASSERT(false, "This should never be possible!");
IDB_REPORT_INTERNAL_ERR();
});
So they can be easily extracted to a helper function and I think the function should return the count as Result<int64_t, nsresult
.
One of the arguments for the function needs to be DatabaseConnection* aConnection
for sure.
Then we need to extract the query which is a bit harder. Let me know if this looks ok to you so far.
Comment 6•3 years ago
|
||
I've been working on this and have been having problem with extracting a common mParams value. I'm trying to do so by using a ternary operator as so
const ObjectStoreCountParams oscParams;
const IndexCountParams icParams;
const auto mParams = (requestType == "ObjectStoreCountRequestOp") ? oscParams : icParams;
however I'm getting a compile time error that states there are incompatible types when it comes to the ? operator. Any one have any clue as to what I did wrong here or how to fix this error?
Comment 7•3 years ago
|
||
Ok so I've written the helper function for the most part, now I'm just confused as how to return the count
nsresult NormalTransactionOp::CommonCountRequestOp(DatabaseConnection* aConnection, RequestParams&& aParams){
MOZ_ASSERT(aConnection);
aConnection->AssertIsOnConnectionThread();
const SafeRefPtr<FullIndexMetadata> mMetadata;
if(aParams.type() == RequestParams::TObjectStoreCountParams){
AUTO_PROFILER_LABEL("ObjectStoreCountRequestOp::DoDatabaseWork", DOM);
}else{
AUTO_PROFILER_LABEL("IndexCountRequestOp::DoDatabaseWork", DOM);
}
const auto indexTable = mMetadata->mCommonMetadata.unique()
? "unique_index_data "_ns
: "index_data "_ns;
IndexCountParams icParams;
ObjectStoreCountParams oscParams;
auto keyRangeClause = MaybeGetBindingClauseForKeyRange((aParams.type() == RequestParams::TObjectStoreCountParams) ? oscParams.optionalKeyRange() : icParams.optionalKeyRange(), kColumnNameKey);
IDB_TRY_INSPECT(
const auto& maybeStmt,
(aParams.type() == RequestParams::TObjectStoreCountParams) ? aConnection->BorrowAndExecuteSingleStepStatement(
"SELECT count(*) "
"FROM object_data "
"WHERE object_store_id = :"_ns +
kStmtParamNameObjectStoreId + keyRangeClause,
[¶ms = icParams](auto& stmt) -> mozilla::Result<Ok, nsresult> {
IDB_TRY(stmt.BindInt64ByName(kStmtParamNameObjectStoreId,
params.objectStoreId()));
if (params.optionalKeyRange().isSome()) {
IDB_TRY(BindKeyRangeToStatement(params.optionalKeyRange().ref(),
&stmt));
}
return Ok{};
})
: aConnection->BorrowAndExecuteSingleStepStatement(
"SELECT count(*) "
"FROM "_ns +
indexTable + "WHERE index_id = :"_ns + kStmtParamNameIndexId +
keyRangeClause,
[¶ms = icParams, &metadata = mMetadata](auto& stmt) -> mozilla::Result<Ok, nsresult> {
IDB_TRY(stmt.BindInt64ByName(kStmtParamNameIndexId,
metadata->mCommonMetadata.id()));
if (params.optionalKeyRange().isSome()) {
IDB_TRY(BindKeyRangeToStatement(
params.optionalKeyRange().ref(), &stmt));
}
return Ok{};
}));
IDB_TRY(OkIf(maybeStmt.isSome()), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR,
[](const auto) {
// XXX Why do we have an assertion here, but not at most other
// places using IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
MOZ_ASSERT(false, "This should never be possible!");
IDB_REPORT_INTERNAL_ERR();
});
const auto& stmt = *maybeStmt;
const int64_t count = stmt->AsInt64(0);
IDB_TRY(OkIf(count >= 0), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR, [](const auto) {
// XXX Why do we have an assertion here, but not at most other places using
// IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
MOZ_ASSERT(false, "This should never be possible!");
IDB_REPORT_INTERNAL_ERR();
});
return NS_OK;
}
Comment 8•3 years ago
|
||
Can you translate that into a diff/patch ?
Thanks.
Comment 9•3 years ago
|
||
Is there documentation that would tell me how to do that? This is my first bug and would like to know how to do so. Also how would I return the count for the method I wrote. I know that IndexCountRequestOp::DoDatabaseWork and ObjectStoreCountRequestOp::DoDatabaseWork use mResponse to set the count but NormalTransactionOp has no such variable.
Comment 10•3 years ago
|
||
Also forgot to ask but can I be assigned to this task?
Comment 11•3 years ago
|
||
Sure, I assigned the bug to you.
Regarding making a patch, I don't know how you downloaded the source code. If you used Mercurial (hg) for that, then you can just run hg diff >patch.diff
and attach the file to this bug.
A more advanced option is to use Phabricator, but you don't have to worry about that for now.
Comment 12•3 years ago
|
||
Here is that patch file, hope it helps.
Comment 13•3 years ago
|
||
Comment 14•3 years ago
|
||
Just put in another patch file, not sure what class CountRequestOp was doing there, probably forgot to delete it from something I was previously trying a month ago.
Comment 15•3 years ago
|
||
This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.
Comment 16•3 years ago
|
||
Hi Nathaniel, I fear we overlooked that patch for a while, sorry. Jan, can you take a look once back?
Comment 17•3 years ago
|
||
This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.
Comment 18•2 years ago
|
||
Hi, just wanted to know if the patch solved the issue>
Comment 19•10 months ago
|
||
Sorry for really long delay. I quickly checked the patch and it seems there's still work to be done.
Description
•