Load a placeholder when a url to an image is broken. #5366

Adenilson · 2015-03-25T22:34:36Z

I decided to use the old Netscape broken image link icon (later we may
replace the image asset for something more trendier).

Adenilson · 2015-03-25T22:34:51Z

See #5330 .

frewsxcv · 2015-03-25T22:45:14Z

components/net/image_cache_task.rs

+        placeholder_url.push("rippy.jpg");
+        let image = load_image_data(Url::from_file_path(&*placeholder_url).unwrap(), resource_task, &self.placeholder_data);
+
+        if image != Err(()) {


This should probably be a match so you don't have to manually unwrap below

hoppipolla-critic-bot · 2015-03-25T23:09:06Z

Critic review: https://critic.hoppipolla.co.uk/r/4372

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

Adenilson · 2015-03-26T22:10:07Z

Should I squash it now?

jdm · 2015-03-26T22:21:09Z

Yes, go ahead.

Adenilson · 2015-03-26T22:37:30Z

Going to rebase.

Adenilson · 2015-03-26T22:44:22Z

Done.


        auto merge of #5366 : Adenilson/servo/loadPlaceholder01, r=jdm

I decided to use the old Netscape broken image link icon (later we may replace the image asset for something more trendier).

Adenilson · 2015-03-27T00:23:21Z

Yep, it triggered regressions on net unit tests. Investigating it now.

Adenilson · 2015-03-27T02:26:58Z

I talked with @larsbergstrom and it seems some of the tests expect a request to fail, the issue being that this patch changed the behavior (i.e. we always return a valid image for invalid urls).

It will require to inspect each failing unit test to see if they still make sense given this change on behavior.

Adenilson · 2015-03-27T18:54:05Z

Ok, should be fine by now.


        auto merge of #5366 : Adenilson/servo/loadPlaceholder01, r=jdm

I decided to use the old Netscape broken image link icon (later we may replace the image asset for something more trendier).

jdm · 2015-03-28T19:04:20Z

src/lib.rs:103:17: 104:75 error: this function takes 4 parameters but 3 parameters were supplied [E0061]
src/lib.rs:103                 ImageCacheTask::new_sync(resource_task.clone(), shared_task_pool,
src/lib.rs:104                                          time_profiler_chan_clone.clone())
src/lib.rs:106:17: 107:70 error: this function takes 4 parameters but 3 parameters were supplied [E0061]
src/lib.rs:106                 ImageCacheTask::new(resource_task.clone(), shared_task_pool,
src/lib.rs:107                                     time_profiler_chan_clone.clone())
error: aborting due to 2 previous errors
Could not compile `b2s`.

Adenilson · 2015-03-28T19:58:23Z

Gonk should be ok now.

jdm · 2015-03-28T21:46:56Z

Squash these, please.

Adenilson · 2015-03-28T23:40:25Z

Squashed.


        auto merge of #5366 : Adenilson/servo/loadPlaceholder01, r=jdm

I decided to use the old Netscape broken image link icon (later we may replace the image asset for something more trendier).

Ms2ger · 2015-03-30T10:15:45Z

@Adenilson: and now a rebase, please


        Load a placeholder when a url to an image is broken.

I decided to use the old Netscape broken image link icon (later we may replace the image asset for something more trendier). The ref test will expect that a failed load should display the rippy image. ImageCacheTask users can define if a placeholder image should be loaded at start up or not. This enables both the new behavior (e.g. always return an image even for broken urls) as also the previous one.

Adenilson · 2015-03-30T17:18:21Z

Rebased.

jdm · 2015-03-30T18:04:32Z

r+

bors-servo · 2015-03-30T18:40:10Z

saw approval from jdm
at Adenilson@cdebb3c

bors-servo · 2015-03-30T18:40:11Z

merging Adenilson/servo/loadPlaceholder01 = cdebb3c into auto

bors-servo · 2015-03-30T18:40:12Z

Adenilson/servo/loadPlaceholder01 = cdebb3c merged ok, testing candidate = 1e282d5

bors-servo · 2015-03-30T19:34:04Z

all tests pass:
success: http://build.servo.org/builders/linux1/builds/1218
success: http://build.servo.org/builders/linux2/builds/1216
success: http://build.servo.org/builders/android/builds/1252
success: http://build.servo.org/builders/mac1/builds/1431
success: http://build.servo.org/builders/mac2/builds/1434
success: http://build.servo.org/builders/gonk/builds/398

bors-servo · 2015-03-30T19:37:03Z

fast-forwarding master to auto = 1e282d5


        auto merge of #5366 : Adenilson/servo/loadPlaceholder01, r=jdm

I decided to use the old Netscape broken image link icon (later we may replace the image asset for something more trendier).


        fix possible bugs

The previous version would have ignored a message like: `I'm talking about servo/servo#5366 here` Also could have ignored some issues.

jdm added the S-awaiting-review label Mar 25, 2015

jdm closed this Mar 25, 2015

jdm reopened this Mar 25, 2015

jdm closed this Mar 25, 2015

jdm reopened this Mar 25, 2015

frewsxcv reviewed Mar 25, 2015
View changes

pcwalton force-pushed the servo:master branch from 7f587f6 to b1a35f5 Mar 25, 2015

jgraham closed this Mar 25, 2015

jgraham reopened this Mar 25, 2015

Adenilson force-pushed the Adenilson:loadPlaceholder01 branch from 6b34e88 to cabfa0b Mar 26, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Mar 26, 2015

jdm added S-needs-squash S-needs-rebase and removed S-needs-code-changes labels Mar 26, 2015

Adenilson force-pushed the Adenilson:loadPlaceholder01 branch from e1e72bf to cbdabcc Mar 26, 2015

Adenilson force-pushed the Adenilson:loadPlaceholder01 branch from cbdabcc to 534591c Mar 26, 2015

jdm added S-awaiting-merge and removed S-needs-rebase S-needs-squash labels Mar 26, 2015

jdm added S-tests-failed and removed S-awaiting-merge labels Mar 27, 2015

jdm added S-awaiting-merge and removed S-tests-failed labels Mar 27, 2015

jdm added S-needs-code-changes and removed S-awaiting-merge labels Mar 28, 2015

jdm added S-needs-squash and removed S-needs-code-changes labels Mar 28, 2015

Adenilson force-pushed the Adenilson:loadPlaceholder01 branch from e7afefb to 41e0203 Mar 28, 2015

Adenilson force-pushed the Adenilson:loadPlaceholder01 branch from afc26fb to ae1d40f Mar 29, 2015

Ms2ger added S-needs-rebase and removed S-needs-squash labels Mar 30, 2015

Adenilson force-pushed the Adenilson:loadPlaceholder01 branch from ae1d40f to cdebb3c Mar 30, 2015

bors-servo closed this Mar 30, 2015

bors-servo merged commit cdebb3c into servo:master Mar 30, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

default all tests passed

Adenilson mentioned this pull request Mar 30, 2015

Draw a placeholder when loading an image fails #5330

Closed

mseri added a commit to mseri/crowbot that referenced this pull request Feb 27, 2016

fix possible bugs

0eacf72

The previous version would have ignored a message like: `I'm talking about servo/servo#5366 here` Also could have ignored some issues.

servo / servo

Load a placeholder when a url to an image is broken. #5366

Load a placeholder when a url to an image is broken. #5366

Adenilson commented Mar 25, 2015

Adenilson commented Mar 25, 2015

This comment has been minimized.

hoppipolla-critic-bot commented Mar 25, 2015

Adenilson commented Mar 26, 2015

jdm commented Mar 26, 2015

Adenilson commented Mar 26, 2015

Adenilson commented Mar 26, 2015

Adenilson commented Mar 27, 2015

Adenilson commented Mar 27, 2015

Adenilson commented Mar 27, 2015

jdm commented Mar 28, 2015

Adenilson commented Mar 28, 2015

jdm commented Mar 28, 2015

Adenilson commented Mar 28, 2015

Ms2ger commented Mar 30, 2015

Adenilson commented Mar 30, 2015

This comment has been minimized.

jdm commented on `cdebb3c` Mar 30, 2015

This comment has been minimized.

bors-servo commented on `cdebb3c` Mar 30, 2015

This comment has been minimized.

bors-servo replied Mar 30, 2015

This comment has been minimized.

bors-servo replied Mar 30, 2015

This comment has been minimized.

bors-servo replied Mar 30, 2015

This comment has been minimized.

bors-servo replied Mar 30, 2015

servo / servo

Join GitHub today

Load a placeholder when a url to an image is broken. #5366

Load a placeholder when a url to an image is broken. #5366

Conversation

Adenilson commented Mar 25, 2015

Adenilson commented Mar 25, 2015

This comment has been minimized.

frewsxcv Mar 25, 2015

hoppipolla-critic-bot commented Mar 25, 2015

Adenilson commented Mar 26, 2015

jdm commented Mar 26, 2015

Adenilson commented Mar 26, 2015

Adenilson commented Mar 26, 2015

Adenilson commented Mar 27, 2015

Adenilson commented Mar 27, 2015

Adenilson commented Mar 27, 2015

jdm commented Mar 28, 2015

Adenilson commented Mar 28, 2015

jdm commented Mar 28, 2015

Adenilson commented Mar 28, 2015

Ms2ger commented Mar 30, 2015

Adenilson commented Mar 30, 2015

This comment has been minimized.

jdm commented on cdebb3c Mar 30, 2015

This comment has been minimized.

bors-servo commented on cdebb3c Mar 30, 2015

This comment has been minimized.

bors-servo replied Mar 30, 2015

This comment has been minimized.

bors-servo replied Mar 30, 2015

This comment has been minimized.

bors-servo replied Mar 30, 2015

This comment has been minimized.

bors-servo replied Mar 30, 2015

jdm commented on `cdebb3c` Mar 30, 2015

bors-servo commented on `cdebb3c` Mar 30, 2015