Bug 113043 – build warning for unused function parameters in indexeddb

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Charles Wei]

When building webkit for blackberry, there's the following warmings like:

Source/WebCore/Modules/indexeddb/IDBCallbacks.h:71:18: warning: unused parameter 'existingVersion' [-Wunused-parameter]

We should remove the parameter name if it's not used in the function.

[Charles Wei]

Created attachment 194514 [details]
Patch

[Charles Wei]

Created attachment 194822 [details]
Patch

[Joshua Bell]

Comment on attachment 194822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194822&action=review

Thanks for the cleanup!

> Source/WebCore/Modules/indexeddb/IDBCallbacks.h:71
> +    virtual void onBlocked(int64_t /* existingVersion */) { ASSERT_NOT_REACHED(); }

It looks like WebKit style is to not have spaces inside the comment, e.g. (int64_t /*existingVersion*/)

> Source/WebCore/Modules/indexeddb/IDBCallbacks.h:73
> +    virtual void onUpgradeNeeded(int64_t /* oldVersion */, PassRefPtr<IDBDatabaseBackendInterface>, const IDBDatabaseMetadata&) { ASSERT_NOT_REACHED(); }

Ditto.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:300
> +IndexedDB::CursorDirection IDBCursor::stringToDirection(const String& directionString, ScriptExecutionContext*, ExceptionCode& ec)

Please just remove the ScriptExecutionContext argument from this method and all callers. It is no longer used now that we've dropped support for legacy constants.

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:223
> +void IDBCursorBackendImpl::prefetchReset(int usedPrefetches, int /* unusedPrefetches */)

Remove extra spaces inside /* ... */

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:918
> +void IDBDatabaseBackendImpl::setIndexesReady(int64_t transactionId, int64_t /* objectStoreId */, const Vector<int64_t>& indexIds)

Ditto.

> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:729
> +int compare(const LevelDBSlice& a, const LevelDBSlice& b, bool /* ignoreDuplicates */, bool& ok)

Ditto.

> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:752
> +int compare<ExistsEntryKey>(const LevelDBSlice& a, const LevelDBSlice& b, bool /* ignoreDuplicates */, bool& ok)

Ditto.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:339
> +IndexedDB::TransactionMode IDBTransaction::stringToMode(const String& modeString, ScriptExecutionContext*, ExceptionCode& ec)

Please just remove the ScriptExecutionContext argument from the method and all callers. It is no longer used now that we've dropped support for legacy constants.

[Charles Wei]

Created attachment 194995 [details]
Patch

[Charles Wei]

Thanks for the comments, Joshua.

About the style for inline comments "/* */",  there are no standard style guideline in webkit coding standard about whether to have a white space after leading "/*" and before "*/".  There are both styles (with and without the white space) in webkit code, both seem OK.

But I personally prefer to have the white space to make the code easier to read.  Do you agree, Joshua ?

About your other comments to remove unused ScriptExecutionContext,  I've done that with the new patch.

Thanks.

[Build Bot]

Comment on attachment 194995 [details]
Patch

Attachment 194995 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17311292

[Joshua Bell]

(In reply to comment #5)
> About the style for inline comments "/* */",  there are no standard style guideline in webkit coding standard about whether to have a white space after leading "/*" and before "*/".  There are both styles (with and without the white space) in webkit code, both seem OK.

Okay. I guess the files I looked at all used the more compact form. Doesn't matter.
 
> About your other comments to remove unused ScriptExecutionContext,  I've done that with the new patch.

Cool, looks good to me.

tony@ - could you r? this?

[Tony Chang]

Comment on attachment 194995 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194995&action=review

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:223
> -void IDBCursorBackendImpl::prefetchReset(int usedPrefetches, int unusedPrefetches)
> +void IDBCursorBackendImpl::prefetchReset(int usedPrefetches, int /* unusedPrefetches */)

Nit: WebKit style is to normally leave parameters unnamed if they are unused.  The name can be found in the header file if you want to know what the param is for.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:918
> -void IDBDatabaseBackendImpl::setIndexesReady(int64_t transactionId, int64_t objectStoreId, const Vector<int64_t>& indexIds)
> +void IDBDatabaseBackendImpl::setIndexesReady(int64_t transactionId, int64_t /* objectStoreId */, const Vector<int64_t>& indexIds)

Remove objectStoreId

> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:729
> -int compare(const LevelDBSlice& a, const LevelDBSlice& b, bool ignoreDuplicates, bool& ok)
> +int compare(const LevelDBSlice& a, const LevelDBSlice& b, bool /* ignoreDuplicates */, bool& ok)

Remove ignoreDuplicates

> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:752
> -int compare<ExistsEntryKey>(const LevelDBSlice& a, const LevelDBSlice& b, bool ignoreDuplicates, bool& ok)
> +int compare<ExistsEntryKey>(const LevelDBSlice& a, const LevelDBSlice& b, bool /* ignoreDuplicates */, bool& ok)

Remove ignoreDuplicates

> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:775
> -int compare<ObjectStoreDataKey>(const LevelDBSlice& a, const LevelDBSlice& b, bool ignoreDuplicates, bool& ok)
> +int compare<ObjectStoreDataKey>(const LevelDBSlice& a, const LevelDBSlice& b, bool /* ignoreDuplicates */, bool& ok)

Remove ignoreDuplicates

> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:205
> -void ExecutableWithDatabase::start(IDBFactory* idbFactory, SecurityOrigin* securityOrigin, const String& databaseName)
> +void ExecutableWithDatabase::start(IDBFactory* idbFactory, SecurityOrigin*, const String& databaseName)

This is a good example of WebKit style.

[Charles Wei]

Comment on attachment 194995 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194995&action=review

>> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:223
>> +void IDBCursorBackendImpl::prefetchReset(int usedPrefetches, int /* unusedPrefetches */)
> 
> Nit: WebKit style is to normally leave parameters unnamed if they are unused.  The name can be found in the header file if you want to know what the param is for.

Agree.

[Charles Wei]

Created attachment 195239 [details]
Patch

[WebKit Review Bot]

Comment on attachment 195239 [details]
Patch

Clearing flags on attachment: 195239

Committed r147029: <http://trac.webkit.org/changeset/147029>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.