Bug 12686 - REGRESSION: Bloglines.com Feeds tab cannot expand folders in TOT
Summary: REGRESSION: Bloglines.com Feeds tab cannot expand folders in TOT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 420+
Hardware: Mac (Intel) OS X 10.4
: P1 Normal
Assignee: Nobody
URL: http://www.bloglines.com/myblogs
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-02-07 16:35 PST by Charles Ying
Modified: 2007-03-06 18:50 PST (History)
3 users (show)

See Also:


Attachments
Bloglines screenshot with Expected results (Stock Safari + Firefox) (113.68 KB, image/jpeg)
2007-02-07 16:36 PST, Charles Ying
no flags Details
Bloglines error results in TOT WebKit and console error message (117.05 KB, image/jpeg)
2007-02-07 16:36 PST, Charles Ying
no flags Details
Reduced test case. (514 bytes, text/html)
2007-02-15 17:40 PST, Yongjun Zhang
no flags Details
patch for review (3.29 KB, patch)
2007-02-21 17:02 PST, Yongjun Zhang
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charles Ying 2007-02-07 16:35:23 PST
Repro steps:

1. Go to URL, log into Bloglines
2. Create a folder with feeds inside it
3. Try to expand the folder by clicking on the + button to the left of the folder
4. The folder does not expand, and a message is logged: "(event handler):Null value" on the Terminal

WebKit TOT does not work, (screenshot attached)
Safari 2.0.4 (419.3) works fine (screenshot attached)
Firefox 2.0.0.1 (Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1) works fine.
Comment 1 Charles Ying 2007-02-07 16:36:15 PST
Created attachment 13020 [details]
Bloglines screenshot with Expected results (Stock Safari + Firefox)
Comment 2 Charles Ying 2007-02-07 16:36:53 PST
Created attachment 13021 [details]
Bloglines error results in TOT WebKit and console error message
Comment 3 Maciej Stachowiak 2007-02-10 19:12:49 PST
<rdar://problem/4990043>
Comment 4 Yongjun Zhang 2007-02-15 17:40:44 PST
Created attachment 13194 [details]
Reduced test case.

The reduced test case shows:

In Safar 2.0, 'typeof document.getElementById("123")' returns "undefined" while TOT returns "object" and firefox returns something else (not "object", maybe "function").

However, the real problem is TOT doesn't support 'instanceof' for DOM objects, and the line "wh instanceof Element" returns false as a result. I wonder if we need to make DOMNode::implementsHasInstance() return true.
Comment 5 Yongjun Zhang 2007-02-15 17:47:25 PST
Just a quick check, I enabled hasInstance call for DOM object by making DOMNode::implementsHasInstance() return true.  The site works fine now.
Comment 6 mitz 2007-02-17 04:13:10 PST
See also bug 10686
Comment 7 Maciej Stachowiak 2007-02-20 02:01:49 PST
I'd love to see a patch along these lines.
Comment 8 Yongjun Zhang 2007-02-21 17:02:24 PST
Created attachment 13306 [details]
patch for review

Enable hasInstance for DOMObject by making implementsHasInstance return true.
Comment 9 Darin Adler 2007-02-21 17:16:22 PST
Comment on attachment 13306 [details]
patch for review

I believe this patch is incorrect. The constructor objects should return true for implementsHasInstance, but actual DOM element instances should not.

Thus I think the implementsHasInstance function should be in the constructor class definition inside CodeGeneratorJS.pm. It can go right above the implementsConstruct function.

To minimize the total amount of code inside CodeGeneratorJS.pm, we might want to make a DOMConstructor base class to inherit from. That's not needed to fix this bug, but I do see a reasonable amount of code that could be moved out of the auto-generation script, including:

    1) the setPrototype call in the constructor
    2) the getValueProperty member function
    3) the implementsConstruct member function
    4) the implementsHasInstance member function

But I'm not sure it's worth it.

We use spaces for indenting, not tabs. The patch has a number of tabs in it.
Comment 10 David Kilzer (:ddkilzer) 2007-03-06 18:50:36 PST
Fix committed by Kevin McCullough in r19993.

http://trac.webkit.org/projects/webkit/changeset/19993