On 1/18/11 7:31 PM, Erich Hoover wrote:
On Tue, Jan 18, 2011 at 3:08 AM, Jacek Caban <jacek@codeweavers.com
mailto:jacek@codeweavers.com> wrote:
... I'm not sure what you mean by "hijack the IOleCommandTarget". All we
do is implementing client's IOleCommandTarget. It's something different from document's one.
I understand that, but apparently the native implementation (testing on
the test bot WXPPROSP3) sends the command to the client
IOleCommandTarget instead of the document IOleCommandTarget (at least
under the conditions of the webbrowser tests). That is what I mean by hijacking, the command is going to the "wrong" target. My guess would be that there is some sort of priority mechanism, though I have no idea how it would work (except maybe "if there's a client/container then send to that target, else send to the document target).
That sounds reasonable.
I've attached a test where I disabled the client/container, and you can see that it then gets passed through (QueryStatusWB will return
success instead of passing through the client target and returning failure):
Hmm, it means that another run of tests (at least required subset of existing test_WebBrowser) with client's IOleCommandTarget disabled would be interesting. Do you feel like writing it? Otherwise we'd need at least a FIXME in this case.
Jacek
On Wed, Jan 19, 2011 at 1:05 PM, Jacek Caban jacek@codeweavers.com wrote:
On 1/18/11 7:31 PM, Erich Hoover wrote:
I've attached a test where I disabled the client/container, and you can see that it then gets passed through (QueryStatusWB will return
success instead of passing through the client target and returning failure):
Hmm, it means that another run of tests (at least required subset of existing test_WebBrowser) with client's IOleCommandTarget disabled would be interesting. Do you feel like writing it? Otherwise we'd need at least a FIXME in this case.
I've been working on putting such a test together, I actually just finished it up when I got your message. It's attached to this email, and I would appreciate it if you would take a look - it ends up being rather non-trivial since native caches the IOleCommandTarget on creation of the container.
Corresponding test results: https://testbot.winehq.org/JobDetails.pl?Key=8449
Erich Hoover ehoover@mines.edu
On 1/19/11 9:31 PM, Erich Hoover wrote:
On Wed, Jan 19, 2011 at 1:05 PM, Jacek Cabanjacek@codeweavers.com wrote:
On 1/18/11 7:31 PM, Erich Hoover wrote:
I've attached a test where I disabled the client/container, and you can see that it then gets passed through (QueryStatusWB will return
success instead of passing through the client target and returning failure):
Hmm, it means that another run of tests (at least required subset of existing test_WebBrowser) with client's IOleCommandTarget disabled would be interesting. Do you feel like writing it? Otherwise we'd need at least a FIXME in this case.
I've been working on putting such a test together, I actually just finished it up when I got your message. It's attached to this email, and I would appreciate it if you would take a look - it ends up being rather non-trivial since native caches the IOleCommandTarget on creation of the container.
Corresponding test results: https://testbot.winehq.org/JobDetails.pl?Key=8449
Thanks. I have a few comments:
+static int OleContainer_use_custom_target = TRUE;
Please name it without mixing naming convention, something like use_container_olecmd would do.
+ case IDM_STOP: + prgCmds[0].cmdf = 0; + return S_OK;
CHECK_EXPECT(QueryStatus_STOP) would be nice here.
+ ok(status& success_flag, "OLECMDID_STOP not enabled/supported: %08x\n", status);
You could test the exact value here: ok(status == ...)
+ ok(!(status& success_flag), "IDM_STOP enabled/supported: %08x\n", status);
And here.
+ +static void test_CommandTargetPassthru(int use_custom_target) +{
test_CommandTargetPassthru(TRUE) could go to existing test_WebBrowser.
+ + if (!target) + return E_FAIL;
A test for this case would be nice. Also with this patch, testing ExecEB shouldn't be too hard, let's test it as well.
Jacek
On Thu, Jan 20, 2011 at 4:00 AM, Jacek Caban jacek@codeweavers.com wrote:
...
- ok(status& success_flag, "OLECMDID_STOP not enabled/supported: %08x\n", status);
You could test the exact value here: ok(status == ...)
I was trying to avoid adding some conditional todo_wine calls (for some reason native does not respond with "supported", even though it does in the mshtml tests), please see if you like the attached better.
test_CommandTargetPassthru(TRUE) could go to existing test_WebBrowser.
Would it not be better to just keep these all in one place, since for the tests without the custom target it's necessary to create and initialize a web-browser?
- if (!target)
- return E_FAIL;
A test for this case would be nice. Also with this patch, testing ExecEB shouldn't be too hard, let's test it as well.
Ugg, apparently MSDN lies horribly - it's actually E_UNEXPECTED. Please check the attached for both.
Corresponding test results: https://testbot.winehq.org/JobDetails.pl?Key=8482
Thanks for all your feedback!
Erich Hoover ehoover@mines.edu
On 1/20/11 6:40 PM, Erich Hoover wrote:
On Thu, Jan 20, 2011 at 4:00 AM, Jacek Cabanjacek@codeweavers.com wrote:
...
- ok(status& success_flag, "OLECMDID_STOP not enabled/supported: %08x\n", status);
You could test the exact value here: ok(status == ...)
I was trying to avoid adding some conditional todo_wine calls
Hiding differences between Wine and Windows in tests is the worst thing you can do. When one is trying to find a bug, tested parts of implementation are much less suspicious than not or poorly tested. If you know about more such things in the patch, please fix them. You don't write the test to pass on your implementation. You write it to know the right implementation.
(for some reason native does not respond with "supported", even though it does in the mshtml tests), please see if you like the attached better.
Or it proves that it's not a simple forward to mshtml?
test_CommandTargetPassthru(TRUE) could go to existing test_WebBrowser.
Would it not be better to just keep these all in one place, since for the tests without the custom target it's necessary to create and initialize a web-browser?
We still have them in separated functions you've called test_QueryStatusWB and test_ExecWB. We most likely will want more tests on WebBrowser instance without container's IOleCommandTarget, and it would be nice if we could reuse test_CommandTargetPassthru for that (well, the name would be better more generic as well).
- if (!target)
return E_FAIL;A test for this case would be nice. Also with this patch, testing ExecEB shouldn't be too hard, let's test it as well.
Ugg, apparently MSDN lies horribly - it's actually E_UNEXPECTED.
That's why we need tests :)
Jacek
On Sun, Jan 23, 2011 at 11:32 AM, Jacek Caban jacek@codeweavers.com wrote:
On 1/20/11 6:40 PM, Erich Hoover wrote:
On Thu, Jan 20, 2011 at 4:00 AM, Jacek Cabanjacek@codeweavers.com wrote:
- ok(status& success_flag, "OLECMDID_STOP not enabled/supported: %08x\n", status);
You could test the exact value here: ok(status == ...)
I was trying to avoid adding some conditional todo_wine calls
Hiding differences between Wine and Windows in tests is the worst thing you can do. When one is trying to find a bug, tested parts of implementation are much less suspicious than not or poorly tested. If you know about more such things in the patch, please fix them. You don't write the test to pass on your implementation. You write it to know the right implementation.
I would say that testing the mask is not about hiding differences, it makes it clear that this particular test is meant to demonstrate whether a specific feature is available. At some point you have to make a trade-off between making a particular test completely comprehensive or having it be clear exactly what is being tested. In this particular component there are a lot of things that are not yet implemented, so it's a little difficult to add a self-contained test for a feature without it ballooning into a bunch of unrelated issues. There are two other such "issues" I have become aware of with the current implementation that could be added as tests, but I would say that they both belong in separate patches (if you want them at all):
1) The IOleCommandTarget is cached when the WebBrowser is initialized (rather than being requested each time it is used) 2) The document is initialized much earlier on native than it is in Wine
My goal here is to implement (as well as I can) a single feature that I've had sitting in my stack of unsubmitted patches, I'm not trying to implement everything in this component in one fell swoop. If that's really what it's going to take to get this feature implemented then I can take the time to implement more, but it'll take me a while and I'd really like to move this item off of my plate so I can clean up some of my other unsubmitted patches before they become too far out of sync.
(for some reason native does not respond with "supported", even though it does in the mshtml tests), please see if you like the attached better.
Or it proves that it's not a simple forward to mshtml?
It's possible that they masked the supported flag out, especially since all of the examples I've seen on MSDN check for the enabled flag rather than the supported flag. However, I doubt that they copy-pasted the mshtml code and then made changes to it. I'd say that even if they did it'd be better to forward to mshtml and discover if there are any differences (and where they are) as people test applications that use this functionality.
We still have them in separated functions you've called test_QueryStatusWB and test_ExecWB. We most likely will want more tests on WebBrowser instance without container's IOleCommandTarget, and it would be nice if we could reuse test_CommandTargetPassthru for that (well, the name would be better more generic as well).
Sorry, for some reason I was a bit dense and didn't catch exactly what you were planning here. Please see if the attached is more to your liking, the test bot results are available here: https://testbot.winehq.org/JobDetails.pl?Key=8575
Erich Hoover ehoover@mines.edu
-
Erich Hoover -
Jacek Caban