Summary: | Cleanup code that finds and loads a media engine | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Eric Carlson
2015-09-19 09:34:10 PDT
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 |