Summary: | Disconnecting DOMWindow properties is fragile and overly complicated | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||||
Component: | New Bugs | Assignee: | 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
Adam Barth
2012-01-06 04:28:03 PST
Created attachment 121423 [details]
Patch
Created attachment 121424 [details]
Patch
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. > 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 on attachment 121424 [details]
Patch
I don't know how these "MemoryInfo" properties were allowed into WebKit. They feel wrong.
r=me
> 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 on attachment 121424 [details]
Patch
Need to land the earlier patch first.
Created attachment 121541 [details]
Patch for landing
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.
Created attachment 121542 [details]
Patch for landing
Comment on attachment 121542 [details] Patch for landing Clearing flags on attachment: 121542 Committed r104380: <http://trac.webkit.org/changeset/104380> All reviewed patches have been landed. Closing bug. |