Support <picture> in image extraction

Support <picture> in image extraction

BUG=671691
R=mdjones@chromium.org

Committed: 4540f3524df13b97108a0cae312a0c7e6245204f

[wychen, 2017-01-16 19:06:19]

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

[wychen, 2017-01-16 19:06:19]

PTAL

[mdjones, 2017-01-17 17:41:47]

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
File java/org/chromium/distiller/extractors/embeds/ImageExtractor.java (right):

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
java/org/chromium/distiller/extractors/embeds/ImageExtractor.java:54: } else if
("PICTURE".equals(e.getTagName())) {
How would you feel about having some sort of utility function in this class
called "getImageElement"? That way we could eliminate the duplicate
"img"/"picture" tag checks here and below. It's not that hard to read now, but
adding another tag (maybe div) would further complicate this. wdyt?

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
java/org/chromium/distiller/extractors/embeds/ImageExtractor.java:55: return new
WebImage(e, width, height, imgSrc);
nit: indented too far.

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
java/org/chromium/distiller/extractors/embeds/ImageExtractor.java:61: if (img !=
null) {
nit: early return instead of nesting?

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
File java/org/chromium/distiller/webdocument/WebImage.java (right):

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
java/org/chromium/distiller/webdocument/WebImage.java:111: list.add(srcUrl);
nit: this can be in-lined, but it doesn't look like we are doing one-line ifs in
this code. I'll leave that up to you.

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
File java/org/chromium/distiller/webdocument/WebTable.java (right):

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
java/org/chromium/distiller/webdocument/WebTable.java:57: if
(!ie.getSrc().isEmpty()) {
nit: same in-line comment as above.

https://codereview.chromium.org/2638823002/diff/1/javatests/org/chromium/dist...
File javatests/org/chromium/distiller/webdocument/WebImageTest.java (right):

https://codereview.chromium.org/2638823002/diff/1/javatests/org/chromium/dist...
javatests/org/chromium/distiller/webdocument/WebImageTest.java:45:
//assertEquals("", wi.generateOutput(false));
Remove commented line.

[wychen, 2017-01-18 17:29:55]

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
File java/org/chromium/distiller/extractors/embeds/ImageExtractor.java (right):

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
java/org/chromium/distiller/extractors/embeds/ImageExtractor.java:54: } else if
("PICTURE".equals(e.getTagName())) {
On 2017/01/17 17:41:46, mdjones wrote:
> How would you feel about having some sort of utility function in this class
> called "getImageElement"? That way we could eliminate the duplicate
> "img"/"picture" tag checks here and below. It's not that hard to read now, but
> adding another tag (maybe div) would further complicate this. wdyt? 

Done.

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
java/org/chromium/distiller/extractors/embeds/ImageExtractor.java:55: return new
WebImage(e, width, height, imgSrc);
On 2017/01/17 17:41:47, mdjones wrote:
> nit: indented too far.

Done.

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
java/org/chromium/distiller/extractors/embeds/ImageExtractor.java:61: if (img !=
null) {
On 2017/01/17 17:41:46, mdjones wrote:
> nit: early return instead of nesting?

Done.

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
File java/org/chromium/distiller/webdocument/WebImage.java (right):

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
java/org/chromium/distiller/webdocument/WebImage.java:111: list.add(srcUrl);
On 2017/01/17 17:41:47, mdjones wrote:
> nit: this can be in-lined, but it doesn't look like we are doing one-line ifs
in
> this code. I'll leave that up to you. 

Acknowledged.

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
File java/org/chromium/distiller/webdocument/WebTable.java (right):

https://codereview.chromium.org/2638823002/diff/1/java/org/chromium/distiller...
java/org/chromium/distiller/webdocument/WebTable.java:57: if
(!ie.getSrc().isEmpty()) {
On 2017/01/17 17:41:47, mdjones wrote:
> nit: same in-line comment as above.

Acknowledged.

https://codereview.chromium.org/2638823002/diff/1/javatests/org/chromium/dist...
File javatests/org/chromium/distiller/webdocument/WebImageTest.java (right):

https://codereview.chromium.org/2638823002/diff/1/javatests/org/chromium/dist...
javatests/org/chromium/distiller/webdocument/WebImageTest.java:45:
//assertEquals("", wi.generateOutput(false));
On 2017/01/17 17:41:47, mdjones wrote:
> Remove commented line.

Done.

[mdjones, 2017-01-18 19:51:36]

lgtm

[wychen, 2017-01-19 00:49:50]

Description was changed from

==========
Support <picture> in image extraction

BUG=671691
==========

to

==========
Support <picture> in image extraction

BUG=671691
R=mdjones@chromium.org

Committed: 4540f3524df13b97108a0cae312a0c7e6245204f
==========

[wychen, 2017-01-19 00:49:51]

Committed patchset #3 (id:40001) manually as
4540f3524df13b97108a0cae312a0c7e6245204f (presubmit successful).