[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 #11 from John Wilander <wilander at apple.com> ---
Comment on attachment 406201
  --> https://bugs.webkit.org/attachment.cgi?id=406201
Patch

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

Looks good to me. I trust Keith has reviewed the JSC parts which I'm not proficient in. I focused on the observer.

> Source/WebCore/dom/Document.h:639
> +        Optional<FrameIdentifier> frameID() const;

Double indent.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:288
> +        // We use HashMap.add here because there is a chance that script URL is none (in case of microtasks).

I typically put empty parenthesis after function names to make it obvious that they are functions. Here that would be HashMap.add().

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

Ditto.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:320
> +        return 0;

Is 0 guaranteed to mean "empty" here? Or do we risk having all scripts be identified as 0? A better way is to use Optional<> with WTF::nullopt representing the empty result.

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

Can we come up with a better variable name than temp?

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:344
> +    return 0;

Same thing for 0 here.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:350
> +        return 0;

Same thing for 0 here.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:364
> +    return 0;

Same thing for 0 here.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:370
> +        return { };

Is an empty string the right thing to return here or could Optional<> work? This one doesn't seem as important though since it's just for text output. Not commenting further on returning empty strings. Doesn't seem like a big issue.

-- 
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/a81ef0ad/attachment-0001.htm>