Bug 179867 – Eliminate some cases of double hashing, other related refactoring

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

[Darin Adler]

Eliminate some cases of double hashing, other related refactoring

[Darin Adler]

Created attachment 327328 [details]
Patch

[EWS Watchlist]

Comment on attachment 327328 [details]
Patch

Attachment 327328 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5295660

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 327330 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Darin Adler]

Created attachment 327331 [details]
Patch

[Sam Weinig]

Comment on attachment 327331 [details]
Patch

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

> Source/WebCore/html/FormController.cpp:325
> +        String signature = formSignature(*form);

I would use auto here.

> Source/WebCore/html/FormController.h:37
> +using FormControlState = Vector<String>;

Nice.

> Source/WebCore/html/HTMLTextAreaElement.cpp:117
> +    return m_isDirty ? FormControlState { { value() } } : FormControlState { };

In cases like these, I wonder if 

if (!m_dirty)
    return { };
return { { value() } };

is nicer due to not needing to restate FormControlState.  But looking at it, I think the ternary is nicer.

[Darin Adler]

Committed r225037: <https://trac.webkit.org/changeset/225037>

[Radar WebKit Bug Importer]

<rdar://problem/35639247>

[Don Olmstead]

I'm seeing compile issues with the AppleWin debug build that seem to point to this patch when compiling WebKitLegacy.

warning C4373: 'WebCore::InspectorDOMAgent::resolveNode': virtual function overrides 'Inspector::DOMBackendDispatcherHandler::resolveNode', previous versions of the compiler did not override when parameters only differed by const/volatile qualifiers

https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4373

Adding #pragma warning( disable : 4373) to InspectorDOMAgent.h seems to get things to build.

Should the definitions in DOMBackendDispatchHandler be updated or should we just be disabling this warning on Windows?

[Darin Adler]

(In reply to Don Olmstead from comment #8)
> Should the definitions in DOMBackendDispatchHandler be updated or should we
> just be disabling this warning on Windows?

The warning doesn’t point to any problem we actually have. It points to problems we might have had with "previous versions of the compiler". It’s OK to disable the warning.

But I think that an even better fix for this issues is to remove *all* the bogus const from DOMBackendDispatchHandler and wherever else we have it. There is not a good reason to mark function arguments themselves const.

[Blaze Burg]

(In reply to Darin Adler from comment #9)
> (In reply to Don Olmstead from comment #8)
> > Should the definitions in DOMBackendDispatchHandler be updated or should we
> > just be disabling this warning on Windows?
> 
> The warning doesn’t point to any problem we actually have. It points to
> problems we might have had with "previous versions of the compiler". It’s OK
> to disable the warning.
> 
> But I think that an even better fix for this issues is to remove *all* the
> bogus const from DOMBackendDispatchHandler and wherever else we have it.
> There is not a good reason to mark function arguments themselves const.

Thanks for the suggestion. I'll take a look at this when I get some free time to hack on the protocol generator some more. Filed: https://bugs.webkit.org/show_bug.cgi?id=180043