Issue 234403004: Rename V8TRYCATCH_* macros in v8/V8BindingMacros.h

Issue 234403004: Rename V8TRYCATCH_* macros in v8/V8BindingMacros.h (Closed)

Created:
6 years, 8 months ago by Nils Barth (inactive)

Modified:
6 years, 8 months ago

Reviewers:
haraken

CC:
blink-reviews, Nils Barth (inactive), arv+blink, jsbell+bindings_chromium.org, sof, kouhei+bindings_chromium.org, abarth-chromium, marja+watch_chromium.org, dglazkov+blink, dominicc+watchlist_chromium.org, adamk+blink_chromium.org, haraken, Nate Chapin, Inactive, yhirano

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

Rename V8TRYCATCH_* macros in v8/V8BindingMacros.h The macros in V8BindingMacros.h have confusing and inconsistent naming. This makes them more regular. Yutaka (yhirano@) is adding more macros, so we'd like to regularize naming before that. Request of haraken@: https://codereview.chromium.org/232563003/#msg7 ...in support of: API functions returning Promises should not throw exceptions. https://codereview.chromium.org/232563003/ R=haraken Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171326

Patch Set 1 #

Total comments: 12

Patch Set 2 : Revised, renamed #

Patch Set 3 : Typo #

Total comments: 3

Patch Set 4 : Tweaks #

Patch Set 5 : BOOL_BOOL -> BOOL #

Created: 6 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+510 lines, -502 lines)			Patch
M	Source/bindings/scripts/v8_methods.py	View	1 2 3	1 chunk	+5 lines, -4 lines	0 comments	Download
M	Source/bindings/scripts/v8_types.py	View	1	1 chunk	+4 lines, -4 lines	0 comments	Download
M	Source/bindings/templates/interface.cpp	View	1 2	2 chunks	+3 lines, -3 lines	0 comments	Download
M	Source/bindings/templates/methods.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/tests/results/V8SVGTestInterface.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/tests/results/V8TestInterface.cpp	View	1	25 chunks	+27 lines, -27 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestInterface2.cpp	View	1	8 chunks	+11 lines, -11 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestInterfaceCheckSecurity.cpp	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestInterfaceConstructor.cpp	View	1	1 chunk	+8 lines, -8 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestInterfaceConstructor2.cpp	View	1	3 chunks	+7 lines, -7 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestInterfaceConstructor3.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/tests/results/V8TestInterfaceEventConstructor.cpp	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestInterfaceGarbageCollected.cpp	View	1	3 chunks	+3 lines, -3 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestInterfaceNamedConstructor.cpp	View	1	2 chunks	+6 lines, -6 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestInterfaceNamedConstructor2.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/tests/results/V8TestInterfaceNode.cpp	View	1	5 chunks	+5 lines, -5 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestInterfaceWillBeGarbageCollected.cpp	View	1	3 chunks	+3 lines, -3 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestNode.cpp	View	1	3 chunks	+3 lines, -3 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestObject.cpp	View	1	222 chunks	+234 lines, -234 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestSpecialOperations.cpp	View	1	2 chunks	+3 lines, -3 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestTypedefs.cpp	View	1	7 chunks	+8 lines, -8 lines	0 comments	Download
M	Source/bindings/v8/CustomElementConstructorBuilder.cpp	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	Source/bindings/v8/Dictionary.cpp	View	1 2 3 4	9 chunks	+10 lines, -10 lines	0 comments	Download
M	Source/bindings/v8/ScriptController.cpp	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/ScriptEventListener.cpp	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/ScriptValue.cpp	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/V8AbstractEventListener.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/V8Binding.h	View	1 2 3 4	3 chunks	+4 lines, -4 lines	0 comments	Download
M	Source/bindings/v8/V8Binding.cpp	View	1 2 3 4	9 chunks	+9 lines, -9 lines	0 comments	Download
M	Source/bindings/v8/V8BindingMacros.h	View	1 2 3 4	2 chunks	+52 lines, -45 lines	0 comments	Download
M	Source/bindings/v8/V8Initializer.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/V8WorkerGlobalScopeEventListener.cpp	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/WorkerScriptController.cpp	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/custom/V8BlobCustomHelpers.cpp	View	1 2 3 4	4 chunks	+6 lines, -6 lines	0 comments	Download
M	Source/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/custom/V8CustomEventCustom.cpp	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	Source/bindings/v8/custom/V8CustomXPathNSResolver.cpp	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/custom/V8DeviceMotionEventCustom.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/custom/V8DeviceOrientationEventCustom.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/custom/V8DocumentCustom.cpp	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/bindings/v8/custom/V8ElementCustom.cpp	View	1	2 chunks	+4 lines, -4 lines	0 comments	Download
M	Source/bindings/v8/custom/V8FileCustom.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/custom/V8HTMLAllCollectionCustom.cpp	View	1 2 3 4	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/bindings/v8/custom/V8HistoryCustom.cpp	View	1	2 chunks	+4 lines, -4 lines	0 comments	Download
M	Source/bindings/v8/custom/V8InspectorFrontendHostCustom.cpp	View	1 2 3 4	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/bindings/v8/custom/V8MessageEventCustom.cpp	View	1	1 chunk	+5 lines, -5 lines	0 comments	Download
M	Source/bindings/v8/custom/V8PromiseCustom.cpp	View	1	11 chunks	+23 lines, -23 lines	0 comments	Download
M	Source/bindings/v8/custom/V8SQLTransactionCustom.cpp	View	1	3 chunks	+5 lines, -5 lines	0 comments	Download
M	Source/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp	View	1	3 chunks	+5 lines, -5 lines	0 comments	Download
M	Source/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/custom/V8WindowCustom.cpp	View	1	3 chunks	+6 lines, -6 lines	0 comments	Download
M	Source/bindings/v8/custom/V8XMLHttpRequestCustom.cpp	View	1	3 chunks	+5 lines, -5 lines	0 comments	Download
M	Source/bindings/v8/custom/V8XSLTProcessorCustom.cpp	View	1	3 chunks	+7 lines, -7 lines	0 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

