Clean up attributes of image elements

Clean up attributes of image elements

Only retain some white-listed attributes of image elements.
Also, do not add an empty "srcset" attribute.

BUG=567955
R=mdjones@chromium.org

Committed: 79912086f9340c8c57f46fe4203019b8f2a3f0e2

[wychen, 2015-12-09 05:15:18]

Patchset #1 (id:1) has been deleted

[wychen, 2015-12-09 05:17:39]

wychen@chromium.org changed reviewers:
+ mdjones@chromium.org

[wychen, 2015-12-09 05:17:40]

PTAL

https://codereview.chromium.org/1507373003/diff/20001/javatests/org/chromium/...
File javatests/org/chromium/distiller/ContentExtractorTest.java (right):

https://codereview.chromium.org/1507373003/diff/20001/javatests/org/chromium/...
javatests/org/chromium/distiller/ContentExtractorTest.java:92: // Also test
images in WebImage, WebText, and WebTable.
TODO(wychen): how do I get images inside a WebText?

[mdjones, 2015-12-09 17:40:16]

https://codereview.chromium.org/1507373003/diff/20001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/WebImage.java (right):

https://codereview.chromium.org/1507373003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/WebImage.java:48:
ie.setSrc(ie.getSrc());
Instead of removing all the attributes manually, wouldn't it be easier to simply
create a new image element and set the attributes we actually want?  I believe
this is just src and srcset. This would replace cloning the element.

https://codereview.chromium.org/1507373003/diff/20001/javatests/org/chromium/...
File javatests/org/chromium/distiller/ContentExtractorTest.java (right):

https://codereview.chromium.org/1507373003/diff/20001/javatests/org/chromium/...
javatests/org/chromium/distiller/ContentExtractorTest.java:92: // Also test
images in WebImage, WebText, and WebTable.
On 2015/12/09 05:17:40, wychen wrote:
> TODO(wychen): how do I get images inside a WebText?

This is an unsolved problem. Since tables are currently treated as a singular
element, it can be argued that we simply ignore images contained in them, or at
least leave them unaltered. If we decide to start traversing into tables, we
would have to make a number of changes to our extraction logic and possibly eval
data. A mix of both is complicated; we can discuss this more offline.

[wychen, 2015-12-09 22:27:54]

https://codereview.chromium.org/1507373003/diff/20001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/WebImage.java (right):

https://codereview.chromium.org/1507373003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/WebImage.java:48:
ie.setSrc(ie.getSrc());
On 2015/12/09 17:40:16, mdjones wrote:
> Instead of removing all the attributes manually, wouldn't it be easier to
simply
> create a new image element and set the attributes we actually want?  I believe
> this is just src and srcset. This would replace cloning the element.

Done.

[mdjones, 2015-12-10 01:12:46]

lgtm

https://codereview.chromium.org/1507373003/diff/40001/java/org/chromium/disti...
File java/org/chromium/distiller/DomUtil.java (right):

https://codereview.chromium.org/1507373003/diff/40001/java/org/chromium/disti...
java/org/chromium/distiller/DomUtil.java:219: stripImageElements(clonedSubtree);
It could be argued that images as part of a table should retain their
formatting, but since it looks like we are removing other table attributes, this
is fine.

[wychen, 2015-12-10 05:37:16]

Patchset #3 (id:60001) has been deleted

[wychen, 2015-12-10 05:39:17]

Keep alt as well. I wonder what else is needed. I'm on the fence of black and
white list now.

https://codereview.chromium.org/1507373003/diff/40001/java/org/chromium/disti...
File java/org/chromium/distiller/DomUtil.java (right):

https://codereview.chromium.org/1507373003/diff/40001/java/org/chromium/disti...
java/org/chromium/distiller/DomUtil.java:219: stripImageElements(clonedSubtree);
On 2015/12/10 01:12:45, mdjones wrote:
> It could be argued that images as part of a table should retain their
> formatting, but since it looks like we are removing other table attributes,
this
> is fine.

Acknowledged.

https://codereview.chromium.org/1507373003/diff/80001/java/org/chromium/disti...
File java/org/chromium/distiller/DomUtil.java (right):

https://codereview.chromium.org/1507373003/diff/80001/java/org/chromium/disti...
java/org/chromium/distiller/DomUtil.java:304:
imgElement.removeAttribute("style");
Mysterious! Without this line, the tests would fail. This shouldn't be needed.

[mdjones, 2015-12-10 17:05:22]

I'm still in favor of using a white list at this point. There are a very large
number of attributes that we don't want (possibly unlimited with data-*) and
only a few that we do.

https://codereview.chromium.org/1507373003/diff/80001/java/org/chromium/disti...
File java/org/chromium/distiller/DomUtil.java (right):

https://codereview.chromium.org/1507373003/diff/80001/java/org/chromium/disti...
java/org/chromium/distiller/DomUtil.java:299: if (name != "src" && name != "alt"
&& name != "srcset") {
Even though this gets converted to JavaScript, I'd prefer these be
"src".equals(name).

https://codereview.chromium.org/1507373003/diff/80001/java/org/chromium/disti...
java/org/chromium/distiller/DomUtil.java:304:
imgElement.removeAttribute("style");
On 2015/12/10 05:39:16, wychen wrote:
> Mysterious! Without this line, the tests would fail. This shouldn't be needed.

I think "style" is a special property for elements. e.style in javascript will
return the whole style for the element, not just what is in the attribute.

[wychen, 2015-12-16 01:00:17]

Description was changed from

==========
Clean up attributes of image elements

Strip "id", "style", and "align" attributes away from image elements. Also,
do not add an empty "srcset" attribute.

BUG=567955
==========

to

==========
Clean up attributes of image elements

Only retain some white-listed attributes of image element.
Also, do not add an empty "srcset" attribute.

BUG=567955
==========

[wychen, 2015-12-16 01:00:44]

Description was changed from

==========
Clean up attributes of image elements

Only retain some white-listed attributes of image element.
Also, do not add an empty "srcset" attribute.

BUG=567955
==========

to

==========
Clean up attributes of image elements

Only retain some white-listed attributes of image elements.
Also, do not add an empty "srcset" attribute.

BUG=567955
==========

[wychen, 2015-12-16 01:13:00]

Description was changed from

==========
Clean up attributes of image elements

Only retain some white-listed attributes of image elements.
Also, do not add an empty "srcset" attribute.

BUG=567955
==========

to

==========
Clean up attributes of image elements

Only retain some white-listed attributes of image elements.
Also, do not add an empty "srcset" attribute.

BUG=567955
R=mdjones@chromium.org

Committed: 79912086f9340c8c57f46fe4203019b8f2a3f0e2
==========

[wychen, 2015-12-16 01:13:01]

Committed patchset #5 (id:120001) manually as
79912086f9340c8c57f46fe4203019b8f2a3f0e2 (presubmit successful).