147366 – [PerformanceTiming] Don't expose the restrictedKeyMap() HashMap

Bug 147366 - [PerformanceTiming] Don't expose the restrictedKeyMap() HashMap

Summary: [PerformanceTiming] Don't expose the restrictedKeyMap() HashMap

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	New Bugs (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Zan Dobersek

URL:
Keywords:

Depends on:
Blocks:

Reported:	2015-07-28 12:38 PDT by Zan Dobersek
Modified:	2016-01-04 23:46 PST (History)
CC List:	2 users (show)

See Also:

Attachments
Patch (5.11 KB, patch) 2015-07-28 12:43 PDT, Zan Dobersek	no flags	Details \| Formatted Diff \| Diff
Patch (6.86 KB, patch) 2015-08-13 11:22 PDT, Zan Dobersek	no flags	Details \| Formatted Diff \| Diff
Patch (6.66 KB, patch) 2016-01-03 06:49 PST, Zan Dobersek	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Zan Dobersek 2015-07-28 12:38:42 PDT

restrictedKeyMap() in PerformanceUserTiming.cpp should return a const reference to the HashMap

Comment 1 Zan Dobersek 2015-07-28 12:43:17 PDT

Created attachment 257670 [details]
Patch

Comment 2 Darin Adler 2015-08-09 18:04:23 PDT

Comment on attachment 257670 [details]
Patch

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

> Source/WebCore/page/PerformanceUserTiming.cpp:68
> +    static NeverDestroyed<RestrictedKeyMap> map;
> +    if (map.get().isEmpty()) {
> +        map.get().add("navigationStart", &PerformanceTiming::navigationStart);
> +        map.get().add("unloadEventStart", &PerformanceTiming::unloadEventStart);
> +        map.get().add("unloadEventEnd", &PerformanceTiming::unloadEventEnd);
> +        map.get().add("redirectStart", &PerformanceTiming::redirectStart);
> +        map.get().add("redirectEnd", &PerformanceTiming::redirectEnd);
> +        map.get().add("fetchStart", &PerformanceTiming::fetchStart);
> +        map.get().add("domainLookupStart", &PerformanceTiming::domainLookupStart);
> +        map.get().add("domainLookupEnd", &PerformanceTiming::domainLookupEnd);
> +        map.get().add("connectStart", &PerformanceTiming::connectStart);
> +        map.get().add("connectEnd", &PerformanceTiming::connectEnd);
> +        map.get().add("secureConnectionStart", &PerformanceTiming::secureConnectionStart);
> +        map.get().add("requestStart", &PerformanceTiming::requestStart);
> +        map.get().add("responseStart", &PerformanceTiming::responseStart);
> +        map.get().add("responseEnd", &PerformanceTiming::responseEnd);
> +        map.get().add("domLoading", &PerformanceTiming::domLoading);
> +        map.get().add("domInteractive", &PerformanceTiming::domInteractive);
> +        map.get().add("domContentLoadedEventStart", &PerformanceTiming::domContentLoadedEventStart);
> +        map.get().add("domContentLoadedEventEnd", &PerformanceTiming::domContentLoadedEventEnd);
> +        map.get().add("domComplete", &PerformanceTiming::domComplete);
> +        map.get().add("loadEventStart", &PerformanceTiming::loadEventStart);
> +        map.get().add("loadEventEnd", &PerformanceTiming::loadEventEnd);
> +    };

This is not a good way to create the map. All the adds end up getting inlined. Instead we should use a loop to build the map. I think the best practice is to write a separate create function.

UserTiming::findExistingMarkStartTime also has double hash lookup mistakes in it.

I’m not even sure we should expose the map. I think that a function that takes a string and returns a possibly-null NavigationTimingFunction would be better.

Comment 3 Zan Dobersek 2015-08-13 11:22:46 PDT

Created attachment 258904 [details]
Patch

Comment 4 WebKit Commit Bot 2015-08-13 11:25:49 PDT

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


ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:46:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:47:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:48:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:49:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:50:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:51:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:52:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:53:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:54:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:55:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:56:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:57:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:58:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:59:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:60:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:61:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:62:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:63:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:64:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:65:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:66:  Missing space before {  [whitespace/braces] [5]
Total errors found: 21 in 2 files


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

Comment 5 Zan Dobersek 2016-01-03 06:49:55 PST

Created attachment 268136 [details]
Patch

Comment 6 WebKit Commit Bot 2016-01-03 06:51:36 PST

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


ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:46:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:47:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:48:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:49:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:50:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:51:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:52:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:53:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:54:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:55:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:56:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:57:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:58:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:59:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:60:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:61:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:62:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:63:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:64:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:65:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/PerformanceUserTiming.cpp:66:  Missing space before {  [whitespace/braces] [5]
Total errors found: 21 in 2 files


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

Comment 7 Zan Dobersek 2016-01-04 23:46:32 PST

Comment on attachment 268136 [details]
Patch

Clearing flags on attachment: 268136

Committed r194578: <http://trac.webkit.org/changeset/194578>

Comment 8 Zan Dobersek 2016-01-04 23:46:39 PST

All reviewed patches have been landed.  Closing bug.