The Realplayer plugin does not work correctly for the following URLs. http://www.mozilla.org/quality/browser/front-end/testcases/plugins/ra3.html http://www.mozilla.org/quality/browser/front-end/testcases/plugins/ra4.html http://www.mozilla.org/quality/browser/front-end/testcases/plugins/ra2.html The issue occurs for OBJECT tags defined with the "DATA" attribute which contains a URI. <object data="http://www.wrn.org/audio/kol_eng1.ram" type="audio/x-pn-realaudio-plugin" width="300" height="120" autostart="true"> <param name="controls" value="default"> Audio </object> This works in Firefox and it seems like it adds an additional "SRC" attribute to the list of parameters passed to the plugin. This attribute contains the same URI as that in the "DATA" attribute.
I can confirm the bug as PlatformOnly. The bug is visible only on Windows with RealPlayer 11. Safari for Mac OS X works fine with all of the above mentioned test cases.
Created attachment 30940 [details] Fix Fix follows what we've been doing in Chrome, and what Firefox does as well (see http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#3045)
Comment on attachment 30940 [details] Fix Shouldn't we only do this hack for a specific set of plugins? I would think this code should also be moved into a static function and then only called for that set of plugins which need it. mapDataParamToSrc() or something. Also we have a caseInsensitiveEqual which doesn't copy the string like lower() does. single line ifs don't get { } in WK style.
Comment on attachment 30940 [details] Fix r- for my above nits.
Eric: thanks for the review, comments: > Shouldn't we only do this hack for a specific set of plugins? Firefox does it for all plugins (and Chrome has done it likewise since launch). Since I don't know what other old plugins are done this way, I'd prefer to leave as is. I think if it had caused any problems, FF would have changed it. > Also we have a caseInsensitiveEqual which doesn't copy the string like lower() does. I can't find this function? The only thing I found is stringIsCaseInsensitiveEqualToString but that's in WebCore\platform\mac\WebCoreNSStringExtras.h. > single line ifs don't get { } in WK style. done
Created attachment 31082 [details] Took out brace brakets
Comment on attachment 31082 [details] Took out brace brakets I do not think that this is the way to go - What if other plug-ins are confused if they see both a src and a data attribute. I'm not sure what the bug is here - does this affect Firefox? Is it Windows only? Couldn't you just put this workaround in the chrome specific frame loader client?
(In reply to comment #7) > (From update of attachment 31082 [details] [review]) > I do not think that this is the way to go - What if other plug-ins are confused > if they see both a src and a data attribute. I'm not sure what the bug is here The bug is that some pages are written with data=url attribute, instead of src=url. > - does this affect Firefox? No, because Firefox does the renaming which this patch follows from. > Is it Windows only? I only saw this on Windows, although Firefox does this on all platforms (see the link above to the Firefox source). > Couldn't you just put this > workaround in the chrome specific frame loader client? That's what we have now to fix the problem in Chrome, but I thought moving it to WebKit code would benefit all ports. These pages don't render correctly on WebKit browsers unless they add this code.
(In reply to comment #5) > Eric: thanks for the review, comments: > > > Shouldn't we only do this hack for a specific set of plugins? > > Firefox does it for all plugins (and Chrome has done it likewise since launch). > Since I don't know what other old plugins are done this way, I'd prefer to > leave as is. I think if it had caused any problems, FF would have changed it. Agreed, we should match FF here. > > Also we have a caseInsensitiveEqual which doesn't copy the string like lower() does. > > I can't find this function? The only thing I found is > stringIsCaseInsensitiveEqualToString but that's in > WebCore\platform\mac\WebCoreNSStringExtras.h. Yeah, I meant "equalIgnoringCase", I just couldn't remember the name.
Comment on attachment 31082 [details] Took out brace brakets I don't fully understand Anders comment above since you already seem to have answered at least one of his questions in comment 5 and comment 2. I still think this would read better if the code was broken out into a static function something like: static void mapDataToSrcForOldPlugins(Vector<String>& paramNames, Vector<String>& paramValues) { ... } I'm not sure if .lower() or equalIgnoringCase is faster. equalIgnoringCase does avoid the allocation caused by lower() though. Having looked at the FF source and read your additional comments, I agree, we should make this change. You should consider referencing the FF source in your ChangeLog. We still need a reply from Anders, but I'm ready to r+ this otherwise. I'd love to see a new patch w/ the nits fixed, but otherwise this is ready to go.
Created attachment 31102 [details] Updated patch
Comment on attachment 31102 [details] Updated patch This looks good to me. Anders, please feel free to r- this if you're still opposed. As far as I can tell jam's change makes us match firefox.
Comment on attachment 31102 [details] Updated patch Actually, it should be possible to make a test case for this, no? We'd put the test case under platform/win I expect, or maybe we'd just skip it on the Mac. But we should be able to make a simple layout test which confirms this is fixed, no?
Created attachment 31120 [details] Added regression test I spent half a day on this, but it doesn't look like layout tests are runnable on Windows at the moment. I've verified what I can by running the plugin dll in the browser and tracing through the plugin. I don't have a mac (one on order), so I couldn't build and verify that this test passes. Can you verify that the test passes for you on Windows?
oops, I meant "verify it works for you on Mac"
Created attachment 31121 [details] Fixed misspelling.
btw here's the companion change to chromium: http://codereview.chromium.org/118486 I was able to successfully run the layout test there on Windows.
Comment on attachment 31121 [details] Fixed misspelling. You did more than what I had expected. I figured you'd just add a test which used realplayer. That said, I think I would change this a little bit to just store off the parameters to that JS can access them later. that way we can test things like param ordering too. :) I'll fix up the test some when I land this for you later.
ping? I think this patch is ready to check in, since it's independent of checking for order of parameters.
Agreed. Sorry for the holdup.
I went to land this but the changelogs haven't been filled out for LayoutTests and WebKitTools
Created attachment 31567 [details] added missing ChangeLogs
Transfered review flag to newest patch.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog A LayoutTests/plugins/netscape-plugin-map-data-to-src-expected.txt A LayoutTests/plugins/netscape-plugin-map-data-to-src.html M WebCore/ChangeLog M WebCore/rendering/RenderPartObject.cpp M WebKitTools/ChangeLog M WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.cpp M WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.h M WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp M WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp Committed r45324 http://trac.webkit.org/changeset/45324
Sorry that took so long. In the end, I did not make any modifications to the posted patch.