[webkit-reviews] review granted: [Bug 175723] [Beacon] Improve error reporting : [Attachment 318549] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 175723: [Beacon] Improve error reporting
https://bugs.webkit.org/show_bug.cgi?id=175723

Attachment 318549: Patch

https://bugs.webkit.org/attachment.cgi?id=318549&action=review




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 318549
  --> https://bugs.webkit.org/attachment.cgi?id=318549
Patch

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

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:49
> +    auto inflightBeacons = WTFMove(m_inflightBeacons);
> +    for (auto& beacon : inflightBeacons)
> +	   beacon->removeClient(*this);

Is there really some risk of reentry here? If not, it seems unnecessary to move
m_inflightBeacons into a local variable.

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:54
> +    NavigatorBeacon* supplement =
static_cast<NavigatorBeacon*>(Supplement<Navigator>::from(navigator,
supplementName()));

Please consider auto* here.

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:74
> +    bool wasRemoved = m_inflightBeacons.removeFirst(&resource);

If the operations we need are append and removeFirst, then normally we would
want to use a Deque, not a Vector.

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:92
> +    String description = error.localizedDescription();

Strange to use a "localized" description and then append hard-coded English
text to it.

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:95
> +	       messageMiddle = ASCIILiteral(" due to access control checks.");

Why does this even compile? We are assigning to a const char*. Or we can have
the local variable be of type ASCIILiteral, which I think would be better.

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:100
> +    document->addConsoleMessage(MessageSource::Network, MessageLevel::Error,
makeString("Beacon API cannot load ", error.failingURL().string(),
messageMiddle, description));

We could do ASCIILiteral(messageMiddle) here.

> Source/WebCore/Modules/beacon/NavigatorBeacon.h:51
> +    static NavigatorBeacon* from(Navigator*);

Can this take a reference instead of a pointer?

> Source/WebCore/Modules/beacon/NavigatorBeacon.h:56
> +    // CachedRawResourceClient
> +    void notifyFinished(CachedResource&) final;
> +    void logError(const ResourceError&);

Seems unnecessary to comment which class we are overriding here. But especially
strange to label this paragraph with the class name, but only one of the two
functions here is an override.

> Source/WebCore/platform/network/PingHandle.h:48
> +	   , m_timeoutTimer(*this, &PingHandle::timeoutTimerFired)

Consider using the lambda form of Timer instead of this one? I think we should
use it in all new code, and only use the pointer-to-member-function one in
legacy code.