Closed Bug 206901 Opened 21 years ago Closed 21 years ago

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

Categories

(Core Graveyard :: Embedding: APIs, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.5beta

People

(Reporter: adamlock, Assigned: adamlock)

References

Details

Attachments

(2 files, 5 obsolete files)

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.
Attached patch Work in progress pt 1 (obsolete) — Splinter Review
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.
Attached patch Work in progress pt 2 (obsolete) — Splinter Review
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.
Attached file Work in progress pt 3 (obsolete) —
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.
Target Milestone: --- → mozilla1.5beta
Attached patch Patch 1 (obsolete) — Splinter Review
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.
Attachment #124370 - Attachment is obsolete: true
Attachment #124373 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #124371 - Attachment is obsolete: true
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
Attachment #125098 - Flags: superreview?(alecf)
Attachment #125098 - Flags: review?(dbradley)
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.
Attachment #125099 - Flags: superreview?(alecf)
Attachment #125099 - Flags: review?(dbradley)
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)
Attachment #125099 - Flags: superreview?(alecf) → superreview+
Comment on attachment 125098 [details] [diff] [review]
Patch 1

sr=alecf
Attachment #125098 - Flags: superreview?(alecf) → superreview+
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?
Attachment #125098 - Flags: review?(dbradley) → review+
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?
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.
>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 ;-)
Attached patch Patch 1Splinter Review
Carrying r/sr forward. First patch updated to detab makefile, supply missing
stdafx.h and remove a nameless struct.
Attachment #125098 - Attachment is obsolete: true
Attached patch Patch 2Splinter Review
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.
Attachment #125099 - Attachment is obsolete: true
Ignore the line in the last patch that says "if (idxForSearch || idxForSearch <
0)". It's already been corrected to just "if (idxForSearch < 0)"
Comment on attachment 125506 [details] [diff] [review]
Patch 2

r=dbradley
Attachment #125506 - Flags: review+
Comment on attachment 125099 [details] [diff] [review]
Patch

cleaning up request queue
Attachment #125099 - Flags: review?(dbradley) → review-
The common/ dir has been created with all the files. The old files will be
removed and the changes made 'live' tomorrow morning.
Fix is checked in, thanks all.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: