Bug 65347

Summary:

Refactor JS objects to allocate in static create methods rather than constructors

Product:

WebKit

Reporter:

Mark Hahnenberg <mhahnenberg>

Component:

JavaScriptCore

Assignee:

Mark Hahnenberg <mhahnenberg>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ggaren, oliver, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

65288, 66265, 66567

Bug Blocks:

Attachments:

Description	Flags
Moved all allocation out of initialization lists in subclass of JSCell	none
Fixing build errors	none
Fixing build errors	none
Revised patch	none
Fixing qt build	none
Moving all allocation in constructors into finishAllocations() method	none
Fixing build errors	none
Fixing build errors	none

Mark Hahnenberg

Reported 2011-07-28 16:02:37 PDT

Now that we have static create methods and we can assert that we allocate only when we're not initializing another object, we want to move as much of the allocation done inside of constructors as possible into their static create methods. Currently we clear the m_initializingObject flag inside of JSGlobalData if we do any sort of nested allocation within a constructor and then set it back to its original value after that allocation is complete. Ideally, we wouldn't have to do this at all.

Attachments
Moved all allocation out of initialization lists in subclass of JSCell (35.40 KB, patch) 2011-08-10 19:25 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Fixing build errors (28.31 KB, patch) 2011-08-11 09:00 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Fixing build errors (28.48 KB, patch) 2011-08-11 10:12 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Revised patch (32.14 KB, patch) 2011-08-15 09:57 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Fixing qt build (32.12 KB, patch) 2011-08-15 11:06 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Moving all allocation in constructors into finishAllocations() method (69.93 KB, patch) 2011-08-15 14:23 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Fixing build errors (43.69 KB, patch) 2011-08-15 14:50 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Fixing build errors (43.69 KB, patch) 2011-08-15 16:01 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Mark Hahnenberg

Comment 1 2011-08-10 19:25:48 PDT

Created attachment 103570 [details] Moved all allocation out of initialization lists in subclass of JSCell

Mark Hahnenberg

Comment 2 2011-08-11 09:00:53 PDT

Created attachment 103633 [details] Fixing build errors

Early Warning System Bot

Comment 3 2011-08-11 09:16:05 PDT

Comment on attachment 103633 [details] Fixing build errors Attachment 103633 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9357258

Mark Hahnenberg

Comment 4 2011-08-11 10:12:37 PDT

Created attachment 103642 [details] Fixing build errors

Geoffrey Garen

Comment 5 2011-08-12 18:17:59 PDT

Comment on attachment 103642 [details] Fixing build errors View in context: https://bugs.webkit.org/attachment.cgi?id=103642&action=review The rest of this patch looks good. But let's get rid of that cast. > Source/WebCore/bindings/js/JSDOMBinding.h:91 > - return cacheDOMStructure(globalObject, WrapperClass::createStructure(exec->globalData(), WrapperClass::createPrototype(exec, globalObject)), &WrapperClass::s_info); > + return cacheDOMStructure(globalObject, WrapperClass::createStructure(exec->globalData(), JSC::JSValue((JSC::JSCell*)WrapperClass::createPrototype(exec, globalObject))), &WrapperClass::s_info); Let's find the missing #include that necessitated this cast. I'd rather not add a cast like this if we don't have to.

Mark Hahnenberg

Comment 6 2011-08-15 09:57:34 PDT

Created attachment 103920 [details] Revised patch

Early Warning System Bot

Comment 7 2011-08-15 10:08:57 PDT

Comment on attachment 103920 [details] Revised patch Attachment 103920 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9379723

Mark Hahnenberg

Comment 8 2011-08-15 11:06:09 PDT

Created attachment 103928 [details] Fixing qt build

Geoffrey Garen

Comment 9 2011-08-15 13:02:16 PDT

Comment on attachment 103928 [details] Fixing qt build r=me

WebKit Review Bot

Comment 10 2011-08-15 14:12:26 PDT

Comment on attachment 103928 [details] Fixing qt build Clearing flags on attachment: 103928 Committed r93059: <http://trac.webkit.org/changeset/93059>

WebKit Review Bot

Comment 11 2011-08-15 14:12:30 PDT

All reviewed patches have been landed. Closing bug.

Mark Hahnenberg

Comment 12 2011-08-15 14:23:51 PDT

Created attachment 103952 [details] Moving all allocation in constructors into finishAllocations() method

Mark Hahnenberg

Comment 13 2011-08-15 14:50:26 PDT

Created attachment 103958 [details] Fixing build errors

Mark Hahnenberg

Comment 14 2011-08-15 16:01:20 PDT

Created attachment 103970 [details] Fixing build errors

Mark Hahnenberg

Comment 15 2011-08-15 16:52:57 PDT

Reopening this bug because its description/goal is not yet complete. Also opening other bugs to resolve this discrepancy a step at a time.

Mark Hahnenberg

Comment 16 2011-09-20 14:59:47 PDT

All subtasks complete, closing bug.

Note You need to log in before you can comment on or make changes to this bug.