Re: [xwiki-devs] [xwiki-notifications] r16937 - in platform/core/trunk/xwiki-xml/src/main: java/org/xwiki/xml/html java/org/xwiki/xml/html/filter java/org/xwiki/xml/internal/html java/org/xwiki/xml/internal/html/filter resources/META-INF/plexus
19 Feb
2009
19 Feb
'09
2:38 p.m.
Hi Asiri,
see below
On Feb 19, 2009, at 1:21 PM, asiri (SVN) wrote:
Author: asiri
Date: 2009-02-19 13:21:06 +0100 (Thu, 19 Feb 2009)
New Revision: 16937
Added:
platform/core/trunk/xwiki-xml/src/main/java/org/xwiki/xml/html/
HTMLConstants.java
platform/core/trunk/xwiki-xml/src/main/java/org/xwiki/xml/html/
filter/AbstractHTMLFilter.java
platform/core/trunk/xwiki-xml/src/main/java/org/xwiki/xml/html/
filter/ElementFilterCriterion.java
platform/core/trunk/xwiki-xml/src/main/java/org/xwiki/xml/html/
filter/HTMLFilter.java
platform/core/trunk/xwiki-xml/src/main/java/org/xwiki/xml/internal/
html/filter/FontFilter.java
platform/core/trunk/xwiki-xml/src/main/java/org/xwiki/xml/internal/
html/filter/ListFilter.java
Removed:
platform/core/trunk/xwiki-xml/src/main/java/org/xwiki/xml/html/
filter/CleaningFilter.java
platform/core/trunk/xwiki-xml/src/main/java/org/xwiki/xml/internal/
html/HTMLConstants.java
platform/core/trunk/xwiki-xml/src/main/java/org/xwiki/xml/internal/
html/filter/DocTypeCleaningFilter.java
platform/core/trunk/xwiki-xml/src/main/java/org/xwiki/xml/internal/
html/filter/ListCleaningFilter.java
Modified:
platform/core/trunk/xwiki-xml/src/main/java/org/xwiki/xml/internal/
html/DefaultHTMLCleaner.java
platform/core/trunk/xwiki-xml/src/main/resources/META-INF/plexus/
components.xml
Log:
XWIKI-3240: 'style' attribute of <font> tag discarded when cleaning
html
* Fixed the issue.
* Changed all cleaning filters to use w3c dom document instead of
jdom.
Copied: platform/core/trunk/xwiki-xml/src/main/java/org/xwiki/xml/
html/HTMLConstants.java (from rev 16930, platform/core/trunk/xwiki-
xml/src/main/java/org/xwiki/xml/internal/html/HTMLConstants.java)
===================================================================
--- platform/core/trunk/xwiki-xml/src/main/java/org/xwiki/xml/html/
HTMLConstants.java (rev 0)
+++ platform/core/trunk/xwiki-xml/src/main/java/org/xwiki/xml/html/
HTMLConstants.java 2009-02-19 12:21:06 UTC (rev 16937)
@@ -0,0 +1,190 @@
+/*
+ * See the NOTICE file distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ *
+ */
+package org.xwiki.xml.html;
+
+/**
+ * HTML Constants used by the HTML Cleaner.
+ *
+ * @version $Id: $
+ * @since 1.6M2
+ */
+public interface HTMLConstants
Can we use an Enum type instead?
I'm not sure why you changed CleaningFilter to HTMLFilter. They are
HTML filters for sure but not any type of HTML filters they are HTML
Cleaning Filters. It's not very important but I had chosen
CleaningFilter name voluntarily so I wanted to know what's your
rationale for changing the name.
Why have you removed the doc type cleaning filter and instead
"hardcoded" it in the default cleaner implementation?
Note: Haven't checked the criterion stuff yet, so I'm not sure what
this is about.
Question: have you been careful about performance? HTML cleaning is
the most costly thing in our rendering system so we have to be very
careful. Have you measured the rendering test speed before and after
as an indication?
Thanks
-Vincent
[snip]
20 Feb
20 Feb
4:26 a.m.
New subject: [xwiki-devs] [xwiki-notifications] r16937 - in platform/core/trunk/xwiki-xml/src/main: java/org/xwiki/xml/html java/org/xwiki/xml/html/filter java/org/xwiki/xml/internal/html java/org/xwiki/xml/internal/html/filter resources/META-INF/plexus
Hi Vincent,
Can we use an Enum type instead?
Yep, sounds good.
I'm not sure why you changed CleaningFilter to HTMLFilter. They are
HTML filters for sure but not any type of HTML filters
they are HTML
Cleaning Filters. It's not very important but I had chosen
CleaningFilter name voluntarily so I wanted to know what's your
rationale for changing the name.
Now I think about it, CleaningFilter sounds better. I was used to HTMLFilter
in officeimporter and wanted keep the same name so that I can change
officeimporter to use core-xml classes instead of it's own types. I will
change officeimporter also to use CleaningFilters from xwiki-xml.
Why have you removed the doc type cleaning filter and instead
"hardcoded" it in the default cleaner
implementation?
This was an unforseen tradgedy that happened to me. I decided to switch to
w3c dom from jdom for several reasons:
1. HTMLCleaner returns a w3c DOM and we convert TagNode (htmlcleaner) into
JDOM only to apply the cleaning filters and then we convert it back to w3c
DOM. Now, If we can base our CleaningFilters on w3c DOM api, there is no
need to convert to jdom and we can directly work with w3c DOM eliminating
the overhead of converting between types.
2. DOM Api is easy to use compared to JDOM api (from what i've been through)
3. OpenOfficeHTMLCleaner already uses w3c DOM filters (it has to, because
HTMLCleaner returns a w3c DOM)
However because of this problem :
http://sourceforge.net/forum/forum.php?thread_id=3021756&forum_id=637245 +
the fact that w3c DOM does not allow DocType to be set after the Document
has been created (jdom allows this), made me hard code the DocType into the
DefaultHTMLCleaner and we still have to do the conversion to jdom just to
set the DocType.
Anyway, I believe
http://sourceforge.net/forum/forum.php?thread_id=3021756&forum_id=637245 is
a bug in htmlcleaner (waiting for a reply) and when it get's fixed (if it
get's fixed) we can remove the dependency on jdom.
Note: Haven't checked the criterion stuff yet, so I'm not sure what
this is about.
Question: have you been careful about performance? HTML cleaning is
the most costly thing in our rendering system so we have to be very
careful. Have you measured the rendering test speed before and after
as an indication?
After you mentioned it, I did some testing and following are the results:
Brefore:
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.248 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.15 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.259 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.511 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.788 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.61 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.614 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.586 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.625 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.235 sec
Average: 5.4626
After:
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.39 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.674 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.264 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.465 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.423 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.367 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.232 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.34 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.408 sec
Tests run: 762, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.437 sec
Average: 5.40
The results seem to be pretty consistent with the earlier ones. I Don't
think the slight reduction in average test execution times means much(?)
But I believe that once we manage to get rid of the conversion to jdom, the
performance would increase. I'm going to try this out with a custom method
to directly convert into w3c DOM and see hwo much we can gain from that.
Thanks.
- Asiri
8:41 a.m.
New subject: [xwiki-devs] [xwiki-notifications] r16937 - in platform/core/trunk/xwiki-xml/src/main: java/org/xwiki/xml/html java/org/xwiki/xml/html/filter java/org/xwiki/xml/internal/html java/org/xwiki/xml/internal/html/filter resources/META-INF/plexus
On Fri, Feb 20, 2009 at 8:56 AM, Asiri Rathnayake <
asiri.rathnayake(a)gmail.com> wrote:
Hi Vincent,
Can we use an Enum type instead?
Yep, sounds good.
On second thoughts, I don't see a benefit of using an Enum type because we
cannot substitute an Enum type where a String is required. For an example,
we would have something like below:
<code>
public enum HTMLTag
{
B("b"), STRONG("strong");
private final String name;
HTMLTag(String name)
{
this.name = name;
}
public String toString()
{
return this.name;
}
}
</code>
And then where HTML tag names are used, we will have to use:
<code>
TagTransformation tt = new TagTransformation(HTMLTag.B.toString(),
HTMLTag.STRONG.toString(), false);
</code>
But I think the following (what we have now) is much more readable:
<code>
TagTransformation tt = new TagTransformation(HTMLConstants.B,
HTMLConstants.STRONG, false);
</code>
WDYT?
- Asiri
5687
days inactive
5688
days old
2 comments
2 participants
participants (2)
-
Asiri Rathnayake -
Vincent Massol