[Webkit-unassigned] [Bug 215208] JS API access capture and their linking with executing scripts

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=215208

--- Comment #4 from katherine_cheney at apple.com ---
Comment on attachment 406118
  --> https://bugs.webkit.org/attachment.cgi?id=406118
Patch

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

Nice job on the bots!

I see a lot of repetition in the if (RuntimeEnabledFeatures::sharedFeatures().webAPIStatisticsEnabled()) checks. Is it possible to split that code out into a separate function that takes a String argument?

I mostly left comments on how to combine some lines of code.

> Source/WebCore/loader/ExecutionStack.h:62
> +    // This HashMap maintains the mapping between FrameID, ScriptID, and Script URLs

Comment is missing a period.

> Source/WebCore/loader/ExecutionStack.h:65
> +    // This HashMap maintains the APIs called by a script in a docuemnt.

Oops, typo -- this should be document.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:104
> +    RELEASE_ASSERT(!isEphemeral());

Do we want this to crash in this case? It shouldn't be possible to hit this code, but maybe a debug assert would be better here?

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:281
> +    RegistrableDomain registrableDomain { document.url() };

I think you can delete this line and just initialize in the function call, see below.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:282
> +    uint64_t frameID { document.frameID()->toUInt64() };

You can use auto here, it's shorter.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:284
> +    auto& executionStack = ensureExecutionStackForRegistrableDomain(registrableDomain);

auto& executionStack = ensureExecutionStackForRegistrableDomain(RegistrableDomain{ document.url() });

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:286
> +    // register script id with script url for each frame

Comments usually start with a capital letter and end with a period.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:290
> +        // add will ensure that the exisitng script URL does not get overwritten.

Ditto here about capitalization.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:325
> +    uint64_t frameID { document.frameID()->toUInt64() };

Ditto about using auto.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:327
> +    auto& executionStack = ensureExecutionStackForRegistrableDomain(registrableDomain);

auto& executionStack = ensureExecutionStackForRegistrableDomain(RegistrableDomain{ document.url() });

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:334
> +            temp = executingScriptsStackIterator->value.last();

auto temp = executingScriptsStackIterator->value.last(); ?

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:359
> +    auto& executionStack = ensureExecutionStackForRegistrableDomain(registrableDomain);

Same comments as above.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:378
> +    auto& executionStack = ensureExecutionStackForRegistrableDomain(registrableDomain);

Same comments as above.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:419
> +    auto& statistics = ensureExecutionStackForRegistrableDomain(registrableDomain);

Same comments here as above -- you can use auto for the frameID and call RegistrableDomain { document.url() } in the function call.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:430
> +            documentWithScriptsIterator->value.set(activeScriptID, apis);

I think you can use WTFMove(apis) here and subsequent calls.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:437
> +        apisExecutingInScript.set(activeScriptID, apis);

Ditto, WTFMove(apis)

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:455
> +    auto& statistics = ensureExecutionStackForRegistrableDomain(registrableDomain);

auto& statistics = ensureExecutionStackForRegistrableDomain(RegistrableDomain { document.url() });

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:456
> +    String APIJSONString = statistics.toJSONString(frameID);

String APIJSONString = statistics.toJSONString(frameID { document.frameID()->toUInt64() });

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200807/88a15a91/attachment.htm>