Cleanup code that finds and loads a media engine
<rdar://problem/22770847>
Created attachment 261576 [details] Proposed patch.
Attachment 261576 [details] did not pass style-queue: ERROR: Source/WebCore/html/HTMLMediaElement.cpp:1312: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 261585 [details] Proposed patch.
Comment on attachment 261585 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=261585&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:42 > +URLRegistry* MediaStream::s_registry = 0; nullptr, but also, why not use a value that is private to the cpp file instead of a static class member? > Source/WebCore/Modules/mediastream/MediaStream.cpp:50 > +MediaStream* MediaStream::lookup(const String& url) Why does this take a String if it’s a URL? > Source/WebCore/Modules/mediastream/MediaStream.cpp:55 > + if (s_registry) > + return static_cast<MediaStream*>(s_registry->lookup(url)); > + > + return nullptr; Normally we’d do an early return for the failure case. > Source/WebCore/Modules/mediastream/MediaStream.h:55 > + static void setRegistry(URLRegistry*); Would be better for this to take a reference instead of a pointer, unless we can use this to clear out the registry. > Source/WebCore/Modules/mediastream/MediaStream.h:56 > + static MediaStream* lookup(const String&); Since it's a verb, it should be two words, "look up", not the noun "lookup". So "lookUp".
Created attachment 261587 [details] Updated patch.
Created attachment 261588 [details] Patch for the bots.
(In reply to comment #5) > Comment on attachment 261585 [details] > Proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=261585&action=review > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:42 > > +URLRegistry* MediaStream::s_registry = 0; > > nullptr, but also, why not use a value that is private to the cpp file > instead of a static class member? > Fix both issues. > > Source/WebCore/Modules/mediastream/MediaStream.cpp:50 > > +MediaStream* MediaStream::lookup(const String& url) > > Why does this take a String if it’s a URL? > It calls a method in URLRegistry that takes a String, but that should be changed. Fixed this for now, will follow up with a patch for URLRegistry. > > Source/WebCore/Modules/mediastream/MediaStream.cpp:55 > > + if (s_registry) > > + return static_cast<MediaStream*>(s_registry->lookup(url)); > > + > > + return nullptr; > > Normally we’d do an early return for the failure case. > Fixed. > > Source/WebCore/Modules/mediastream/MediaStream.h:55 > > + static void setRegistry(URLRegistry*); > > Would be better for this to take a reference instead of a pointer, unless we > can use this to clear out the registry. > Fixed. > > Source/WebCore/Modules/mediastream/MediaStream.h:56 > > + static MediaStream* lookup(const String&); > > Since it's a verb, it should be two words, "look up", not the noun "lookup". > So "lookUp". > Fixed. Thanks!
Created attachment 261589 [details] APFTB
Committed r190020: https://trac.webkit.org/r190020