49703 2010-11-17 18:03:27 -0800 HTMLScriptRunner and ScriptElement duplicate code 2011-03-01 00:12:09 -0800 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 49701 1 rniwa rniwa abarth ap darin eric tony oldest_to_newest 310652 0 rniwa 2010-11-17 18:03:27 -0800 HTMLScriptRunner and ScriptElement implement very similar features. We should try to share code between the two as much as possible. 313431 1 74787 rniwa 2010-11-24 13:13:07 -0800 Created attachment 74787 work in progress 314158 2 74787 darin 2010-11-27 18:17:41 -0800 Comment on attachment 74787 work in progress View in context: https://bugs.webkit.org/attachment.cgi?id=74787&action=review > WebCore/dom/ScriptElement.h:48 > + void evaluateScriptWithEvents(CachedScript*); > + void executeScriptWithEvents(const ScriptSourceCode&, bool errorOccurred); I think these both can be called executeScript, no need to call one evaluate and one execute. C++ overloading will work fine. The use of boolean for “errorOccurred” is strange, though. A boolean that when true means, “no don’t execute the script at all, just send an error event”, is not good design. Two separate functions would be better even though it’s slightly more code at the call site. 314165 3 rniwa 2010-11-27 20:19:57 -0800 Thanks for the feedback, Darin! (In reply to comment #2) > > WebCore/dom/ScriptElement.h:48 > > + void evaluateScriptWithEvents(CachedScript*); > > + void executeScriptWithEvents(const ScriptSourceCode&, bool errorOccurred); > > I think these both can be called executeScript, no need to call one evaluate and one execute. C++ overloading will work fine. It turned out that ScriptController's evaluate and execute functions differ slightly. They have slightly different rules on how to choose DOMWrapperWorld, and while execute may trigger layout, evaluate never does. I don't like this ever-slight difference between the two and the fact names "evaluate" and "execute" do not convey any of those differences. But we should probably resolve that in a separate patch. > The use of boolean for “errorOccurred” is strange, though. A boolean that when true means, “no don’t execute the script at all, just send an error event”, is not good design. Two separate functions would be better even though it’s slightly more code at the call site. This is to do with what sourceFromPendingScript does and releaseElementAndClear: http://trac.webkit.org/browser/trunk/WebCore/html/parser/HTMLScriptRunner.cpp 128 bool errorOccurred = false; 129 ScriptSourceCode sourceCode = sourceFromPendingScript(pendingScript, errorOccurred); 130 131 // Stop watching loads before executeScript to prevent recursion if the script reloads itself. 132 if (pendingScript.cachedScript() && pendingScript.watchingForLoad()) 133 stopWatchingForLoad(pendingScript); 134 135 // Clear the pending script before possible rentrancy from executeScript() 136 RefPtr<Element> element = pendingScript.releaseElementAndClear(); 137 if (ScriptElement* scriptElement = toScriptElement(element.get())) { 138 NestingLevelIncrementer nestingLevelIncrementer(m_scriptNestingLevel); 139 IgnoreDestructiveWriteCountIncrementer ignoreDestructiveWriteCountIncrementer(m_document); 140 if (errorOccurred) 141 element->dispatchEvent(createScriptErrorEvent()); 142 else { 143 ASSERT(isExecutingScript()); 144 scriptElement->executeScript(sourceCode); 145 element->dispatchEvent(createScriptLoadEvent()); 146 } 147 } 148 ASSERT(!m_scriptNestingLevel); We need to call releaseElementAndClear in order to avoid running the same script twice but it'll clear the script cache (removes itself from the resource loader) so we need to obtain the SourceCode before calling this function. But then we can't pass CachedScript to executeScriptWithEvents as in evaluateScriptWithEvents. I need to spend some time thinking about how to clean up all of these dependencies and legacy structures. 359698 4 rniwa 2011-03-01 00:12:09 -0800 http://trac.webkit.org/changeset/79114 fixed this. 74787 2010-11-24 13:13:07 -0800 2010-11-27 18:17:41 -0800 work in progress wip49703 text/plain 3007 rniwa SW5kZXg6IFdlYkNvcmUvZG9tL1NjcmlwdEVsZW1lbnQuY3BwCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNv cmUvZG9tL1NjcmlwdEVsZW1lbnQuY3BwCShyZXZpc2lvbiA3MjY0MykKKysrIFdlYkNvcmUvZG9t L1NjcmlwdEVsZW1lbnQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yMjMsNyArMjIzLDcgQEAKICAg ICB9CiB9CiAKLXZvaWQgU2NyaXB0RWxlbWVudDo6ZXhlY3V0ZShDYWNoZWRTY3JpcHQqIGNhY2hl ZFNjcmlwdCkKK3ZvaWQgU2NyaXB0RWxlbWVudDo6ZXZhbHVhdGVTY3JpcHRXaXRoRXZlbnRzKENh Y2hlZFNjcmlwdCogY2FjaGVkU2NyaXB0KQogewogICAgIEFTU0VSVChjYWNoZWRTY3JpcHQpOwog ICAgIGlmIChjYWNoZWRTY3JpcHQtPmVycm9yT2NjdXJyZWQoKSkKQEAgLTIzNSw2ICsyMzUsMTYg QEAKICAgICBjYWNoZWRTY3JpcHQtPnJlbW92ZUNsaWVudCh0aGlzKTsKIH0KIAordm9pZCBTY3Jp cHRFbGVtZW50OjpleGVjdXRlU2NyaXB0V2l0aEV2ZW50cyhjb25zdCBTY3JpcHRTb3VyY2VDb2Rl JiBzb3VyY2VDb2RlLCBib29sIGVycm9yT2NjdXJyZWQpCit7CisgICAgaWYgKGVycm9yT2NjdXJy ZWQpCisgICAgICAgIGRpc3BhdGNoRXJyb3JFdmVudCgpOworICAgIGVsc2UgeworICAgICAgICBl eGVjdXRlU2NyaXB0KHNvdXJjZUNvZGUpOworICAgICAgICBkaXNwYXRjaExvYWRFdmVudCgpOwor ICAgIH0KK30KKwogdm9pZCBTY3JpcHRFbGVtZW50Ojpub3RpZnlGaW5pc2hlZChDYWNoZWRSZXNv dXJjZSogbykKIHsKICAgICBBU1NFUlRfVU5VU0VEKG8sIG8gPT0gbV9jYWNoZWRTY3JpcHQpOwpJ bmRleDogV2ViQ29yZS9kb20vQXN5bmNTY3JpcHRSdW5uZXIuY3BwCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdl YkNvcmUvZG9tL0FzeW5jU2NyaXB0UnVubmVyLmNwcAkocmV2aXNpb24gNzI2NDMpCisrKyBXZWJD b3JlL2RvbS9Bc3luY1NjcmlwdFJ1bm5lci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTg0LDcgKzg0 LDcgQEAKICAgICBzY3JpcHRzLnN3YXAobV9zY3JpcHRzVG9FeGVjdXRlU29vbik7CiAgICAgc2l6 ZV90IHNpemUgPSBzY3JpcHRzLnNpemUoKTsKICAgICBmb3IgKHNpemVfdCBpID0gMDsgaSA8IHNp emU7ICsraSkgewotICAgICAgICBzY3JpcHRzW2ldLmZpcnN0LT5leGVjdXRlKHNjcmlwdHNbaV0u c2Vjb25kLmdldCgpKTsKKyAgICAgICAgc2NyaXB0c1tpXS5maXJzdC0+ZXZhbHVhdGVTY3JpcHRX aXRoRXZlbnRzKHNjcmlwdHNbaV0uc2Vjb25kLmdldCgpKTsKICAgICAgICAgc2NyaXB0c1tpXS5m aXJzdC0+ZWxlbWVudCgpLT5kZXJlZigpOyAvLyBCYWxhbmNlcyByZWYoKSBpbiBleGVjdXRlU2Ny aXB0U29vbigpLgogICAgICAgICBtX2RvY3VtZW50LT5kZWNyZW1lbnRMb2FkRXZlbnREZWxheUNv dW50KCk7CiAgICAgfQpJbmRleDogV2ViQ29yZS9kb20vU2NyaXB0RWxlbWVudC5oCj09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT0KLS0tIFdlYkNvcmUvZG9tL1NjcmlwdEVsZW1lbnQuaAkocmV2aXNpb24gNzI2NDMpCisrKyBX ZWJDb3JlL2RvbS9TY3JpcHRFbGVtZW50LmgJKHdvcmtpbmcgY29weSkKQEAgLTQ0LDcgKzQ0LDgg QEAKICAgICBTdHJpbmcgc2NyaXB0Q29udGVudCgpIGNvbnN0OwogICAgIGJvb2wgc2hvdWxkRXhl Y3V0ZUFzSmF2YVNjcmlwdCgpIGNvbnN0OwogICAgIHZvaWQgZXhlY3V0ZVNjcmlwdChjb25zdCBT Y3JpcHRTb3VyY2VDb2RlJik7Ci0gICAgdm9pZCBleGVjdXRlKENhY2hlZFNjcmlwdCopOworICAg IHZvaWQgZXZhbHVhdGVTY3JpcHRXaXRoRXZlbnRzKENhY2hlZFNjcmlwdCopOworICAgIHZvaWQg ZXhlY3V0ZVNjcmlwdFdpdGhFdmVudHMoY29uc3QgU2NyaXB0U291cmNlQ29kZSYsIGJvb2wgZXJy b3JPY2N1cnJlZCk7CiAKICAgICAvLyBYTUwgcGFyc2VyIGNhbGxzIHRoZXNlCiAgICAgdmlydHVh bCBTdHJpbmcgc291cmNlQXR0cmlidXRlVmFsdWUoKSBjb25zdCA9IDA7CkluZGV4OiBXZWJDb3Jl L2h0bWwvcGFyc2VyL0hUTUxTY3JpcHRSdW5uZXIuY3BwCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUv aHRtbC9wYXJzZXIvSFRNTFNjcmlwdFJ1bm5lci5jcHAJKHJldmlzaW9uIDcyNjQzKQorKysgV2Vi Q29yZS9odG1sL3BhcnNlci9IVE1MU2NyaXB0UnVubmVyLmNwcAkod29ya2luZyBjb3B5KQpAQCAt MTM3LDEzICsxMzcsNyBAQAogICAgIGlmIChTY3JpcHRFbGVtZW50KiBzY3JpcHRFbGVtZW50ID0g dG9TY3JpcHRFbGVtZW50KGVsZW1lbnQuZ2V0KCkpKSB7CiAgICAgICAgIE5lc3RpbmdMZXZlbElu Y3JlbWVudGVyIG5lc3RpbmdMZXZlbEluY3JlbWVudGVyKG1fc2NyaXB0TmVzdGluZ0xldmVsKTsK ICAgICAgICAgSWdub3JlRGVzdHJ1Y3RpdmVXcml0ZUNvdW50SW5jcmVtZW50ZXIgaWdub3JlRGVz dHJ1Y3RpdmVXcml0ZUNvdW50SW5jcmVtZW50ZXIobV9kb2N1bWVudCk7Ci0gICAgICAgIGlmIChl cnJvck9jY3VycmVkKQotICAgICAgICAgICAgZWxlbWVudC0+ZGlzcGF0Y2hFdmVudChjcmVhdGVT Y3JpcHRFcnJvckV2ZW50KCkpOwotICAgICAgICBlbHNlIHsKLSAgICAgICAgICAgIEFTU0VSVChp c0V4ZWN1dGluZ1NjcmlwdCgpKTsKLSAgICAgICAgICAgIHNjcmlwdEVsZW1lbnQtPmV4ZWN1dGVT Y3JpcHQoc291cmNlQ29kZSk7Ci0gICAgICAgICAgICBlbGVtZW50LT5kaXNwYXRjaEV2ZW50KGNy ZWF0ZVNjcmlwdExvYWRFdmVudCgpKTsKLSAgICAgICAgfQorICAgICAgICBzY3JpcHRFbGVtZW50 LT5leGVjdXRlU2NyaXB0V2l0aEV2ZW50cyhzb3VyY2VDb2RlLCBlcnJvck9jY3VycmVkKTsKICAg ICB9CiAgICAgQVNTRVJUKCFtX3NjcmlwdE5lc3RpbmdMZXZlbCk7CiB9Cg==