Bug 49036 – [chromium] fix getintidentifier-special-values.html using TestNetscapePlugIn

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

[chromium] fix getintidentifier-special-values.html using TestNetscapePlugIn

[Tony Chang]

Created attachment 73000 [details]
Patch

[Tony Chang]

I talked to Ananta about this and he can't remember exactly why this change was needed.  The test cases in the bug tracker are no longer valid because the sites mentioned (microsoft.com and msdn.microsoft.com) no longer use silverlight.  I manually tested the following:

- http://joel.neubeck.net/2008/06/sl2b2-media-player-markers/
- a few sites linked from silverlight.net
- netflix streaming

[Tony Chang]

Maybe jam or stuart can review this?  I'm not sure who owns this code these days.

[Stuart Morgan]

I'm not familiar with this code, so I'm probably not a good reviewer.

Does this test failing mean that Apple's WebKit returns a double? If so, I'm fine with this from the Mac standpoint certainly, but if both Gecko and Safari return ints this seems like it would be a dangerous change.

[Tony Chang]

(In reply to comment #4)
> I'm not familiar with this code, so I'm probably not a good reviewer.
> 
> Does this test failing mean that Apple's WebKit returns a double? If so, I'm fine with this from the Mac standpoint certainly, but if both Gecko and Safari return ints this seems like it would be a dangerous change.

Safari returns a double and Firefox returns an int.  We originally were matching Firefox, but this change makes us match Safari since it doesn't seem to be necessary anymore.

[Stuart Morgan]

As long as we're matching something else I'm fine with this. I mostly deal with the Mac though, where matching Safari is pretty much a guarantee existing plugins will work ;)

[Tony Chang]

Can someone review this?  dglazkov?  fishd?

[John Abd-El-Malek]

sorry just saw this.  I'm not a WebKit reviewer.  I'm also not sure why the code is like that.  if it doesn't break any tests, try it :)

[Adam Barth]

Comment on attachment 73000 [details]
Patch

If JAM is ok with this change, it's fine with me.

[Tony Chang]

Committed r72243: <http://trac.webkit.org/changeset/72243>

[David Levin]

Rolled out here: http://trac.webkit.org/changeset/72271


Seemed to cause a large set of Chromium layout tests to start failing.

[David Levin]

Comment on attachment 73000 [details]
Patch

Clearing r+ on bad patch and marking obsolete.

We just went from 103 layout test failures on Win to 3 with this rolled out!

[Tony Chang]

Created attachment 74285 [details]
Patch

[Tony Chang]

(In reply to comment #11)
> Rolled out here: http://trac.webkit.org/changeset/72271
>
> Seemed to cause a large set of Chromium layout tests to start failing.

Sorry about that!  I didn't realize this change could cause non-plugin tests to fail.  Turns out our C++ bindings uses NPObject.  This patch fixes the calls in DRT and http://codereview.chromium.org/5209004/ fixes the calls in test_shell.

[Tony Chang]

Created attachment 74581 [details]
Patch

[Tony Chang]

(In reply to comment #15)
> Created an attachment (id=74581) [details]
> Patch

Updated to include drt_expectations.txt update since I've switched DRT to using the upstream TestNetscapePlugIn.

Adam, can you review?  The analogous change to test_shell went in here:
http://codereview.chromium.org/5209004/

[Adam Barth]

Comment on attachment 74581 [details]
Patch

Yay for testing.  :)

[Tony Chang]

Committed r72974: <http://trac.webkit.org/changeset/72974>