|
| 1 | +2020-09-10 Devin Rousso < [email protected]> |
| 2 | + |
| 3 | + Web Inspector: modernize generated backend protocol code |
| 4 | + https://bugs.webkit.org/show_bug.cgi?id=216302 |
| 5 | + <rdar://problem/68547649> |
| 6 | + |
| 7 | + Reviewed by Brian Burg. |
| 8 | + |
| 9 | + Previously, the inspector protocol was expressed in code in a somewhat confusing way: |
| 10 | + - the error string was the first argument |
| 11 | + - required parameters were `T` or `const T&` |
| 12 | + - optional parameters were `const T*` |
| 13 | + - enum parameters were the underlying type requiring the backend dispatcher handler to |
| 14 | + process it instead of it being preprocessed |
| 15 | + - required returns were `T&` |
| 16 | + - optional returns were `T*` |
| 17 | + This doesn't really make for easy/obvious reading of code since the order of arguments is |
| 18 | + not weird (e.g. error string first), and that there are references/pointers to primitive |
| 19 | + types. |
| 20 | + |
| 21 | + This patch cleans up the generated inspector protocol code to be: |
| 22 | + - required parameters are `T` or `Ref<T>&&` |
| 23 | + - optional parameters are `Optional<T>&&` or `RefPtr<T>&&` |
| 24 | + - enum parameters are preprocessed and passed to the backend dispatcher handler if valid |
| 25 | + - synchronous commands return `Expected<X, ErrorString>` using the same types/rules above |
| 26 | + where `X` is either a single return or a `std::tuple` of multiple returns |
| 27 | + |
| 28 | + The one exception to the above is `String`, which is already a tri-state of `nullString()`, |
| 29 | + `emptyString()`, and something set, so there's no need to use `Optional<String>`. |
| 30 | + |
| 31 | + Also use `Protocol` objects/`typedefs` wherever possible to further relate the protocol |
| 32 | + JSON and the actual backend dispatcher handler implementation. |
| 33 | + |
| 34 | + * inspector/scripts/codegen/generator.py: |
| 35 | + (Generator.generate_includes_from_entries): |
| 36 | + * inspector/scripts/codegen/cpp_generator_templates.py: |
| 37 | + * inspector/scripts/codegen/cpp_generator.py: |
| 38 | + (CppGenerator.helpers_namespace): |
| 39 | + (CppGenerator.cpp_getter_method_for_type): |
| 40 | + (CppGenerator.cpp_setter_method_for_type): |
| 41 | + (CppGenerator.cpp_protocol_type_for_type): |
| 42 | + (CppGenerator.cpp_type_for_type_member_argument): Added. |
| 43 | + (CppGenerator.cpp_type_for_command_parameter): Added. |
| 44 | + (CppGenerator.cpp_type_for_command_return_declaration): Added. |
| 45 | + (CppGenerator.cpp_type_for_command_return_argument): Added. |
| 46 | + (CppGenerator.cpp_type_for_event_parameter): Added. |
| 47 | + (CppGenerator.cpp_type_for_enum): Added. |
| 48 | + (CppGenerator.should_move_argument): Added. |
| 49 | + (CppGenerator.should_release_argument): Added. |
| 50 | + (CppGenerator.should_dereference_argument): Added. |
| 51 | + (CppGenerator.cpp_protocol_type_for_type_member): Deleted. |
| 52 | + (CppGenerator.cpp_type_for_unchecked_formal_in_parameter): Deleted. |
| 53 | + (CppGenerator.cpp_type_for_checked_formal_event_parameter): Deleted. |
| 54 | + (CppGenerator.cpp_type_for_type_member): Deleted. |
| 55 | + (CppGenerator.cpp_type_for_type_with_name): Deleted. |
| 56 | + (CppGenerator.cpp_type_for_formal_out_parameter): Deleted. |
| 57 | + (CppGenerator.cpp_type_for_formal_async_parameter): Deleted. |
| 58 | + (CppGenerator.cpp_type_for_stack_in_parameter): Deleted. |
| 59 | + (CppGenerator.cpp_type_for_stack_out_parameter): Deleted. |
| 60 | + (CppGenerator.cpp_assertion_method_for_type_member): Deleted. |
| 61 | + (CppGenerator.cpp_assertion_method_for_type_member.assertion_method_for_type): Deleted. |
| 62 | + (CppGenerator.should_use_wrapper_for_return_type): Deleted. |
| 63 | + (CppGenerator.should_use_references_for_type): Deleted. |
| 64 | + (CppGenerator.should_pass_by_copy_for_return_type): Deleted. |
| 65 | + * inspector/scripts/codegen/generate_cpp_alternate_backend_dispatcher_header.py: |
| 66 | + (CppAlternateBackendDispatcherHeaderGenerator._generate_secondary_header_includes): |
| 67 | + (CppAlternateBackendDispatcherHeaderGenerator._generate_handler_declaration_for_command): |
| 68 | + * inspector/scripts/codegen/generate_cpp_backend_dispatcher_header.py: |
| 69 | + (CppBackendDispatcherHeaderGenerator.generate_output): |
| 70 | + (CppBackendDispatcherHeaderGenerator._generate_secondary_header_includes): |
| 71 | + (CppBackendDispatcherHeaderGenerator._generate_handler_declarations_for_domain): |
| 72 | + (CppBackendDispatcherHeaderGenerator._generate_handler_declaration_for_command): |
| 73 | + (CppBackendDispatcherHeaderGenerator._generate_async_handler_declaration_for_command): |
| 74 | + (CppBackendDispatcherHeaderGenerator._generate_dispatcher_declaration_for_command): |
| 75 | + (CppBackendDispatcherHeaderGenerator._generate_anonymous_enum_for_parameter): Deleted. |
| 76 | + * inspector/scripts/codegen/generate_cpp_backend_dispatcher_implementation.py: |
| 77 | + (CppBackendDispatcherImplementationGenerator._generate_secondary_header_includes): |
| 78 | + (CppBackendDispatcherImplementationGenerator._generate_small_dispatcher_switch_implementation_for_domain): |
| 79 | + (CppBackendDispatcherImplementationGenerator._generate_async_dispatcher_class_for_domain): |
| 80 | + (CppBackendDispatcherImplementationGenerator._generate_dispatcher_implementation_for_command): |
| 81 | + * inspector/scripts/codegen/generate_cpp_frontend_dispatcher_header.py: |
| 82 | + (CppFrontendDispatcherHeaderGenerator._generate_secondary_header_includes): |
| 83 | + (CppFrontendDispatcherHeaderGenerator._generate_dispatcher_declaration_for_event): |
| 84 | + * inspector/scripts/codegen/generate_cpp_frontend_dispatcher_implementation.py: |
| 85 | + (CppFrontendDispatcherImplementationGenerator._generate_secondary_header_includes): |
| 86 | + (CppFrontendDispatcherImplementationGenerator._generate_dispatcher_implementation_for_event): |
| 87 | + * inspector/scripts/codegen/generate_cpp_protocol_types_header.py: |
| 88 | + (CppProtocolTypesHeaderGenerator._generate_secondary_header_includes): |
| 89 | + * inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py: |
| 90 | + (CppProtocolTypesImplementationGenerator._generate_secondary_header_includes): |
| 91 | + (CppProtocolTypesImplementationGenerator._generate_enum_conversion_methods_for_domain): |
| 92 | + (CppProtocolTypesImplementationGenerator._generate_open_field_names): |
| 93 | + (CppProtocolTypesImplementationGenerator._generate_assertion_for_enum): |
| 94 | + * inspector/scripts/codegen/generate_objc_backend_dispatcher_header.py: |
| 95 | + (ObjCBackendDispatcherHeaderGenerator._generate_objc_handler_declaration_for_command): |
| 96 | + * inspector/scripts/codegen/generate_objc_backend_dispatcher_implementation.py: |
| 97 | + (ObjCBackendDispatcherImplementationGenerator._generate_handler_implementation_for_command): |
| 98 | + (ObjCBackendDispatcherImplementationGenerator._generate_success_block_for_command): |
| 99 | + (ObjCBackendDispatcherImplementationGenerator._generate_success_block_for_command.and): |
| 100 | + (ObjCBackendDispatcherImplementationGenerator._generate_conversions_for_command.in_param_expression): |
| 101 | + (ObjCBackendDispatcherImplementationGenerator._generate_conversions_for_command): |
| 102 | + (ObjCBackendDispatcherImplementationGenerator._generate_invocation_for_command): |
| 103 | + * inspector/scripts/codegen/generate_objc_frontend_dispatcher_implementation.py: |
| 104 | + (ObjCFrontendDispatcherImplementationGenerator._generate_event): |
| 105 | + (ObjCFrontendDispatcherImplementationGenerator._generate_event_out_parameters): |
| 106 | + * inspector/scripts/codegen/objc_generator_templates.py: |
| 107 | + * inspector/scripts/codegen/objc_generator.py: |
| 108 | + (ObjCGenerator.protocol_type_for_type): |
| 109 | + (ObjCGenerator.objc_type_for_param_internal): |
| 110 | + (ObjCGenerator.objc_protocol_import_expression_for_parameter): |
| 111 | + |
| 112 | + * inspector/protocol/Page.json: |
| 113 | + Now that enums are processed before being passed to backend dispacher handlers, the |
| 114 | + `appearance` parameter of `Page.setForcedAppearance` must be marked `optional` as |
| 115 | + there's no way for it to accept an empty string, as that's not possible for an enum. |
| 116 | + |
| 117 | + * inspector/agents/InspectorAgent.h: |
| 118 | + * inspector/agents/InspectorAgent.cpp: |
| 119 | + * inspector/agents/InspectorAuditAgent.h: |
| 120 | + * inspector/agents/InspectorAuditAgent.cpp: |
| 121 | + * inspector/agents/InspectorConsoleAgent.h: |
| 122 | + * inspector/agents/InspectorConsoleAgent.cpp: |
| 123 | + * inspector/agents/InspectorDebuggerAgent.h: |
| 124 | + * inspector/agents/InspectorDebuggerAgent.cpp: |
| 125 | + * inspector/agents/InspectorHeapAgent.h: |
| 126 | + * inspector/agents/InspectorHeapAgent.cpp: |
| 127 | + * inspector/agents/InspectorRuntimeAgent.h: |
| 128 | + * inspector/agents/InspectorRuntimeAgent.cpp: |
| 129 | + * inspector/agents/InspectorScriptProfilerAgent.h: |
| 130 | + * inspector/agents/InspectorScriptProfilerAgent.cpp: |
| 131 | + * inspector/agents/InspectorTargetAgent.h: |
| 132 | + * inspector/agents/InspectorTargetAgent.cpp: |
| 133 | + * inspector/agents/JSGlobalObjectAuditAgent.h: |
| 134 | + * inspector/agents/JSGlobalObjectAuditAgent.cpp: |
| 135 | + * inspector/agents/JSGlobalObjectDebuggerAgent.h: |
| 136 | + * inspector/agents/JSGlobalObjectDebuggerAgent.cpp: |
| 137 | + * inspector/agents/JSGlobalObjectRuntimeAgent.h: |
| 138 | + * inspector/agents/JSGlobalObjectRuntimeAgent.cpp: |
| 139 | + * inspector/JSGlobalObjectConsoleClient.cpp: |
| 140 | + * inspector/JSGlobalObjectInspectorController.cpp: |
| 141 | + Elided backend dispatcher handler changes describe above. |
| 142 | + |
| 143 | + * bindings/ScriptValue.cpp: |
| 144 | + (Inspector::jsToInspectorValue): |
| 145 | + * inspector/AsyncStackTrace.h: |
| 146 | + * inspector/AsyncStackTrace.cpp: |
| 147 | + (Inspector::AsyncStackTrace::buildInspectorObject const): |
| 148 | + * inspector/ConsoleMessage.cpp: |
| 149 | + (Inspector::ConsoleMessage::addToFrontend): |
| 150 | + * inspector/InjectedScriptBase.h: |
| 151 | + * inspector/InjectedScriptBase.cpp: |
| 152 | + (Inspector::InjectedScriptBase::makeEvalCall): |
| 153 | + (Inspector::InjectedScriptBase::checkCallResult): |
| 154 | + (Inspector::InjectedScriptBase::checkAsyncCallResult): |
| 155 | + * inspector/InjectedScript.h: |
| 156 | + * inspector/InjectedScript.cpp: |
| 157 | + (Inspector::InjectedScript::execute): |
| 158 | + (Inspector::InjectedScript::evaluate): |
| 159 | + (Inspector::InjectedScript::callFunctionOn): |
| 160 | + (Inspector::InjectedScript::evaluateOnCallFrame): |
| 161 | + (Inspector::InjectedScript::getFunctionDetails): |
| 162 | + (Inspector::InjectedScript::functionDetails): |
| 163 | + (Inspector::InjectedScript::getPreview): |
| 164 | + (Inspector::InjectedScript::getProperties): |
| 165 | + (Inspector::InjectedScript::getDisplayableProperties): |
| 166 | + (Inspector::InjectedScript::getInternalProperties): |
| 167 | + (Inspector::InjectedScript::getCollectionEntries): |
| 168 | + (Inspector::InjectedScript::saveResult): |
| 169 | + (Inspector::InjectedScript::wrapCallFrames const): |
| 170 | + (Inspector::InjectedScript::wrapObject const): |
| 171 | + (Inspector::InjectedScript::wrapJSONString const): |
| 172 | + (Inspector::InjectedScript::wrapTable const): |
| 173 | + (Inspector::InjectedScript::previewValue const): |
| 174 | + * inspector/InjectedScriptManager.cpp: |
| 175 | + (Inspector::InjectedScriptManager::injectedScriptForObjectId): |
| 176 | + * inspector/InspectorBackendDispatcher.h: |
| 177 | + * inspector/InspectorBackendDispatcher.cpp: |
| 178 | + (Inspector::BackendDispatcher::CallbackBase::sendSuccess): |
| 179 | + (Inspector::BackendDispatcher::dispatch): |
| 180 | + (Inspector::BackendDispatcher::sendResponse): |
| 181 | + (Inspector::BackendDispatcher::getPropertyValue): |
| 182 | + (Inspector::BackendDispatcher::getBoolean): |
| 183 | + (Inspector::BackendDispatcher::getInteger): |
| 184 | + (Inspector::BackendDispatcher::getDouble): |
| 185 | + (Inspector::BackendDispatcher::getString): |
| 186 | + (Inspector::BackendDispatcher::getValue): |
| 187 | + (Inspector::BackendDispatcher::getObject): |
| 188 | + (Inspector::BackendDispatcher::getArray): |
| 189 | + (Inspector::castToInteger): Deleted. |
| 190 | + (Inspector::castToNumber): Deleted. |
| 191 | + * inspector/InspectorProtocolTypes.h: |
| 192 | + (Inspector::Protocol::BindingTraits<JSON::ArrayOf<T>>::runtimeCast): |
| 193 | + (Inspector::Protocol::BindingTraits<JSON::ArrayOf<T>>::assertValueHasExpectedType): |
| 194 | + * inspector/remote/socket/RemoteInspectorConnectionClient.cpp: |
| 195 | + (Inspector::RemoteInspectorConnectionClient::extractEvent): |
| 196 | + * inspector/remote/socket/RemoteInspectorSocket.cpp: |
| 197 | + (Inspector::RemoteInspector::pushListingsNow): |
| 198 | + * runtime/TypeSet.cpp: |
| 199 | + (JSC::StructureShape::inspectorRepresentation): |
| 200 | + `JSON` classes now use `Ref&&` wherever possible and `Optional` instead of an out parameter |
| 201 | + for `get*`/`as*` so that values can be more easily manipulated and can be confidently known |
| 202 | + to exist. |
| 203 | + |
| 204 | + * inspector/scripts/tests/enum-values.json: |
| 205 | + * inspector/scripts/tests/expected/command-targetType-matching-domain-debuggableType.json-result: |
| 206 | + * inspector/scripts/tests/expected/commands-with-async-attribute.json-result: |
| 207 | + * inspector/scripts/tests/expected/commands-with-optional-call-return-parameters.json-result: |
| 208 | + * inspector/scripts/tests/expected/definitions-with-mac-platform.json-result: |
| 209 | + * inspector/scripts/tests/expected/domain-debuggableTypes.json-result: |
| 210 | + * inspector/scripts/tests/expected/domain-targetType-matching-domain-debuggableType.json-result: |
| 211 | + * inspector/scripts/tests/expected/domain-targetTypes.json-result: |
| 212 | + * inspector/scripts/tests/expected/domains-with-varying-command-sizes.json-result: |
| 213 | + * inspector/scripts/tests/expected/enum-values.json-result: |
| 214 | + * inspector/scripts/tests/expected/event-targetType-matching-domain-debuggableType.json-result: |
| 215 | + * inspector/scripts/tests/expected/events-with-optional-parameters.json-result: |
| 216 | + * inspector/scripts/tests/expected/generate-domains-with-feature-guards.json-result: |
| 217 | + * inspector/scripts/tests/expected/same-type-id-different-domain.json-result: |
| 218 | + * inspector/scripts/tests/expected/shadowed-optional-type-setters.json-result: |
| 219 | + * inspector/scripts/tests/expected/should-strip-comments.json-result: |
| 220 | + * inspector/scripts/tests/expected/type-declaration-aliased-primitive-type.json-result: |
| 221 | + * inspector/scripts/tests/expected/type-declaration-array-type.json-result: |
| 222 | + * inspector/scripts/tests/expected/type-declaration-enum-type.json-result: |
| 223 | + * inspector/scripts/tests/expected/type-declaration-object-type.json-result: |
| 224 | + * inspector/scripts/tests/expected/type-requiring-runtime-casts.json-result: |
| 225 | + * inspector/scripts/tests/expected/type-with-open-parameters.json-result: |
| 226 | + * inspector/scripts/tests/expected/version.json-result: |
| 227 | + |
1 | 228 | 2020-09-09 Saam Barati < [email protected]>
|
2 | 229 |
|
3 | 230 | OutOfBoundsSaneChain operations should use their own heap locations
|
|
0 commit comments