Bug 137636 - Web Inspector: move unchecked InspectorValues into Protocol namespace
Summary: Web Inspector: move unchecked InspectorValues into Protocol namespace
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-10-11 10:00 PDT by Brian Burg
Modified: 2018-01-24 14:47 PST (History)
13 users (show)

See Also:


Attachments
Patch (430.62 KB, patch)
2014-10-11 10:16 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (431.07 KB, patch)
2014-10-11 15:29 PDT, Brian Burg
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-10-11 10:00:00 PDT
In efforts to use unchecked inspector objects less, these are moved into the Protocol namespace. Subsequent patches will start converting agents to use Protocol::Domain::Object instances instead of unchecked objects.

Renamings:

Inspector{Object, Value, Array} -> Protocol::Unchecked{Object, Value, Array}
InspectorString, InspectorBasicValue -> Protocol::ScalarValue

Refactor:
* Remove ArrayItemHelper
* Combine InspectorValues.h and InspectorProtocolTypes.h
Comment 1 Radar WebKit Bug Importer 2014-10-11 10:00:37 PDT
<rdar://problem/18623109>
Comment 2 Brian Burg 2014-10-11 10:16:13 PDT
Created attachment 239678 [details]
Patch
Comment 3 WebKit Commit Bot 2014-10-11 10:16:59 PDT
This patch modifies the WEB_REPLAY inputs generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-input-generator-tests --reset-results`)

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)
Comment 4 WebKit Commit Bot 2014-10-11 10:17:25 PDT
Attachment 239678 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:49:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:50:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:51:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:52:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:53:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:54:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:55:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:56:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:57:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:58:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:59:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:101:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:408:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:446:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:449:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:492:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:761:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:772:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.h:97:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.h:445:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.h:450:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.h:455:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.h:460:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.h:465:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.h:470:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.h:475:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.h:480:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.h:485:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/inspector/InspectorStyleSheet.h:66:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:47:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py:253:  whitespace before ')'  [pep8/E202] [5]
Total errors found: 32 in 92 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Brian Burg 2014-10-11 11:33:22 PDT
Looks like I will need to revert the InspectorString -> ScalarValue change. It's not safe to put WTF::String inside a union.
Comment 6 Brian Burg 2014-10-11 15:29:08 PDT
Created attachment 239688 [details]
Patch
Comment 7 WebKit Commit Bot 2014-10-11 15:30:46 PDT
Attachment 239688 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:49:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:50:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:51:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:52:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:53:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:54:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:55:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:56:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:57:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:58:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.cpp:59:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:47:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py:253:  whitespace before ')'  [pep8/E202] [5]
Total errors found: 14 in 92 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Joseph Pecoraro 2014-10-16 12:09:37 PDT
Comment on attachment 239688 [details]
Patch

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

Looks good to me. Despite my initial aversion to the name it actually is reasonable and a keen naming choice.

> Source/JavaScriptCore/ChangeLog:11
> +        whose fields are shape-checked when going across the channel to or from the frontend>

Typo: '>'

> Source/JavaScriptCore/inspector/ConsoleMessage.cpp:227
> -        jsonObj->setStackTrace(m_callStack->buildInspectorArray());
> +        jsonObj->setStackTrace(m_callStack->buildUncheckedArray());

Maybe it is just me, but I find "buildInspectorArray" to be clearer than "buildUncheckedArray".

> Source/WebCore/inspector/InspectorApplicationCacheAgent.h:39
> +class UncheckedObject;
> +class UncheckedValue;

In the wrong namespace. Needs namespace Protocol {}

> Source/WebCore/inspector/InspectorPageAgent.h:47
> +class UncheckedArray;

Namespace.

> Source/WebCore/inspector/InspectorResourceAgent.h:45
> +class UncheckedArray;

Namespace.

> Source/WebCore/inspector/InspectorWorkerAgent.h:40
> +class UncheckedObject;

Namespace. Given that these are in the wrong namespace maybe they are unnecessary forward declarations due to the includes. So maybe they can just be removed.
Comment 9 Joseph Pecoraro 2018-01-24 14:42:51 PST
I don't think this is applicable anymore, right?