225045 - [ActiveX] OLECMDID_PAGESETUP gives an exception

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: ActiveX Wrapper, defect)

Description

[irongut]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007
Build Identifier: 

OLECMDID_PAGESETUP is defined in MozillaBrowser.h but gives an exception.
Other commands that are defined work fine, e.g. OLECMDID_PRINT.
Code used works fine with IE based WebBrowser control.
Reproduced with control from v1.4, v1.4.1 and v1.5.


Reproducible: Always

Steps to Reproduce:
1. Embed Mozilla Control in an app
2. Call OLECMDID_PAGESETUP of IOleCommandTarget interface


Actual Results:  
In Delphi, got the following exception:
EOleException: Trying to revoke a drop target that has not been registered.


Expected Results:  
Display a Page Setup dialog.

Comment 1

[Adam Lock]

That's a strange error message.

The implementation of OLECMDID_PAGESETUP return S_OK but it will pop up a "No
page setup yet!" message before that. The function does nothing else.

Do you have a code sample of how you are calling it - perhaps it doesn't like
one of the args?

Comment 2

[irongut]

Yes it is a strange error message. Delphi seems to spawn that exception any time
it has problems with OLE. You even get it if you forget to initialise Delphi's
OLE handling code. I don't think the wording means much.

Here is my code, this works fine in the MS control:

procedure TfrmMain.mPageSetupClick(Sender: TObject);
{show Page Setup dialog}
begin
  try
    {ensure not busy or printing before showing dialog}
    if not(mzGecko.Busy) and (mzGecko.QueryStatusWB(OLECMDID_PAGESETUP) > 0)
      {show page setup dialog}
      then mzGecko.ExecWB(OLECMDID_PAGESETUP, OLECMDEXECOPT_PROMPTUSER);
  except
    {handle exceptions}
    on E : Exception do
      MessageDlg('ERROR: Unable to show Page Setup dialog. ' + #13 + E.ClassName
        + ': ' + E.Message + '.', mtError, [mbOk], 0);
    end; {try..except..}
end; {procedure TfrmMain.mPageSetupClick}

Most of the above is exception handling, the calling line is:

mzGecko.ExecWB(OLECMDID_PAGESETUP, OLECMDEXECOPT_PROMPTUSER);

BTW, still getting this exception with v1.6.

Comment 3

[Adam Lock]

I don't know if specifying OLECMDEXECOPT_PROMPTUSER makes a difference - my code
doesn't test the args since it assumes page setup means prompting the user.

I might adopt this bug for a proper implementation of dialog. I'll also check to
see if there is anything about ExecWB which might cause this problem.

Comment 4

[irongut]

(In reply to comment #3)
> I don't know if specifying OLECMDEXECOPT_PROMPTUSER makes a difference - my
> code doesn't test the args since it assumes page setup means prompting the
> user.

BINGO!

Changed the flag to OLECMDEXECOPT_DODEFAULT and now I get the 'No page setup
yet!' message you expected.

I get the same exception for a number of other ExecWB commands. I just tried
OLECMDID_SAVEAS and that now works perfectly. Too busy to do more now, I'll try
the others next week and report in the relevant bugs.

Looks like you need to do something with the args for compatibility with the IE
control. :)

Comment 5

[Adam Lock]

I'm going to hijack this bug seeing as I've written page setup. I'll attach that
patch although it does not contain any fixes for this prompt user thingy.

Comment 6

[Adam Lock]

Attachment: Patch

This patch implements the Page Setup dialog box. Functionality and layout is
identical to the page setup dialog in Firefox.

Comment 7

[Adam Lock]

Attachment: Patch for user prompting thing

This is the patch for user prompting thing.

Basically I was testing for the OLECMDEXECOPT_SHOWHELP (value 3) thinking it
was a flag when it isn't. 

http://lxr.mozilla.org/seamonkey/source/embedding/browser/activex/src/common/IOleCommandTargetImpl.h#241


This meant if you supplied OLECMDEXECOPT_PROMPTUSER (value 1) or anything else
to ExecWB, the test would fail and the code would fall out of the bottom with
an OLECMDERR_E_NOTSUPPORTED error. The patch changes the code to do a
comparison.

Comment 8

[Alexandre Trémon]

Comment on attachment 144259 [details] [diff] [review]
Patch

Adam,

