Bug 122301 - URLMediaSource.idl and URLMediaStream.idl are wrong
Summary: URLMediaSource.idl and URLMediaStream.idl are wrong
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on: 122443
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-03 14:47 PDT by Chris Dumez
Modified: 2013-10-07 12:11 PDT (History)
15 users (show)

See Also:


Attachments
Patch (41.45 KB, patch)
2013-10-07 08:44 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (19.53 KB, patch)
2013-10-07 08:45 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (18.51 KB, patch)
2013-10-07 09:00 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (18.51 KB, patch)
2013-10-07 09:08 PDT, Jer Noble
pnormand: review+
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Patch for committing. (38.37 KB, patch)
2013-10-07 11:41 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2013-10-03 14:47:16 PDT
URLMediaSource.idl and URLMediaStream.idl are partial interfaces that extend the URL interface. However, this has no impact at the moment and it causes the bindings generator to generate a warning because the URL interface does not exist. The interface is called DOMURL in WebKit.

I assume this code was imported from Blink where the IDL interfaces' name matches their JS name (I tried to do the same in WebKit but Darin thought the IDL interfaces' name should be consistent with the implementation name instead).

You should update those partial interfaces as follows:
- partial interface URL {
+ partial interface DOMURL {

so that is actually does something and affects the behavior on JS side.

Jer Noble, could you please look into this since I believe you are working on this?

--
Bindings generator warning:
Use of uninitialized value $targetIdlFile in hash element at Source/WebCore/bindings/scripts/preprocess-idls.pl line 128.

We should probably have the script die instead in this case and print a useful error message to let the developer know its partial interface (or IDL implements statement is wrong) is wrong.
Comment 1 Darin Adler 2013-10-03 14:51:19 PDT
(In reply to comment #0)
> (I tried to do the same in WebKit but Darin thought the IDL interfaces' name should be consistent with the implementation name instead).

That’s incorrect. My comment was about the IDL file names, not about the IDL interface names inside the files.
Comment 2 Chris Dumez 2013-10-03 14:54:15 PDT
(In reply to comment #1)
> (In reply to comment #0)
> > (I tried to do the same in WebKit but Darin thought the IDL interfaces' name should be consistent with the implementation name instead).
> 
> That’s incorrect. My comment was about the IDL file names, not about the IDL interface names inside the files.

Ok, then I might have misunderstood you at the time. In any case, it makes the generator's work easier if the interface's name matches the file name (at I believe the generators relies on that).

Those files should probably be renamed to DOMURLMediaSource.idl / DOMURLMediaStream.idl then, for consistency.
Comment 3 Jer Noble 2013-10-03 15:08:14 PDT
(In reply to comment #0)
> Jer Noble, could you please look into this since I believe you are working on this?

Sure thing.
Comment 4 Philippe Normand 2013-10-07 05:23:38 PDT
See bug 122443
Comment 5 Jer Noble 2013-10-07 08:44:48 PDT
Created attachment 213590 [details]
Patch
Comment 6 Jer Noble 2013-10-07 08:45:53 PDT
Created attachment 213591 [details]
Patch

Here's a version of the patch with rename detection enabled.
Comment 7 Jer Noble 2013-10-07 08:48:53 PDT
Bug 122444 was committed while I was writing this patch, so I need to incorporate it and rebaseline.
Comment 8 Philippe Normand 2013-10-07 08:51:28 PDT
Comment on attachment 213591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213591&action=review

> Source/WebCore/ChangeLog:10
> +        by a MEDIA_SOURCE conditional.

I guess you mean MEDIA_STREAM here. Sorry about the collision with bug 122444
Comment 9 Jer Noble 2013-10-07 09:00:56 PDT
Created attachment 213594 [details]
Patch
Comment 10 Philippe Normand 2013-10-07 09:05:23 PDT
Comment on attachment 213594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213594&action=review

> Source/WebCore/Modules/mediastream/DOMURLMediaStream.idl:31
> +    Conditional=MEDIA_SOURCE

MEDIA_STREAM :)
Comment 11 Jer Noble 2013-10-07 09:07:16 PDT
ack!(In reply to comment #10)
> (From update of attachment 213594 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213594&action=review
> 
> > Source/WebCore/Modules/mediastream/DOMURLMediaStream.idl:31
> > +    Conditional=MEDIA_SOURCE
> 
> MEDIA_STREAM :)

Whoops. :)
Comment 12 Jer Noble 2013-10-07 09:08:32 PDT
Created attachment 213596 [details]
Patch

MEDIA_SOURCE -> MEDIA_STREAM
Comment 13 Philippe Normand 2013-10-07 09:10:43 PDT
Comment on attachment 213596 [details]
Patch

Ok, thanks!
Comment 14 EFL EWS Bot 2013-10-07 09:27:44 PDT
Comment on attachment 213596 [details]
Patch

Attachment 213596 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3649002
Comment 15 Philippe Normand 2013-10-07 09:32:13 PDT
/mnt/eflews/webkit/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSDOMURL.cpp:178:23: error: 'JSC::EncodedJSValue WebCore::jsDOMURLConstructorFunctionCreateObjectURL1(JSC::ExecState*)' defined but not used [-Werror=unused-function]
cc1plus: all warnings being treated as errors

I think this patch should depend on the one in bug 122443
Comment 16 EFL EWS Bot 2013-10-07 09:51:11 PDT
Comment on attachment 213596 [details]
Patch

Attachment 213596 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3634020
Comment 17 Jer Noble 2013-10-07 11:41:06 PDT
Created attachment 213607 [details]
Patch for committing.
Comment 18 Jer Noble 2013-10-07 12:10:57 PDT
Committed r157054: <http://trac.webkit.org/changeset/157054>