Bug 75699 – Disconnecting DOMWindow properties is fragile and overly complicated

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Disconnecting DOMWindow properties is fragile and overly complicated

[Adam Barth]

Created attachment 121423 [details]
Patch

[Adam Barth]

Created attachment 121424 [details]
Patch

[Alexey Proskuryakov]

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.

[Adam Barth]

> 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.

[Alexey Proskuryakov]

Comment on attachment 121424 [details]
Patch

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

r=me

[Adam Barth]

> 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.

[Adam Barth]

Comment on attachment 121424 [details]
Patch

Need to land the earlier patch first.

[Adam Barth]

Created attachment 121541 [details]
Patch for landing

[WebKit Review Bot]

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.

[Adam Barth]

Created attachment 121542 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 121542 [details]
Patch for landing

Clearing flags on attachment: 121542

Committed r104380: <http://trac.webkit.org/changeset/104380>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.