Bug 44036 - Port Chromium's WebEntities to HTMLEntityTable
Summary: Port Chromium's WebEntities to HTMLEntityTable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 43949
  Show dependency treegraph
 
Reported: 2010-08-15 12:59 PDT by Eric Seidel (no email)
Modified: 2010-08-16 04:57 PDT (History)
10 users (show)

See Also:


Attachments
Patch (6.70 KB, patch)
2010-08-15 13:07 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (4.78 KB, patch)
2010-08-15 15:30 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (4.79 KB, patch)
2010-08-15 15:55 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Fix for test_shell_tests (1.32 KB, patch)
2010-08-16 04:51 PDT, Yuta Kitamura
hamaji: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-08-15 12:59:34 PDT
Remove Chromium's broken WebEntities code since it depends on removed WebCore files
Comment 1 Eric Seidel (no email) 2010-08-15 13:07:25 PDT
Created attachment 64452 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-08-15 13:08:33 PDT
CCing the current webkit gardner.
Comment 3 Mihai Parparita 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.
Comment 4 Adam Barth 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.
Comment 5 Eric Seidel (no email) 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.
Comment 6 Darin Fisher (:fishd, Google) 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_.
Comment 7 Adam Barth 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.
Comment 8 Adam Barth 2010-08-15 15:30:49 PDT
Created attachment 64457 [details]
Patch
Comment 9 WebKit Review Bot 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.
Comment 10 Adam Barth 2010-08-15 15:55:46 PDT
Created attachment 64458 [details]
Patch
Comment 11 WebKit Review Bot 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.
Comment 12 Adam Barth 2010-08-15 17:27:45 PDT
btw, I think this patch is wrong.  I probably prints out an extra ";" after each entity.
Comment 13 Eric Seidel (no email) 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. :)
Comment 14 Eric Seidel (no email) 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.
Comment 15 WebKit Commit Bot 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>
Comment 16 WebKit Commit Bot 2010-08-15 18:49:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Yuta Kitamura 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...
Comment 18 Yuta Kitamura 2010-08-16 04:51:53 PDT
Created attachment 64484 [details]
Fix for test_shell_tests
Comment 19 Shinichiro Hamaji 2010-08-16 04:55:30 PDT
Comment on attachment 64484 [details]
Fix for test_shell_tests

rs=me
Comment 20 Shinichiro Hamaji 2010-08-16 04:57:34 PDT
Committed r65411: <http://trac.webkit.org/changeset/65411>