Bug 44036 – Port Chromium's WebEntities to HTMLEntityTable

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Remove Chromium's broken WebEntities code since it depends on removed WebCore files

[Eric Seidel (no email)]

Created attachment 64452 [details]
Patch

[Eric Seidel (no email)]

CCing the current webkit gardner.

[Mihai Parparita]

+Yaar, Darin and Johnny, since WebEntities was added by Yaar and reviewed by Darin in http://trac.webkit.org/changeset/52268 based on code written by Johnny, in case any of them have opinions on how to best re-implement this.

[Adam Barth]

Comment on attachment 64452 [details]
Patch

This patch is pretty lame.  We should have someone who knows what this code is for comment on whether this is the right approach.

[Eric Seidel (no email)]

I'm in no rush anywhere.  Mostly posting this for the gardner, since the chromium build is broken for anyone who does a clean build.

[Darin Fisher (:fishd, Google)]

This looks like it is going to break chromium's page-save-as code.  Can the breaking changes made to WebCore be reverted until this is sorted out properly?

The whole point of this code living in WebKit was to allow for both changes to be made _together_.

[Adam Barth]

> This looks like it is going to break chromium's page-save-as code.  Can the breaking changes made to WebCore be reverted until this is sorted out properly?

I'd rather not.  This change required surgery to all the build systems and updates to a bunch of test expectations.

> The whole point of this code living in WebKit was to allow for both changes to be made _together_.

It seems better to port the code to the new entity representation, as I suggested in https://bugs.webkit.org/show_bug.cgi?id=43949#c35

The code will still be wrong, but at least it will compile.

[Adam Barth]

Created attachment 64457 [details]
Patch

[WebKit Review Bot]

Attachment 64457 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebEntities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Created attachment 64458 [details]
Patch

[WebKit Review Bot]

Attachment 64458 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebEntities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

btw, I think this patch is wrong.  I probably prints out an extra ";" after each entity.

[Eric Seidel (no email)]

I think I might take a stab at moving the lokup down to HTMLEnitySearch.h/cpp.  Then this WebEntity code can have its own blacklist.  That ends up being a function call per character... which is a bad idea, but no worse than any of the rest of this. :)

[Eric Seidel (no email)]

Comment on attachment 64458 [details]
Patch

It likely does add an extra ';'.

I'm tired of fighting this code.  James says this is mostly vestigial.

[WebKit Commit Bot]

Comment on attachment 64458 [details]
Patch

Clearing flags on attachment: 64458

Committed r65388: <http://trac.webkit.org/changeset/65388>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yuta Kitamura]

(In reply to comment #15)
> (From update of attachment 64458 [details])
> Clearing flags on attachment: 64458
> 
> Committed r65388: <http://trac.webkit.org/changeset/65388>

r65388 broke Chromium's test_shell_tests (DomSerializerTests.* will timeout). It loops infinitely inside while loop of populateMapFromHTMLEntityTable function (it seems "entry" variable does not change inside the loop).

Fix seems easy, so I'll try to fix this...

[Yuta Kitamura]

Created attachment 64484 [details]
Fix for test_shell_tests

[Shinichiro Hamaji]

Comment on attachment 64484 [details]
Fix for test_shell_tests

rs=me

[Shinichiro Hamaji]

Committed r65411: <http://trac.webkit.org/changeset/65411>