[ActiveX] OLECMDID_PAGESETUP gives an exception

RESOLVED FIXED

Status

Core Graveyard
Embedding: ActiveX Wrapper
RESOLVED FIXED
14 years ago
5 years ago

People

(Reporter: irongut, Assigned: Adam Lock)

Tracking

({fixed1.7})

Trunk
x86
Windows 2000
fixed1.7

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Assignee)

Comment 1

13 years ago
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?
(Reporter)

Comment 2

13 years ago
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.
(Assignee)

Comment 3

13 years ago
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.
(Reporter)

Comment 4

13 years ago
(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. :)
(Assignee)

Comment 5

13 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 6

13 years ago
Created attachment 144259 [details] [diff] [review]
Patch

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

Updated

13 years ago
Attachment #144259 - Flags: review?(atremon)
(Assignee)

Comment 7

13 years ago
Created attachment 144263 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Attachment #144263 - Flags: review?(atremon)

Comment 8

13 years ago
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?
(Assignee)

Comment 9

13 years ago
Created attachment 144597 [details] [diff] [review]
Full patch

This is the full patch containing the previous two plus the resource changes as
well.
Attachment #144259 - Attachment is obsolete: true
Attachment #144263 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #144259 - Flags: review?(atremon)
(Assignee)

Updated

13 years ago
Attachment #144263 - Flags: review?(atremon)
(Assignee)

Comment 10

13 years ago
Comment on attachment 144597 [details] [diff] [review]
Full patch

Full patch including resources
Attachment #144597 - Flags: review?(atremon)

Comment 11

13 years ago
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

13 years ago
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
(Reporter)

Comment 13

13 years ago
(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.
(Assignee)

Comment 14

13 years ago
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

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

#define IDD_DIALOG1                     217
(Assignee)

Comment 16

13 years ago
Created attachment 144947 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Attachment #144597 - Attachment is obsolete: true

Updated

13 years ago
Attachment #144597 - Flags: review?(atremon) → review+
(Assignee)

Comment 17

13 years ago
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
Attachment #144947 - Flags: superreview?(bzbarsky)
I'm not going to be able to get to this for at least a week, more likely two weeks.
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..
Attachment #144947 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 20

13 years ago
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.
Attachment #144947 - Flags: approval1.7?

Comment 21

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

a=asa (on behalf of drivers) for checkin to 1.7
Attachment #144947 - Flags: approval1.7? → approval1.7+
(Assignee)

Comment 22

13 years ago
Fix checked into trunk and 1.7 branch
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Updated

13 years ago
Keywords: fixed1.7
Component: Embedding: ActiveX Wrapper → Embedding: ActiveX Wrapper
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.