Bug 42875 – Atomize strings used by the HTML5 tree builder

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Atomize strings used by the HTML5 tree builder

[Adam Barth]

Created attachment 62385 [details]
Patch

[Adam Barth]

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

[WebKit Commit Bot]

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

[Adam Barth]

I don't understand how this patch affected the old tree builder.  For example, the bgsound element no longer appears in the DOM...

[Adam Barth]

Created attachment 62452 [details]
Patch

[Adam Barth]

Eric, please take another look.  The fix for this issue is kind of goofy.  :(

[Eric Seidel (no email)]

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?

[Adam Barth]

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.

[Eric Seidel (no email)]

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.

[Darin Adler]

Comment on attachment 62452 [details]
Patch

> +            if (*tags[i] == HTMLNames::bgsoundTag

Why not "using namespace HTMLNames" at the top of this file?

r=me

[Adam Barth]

> Why not "using namespace HTMLNames" at the top of this file?

Will do.

[Adam Barth]

Created attachment 62506 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 62506 [details]
Patch for landing

Clearing flags on attachment: 62506

Committed r64018: <http://trac.webkit.org/changeset/64018>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.