Allow MultiResolutionImageFetcher to fetch image from file:/// protocol

Allow MultiResolutionImageFetcher to fetch image from file scheme URL

This fix is to also check the scheme of image url for fetching image from
file scheme since there is no http status code for file scheme URL.

BUG=246511
TEST=Follow the description of http://crbug.com/246511

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211568

[Hongbo Min, 2013-06-04 07:53:28]

Nico, could you please have a look at this CL? Thanks.

[Nico, 2013-06-04 18:01:20]

https://codereview.chromium.org/16358002/diff/3001/content/renderer/fetchers/...
File content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
(right):

https://codereview.chromium.org/16358002/diff/3001/content/renderer/fetchers/...
content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc:50: if
(http_status_code_ == 200 || (net::IsSupportedImageMimeType(mime_type) &&
This will now also let through a partially downloaded ico file over http, no?

[Hongbo Min, 2013-06-05 09:18:12]

On 2013/06/04 18:01:20, Nico wrote:

Is there any other way to know when icon is downloaded successfully except http
status code?

[Nico, 2013-06-05 15:30:30]

I don't know, but you could check for status == 200 or protocol == file://
maybe? How do other parts of the code that accept data from file:// urls
handle this?


On Wed, Jun 5, 2013 at 2:18 AM, <hongbo.min@intel.com> wrote:

> On 2013/06/04 18:01:20, Nico wrote:
>
> Is there any other way to know when icon is downloaded successfully except
> http
> status code?
>
>
https://codereview.chromium.**org/16358002/<https://codereview.chromium.org/1...
>

[Hongbo Min, 2013-06-07 09:54:49]

Updated with checking image url scheme instead. Pls review it. Thanks.

[Nico, 2013-06-07 16:17:18]

Thanks, I think this is better.

Did you find other code that does things like this?

Is it possible to write a test for this? If not, can you add a TEST= line to the
cl description? Also, can you change the BUG= line to 246511?

[Hongbo Min, 2013-06-08 02:52:10]

On 2013/06/07 16:17:18, Nico wrote:
> Thanks, I think this is better.
> 
> Did you find other code that does things like this?
> 
> Is it possible to write a test for this? If not, can you add a TEST= line to
the
> cl description? Also, can you change the BUG= line to 246511?

I think writing such a test may touch lots of files, e.g. we might need to add
hook methods to get notifications when favicon is completed to download. Since
it is a trivial change, so at this point, I prefer to test by manual.

[Hongbo Min, 2013-06-08 12:22:07]

On 2013/06/08 02:52:10, Hongbo Min wrote:
> On 2013/06/07 16:17:18, Nico wrote:
> > Thanks, I think this is better.
> > 
> > Did you find other code that does things like this?
> > 

Not exactly but similar, see
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...

It determines the title by checking the URL scheme.

[Hongbo Min, 2013-06-12 02:23:26]

ping Nico@

[Nico, 2013-06-13 16:39:26]

lgtm, I think this is fine. Please change the TEST= line to say how to test this
("Click the frobolator, pull nuzzle, verify that maganication beam appears and
is purple"); saying "TEST=Manual" is always wrong.

+abarth to double-check that loading favicons from file:// urls is ok from a
security perspective.

[abarth-chromium, 2013-06-13 20:07:39]

Yeah, if you've gotten this far already, no harm in going further.  :)

https://codereview.chromium.org/16358002/diff/9001/content/renderer/fetchers/...
File content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
(right):

https://codereview.chromium.org/16358002/diff/9001/content/renderer/fetchers/...
content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc:47: GURL
url = static_cast<GURL>(response.url());
No need for a static_cast here.

[commit-bot: I haz the power, 2013-06-14 05:19:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/16358002/9001

[Hongbo Min, 2013-06-14 05:19:50]

On 2013/06/13 16:39:26, Nico wrote:
> lgtm, I think this is fine. Please change the TEST= line to say how to test
this
> ("Click the frobolator, pull nuzzle, verify that maganication beam appears and
> is purple"); saying "TEST=Manual" is always wrong.
> 
> +abarth to double-check that loading favicons from file:// urls is ok from a
> security perspective.

Done! Thanks

[commit-bot: I haz the power, 2013-06-14 05:24:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/16358002/9001

[commit-bot: I haz the power, 2013-06-14 05:33:37]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Hongbo Min, 2013-07-01 05:25:56]

jamesr@, could you pls have a look at this CL? Thanks.

[Hongbo Min, 2013-07-01 05:39:33]

https://codereview.chromium.org/16358002/diff/9001/content/renderer/fetchers/...
File content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
(right):

https://codereview.chromium.org/16358002/diff/9001/content/renderer/fetchers/...
content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc:47: GURL
url = static_cast<GURL>(response.url());
On 2013/06/13 20:07:39, abarth wrote:
> No need for a static_cast here.

Since I don't want to rely on the implicit type conversion between WebURL and
GURL.

[abarth-chromium, 2013-07-01 05:53:34]

https://codereview.chromium.org/16358002/diff/9001/content/renderer/fetchers/...
File content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
(right):

https://codereview.chromium.org/16358002/diff/9001/content/renderer/fetchers/...
content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc:47: GURL
url = static_cast<GURL>(response.url());
On 2013/07/01 05:39:33, Hongbo Min wrote:
> On 2013/06/13 20:07:39, abarth wrote:
> > No need for a static_cast here.
> 
> Since I don't want to rely on the implicit type conversion between WebURL and
> GURL.

Why not?  WebURL is designed to implicitly convert to GURL.  Please remove the
static_cast.

[Hongbo Min, 2013-07-04 08:49:35]

Please have a review again. Thanks.

https://codereview.chromium.org/16358002/diff/9001/content/renderer/fetchers/...
File content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
(right):

https://codereview.chromium.org/16358002/diff/9001/content/renderer/fetchers/...
content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc:47: GURL
url = static_cast<GURL>(response.url());
On 2013/07/01 05:53:34, abarth wrote:
> On 2013/07/01 05:39:33, Hongbo Min wrote:
> > On 2013/06/13 20:07:39, abarth wrote:
> > > No need for a static_cast here.
> > 
> > Since I don't want to rely on the implicit type conversion between WebURL
and
> > GURL.
> 
> Why not?  WebURL is designed to implicitly convert to GURL.  Please remove the
> static_cast.

Done.

[abarth-chromium, 2013-07-08 18:47:45]

LGTM

[commit-bot: I haz the power, 2013-07-09 01:13:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/16358002/32001

[commit-bot: I haz the power, 2013-07-09 01:29:07]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Hongbo Min, 2013-07-11 01:00:53]

ping Nico@

[Nico, 2013-07-11 01:13:33]

On 2013/07/11 01:00:53, Hongbo Min wrote:
> ping Nico@

I lgtm'd this 4 weeks ago already.

[Nico, 2013-07-11 01:14:13]

On 2013/07/11 01:13:33, Nico wrote:
> On 2013/07/11 01:00:53, Hongbo Min wrote:
> > ping Nico@
> 
> I lgtm'd this 4 weeks ago already.

Missing LGTM from an OWNER for these files:
    content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc

I'm probably not in content/renderer/fetchers/OWNERS (or
content/renderer/OWNERS).

jamesr: can you stamp?

[Hongbo Min, 2013-07-11 01:40:23]

On 2013/07/11 01:14:13, Nico wrote:
> On 2013/07/11 01:13:33, Nico wrote:
> > On 2013/07/11 01:00:53, Hongbo Min wrote:
> > > ping Nico@
> > 
> > I lgtm'd this 4 weeks ago already.
> 
> Missing LGTM from an OWNER for these files:
>     content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
> 
> I'm probably not in content/renderer/fetchers/OWNERS (or
> content/renderer/OWNERS).
> 
> jamesr: can you stamp?

[Hongbo Min, 2013-07-11 01:40:45]

On 2013/07/11 01:13:33, Nico wrote:
> On 2013/07/11 01:00:53, Hongbo Min wrote:
> > ping Nico@
> 
> I lgtm'd this 4 weeks ago already.

Nice. Sorry for my duplicated ping ;)

[jamesr, 2013-07-13 01:40:07]

lgtm

https://codereview.chromium.org/16358002/diff/32001/content/renderer/fetchers...
File content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
(right):

https://codereview.chromium.org/16358002/diff/32001/content/renderer/fetchers...
content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc:48: if
(http_status_code_ == 200 || url.SchemeIsFile()) {
why the extra temp variable?

[commit-bot: I haz the power, 2013-07-13 10:10:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/16358002/32001

[commit-bot: I haz the power, 2013-07-13 15:04:21]

Change committed as 211568