Bug 109143 – Drop ExceptionCode from IDB's directionToString and modeToString.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mike West]

jsbell notes:

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

>> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:99
>>      return direction;
> 
> No need for the local variable any more here.

And FYI the directionToString method is only ever called from here now; it was used when we supported both legacy numeric enum and new string values when called from script. Now that the numeric enum values are not supported from script we can drop the ExceptionCode argument from this method. But that can be done in a separate patch.

>> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:125
>>      return mode;
> 
> No need for the local variable any more here.

And FYI, same as above. We can drop the ExceptionCode param from modeToStrng now.

[Mike West]

Created attachment 187010 [details]
Patch

[Mike West]

Jochen, perhaps you can take a look at this patch? Joshua says it's the right thing to do, so I hope the details are trivially reviewable. :)

It doesn't (yet) apply; I'll upload it again for the bots once https://bugs.webkit.org/show_bug.cgi?id=109044 lands.

[Joshua Bell]

Comment on attachment 187010 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        make use of the ExceptionCode they accept. This patch drops the unused

Nitpicking the description: the methods *did* use it, but all callsites ignored the result other than asserting because these methods are now only used with pre-validated data.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:-329
> -        ec = TypeError;

Please add ASSERT_NOT_REACHED() here.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:-365
> -        ec = TypeError;

Please add ASSERT_NOT_REACHED() here.

[Joshua Bell]

(In reply to comment #2)
> Jochen, perhaps you can take a look at this patch? Joshua says it's the right thing to do, so I hope the details are trivially reviewable. :)

I'm Joshua and I approve this patch (with those ASSERT_NOT_REACHED additions). Thanks, Jochen, and thanks Mike for putting it together!

[Mike West]

Created attachment 187296 [details]
Patch

[Mike West]

This should address Joshua's comments.

Jochen, if I haven't exhausted your patience yet today, would you mind taking a look at this patch as well?

Comment on attachment 187296 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 187296 [details]
Patch

Clearing flags on attachment: 187296

Committed r142356: <http://trac.webkit.org/changeset/142356>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.