RESOLVED FIXED 44036
Port Chromium's WebEntities to HTMLEntityTable
https://bugs.webkit.org/show_bug.cgi?id=44036
Summary Port Chromium's WebEntities to HTMLEntityTable
Eric Seidel (no email)
Reported 2010-08-15 12:59:34 PDT
Remove Chromium's broken WebEntities code since it depends on removed WebCore files
Attachments
Patch (6.70 KB, patch)
2010-08-15 13:07 PDT, Eric Seidel (no email)
no flags
Patch (4.78 KB, patch)
2010-08-15 15:30 PDT, Adam Barth
no flags
Patch (4.79 KB, patch)
2010-08-15 15:55 PDT, Adam Barth
no flags
Fix for test_shell_tests (1.32 KB, patch)
2010-08-16 04:51 PDT, Yuta Kitamura
hamaji: review+
Eric Seidel (no email)
Comment 1 2010-08-15 13:07:25 PDT
Eric Seidel (no email)
Comment 2 2010-08-15 13:08:33 PDT
CCing the current webkit gardner.
Mihai Parparita
Comment 3 2010-08-15 13:10:09 PDT
+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 4 2010-08-15 13:18:08 PDT
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)
Comment 5 2010-08-15 13:37:24 PDT
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)
Comment 6 2010-08-15 14:34:01 PDT
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
Comment 7 2010-08-15 14:40:42 PDT
> 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
Comment 8 2010-08-15 15:30:49 PDT
WebKit Review Bot
Comment 9 2010-08-15 15:36:23 PDT
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
Comment 10 2010-08-15 15:55:46 PDT
WebKit Review Bot
Comment 11 2010-08-15 15:59:02 PDT
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
Comment 12 2010-08-15 17:27:45 PDT
btw, I think this patch is wrong. I probably prints out an extra ";" after each entity.
Eric Seidel (no email)
Comment 13 2010-08-15 17:43:17 PDT
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 14 2010-08-15 18:30:18 PDT
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 15 2010-08-15 18:49:18 PDT
Comment on attachment 64458 [details] Patch Clearing flags on attachment: 64458 Committed r65388: <http://trac.webkit.org/changeset/65388>
WebKit Commit Bot
Comment 16 2010-08-15 18:49:23 PDT
All reviewed patches have been landed. Closing bug.
Yuta Kitamura
Comment 17 2010-08-16 04:39:12 PDT
(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
Comment 18 2010-08-16 04:51:53 PDT
Created attachment 64484 [details] Fix for test_shell_tests
Shinichiro Hamaji
Comment 19 2010-08-16 04:55:30 PDT
Comment on attachment 64484 [details] Fix for test_shell_tests rs=me
Shinichiro Hamaji
Comment 20 2010-08-16 04:57:34 PDT
Note You need to log in before you can comment on or make changes to this bug.