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.
Created attachment 297374 [details] Patch
Note that the instances of "const int*" -> "const int* const", etc., were to match the method declarations in the implementation files.
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?
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.
Created attachment 297521 [details] Patch
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.
Created attachment 297527 [details] Patch
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.
(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.
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
(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.
(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.
Created attachment 297532 [details] Patch
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.
Comment on attachment 297532 [details] Patch Clearing flags on attachment: 297532 Committed r210042: <http://trac.webkit.org/changeset/210042>
All reviewed patches have been landed. Closing bug.