Bug 165975 – Address some style problems found by static analysis

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

[Brent Fulgham]

The static analyzer identified some miscellaneous problems in the code:

1. Some methods did not actually override the expected method due to signature differences.
2. Some large arguments were being passed by value.

This patch tidies up these issues.

[Brent Fulgham]

Created attachment 297374 [details]
Patch

[Brent Fulgham]

Note that the instances of "const int*" -> "const int* const", etc., were to match the method declarations in the implementation files.

[Alex Christensen]

Comment on attachment 297374 [details]
Patch

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

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:70
> +    void setBreakpointByUrl(ErrorString&, int lineNumber, const String* const optionalURL, const String* const optionalURLRegex, const int* const optionalColumnNumber, const Inspector::InspectorObject* options, Inspector::Protocol::Debugger::BreakpointId*, RefPtr<Inspector::Protocol::Array<Inspector::Protocol::Debugger::Location>>& locations) final;

This change caused a warning on Windows.  Let's double check it.

> Source/WebKit2/Shared/WebBackForwardListItem.h:47
> -    static PassRefPtr<WebBackForwardListItem> create(BackForwardListItemState, uint64_t pageID);
> +    static PassRefPtr<WebBackForwardListItem> create(BackForwardListItemState&&, uint64_t pageID);

As long as we're touching this, can we make this return a Ref?

[Brent Fulgham]

Comment on attachment 297374 [details]
Patch

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

>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:70
>> +    void setBreakpointByUrl(ErrorString&, int lineNumber, const String* const optionalURL, const String* const optionalURLRegex, const int* const optionalColumnNumber, const Inspector::InspectorObject* options, Inspector::Protocol::Debugger::BreakpointId*, RefPtr<Inspector::Protocol::Array<Inspector::Protocol::Debugger::Location>>& locations) final;
> 
> This change caused a warning on Windows.  Let's double check it.

I think this is due to out-of-sync intermediate files. It's clear that JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp is declared as "const String* const" (etc.), so the headers should be made to match.

[Brent Fulgham]

Created attachment 297521 [details]
Patch

[WebKit Commit Bot]

Attachment 297521 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/PageDebuggerAgent.h:68:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:86:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 297527 [details]
Patch

[WebKit Commit Bot]

Attachment 297527 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/PageDebuggerAgent.h:68:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:86:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

(In reply to comment #4)
> Comment on attachment 297374 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297374&action=review
> 
> >> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:70
> >> +    void setBreakpointByUrl(ErrorString&, int lineNumber, const String* const optionalURL, const String* const optionalURLRegex, const int* const optionalColumnNumber, const Inspector::InspectorObject* options, Inspector::Protocol::Debugger::BreakpointId*, RefPtr<Inspector::Protocol::Array<Inspector::Protocol::Debugger::Location>>& locations) final;
> > 
> > This change caused a warning on Windows.  Let's double check it.
> 
> I think this is due to out-of-sync intermediate files. It's clear that
> JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp is declared as
> "const String* const" (etc.), so the headers should be made to match.

Someday these will use std::optional.

[Blaze Burg]

Comment on attachment 297527 [details]
Patch

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

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:142
> +                return 'const %s* const' % cpp_name

You'll need to rebaseline the expected test results:

./Tools/Scripts/run-inspector-generator-tests --reset-results

[Blaze Burg]

(In reply to comment #6)
> Attachment 297521 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/inspector/PageDebuggerAgent.h:68:  The parameter name
> "const" adds no information, so it should be removed. 
> [readability/parameter_name] [5]
> ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:86: 
> The parameter name "const" adds no information, so it should be removed. 
> [readability/parameter_name] [5]
> Total errors found: 2 in 20 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

LOL, please file a bug about this.

[Brent Fulgham]

(In reply to comment #11)
> (In reply to comment #6)
> > Attachment 297521 [details] did not pass style-queue:
> > 
> > 
> > ERROR: Source/WebCore/inspector/PageDebuggerAgent.h:68:  The parameter name
> > "const" adds no information, so it should be removed. 
> > [readability/parameter_name] [5]
> > ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:86: 
> > The parameter name "const" adds no information, so it should be removed. 
> > [readability/parameter_name] [5]
> > Total errors found: 2 in 20 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against
> > check-webkit-style.
> 
> LOL, please file a bug about this.

Already did! Bug 166067.

[Brent Fulgham]

Created attachment 297532 [details]
Patch

[WebKit Commit Bot]

Attachment 297532 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/PageDebuggerAgent.h:68:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:86:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 297532 [details]
Patch

Clearing flags on attachment: 297532

Committed r210042: <http://trac.webkit.org/changeset/210042>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.