| Summary: | Atomize strings used by the HTML5 tree builder | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||
| Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | commit-queue, eric, hyatt | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Other | ||||||||||
| OS: | OS X 10.5 | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 41123 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Adam Barth
2010-07-23 00:11:12 PDT
Created attachment 62385 [details]
Patch
These numbers are from my laptop on battery, so they're not as rock solid as usual. Your milage may vary: == pre == Running 20 times Ignoring warm-up run (4940) 4330 4334 4304 4390 4409 4338 4331 4353 4349 4360 4364 4368 4378 4379 4378 4399 4416 4396 4429 4468 avg 4373.65 stdev 38.241698445545005 == post == Running 20 times Ignoring warm-up run (4825) 4251 4298 4262 4262 4282 4312 4346 4312 4304 4327 4325 4319 4342 4343 4337 4379 4352 4358 4376 4363 avg 4322.5 stdev 36.51232668565508 Comment on attachment 62385 [details] Patch Rejecting patch 62385 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 20713 test cases. html5lib/runner.html -> failed Exiting early after 1 failures. 15522 tests run. 318.97s total testing time 15521 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 26 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/3566399 I don't understand how this patch affected the old tree builder. For example, the bgsound element no longer appears in the DOM... Created attachment 62452 [details]
Patch
Eric, please take another look. The fix for this issue is kind of goofy. :( Comment on attachment 62452 [details]
Patch
That's just crazy. I wonder how many other tags like this that we're "recognizing" and not supposed to.
I don't understand why this is the correct fix. Why haven't we had trouble when we added <video> and <audio> etc?
It looks like some of the other conditional tags are in the static HashSet<AtomicStringImpl*>* inlineTagList() However, not all of them have their conditions, which leads me to believe those builds are broken. It's also unclear to me whether we parse HTML the same when you change these compile flags. I'll be glad to get rid of this mess. At the moment, the attached patch appears to be the most expedient. OK. I'll look at the patch again this evening. I just want to make sure we're not completely pooping on the old tree builder. I'm certainly not going to ask you to re-arch it or anything, but I'd like to stare at how the other tags are working before r+ing this. Comment on attachment 62452 [details] Patch > + if (*tags[i] == HTMLNames::bgsoundTag Why not "using namespace HTMLNames" at the top of this file? r=me > Why not "using namespace HTMLNames" at the top of this file?
Will do.
Created attachment 62506 [details]
Patch for landing
Comment on attachment 62506 [details] Patch for landing Clearing flags on attachment: 62506 Committed r64018: <http://trac.webkit.org/changeset/64018> All reviewed patches have been landed. Closing bug. |