[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 #9 from Umar Iqbal <uiqbal at apple.com> ---
(In reply to katherine_cheney from comment #4)
> Comment on attachment 406118 [details]
> 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?

Moved the checks inside the if statement block to WebCore::WebResourceLoadObserver

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

Fixed.

> 
> > Source/WebCore/loader/ExecutionStack.h:65
> > +    // This HashMap maintains the APIs called by a script in a docuemnt.
> 
> Oops, typo -- this should be document.

Fixed.

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

I used RELEASE_ASSERT because the WebResourceLoadObserver ::ensureResourceStatisticsForRegistrableDomain also uses it. 

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

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:282
> > +    uint64_t frameID { document.frameID()->toUInt64() };
> 
> You can use auto here, it's shorter.
> 

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:430
> > +            documentWithScriptsIterator->value.set(activeScriptID, apis);
> 
> I think you can use WTFMove(apis) here and subsequent calls.

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

-- 
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/2aa140d9/attachment.htm>