Bug 34979 - [WinCairo, Windows] Implement COM wrappers for DOM traversal
Summary: [WinCairo, Windows] Implement COM wrappers for DOM traversal
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-16 12:57 PST by Emerick Rogul
Modified: 2010-06-21 06:29 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch - implemented numerous methods in DOMCoreClasses.cpp and added classes for missing DOM objects. (152.10 KB, patch)
2010-03-31 11:46 PDT, Peter Nelson
aroben: review-
Details | Formatted Diff | Diff
Patch part #1 - Fixes existing style errors in DOMCoreClasses.h (55.75 KB, patch)
2010-04-02 11:00 PDT, Peter Nelson
no flags Details | Formatted Diff | Diff
Patch part #2 - Class declarations for a number missing DOM objects in DOMCoreClasses.h (45.39 KB, patch)
2010-04-20 02:46 PDT, Peter Nelson
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emerick Rogul 2010-02-16 12:57:52 PST
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).
Comment 1 Emerick Rogul 2010-02-16 13:01:16 PST
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.
---
Comment 2 Peter Nelson 2010-03-31 11:46:30 PDT
Created attachment 52191 [details]
Proposed patch - implemented numerous methods in DOMCoreClasses.cpp and added classes for missing DOM objects.
Comment 3 WebKit Review Bot 2010-03-31 11:47:30 PDT
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.
Comment 4 Adam Roben (:aroben) 2010-03-31 11:51:11 PDT
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.
Comment 5 Peter Nelson 2010-04-02 11:00:05 PDT
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.
Comment 6 Eric Seidel (no email) 2010-04-05 16:58:47 PDT
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++?)
Comment 7 Eric Seidel (no email) 2010-04-05 16:59:30 PDT
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.
Comment 8 WebKit Commit Bot 2010-04-05 23:48:56 PDT
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>
Comment 9 WebKit Commit Bot 2010-04-05 23:49:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Emerick Rogul 2010-04-18 16:57:17 PDT
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.
Comment 11 Adam Roben (:aroben) 2010-04-19 08:06:10 PDT
Re-opening this bug so Peter can keep working on it.
Comment 12 Peter Nelson 2010-04-20 02:46:17 PDT
Created attachment 53790 [details]
Patch part #2 - Class declarations for a number missing DOM objects in DOMCoreClasses.h
Comment 13 Peter Nelson 2010-04-20 03:09:16 PDT
Will unflagging the very first patch as obsolete prevent this bug from being auto-closed again?
Comment 14 Eric Seidel (no email) 2010-04-20 04:22:16 PDT
The commit-queue will automatically close bugs if no patches remain with r? or r+.
Comment 15 Eric Seidel (no email) 2010-05-09 15:00:50 PDT
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?
Comment 16 Adam Barth 2010-06-20 10:37:52 PDT
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.
Comment 17 Adam Roben (:aroben) 2010-06-21 06:29:35 PDT
(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>.