Bug 122301

Summary: URLMediaSource.idl and URLMediaStream.idl are wrong
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: MediaAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, eflews.bot, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, hta, jer.noble, kondapallykalyan, philn, pnormand, rakuco, tommyw, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 122443    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
pnormand: review+, eflews.bot: commit-queue-
Patch for committing. none

Chris Dumez
Reported 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.
Attachments
Patch (41.45 KB, patch)
2013-10-07 08:44 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Patch (19.53 KB, patch)
2013-10-07 08:45 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Patch (18.51 KB, patch)
2013-10-07 09:00 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Patch (18.51 KB, patch)
2013-10-07 09:08 PDT, Jer Noble 		pnormand: review+
eflews.bot: commit-queue-
DetailsFormatted DiffDiff
Patch for committing. (38.37 KB, patch)
2013-10-07 11:41 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Darin Adler
Comment 1 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.
Chris Dumez
Comment 2 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.
Jer Noble
Comment 3 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.
Philippe Normand
Comment 4 2013-10-07 05:23:38 PDT
See bug 122443
Jer Noble
Comment 5 2013-10-07 08:44:48 PDT
Created attachment 213590 [details] Patch
Jer Noble
Comment 6 2013-10-07 08:45:53 PDT
Created attachment 213591 [details] Patch Here's a version of the patch with rename detection enabled.
Jer Noble
Comment 7 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.
Philippe Normand
Comment 8 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
Jer Noble
Comment 9 2013-10-07 09:00:56 PDT
Created attachment 213594 [details] Patch
Philippe Normand
Comment 10 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 :)
Jer Noble
Comment 11 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. :)
Jer Noble
Comment 12 2013-10-07 09:08:32 PDT
Created attachment 213596 [details] Patch MEDIA_SOURCE -> MEDIA_STREAM
Philippe Normand
Comment 13 2013-10-07 09:10:43 PDT
Comment on attachment 213596 [details] Patch Ok, thanks!
EFL EWS Bot
Comment 14 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
Philippe Normand
Comment 15 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
EFL EWS Bot
Comment 16 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
Jer Noble
Comment 17 2013-10-07 11:41:06 PDT
Created attachment 213607 [details] Patch for committing.
Jer Noble
Comment 18 2013-10-07 12:10:57 PDT
Committed r157054: <http://trac.webkit.org/changeset/157054>
Note You need to log in before you can comment on or make changes to this bug.