[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 #14 from Umar Iqbal <uiqbal at apple.com> ---
(In reply to John Wilander from comment #11)
> Comment on attachment 406201 [details]
> 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.

Fixed.

> 
> > 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().

Fixed.

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

Fixed.

> 
> > 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.

Thanks for the suggestion. I use Optional<> now.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:331
> > +            auto temp = executingScriptsStackIterator->value.last();
> 
> Can we come up with a better variable name than temp?
>

Changed name to scriptID.

> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:344
> > +    return 0;
> 
> Same thing for 0 here.

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:350
> > +        return 0;
> 
> Same thing for 0 here.
> 

Fixed.

> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:364
> > +    return 0;
> 
> Same thing for 0 here.
> 

Fixed.

> > 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.


Using Optional<String> now.

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