[ReadingList iOS] Do not store HTTP images when distilling HTTPS pages

[ReadingList iOS] Do not store HTTP images when distilling HTTPS pages

Distilled page must not present mixed content.
This CL skips the mixed content images when distilling a page with DOM
distiller.
The img tag is left empty.
A placeholder may be put later to replace this empty image.

BUG=682325
Review-Url: https://codereview.chromium.org/2644073005
Cr-Commit-Position: refs/heads/master@{#445074}
Committed: https://chromium.googlesource.com/chromium/src/+/1410a29d40e4809519e02ffabf3edf20dee1511c

[Olivier, 2017-01-20 09:10:17]

olivierrobin@chromium.org changed reviewers:
+ gambard@chromium.org, jif@chromium.org

[jif, 2017-01-20 10:25:07]

Description was changed from

==========
[ReadingList iOS] Do not store HTTP images when distilling HTTPS pages

Distilled page must not present mixed content.
This CL skipes the mixed content images when distilling a page with DOM
distiller.
The img tag is left empty.
A placeholder may be put later to replace this empty image.

BUG=682325
==========

to

==========
[ReadingList iOS] Do not store HTTP images when distilling HTTPS pages

Distilled page must not present mixed content.
This CL skips the mixed content images when distilling a page with DOM
distiller.
The img tag is left empty.
A placeholder may be put later to replace this empty image.

BUG=682325
==========

[jif, 2017-01-20 10:45:26]

https://codereview.chromium.org/2644073005/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/url_downloader.cc (right):

https://codereview.chromium.org/2644073005/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:251: std::string
local_image_name = "";
afaik you don't need to initialize local_image_name

https://codereview.chromium.org/2644073005/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:252: if
(!(distilled_url_.SchemeIsCryptographic() &&
Because it confused me a bit, can you rewrite it as:

 <new line>
// Only save images if it is not mixed content.
// Mixed content is HTTP images on HTTPS pages.
bool image_is_mixed_content = distilled_url_.SchemeIsCryptographic() &&
!images[i].url.SchemeIsCryptographic();
if (!image_is_mixed_content) {

https://codereview.chromium.org/2644073005/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:255: // Mixed content is HTTP
imaged on HTTPS pages.
s/imaged/images/

I appreciate the reminder of what is "mixed content".

https://codereview.chromium.org/2644073005/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:257: return std::string();
Optional nit (because not introduced by your CL, and not related to this CL's
change):

Returning an empty updated html seems wrong; it sounds as though the distilled
page will be blank if it contains mixed content images.
Can you add an explanation, maybe in the method's comment in the header?

[Olivier, 2017-01-20 10:54:44]

Thanks

https://codereview.chromium.org/2644073005/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/url_downloader.cc (right):

https://codereview.chromium.org/2644073005/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:251: std::string
local_image_name = "";
On 2017/01/20 10:45:26, jif wrote:
> afaik you don't need to initialize local_image_name

Done.

https://codereview.chromium.org/2644073005/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:252: if
(!(distilled_url_.SchemeIsCryptographic() &&
On 2017/01/20 10:45:26, jif wrote:
> Because it confused me a bit, can you rewrite it as:
> 
>  <new line>
> // Only save images if it is not mixed content.
> // Mixed content is HTTP images on HTTPS pages.
> bool image_is_mixed_content = distilled_url_.SchemeIsCryptographic() &&
> !images[i].url.SchemeIsCryptographic();
> if (!image_is_mixed_content) {

Done.

https://codereview.chromium.org/2644073005/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:255: // Mixed content is HTTP
imaged on HTTPS pages.
On 2017/01/20 10:45:26, jif wrote:
> s/imaged/images/
> 
> I appreciate the reminder of what is "mixed content".

Done.

https://codereview.chromium.org/2644073005/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:257: return std::string();
On 2017/01/20 10:45:26, jif wrote:
> Optional nit (because not introduced by your CL, and not related to this CL's
> change):
> 
> Returning an empty updated html seems wrong; it sounds as though the distilled
> page will be blank if it contains mixed content images.
> Can you add an explanation, maybe in the method's comment in the header?

Returning the html empty will result in a distillation failure, which is an
expected behavior.
Adding a comment in header.

[gambard, 2017-01-20 14:17:17]

lgtm if mardini agrees with the empty images.

[jif, 2017-01-20 15:04:22]

lgtm

[Olivier, 2017-01-20 16:31:49]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2017-01-20 16:32:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 16:40:39]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484929909049540,
"parent_rev": "8a8f9410c5341f0d338e0b99451672fd448db188", "commit_rev":
"1410a29d40e4809519e02ffabf3edf20dee1511c"}

[commit-bot: I haz the power, 2017-01-20 16:41:08]

Description was changed from

==========
[ReadingList iOS] Do not store HTTP images when distilling HTTPS pages

Distilled page must not present mixed content.
This CL skips the mixed content images when distilling a page with DOM
distiller.
The img tag is left empty.
A placeholder may be put later to replace this empty image.

BUG=682325
==========

to

==========
[ReadingList iOS] Do not store HTTP images when distilling HTTPS pages

Distilled page must not present mixed content.
This CL skips the mixed content images when distilling a page with DOM
distiller.
The img tag is left empty.
A placeholder may be put later to replace this empty image.

BUG=682325

Review-Url: https://codereview.chromium.org/2644073005
Cr-Commit-Position: refs/heads/master@{#445074}
Committed:
https://chromium.googlesource.com/chromium/src/+/1410a29d40e4809519e02ffabf3e...
==========

[commit-bot: I haz the power, 2017-01-20 16:41:09]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/1410a29d40e4809519e02ffabf3e...