I miss 
IDD_CUSTOM_FIELD, IDD_PPAGE_FORMAT, IDD_PPAGE_MARGINS and IDD_PAGESETUP
that are probably defined in the resource.h?

Isn't there anything changed in the .rc file?

Comment 9

[Adam Lock]

Attachment: Full patch

This is the full patch containing the previous two plus the resource changes as
well.

Comment 10

[Adam Lock]

Comment on attachment 144597 [details] [diff] [review]
Full patch

Full patch including resources

Comment 11

[Alexandre Trémon]

Comment on attachment 144597 [details] [diff] [review]
Full patch

Adam,

* I had a problem applying the patch when in MozillaControl.rc

This is the ouput:
patching file MozillaControl.rc
Hunk #15 succeeded at 304 with fuzz 2 (offset -8 lines).
missing header for unified diff at line 365 of patch
can't find file to patch at input line 365
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|
--------------------------

I tried without success to call patch.exe with -p 0 option, so I applied the
remaining differences manually.

* In PageSetupDlg.h, line 146 & 312, my compiler didn't like _tstof. Are you
using VC7? I use VC6. I tried to replace my stlib.h and tchar.h with ones I got
from a MS SDK, but the compiler didn't like that either.
Do I have to change for vc7 (what is used by TinderBox?) ?
I changed _tstof() by atof(T2A()), and it works nicely.

* For PageSetupDlg.h: I know that in ATL a lot goes into .h files for
efficiency, but then, when you have to change one line, you have to compile a
lot more than if the code were in a .cpp

* I get an assertion because 'cData' in the call ::SendMessage(hwndTarget,
WM_COMMAND, LOWORD(pCI->nWindowsCmdID), (LPARAM) &cData); is at one point
interpreted as a HWND.

Comment 12

[Alexandre Trémon]

Comment on attachment 144597 [details] [diff] [review]
Full patch

* To detect memory leaks, I tried to use the leaky tool, but I have a lot of
trouble compiling it. Do you use a tool for mem leaks?

* You can add the PageSetupDlg.h file in the MozillaControl.dsp

Comment 13

[irongut]

(In reply to comment #4)
 
> I get the same exception for a number of other ExecWB commands. I just tried
> OLECMDID_SAVEAS and that now works perfectly. Too busy to do more now, I'll 
> try the others next week and report in the relevant bugs.

Double checked but other commands I had problems with are missing from
MozillaBrowser.h so the flag would have no effect. See bug 214884.

Comment 14

[Adam Lock]

Hmm, tstof might have been fixed for .NET. It might have been a missing function
in earlier versions. I stuck everything in the header simply because it means I
didn't have to change two files whenever I changed a function. I don't know why
patch was failing since it was a clean build.

Leakwise I don't believe there is anything to leak. 

I'll try and roll a new patch tomorrow, built and tested against VC98. This may
or may not update the .dsp but that's a trivial thing to fix.

Comment 15

[Alexandre Trémon]

It's no big deal, but in resource.h, on line 37:

#define IDD_DIALOG1                     217

Comment 16

[Adam Lock]

Attachment: Full patch mk2

Patch defines _tstof for older VC98 and removes IDD_DIALOG1 from resource.h. 

Concerning _tstof I thought it better to define it at the top rather than
pollute the code with atof(T2CA(value)) and USES_CONVERSION macros.

Comment 17

[Adam Lock]

Comment on attachment 144947 [details] [diff] [review]
Full patch mk2

Requesting sr please to this implementation of the page setup dialog on the
activex contol and an API fix. r= was given but applied to previous patch by
mistake

Comment 18

[Boris Zbarsky [:bzbarsky]]

I'm not going to be able to get to this for at least a week, more likely two weeks.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 144947 [details] [diff] [review]
Full patch mk2

sr=bzbarsky, I guess, but I really don't know much about win32 apis or even
some of the languages used in this patch, so I'm not sure how meaningful me
reviewing it is..

Comment 20

[Adam Lock]

Comment on attachment 144947 [details] [diff] [review]
Full patch mk2

r is carried over from previous patch.

Requesting 1.7 approval. 

Patch implements page setup dialog in the ActiveX control and fixes a COM
exception.

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 144947 [details] [diff] [review]
Full patch mk2

a=asa (on behalf of drivers) for checkin to 1.7

Comment 22

[Adam Lock]

Fix checked into trunk and 1.7 branch