40393 2010-06-09 16:09:56 -0700 HTML5 Parser: Fix fast/profiler tests that depend on event handler line numbers 2010-06-10 11:18:14 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 39259 1 tonyg tonyg abarth eric oldest_to_newest 236072 0 tonyg 2010-06-09 16:09:56 -0700 HTML5 Parser: Fix fast/profiler tests that depend on event handler line numbers 236073 1 58306 tonyg 2010-06-09 16:15:39 -0700 Created attachment 58306 Patch 236078 2 58306 abarth 2010-06-09 16:33:08 -0700 Comment on attachment 58306 Patch WebCore/html/HTML5Tokenizer.cpp:105 + if (scriptController) I'm worried that constructTreeFromToken can cause script to run synchronously, which could invalidate the |frame|, which would invalidate scriptController. Maybe we need a private helper function to get the script controller? if (ScriptController* scriptController = script()) scriptController->setEventHandlerLineNumber(...) 236178 3 abarth 2010-06-09 23:14:04 -0700 I'm going to spoil you, but I'm going to merge your patch to TOT. :) 236180 4 abarth 2010-06-09 23:35:01 -0700 Committed r60940: <http://trac.webkit.org/changeset/60940> 236272 5 eric 2010-06-10 03:11:00 -0700 Woh. Wasn't this a huge slowdown on the benchmark? We're adding two branches and a virtual function call to the per-token loop, no? 236273 6 eric 2010-06-10 03:12:42 -0700 Why do we need to reset the line number to 0 after each token is processed? 236365 7 tonyg 2010-06-10 07:52:25 -0700 (In reply to comment #3) > I'm going to spoil you, but I'm going to merge your patch to TOT. :) Thanks! Man you're fast. I had the same patch ready but it didn't finish building before the get together last night. There was one more place in Tokenizer that the script() fn could be used. I'll mail a patch for that. 236366 8 tonyg 2010-06-10 07:54:42 -0700 (In reply to comment #6) > Why do we need to reset the line number to 0 after each token is processed? I haven't confirmed, but I'm nearly certain that is for the case when a script dynamically adds an element with an event handler (it shouldn't be associated w/ the line number of the parser). In any case, it seemed safest to match the old Tokenizer's behavior for now. 236367 9 tonyg 2010-06-10 07:55:12 -0700 (In reply to comment #5) > Woh. Wasn't this a huge slowdown on the benchmark? We're adding two branches and a virtual function call to the per-token loop, no? Can you point me to the benchmark to run? 236408 10 abarth 2010-06-10 10:12:51 -0700 > Can you point me to the benchmark to run? http://trac.webkit.org/browser/trunk/WebCore/benchmarks/parser/html-parser.html 236459 11 eric 2010-06-10 11:18:14 -0700 (In reply to comment #8) > (In reply to comment #6) > > Why do we need to reset the line number to 0 after each token is processed? > > I haven't confirmed, but I'm nearly certain that is for the case when a script dynamically adds an element with an event handler (it shouldn't be associated w/ the line number of the parser). In any case, it seemed safest to match the old Tokenizer's behavior for now. Some elements carry a createdByParser flag. But I guess not all. Otherwise that would be easy to check after the fact. 58306 2010-06-09 16:15:39 -0700 2010-06-09 16:45:45 -0700 Patch bug-40393-20100609161537.patch text/plain 1883 tonyg ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg ZWZiMDY1MjkwMzVjZmFmMDI0M2NhNjE5YjI5ZGE5YWJhMDI4ZWUzYi4uNGQ2MThlNzM0NmQyZGRl YTAwYTllMzhkYjNkNWViMzRiMTU3NjhmYiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMSBAQAorMjAxMC0wNi0wOSAgVG9u eSBHZW50aWxjb3JlICA8dG9ueWdAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEhUTUw1IFBhcnNlcjogRml4IGZhc3QvcHJvZmls ZXIgdGVzdHMgdGhhdCBkZXBlbmQgb24gZXZlbnQgaGFuZGxlciBsaW5lIG51bWJlcnMKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTQwMzkzCisKKyAgICAg ICAgVGhpcyBlbXVsYXRlZCB0aGUgb2xkIGJlaGF2aW9yIGluIEhUTUxUb2tlbml6ZXI6cHJvY2Vz c1Rva2VuKCkKKworICAgICAgICBObyBuZXcgdGVzdHMgYmVjYXVzZSBjb3ZlcmVkIGJ5OgorICAg ICAgICAgLSBmYXN0L3Byb2ZpbGVyL2RlYWQtdGltZS5odG1sCisgICAgICAgICAtIGZhc3QvcHJv ZmlsZXIvaW5saW5lLWV2ZW50LWhhbmRsZXIuaHRtbAorICAgICAgICAgLSBmYXN0L3Byb2ZpbGVy L3N0b3AtcHJvZmlsaW5nLWFmdGVyLXNldFRpbWVvdXQuaHRtbAorICAgICAgICAgLSBmYXN0L3By b2ZpbGVyL3Rocm93LWV4Y2VwdGlvbi1mcm9tLWV2YWwuaHRtbAorCisgICAgICAgICogaHRtbC9I VE1MNVRva2VuaXplci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpIVE1MNVRva2VuaXplcjo6cHVt cExleGVyKToKKwogMjAxMC0wNi0wOSAgUGF2ZWwgUG9kaXZpbG92ICA8cG9kaXZpbG92QGNocm9t aXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBZdXJ5IFNlbWlraGF0c2t5LgpkaWZmIC0t Z2l0IGEvV2ViQ29yZS9odG1sL0hUTUw1VG9rZW5pemVyLmNwcCBiL1dlYkNvcmUvaHRtbC9IVE1M NVRva2VuaXplci5jcHAKaW5kZXggMzc1YWRlNDMzMTQzYzhjYWE1MWE5MmFlZDE4ZGY2MjExODZl OGU2Ny4uMGFiYThhMmIwNDRiZWNiMTk4MDgxZDI2NzIyZmM4OTQyZDYxODk3OSAxMDA2NDQKLS0t IGEvV2ViQ29yZS9odG1sL0hUTUw1VG9rZW5pemVyLmNwcAorKysgYi9XZWJDb3JlL2h0bWwvSFRN TDVUb2tlbml6ZXIuY3BwCkBAIC05NSw5ICs5NSwxNiBAQCB2b2lkIEhUTUw1VG9rZW5pemVyOjpw dW1wTGV4ZXIoKQogICAgIEFTU0VSVCghbV9wYXJzZXJTdG9wcGVkKTsKICAgICBBU1NFUlQoIW1f dHJlZUJ1aWxkZXItPmlzUGF1c2VkKCkpOwogICAgIHdoaWxlICghbV9wYXJzZXJTdG9wcGVkICYm IG1fbGV4ZXItPm5leHRUb2tlbihtX3NvdXJjZSwgbV90b2tlbikpIHsKKyAgICAgICAgU2NyaXB0 Q29udHJvbGxlciogc2NyaXB0Q29udHJvbGxlciA9IG1fZG9jdW1lbnQtPmZyYW1lKCkgPyBtX2Rv Y3VtZW50LT5mcmFtZSgpLT5zY3JpcHQoKSA6IDA7CisgICAgICAgIGlmIChzY3JpcHRDb250cm9s bGVyKQorICAgICAgICAgICAgc2NyaXB0Q29udHJvbGxlci0+c2V0RXZlbnRIYW5kbGVyTGluZU51 bWJlcihsaW5lTnVtYmVyKCkgKyAxKTsKKyAgICAgICAgCiAgICAgICAgIG1fdHJlZUJ1aWxkZXIt PmNvbnN0cnVjdFRyZWVGcm9tVG9rZW4obV90b2tlbik7CiAgICAgICAgIG1fdG9rZW4uY2xlYXIo KTsKIAorICAgICAgICBpZiAoc2NyaXB0Q29udHJvbGxlcikKKyAgICAgICAgICAgIHNjcmlwdENv bnRyb2xsZXItPnNldEV2ZW50SGFuZGxlckxpbmVOdW1iZXIoMCk7CisgICAgICAgIAogICAgICAg ICBpZiAoIW1fdHJlZUJ1aWxkZXItPmlzUGF1c2VkKCkpCiAgICAgICAgICAgICBjb250aW51ZTsK IAo=