206901 - [AxPlugin] Share activex control's implementation of the DOM

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Adam Lock]

The Mozilla control has some reasonably working implementations of IE DOM
elements and friends. These should be shared with the plugin to expose greater
functionality to controls which can use it.

I already have a working patch that demonstrates it is doable, but the classes
should be hardened and perhaps made more efficient too as part of this work. For
example, the get_all impl creates a massive and static collection of every DOM
element - perhaps this can be deferred and done dynamically somehow.

This may also be a good opportunity to refactor some of the source code into a
seperate 'common' or 'shared' dir that exports headers and a static lib as this
code would also be useful in accessibility too. This shared code would include
the IE DOM impl, the control site class and some template implementations of
some OLE interfaces.

Comment 1

[Adam Lock]

Attachment: Work in progress pt 1

This patch contains changes to the existing plugin / control to move shared
files out into a common/ directory. It could do with some tidy up to remove
some commented out bits but is reasonably complete.

The major problem I faced here is that the control is compiled with xpcom and
the plugin to with xpcom_glue. Therefore to split out the shared pieces into a
static lib, I had to convert the plugin to use xpcom and hope that the dlls
resolved properly. This appears to be the case, so the patch does that.

Comment 2

[Adam Lock]

Attachment: Work in progress pt 2

This patch contains the diff -u of changes need to move files out of control/
and into common/. Mostly these are just simple cleanup such as fixing #include
bustage caused by some assumptions which didn't hold outside of the control
dir.

Comment 3

[Adam Lock]

Attachment: Work in progress pt 3

This is the Makefile.in from the new common/ dir. This builds an
ax_common_s.lib and exports various headers for classes and templates that are
shared by the control and plugin.

Comment 4

[Adam Lock]

Attachment: Patch 1

Patch 1 of 2.

This contains mostly makefile changes to move common files out of the control/
dir into common/. Patch also contains Makefile.in to build ax_common_s.

The patch also contains diffs to plugin/ to remove some of the proprietary
trace macros and to use the IE DOM collection object from get_all.

Comment 5

[Adam Lock]

Attachment: Patch

Patch 2 of 2

This is a diff -u showing the changes required to the shared source to move it
to its location. Mostly this is just removal of macros and #include cleanup.
The only significant code change is in CIEHtmlElementCollection::item which has
been rewritten after it was discovered to be broken when called with a BSTR
element name.

Comment 6

[Adam Lock]

Comment on attachment 125098 [details] [diff] [review]
Patch 1

Requesting r/sr on this patch to move shared files including IE DOM classes out
of the control/ and into a common/ directory. This patch covers mostly Makefile
changes, plus code in the plugin to make use of the IE DOM classes

Comment 7

[Adam Lock]

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

Requesting r/sr on this patch which demonstrates the changes to macros and
#includes to move source into common/ the only major code change is in the
implementation of CIEHtmlElementCollection::item to support named elements.

Comment 8

[Alec Flett]

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

sr=alecf
(next time, for the reviewers' benefits, can you name your patches something
that explains the difference? That way whe we look in our request queues they
don't look like the same patch)

Comment 9

[Alec Flett]

Comment on attachment 125098 [details] [diff] [review]
Patch 1

sr=alecf

Comment 10

[David Bradley]

Comment on attachment 125098 [details] [diff] [review]
Patch 1

r=dbradley

Looks like some tabs have sneaked into
mozilla/embedding/browser/activex/src/common/Makefile.in

Is there a reason to keep the anonymous struct in PluginInstanceData?

Comment 11

[David Bradley]

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

I couldn't find the definition of TRACE_METHOD. I see no sign of it in
Mozilla's code or VC++'s includes. Was curious where it's located.

CIEHtmlElementCollection::item tests the variant "name" for VT_I4. Are you sure
that VT_I4 is the only integer type that will be passed into here? Would it be
better to attempt a change to VT_I4 if it's not VT_BSTR?

IEHtmlNode.cpp contains just an added blank line, is it worth modifying this
file for this?

Would it be better to make idxForSearch unsigned? The thing that caught my eye
was that you check for exceeding the count, but not for negative values.

For: CComBSTR elementId = NULL; why not just default construct?

Some calls to CIEHtmlElementInstance::CreateInstance check for null and in the
patch it doesn't. So not sure what the property convention is.

In IEHtmlElementCollection.cpp why not just remove the include of stack?

Comment 12

[Adam Lock]

David, in response to your issues I'll submit a new patch soon.

TRACE_METHOD was in StdAfx.h which didn't make it into the patch (I forgot to
cvs add it). Aside from this the macro, it just #includes some atl headers and
common ns files.

The docs for item() explicitly state "VARIANT of type VT_I4 or VT_BSTR that
specifies the object or collection to retrieve". I will add code to attempt to
coerce other numeric types, but clients should be coded to the interface spec.

I removed the blank line as part of a general cleanup of the files as they are
being deleted from control/ and added to common/. IEHtmlNode.cpp is being
deleted and recreated irrespective of this line, but the diff -u picked up on it.

I made idxForSearch signed because VT_I4 is signed too, but you're right I don't
test for negative values and I should do. I'll fix it.

elementId will be fixed

I'll clean up calls to CreateInstance to make them consistent.

I'll remove the spurious #include.

Comment 13

[David Bradley]

>The docs for item() explicitly state "VARIANT of type VT_I4 or VT_BSTR that
>specifies the object or collection to retrieve". I will add code to attempt to
>coerce other numeric types, but clients should be coded to the interface spec.

That's up to. As long as it's document I'm fine with it as it stands, I just
wasn't sure if it was documented as such. I hit other cases where it wasn't
stated and "assumed" and got bitten ;-)

Comment 14

[Adam Lock]

Attachment: Patch 1

Carrying r/sr forward. First patch updated to detab makefile, supply missing
stdafx.h and remove a nameless struct.

Comment 15

[Adam Lock]

Attachment: Patch 2

Carrying sr forward. This patch removes an obsolete #include, accepts VT_UI2
and various other integer types in item(), tests if idxForSearch is negative,
cleans up CreateInstance pointer checking and removes the duff initialiser on
CComBSTR elementId.

Comment 16

[Adam Lock]

Ignore the line in the last patch that says "if (idxForSearch || idxForSearch <
0)". It's already been corrected to just "if (idxForSearch < 0)"

Comment 17

[David Bradley]

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

r=dbradley

Comment 18

[David Bradley]

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

cleaning up request queue

Comment 19

[Adam Lock]

The common/ dir has been created with all the files. The old files will be
removed and the changes made 'live' tomorrow morning.

Comment 20

[Adam Lock]

Fix is checked in, thanks all.