haraken

Sorry about further renaming suggestions: https://codereview.chromium.org/234403004/diff/1/Source/bindings/v8/V8BindingMacros.h File Source/bindings/v8/V8BindingMacros.h (right): https://codereview.chromium.org/234403004/diff/1/Source/bindings/v8/V8BindingMacros.h#newcode37 Source/bindings/v8/V8BindingMacros.h:37: // COMMAND[_VOID][_ARG]... COMMAND[_RETURNTYPE][_ARGUMENTTYPE]... ? ...

6 years, 8 months ago (2014-04-11 02:33:20 UTC) #2

Nils Barth (inactive)

Naming idea, and note about V8StringType macros. https://codereview.chromium.org/234403004/diff/1/Source/bindings/v8/V8BindingMacros.h File Source/bindings/v8/V8BindingMacros.h (right): https://codereview.chromium.org/234403004/diff/1/Source/bindings/v8/V8BindingMacros.h#newcode37 Source/bindings/v8/V8BindingMacros.h:37: // COMMAND[_VOID][_ARG]... ...

6 years, 8 months ago (2014-04-11 03:00:55 UTC) #3

haraken

https://codereview.chromium.org/234403004/diff/1/Source/bindings/v8/V8BindingMacros.h File Source/bindings/v8/V8BindingMacros.h (right): https://codereview.chromium.org/234403004/diff/1/Source/bindings/v8/V8BindingMacros.h#newcode37 Source/bindings/v8/V8BindingMacros.h:37: // COMMAND[_VOID][_ARG]... On 2014/04/11 03:00:56, Nils Barth wrote: > ...

6 years, 8 months ago (2014-04-11 03:28:58 UTC) #4

