Bug 75699

Summary: Disconnecting DOMWindow properties is fragile and overly complicated
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, eric, japhet, macpherson, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 75697    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Description Adam Barth 2012-01-06 04:28:03 PST
Disconnecting DOMWindow properties is fragile and overly complicated
Comment 1 Adam Barth 2012-01-06 04:31:26 PST
Created attachment 121423 [details]
Patch
Comment 2 Adam Barth 2012-01-06 04:34:25 PST
Created attachment 121424 [details]
Patch
Comment 3 Alexey Proskuryakov 2012-01-06 10:32:51 PST
Comment on attachment 121424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121424&action=review

Will DOMWindowProperty be able to reconnect, finally resolving the horrible, horrible bug 34679? Any cleanup in this area should have fixing that bug as its final goal.

> Source/WebCore/page/Console.cpp:339
> -MemoryInfo* Console::memory() const
> +PassRefPtr<MemoryInfo> Console::memory() const

Does this mean that console.memory===console.memory will no longer be true?

> Source/WebCore/page/DOMWindow.cpp:517
> +    // FIXME: Notifications should work like the other properties.

Would be helpful to clarify in which respect they are different. One will be able to discover the context from svn blame, but that's harder than just reading a comment.
Comment 4 Adam Barth 2012-01-06 13:48:20 PST
> Will DOMWindowProperty be able to reconnect, finally resolving the horrible, horrible bug 34679? Any cleanup in this area should have fixing that bug as its final goal.

Yes.  That's one of the things I'm headed towards.  By keeping a list of all the properties, we'll be able to walk that list and reconnect them!

> > Source/WebCore/page/Console.cpp:339
> > -MemoryInfo* Console::memory() const
> > +PassRefPtr<MemoryInfo> Console::memory() const
> 
> Does this mean that console.memory===console.memory will no longer be true?

It never was true.  Even before the patch m_memory was always created fresh every time the property was accessed.  I can add a FIXME about changing this behavior, but I'd prefer to keep functional changes for a future patch.

> > Source/WebCore/page/DOMWindow.cpp:517
> > +    // FIXME: Notifications should work like the other properties.
> 
> Would be helpful to clarify in which respect they are different. One will be able to discover the context from svn blame, but that's harder than just reading a comment.

Will do.
Comment 5 Alexey Proskuryakov 2012-01-06 13:56:26 PST
Comment on attachment 121424 [details]
Patch

I don't know how these "MemoryInfo" properties were allowed into WebKit. They feel wrong.

r=me
Comment 6 Adam Barth 2012-01-06 14:17:07 PST
> I don't know how these "MemoryInfo" properties were allowed into WebKit. They feel wrong.

Looks like it was added in http://trac.webkit.org/changeset/63537.  I agree that the current code doesn't make much sense.
Comment 7 Adam Barth 2012-01-06 17:00:01 PST
Comment on attachment 121424 [details]
Patch

Need to land the earlier patch first.
Comment 8 Adam Barth 2012-01-06 23:34:02 PST
Created attachment 121541 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-01-06 23:36:19 PST
Attachment 121541 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/page/DOMWindowProperty.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Adam Barth 2012-01-06 23:38:32 PST
Created attachment 121542 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-01-07 00:46:55 PST
Comment on attachment 121542 [details]
Patch for landing

Clearing flags on attachment: 121542

Committed r104380: <http://trac.webkit.org/changeset/104380>
Comment 12 WebKit Review Bot 2012-01-07 00:47:01 PST
All reviewed patches have been landed.  Closing bug.