Bug 34979 – [WinCairo, Windows] Implement COM wrappers for DOM traversal

• Component: DOM
• Status: RESOLVED WONTFIX

[Emerick Rogul]

Most of the COM wrappers for DOM traversal are unimplemented in the Windows port.  For example, none of the following  functions defined in WebKit/win/DOMCoreClasses.cpp are defined:

DOMNode::firstChild()
DOMNode::lastChild()
DOMNode::attributes()
DOMNode::hasAttributes()
...
(many others; 53 functions return E_NOTIMPL in DOMCoreClasses.cpp, for example)

Ideally, there should be no functions returning E_NOTIMPL in any of the DOM*.cpp modules (DOMCSSClasses.cpp, DOMCoreClasses.cpp, DOMEventsClasses.cpp, and DOMHTMLClasses.cpp).

[Emerick Rogul]

Brent Fulgham made the following comment on this topic on webkit-help (I'd link directly to it, but the webkit.org server seems to be having issues at the moment):

---
In my own work, I've been performing all DOM traversal in JavaScript,
calling back into C++ to interact with my main application via the
standard JavaScriptCore extension API.  Consequently, I hadn't noticed
that the DOM logic was all unimplemented.  Consequently, the
underlying logic for DOM traversal is certainly present in the Windows
build, but the specific API connecting these routines to the COM
wrapper don't seem to be present.

Please file a bug and we'll see what we can do about getting things up
and running.
---

[Peter Nelson]

Created attachment 52191 [details]
Proposed patch - implemented numerous methods in DOMCoreClasses.cpp and added classes for missing DOM objects.

[WebKit Review Bot]

Attachment 52191 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
xtra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2455:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2458:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2461:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2464:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2467:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2470:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2473:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2476:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2479:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2484:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2489:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2493:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2497:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2500:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2506:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2511:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2514:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2517:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2520:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2523:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2526:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2530:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2534:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2537:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2540:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/win/DOMCoreClasses.h:2541:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2544:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2547:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/win/DOMCoreClasses.h:2550:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/win/DOMCoreClasses.h:2551:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/win/DOMCoreClasses.h:2554:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1280 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 52191 [details]
Proposed patch - implemented numerous methods in DOMCoreClasses.cpp and added classes for missing DOM objects.

Thanks for contributing this patch!

This patch is a bit too big to be reviewed in a timely manner. Could you please break it up into smaller pieces that can each be reviewed and landed independently?

You should also fix the errors that the style checker uncovered.

[Peter Nelson]

Created attachment 52424 [details]
Patch part #1 - Fixes existing style errors in DOMCoreClasses.h

Part #1 of several smaller patches for this bug.  Though it doesn't fix anything directly related to the bug, I didn't think it important enough to require it's own report, and these changes are required for the subsequent patches.

[Eric Seidel (no email)]

Comment on attachment 52424 [details]
Patch part #1 - Fixes existing style errors in DOMCoreClasses.h

This looks to be just style fixes, which looks OK.  (Assuming our COM style guides are the same as the rest of our C++?)

[Eric Seidel (no email)]

Comment on attachment 52424 [details]
Patch part #1 - Fixes existing style errors in DOMCoreClasses.h

Based on Adam's comment about style above, I'm going to assume that check-webkit-style is doing the right thing for these COM files and r+/cq+ this.

[WebKit Commit Bot]

Comment on attachment 52424 [details]
Patch part #1 - Fixes existing style errors in DOMCoreClasses.h

Clearing flags on attachment: 52424

Committed r57129: <http://trac.webkit.org/changeset/57129>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Emerick Rogul]

Apologies if I'm misreading the comment trail on this bug, but it appears that it's been closed as RESOLVED FIXED when only a small style-specific patch has been landed.  I think it was Peter's intent to add more patches to this bug.

[Adam Roben (:aroben)]

Re-opening this bug so Peter can keep working on it.

[Peter Nelson]

Created attachment 53790 [details]
Patch part #2 - Class declarations for a number missing DOM objects in DOMCoreClasses.h

[Peter Nelson]

Will unflagging the very first patch as obsolete prevent this bug from being auto-closed again?

[Eric Seidel (no email)]

The commit-queue will automatically close bugs if no patches remain with r? or r+.

[Eric Seidel (no email)]

Comment on attachment 53790 [details]
Patch part #2 - Class declarations for a number missing DOM objects in DOMCoreClasses.h

We strongly prefer one-patch per bug.  Bug numbers are cheap.  You can always use "master" bugs for large features and related all the sub-bugs as blockers.

Why can't these be autogenerated?

[Adam Barth]

Comment on attachment 53790 [details]
Patch part #2 - Class declarations for a number missing DOM objects in DOMCoreClasses.h

I agree with Eric.  Let's try to auto-generate this code from the IDL.

[Adam Roben (:aroben)]

(In reply to comment #16)
> (From update of attachment 53790 [details])
> I agree with Eric.  Let's try to auto-generate this code from the IDL.

We actually had some experimental support for generating these bindings. There were still a few issues to work out IIRC. It was removed in <http://trac.webkit.org/changeset/52921>.

[Ahmad Saleem]

@rniwa - do we need this now? [Windows] port is gone while we still have [WinCairo] so hence I am just asking. Trying to clean-up some old bugs. Thanks!

[Fujii Hironori]

Windows WK1 was removed.