https://codereview.chromium.org/234403004/diff/1/Source/bindings/v8/V8Binding...
File Source/bindings/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/234403004/diff/1/Source/bindings/v8/V8Binding...
Source/bindings/v8/V8BindingMacros.h:37: // COMMAND[_VOID][_ARG]...
On 2014/04/11 03:00:56, Nils Barth wrote:
> On 2014/04/11 02:33:20, haraken wrote:
> > 
> > COMMAND[_RETURNTYPE][_ARGUMENTTYPE]... ?
> > 
> > Currently you are using "V8TRYCATCH" and "V8STRINGRESOURCE" for COMMAND, but
> > "TONATIVE" and "TOSTRING" might be clearer.
> > 
> > These macros are for converting V8 values to native C++ values with a
TryCatch
> > block.
> 
> How about
> TONATIVE[_TYPE][_ARGUMENTTYPE]...
> That way we get
> TONATIVE_VOID*
> TONATIVE_BOOL*
> TONATIVE_STRING*
> ...etc.?

Actually, TONATIVE_BOOL and TONATIVE_STRING are different.

- TONATIVE_BOOL means "please convert this value to some type and return
true/false depending on whether the conversion was a success or not".

- TONATIVE_STRING means "please convert this value to a string".

So I'd prefer using [TONATIVE|TOSTRING]_[RETURNTYPE]_[ARGS].

https://codereview.chromium.org/234403004/diff/1/Source/bindings/v8/V8Binding...
Source/bindings/v8/V8BindingMacros.h:93: type var(value);                       
            \
On 2014/04/11 03:00:56, Nils Barth wrote:
> On 2014/04/11 02:33:20, haraken wrote:
> > 
> > You can drop the |type| parameter, because |type| should always be
> > V8StringResource.
> 
> V8StringResource<> is a class template, and is instantiated by giving args,
> so the type does differ.
> 
> We could reduce this to the template arguments, if any, but that makes the
> binding generator code more complicated (b/c it's no longer specifying the
> full type), and makes these macros less uniform.
> (It also gives the C preprocessor some indigestion, due to the <> tokens
> and blank arguments. It's doable, but ugly.)
> 
> So I think it's simplest as it is.

Makes sense.

Nils Barth (inactive)

Revised; see names in: https://codereview.chromium.org/234403004/diff/40001/Source/bindings/v8/V8BindingMacros.h https://codereview.chromium.org/234403004/diff/1/Source/bindings/v8/V8BindingMacros.h File Source/bindings/v8/V8BindingMacros.h (right): https://codereview.chromium.org/234403004/diff/1/Source/bindings/v8/V8BindingMacros.h#newcode93 Source/bindings/v8/V8BindingMacros.h:93: type var(value); \ On ...

6 years, 8 months ago (2014-04-11 06:01:38 UTC) #5

haraken

LGTM https://codereview.chromium.org/234403004/diff/40001/Source/bindings/v8/V8BindingMacros.h File Source/bindings/v8/V8BindingMacros.h (right): https://codereview.chromium.org/234403004/diff/40001/Source/bindings/v8/V8BindingMacros.h#newcode81 Source/bindings/v8/V8BindingMacros.h:81: #define TONATIVE_BOOL_EXCEPTIONSTATE_BOOL(type, var, value, exceptionState, retVal) \ TONATIVE_BOOL_EXCEPTIONSTATE ...

6 years, 8 months ago (2014-04-11 06:09:26 UTC) #6

Nils Barth (inactive)

Agreed re: naming, thanks! https://codereview.chromium.org/234403004/diff/40001/Source/bindings/v8/V8BindingMacros.h File Source/bindings/v8/V8BindingMacros.h (right): https://codereview.chromium.org/234403004/diff/40001/Source/bindings/v8/V8BindingMacros.h#newcode81 Source/bindings/v8/V8BindingMacros.h:81: #define TONATIVE_BOOL_EXCEPTIONSTATE_BOOL(type, var, value, exceptionState, ...

6 years, 8 months ago (2014-04-11 07:40:16 UTC) #7

Nils Barth (inactive)

The CQ bit was checked by nbarth@chromium.org

6 years, 8 months ago (2014-04-11 07:40:26 UTC) #8

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nbarth@chromium.org/234403004/80001

6 years, 8 months ago (2014-04-11 07:40:30 UTC) #9

Message was sent while issue was closed.

Change committed as 171326

Expand Messages | Collapse Messages