Created attachment 318535 [details] WIP Patch

Created attachment 318538 [details] Patch

<rdar://problem/33969686>

Created attachment 318541 [details] Patch

Created attachment 318549 [details] Patch

Comment on attachment 318549 [details] 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.

Comment on attachment 318549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318549&action=review >> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:49 >> + beacon->removeClient(*this); > > Is there really some risk of reentry here? If not, it seems unnecessary to move m_inflightBeacons into a local variable. I don't think this is an issue currently. I'll get rid of the local. >> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:54 >> + NavigatorBeacon* supplement = static_cast<NavigatorBeacon*>(Supplement<Navigator>::from(navigator, supplementName())); > > Please consider auto* here. Ok. >> 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. I think you may be confused by what removeFirst() does here. It removes the first match in the vector, not the first item in the vector. >> 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. Oops. I guess it works because of the "operator const char*() { return m_characters; }" operator on ASCIILiteral. Will fix. >> 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. Ok. >> Source/WebCore/Modules/beacon/NavigatorBeacon.h:51 >> + static NavigatorBeacon* from(Navigator*); > > Can this take a reference instead of a pointer? Ok. >> Source/WebCore/Modules/beacon/NavigatorBeacon.h:56 >> + 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. Ok. >> 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. This is not new code but I can take a look. Do you mean that we now prefer to inline the body of the timeoutTimerFired() method inside the init list?

Comment on attachment 318549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318549&action=review >>> 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. > > I think you may be confused by what removeFirst() does here. It removes the first match in the vector, not the first item in the vector. Technically, this could be a HashSet. It seemed a little overkill at first but I guess a page could send a lot of beacons. Please let me know if you'd prefer a HashSet.

Created attachment 318556 [details] Patch

(In reply to Chris Dumez from comment #8) > Comment on attachment 318549 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=318549&action=review > > >>> 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. > > > > I think you may be confused by what removeFirst() does here. It removes the first match in the vector, not the first item in the vector. > > Technically, this could be a HashSet. It seemed a little overkill at first > but I guess a page could send a lot of beacons. Please let me know if you'd > prefer a HashSet. It seems we don't currently have the right traits to have a HashSet of CachedResourceHandles.

Comment on attachment 318556 [details] Patch Clearing flags on attachment: 318556 Committed r220946: <http://trac.webkit.org/changeset/220946>

All reviewed patches have been landed. Closing bug.