Bug 149371 - Cleanup code that finds and loads a media engine
Summary: Cleanup code that finds and loads a media engine
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-09-19 09:34 PDT by Eric Carlson
Modified: 2015-09-19 17:44 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch. (11.97 KB, patch)
2015-09-19 09:55 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Proposed patch. (12.25 KB, patch)
2015-09-19 13:25 PDT, Eric Carlson
darin: review+
Details | Formatted Diff | Diff
Updated patch. (12.87 KB, patch)
2015-09-19 14:59 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for the bots. (13.01 KB, patch)
2015-09-19 15:05 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
APFTB (13.00 KB, patch)
2015-09-19 16:24 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2015-09-19 09:34:10 PDT
Cleanup code that finds and loads a media engine
Comment 1 Radar WebKit Bug Importer 2015-09-19 09:35:07 PDT
<rdar://problem/22770847>
Comment 2 Eric Carlson 2015-09-19 09:55:59 PDT
Created attachment 261576 [details]
Proposed patch.
Comment 3 WebKit Commit Bot 2015-09-19 09:58:03 PDT
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.
Comment 4 Eric Carlson 2015-09-19 13:25:13 PDT
Created attachment 261585 [details]
Proposed patch.
Comment 5 Darin Adler 2015-09-19 13:46:44 PDT
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".
Comment 6 Eric Carlson 2015-09-19 14:59:47 PDT
Created attachment 261587 [details]
Updated patch.
Comment 7 Eric Carlson 2015-09-19 15:05:10 PDT
Created attachment 261588 [details]
Patch for the bots.
Comment 8 Eric Carlson 2015-09-19 15:08:54 PDT
(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!
Comment 9 Eric Carlson 2015-09-19 16:24:35 PDT
Created attachment 261589 [details]
APFTB
Comment 10 Eric Carlson 2015-09-19 17:44:58 PDT
Committed r190020: https://trac.webkit.org/r190020