183508 - [AxPlugin] Hlink targetted links don't work

Status: RESOLVED FIXED

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

Description

[Adam Lock]

GET operations initiated by controls via the Hlink API are not being done in the
proper target frame. The IHlinkFrame::Navigate impl in the plugin is not told
via the supplied IHlink object or other means what the targetted frame name is
so it always opens in the current frame.

This might be a matter of the plugin not implementing something such as the
barely documented ITargetFrame interface.

Comment 1

[Adam Lock]

Attachment: Patch

Patch implements one method on the barely documented ITargetFrame interface so
that a target frame can be associated the Hlink navigate methods.

Comment 2

[Adam Lock]

Attachment: Patch 2

Slightly cleaned up

Comment 3

[Adam Lock]

Comment on attachment 111064 [details] [diff] [review]
Patch 2

Hi David, can you r= this? This barely documented MS interface allows controls
that use Hlink* APIs to navigate to targetted windows to work. I've only
implemented the one method which is called - FindFrame and I use it to store
the name of the frame that isn't supplied in subsequently call to the
IHlinkFrame::Navigate method.

I'll attach a testcase too.

Comment 4

[Adam Lock]

Attachment: Test page containing a shockwave flash file that wants to open targetted links through get & post

Comment 5

[David Bradley]

Comment on attachment 111064 [details] [diff] [review]
Patch 2

r=dbradley

I'm not a superreviewer.

Something to check, in FindFrame, I don't know the usage pattern here, but if
pszTargetName is not null on one call, and then null on a subsequent call,
mUseTarget won't get set to null. That may very well be the intent and correct
thing. Just wanted to double check.

One tiny nit, you can use STDMETHOD(foo) and STDMETHODIMP c::foo to reduce the
amount of text for method declarations and implementations. Up to you if you
want to fiddle with that before check-in

Comment 6

[Adam Lock]

Attachment: Patch 3

I've now got the proper definition of ITargetFrame, so I've updated the patch
with it. Nothing else has changed so assuming the r= is carried.

Comment 7

[Adam Lock]

Comment on attachment 112299 [details] [diff] [review]
Patch 3

Alec, can you sr this please? Thanks

Comment 8

[Alec Flett]

Comment on attachment 112299 [details] [diff] [review]
Patch 3

yuck, so we're more or less redefining the interface because most people
probably won't have it? Can't we #ifdef this somehow, like

#ifdef HAVE_ITARGETFRAME_H
#include "ITargetFrame.h"
#define IMozTargetFrame ITargetFrame
#else
MIDL_INTERFACE(...)

#endif

Look at configure.in for how we detect mmintrin.h to determine HAVE_MMINTRIN_H

Also, I'm inferring from the comments that this is what is going on.. the
comments should say explicitly that we're hijacking an interface IID and
copying the interface layout, banking on them matching.
I'm also slightly concerned that this is illegal, but IANAL.

other than that, sr=alecf..

Comment 9

[Adam Lock]

My feeling is that few developers are going to have this interface. It has only
just appeared in the latest Platform SDK and you only get it then if you also
include the Internet SDK component too. I haven't even got it yet, since the
Platform SDK is a 150Mb download (I'm currently in the process of grabbing it
now). So the effort required to detect and #include what is an identical
definition as mine isn't worth it.

There should be no problem declaring it like this anyway since the interface is
frozen and obsolete and the IMozTarget is a straight cut and paste from the rare
htiframe.h containing the original. It will binary compatible with the
implementations found in IE.

Comment 10

[Adam Lock]

Attachment: Patch 4

This patch addresses Alec's concerns. Makefile can #define USE_HTIFACE if it
wants to use the official version of this interface.

Comment 11

[Adam Lock]

Comment on attachment 112328 [details] [diff] [review]
Patch 4

Carrying r/sr over, requesting 1.3b checkin approval. Low risk, plugin is not
part of standard Mozilla build.

r=dbradley
sr=alecf

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 112328 [details] [diff] [review]
Patch 4

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 13

[Adam Lock]

Fix checked in