Bug 149371 – Cleanup code that finds and loads a media engine

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Cleanup code that finds and loads a media engine

[Radar WebKit Bug Importer]

<rdar://problem/22770847>

[Eric Carlson]

Created attachment 261576 [details]
Proposed patch.

[WebKit Commit Bot]

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.

[Eric Carlson]

Created attachment 261585 [details]
Proposed patch.

[Darin Adler]

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

[Eric Carlson]

Created attachment 261587 [details]
Updated patch.

[Eric Carlson]

Created attachment 261588 [details]
Patch for the bots.

[Eric Carlson]

(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!

[Eric Carlson]

Created attachment 261589 [details]
APFTB

[Eric Carlson]

Committed r190020: https://trac.webkit.org/r190020