Jose Manuel Cantera Reporter
	Description • 11 years ago

Since landing the thumbnail bug the photos in the call log appear distorted. see attachment. This should be blocking as it is a clear regression.

	Etienne Segonzac (:etienne)
	Comment 2 • 11 years ago

We should match what we do in the contacts app, use a div with a background image and background-size cover.
It will also prevent some reflows and help APZC.

	David Scravaglieri [:scravag]
	Comment 3 • 11 years ago

we wouldn't block 1.3 for this bug but we must fixe it in 1.4

	Jose Manuel Cantera Reporter
	Comment 4 • 11 years ago

I'm proposing a simple patch for the FB imported contacts, making the thumbnail square as we do with the whole photo. This fixes the problem and I strongly believe this should be in 1.3. as we are regressing even from 1.1. 

For v14 the call log owners can implement what Etienne is suggesting.

	Jose Manuel Cantera Reporter
	Comment 5 • 11 years ago

reducing the scope of the bug and renominating

	Etienne Segonzac (:etienne)
	Comment 7 • 11 years ago

So basically all the time spent discussing on bug 967272 was for nothing?

We don't need to square images in the database, we just need to display them properly, we have the CSS feature for this, this is what we do in the building block. There is absolutely no reason to apply the building block strategy everywhere.

	Etienne Segonzac (:etienne)
	Comment 8 • 11 years ago

(In reply to Etienne Segonzac (:etienne) from comment #7)
> There is absolutely no reason to apply the building block
> strategy everywhere.

*not* to apply

	Etienne Segonzac (:etienne)
	Comment 9 • 11 years ago

(In reply to Jose M. Cantera from comment #6)
> Created attachment 8375430 [details]
> 16245.html

Also why aren't you asking a Contacts app peer for the review?

	Cristian Rodriguez (:crdlc)
	Comment 10 • 11 years ago

LGTM, there are some comments, but minor changes

	Jose Manuel Cantera Reporter
	Comment 11 • 11 years ago

(In reply to Etienne Segonzac (:etienne) from comment #7)
> So basically all the time spent discussing on bug 967272 was for nothing?
> 
> We don't need to square images in the database, we just need to display them
> properly, we have the CSS feature for this, this is what we do in the
> building block. There is absolutely no reason to apply the building block
> strategy everywhere.

I don't disagree and it is what should be done for v1.4, but for the moment I consider less risky to do what I'm doing than changing call log, and all the apps that might have the same problem. 

On the other hand I'm totally disappointed that patches like thumbnails break work that was done years ago and on the other hand (with my end-user hat on) make the phone appear as a low quality thing only for the sake of saving a few milisecs.

	Jose Manuel Cantera Reporter
	Comment 12 • 11 years ago

(In reply to Etienne Segonzac (:etienne) from comment #9)
> (In reply to Jose M. Cantera from comment #6)
> > Created attachment 8375430 [details]
> > 16245.html
> 
> Also why aren't you asking a Contacts app peer for the review?

Cristian is the usual reviewer of the FB integration thing and is more familiar than contacts peer in that code. Also he developed the square image algorithm.

	Francisco Jordano [:arcturus] [:francisco]
	Comment 13 • 11 years ago

Hi folks,

I though we did agree in bug 967272 about not to square images:

https://bugzilla.mozilla.org/show_bug.cgi?id=967272#c47

@Jose, is there any technical reason we are missing to need to square the FB images?

	Jose Manuel Cantera Reporter
	Comment 14 • 11 years ago

(In reply to Francisco Jordano [:arcturus] from comment #13)
> Hi folks,
> 
> I though we did agree in bug 967272 about not to square images:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=967272#c47
> 
> @Jose, is there any technical reason we are missing to need to square the FB
> images?

if you read the bug comments you can see them.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 15 • 11 years ago

I made a small patch that change that makes a minimal change to the call log logic.

Jose does it solve the issue for you ?

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 16 • 11 years ago

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #15)
> Created attachment 8375502 [details] [diff] [review]
> bug972245.calllog.patch
> 
> I made a small patch that change that makes a minimal change to the call log
> logic.
> 
> Jose does it solve the issue for you ?

Thinking about it we should probably revoke the blob too once it is loaded (which is independent of any of the 2 solutions).

	Etienne Segonzac (:etienne)
	Comment 17 • 11 years ago

Comment on attachment 8375502 [details] [diff] [review]
bug972245.calllog.patch

Review of attachment 8375502 [details] [diff] [review]:
-----------------------------------------------------------------

Vivien's patch is shorter, it fixes the issue for all the contacts (not just facebook), and has better performances (when rendering the call log and when importing contacts since we save ourselves a costly canvas operation).

I think we should take this one.

	Jose Manuel Cantera Reporter
	Comment 18 • 11 years ago

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #15)
> Created attachment 8375502 [details] [diff] [review]
> bug972245.calllog.patch
> 
> I made a small patch that change that makes a minimal change to the call log
> logic.
> 
> Jose does it solve the issue for you ?

In the call log yes, but we have the same issue in the SMS app (see attached screenshot). That's why I was proposing my patch, but if we can fix it in the SMS app as well, I would be happy. 

many thanks

	Jose Manuel Cantera Reporter
	Comment 20 • 11 years ago

(In reply to Etienne Segonzac (:etienne) from comment #17)
> Comment on attachment 8375502 [details] [diff] [review]
> bug972245.calllog.patch
> 
> Review of attachment 8375502 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Vivien's patch is shorter, it fixes the issue for all the contacts (not just
> facebook), and has better performances (when rendering the call log and when
> importing contacts since we save ourselves a costly canvas operation).
> 
> I think we should take this one.

I agree, provided we can fix also the issue in the SMS app

	Jose Manuel Cantera Reporter
	Comment 21 • 11 years ago

Comment on attachment 8375502 [details] [diff] [review]
bug972245.calllog.patch

Review of attachment 8375502 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/communications/dialer/js/call_log.js
@@ +378,4 @@
>    //      <span></span>
>    //    </label>
>    //    <aside class="pack-end">
> +  //      <span data-type="img" src="" class="call-log-contact-photo">

nit: src is no longer valid

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 22 • 11 years ago

Sorry Etienne the last patch was not really intended to be reviewed. This one is a bit longer but it remove an extra variable but also revoke blobs urls once they have been used. I'm pretty sure Tarako will love this extra attention.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 23 • 11 years ago

More generally and this is a bit out of the scope of this bug but a better cropping algorithm, even if the current is smart in the sense that it relies on the fact that most people center the person on the picture and exploit that.

There is some code around to detect faces on a picture using JavaScript. It was not very fast on a desktop machine but that was 3 and a half years ago. I wonder if we can revive this code, use 'asm.js' and a worker, as well as fixing the contacts API in order to have some metadata to the photo fields. That will let us take a normal thumbnail and center it using background position using the metadata returns by this algorithm.

Jose let me know if you are interested and I can probably do some archeology in some of my old laptops to find it.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 24 • 11 years ago

Moving the component to Gaia to reflect the fact that it touch a few more things that just the call log.

	Jose Manuel Cantera Reporter
	Comment 25 • 11 years ago

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #23)
> More generally and this is a bit out of the scope of this bug but a better
> cropping algorithm, even if the current is smart in the sense that it relies
> on the fact that most people center the person on the picture and exploit
> that.
> 
> There is some code around to detect faces on a picture using JavaScript. It
> was not very fast on a desktop machine but that was 3 and a half years ago.
> I wonder if we can revive this code, use 'asm.js' and a worker, as well as
> fixing the contacts API in order to have some metadata to the photo fields.
> That will let us take a normal thumbnail and center it using background
> position using the metadata returns by this algorithm.
> 
> Jose let me know if you are interested and I can probably do some archeology
> in some of my old laptops to find it.

Oh yes, that can be exploited for the future visual refresh for Contacts in which photos will put in a circle

	Preeti Raghunath(:Preeti)
	Comment 26 • 11 years ago

blocking on this for 1.3

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 27 • 11 years ago

Julien I'm not sure I cover all the code paths you showned me earlier. Asking f? in case you got a chance to look at that before I see you at the office tomorrow.

	Julien Wajsberg [:julienw]
	Comment 28 • 11 years ago

Yeah I won't look at this today, too tired, sorry, and tomorrow I'm in PTO. So you should ask Steve who can probably look at this while you'll be sleeping.

For Steve, I think the code paths that handle contact pictures are in (possibly) ThreadUI, ContactRenderer, ThreadListUI, Utils, (possibly) GroupView. I think this is more certainly ContactRenderer, ThreadListUI and Utils.

	Julien Wajsberg [:julienw]
	Comment 29 • 11 years ago

Comment on attachment 8375789 [details] [diff] [review]
bug972245.messages.patch

Forwarding the feedback request to Steve.

	Julien Wajsberg [:julienw]
	Comment 30 • 11 years ago

For context, this regressed with bug 967272.

	Etienne Segonzac (:etienne)
	Comment 31 • 11 years ago

Comment on attachment 8375534 [details] [diff] [review]
bug972245.calllog.patch

Review of attachment 8375534 [details] [diff] [review]:
-----------------------------------------------------------------

See comments, probably need a second round.

::: apps/communications/dialer/js/call_log.js
@@ +963,5 @@
> +      // Revoke the blob once it's ready.
> +      var image = new Image();
> +      image.src = url;
> +      image.onload = image.onerror = function() {
> +        URL.revokeObjectURL(url);

from my on-device testing I'm not sure this is working...
(don't see the picture, but if I remove the revoke it works)

@@ +968,5 @@
> +      };
> +    }
> +  },
> +
> +  unloadBackgrounImage: function cl_unloadBackgroundImage(element) {

I think you're missing a 'd'.
The tests would have told you so :)

	Steve Chung [:steveck] Assignee
	Comment 32 • 11 years ago

Comment on attachment 8375789 [details] [diff] [review]
bug972245.messages.patch

Review of attachment 8375789 [details] [diff] [review]:
-----------------------------------------------------------------

There is one img style need to be applied for new structure: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/style/sms.css#L1006.
and here https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/style/sms.css#L193

That's what I found currently, I will spend more time if there exist any image styling need to change or remove.

::: apps/sms/js/contact_renderer.js
@@ +246,4 @@
>        target.appendChild(element);
>  
>        // Revoke contact photo after image onload.
> +      var photo = element.querySelector('span[data-type=img"]');

A quotation mark is missed here.

	Steve Chung [:steveck] Assignee
	Comment 33 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #28)
> For Steve, I think the code paths that handle contact pictures are in
> (possibly) ThreadUI, ContactRenderer, ThreadListUI, Utils, (possibly)
> GroupView. I think this is more certainly ContactRenderer, ThreadListUI and
> Utils.

I could only found some image related issue in ThreadListUI/ContactRenderer, are you sure Utils also has the image issue(we only use image for resizing only)?

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 34 • 11 years ago

(In reply to Etienne Segonzac (:etienne) from comment #31)
> Comment on attachment 8375534 [details] [diff] [review]
> bug972245.calllog.patch
> 
> Review of attachment 8375534 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See comments, probably need a second round.
> 
> ::: apps/communications/dialer/js/call_log.js
> @@ +963,5 @@
> > +      // Revoke the blob once it's ready.
> > +      var image = new Image();
> > +      image.src = url;
> > +      image.onload = image.onerror = function() {
> > +        URL.revokeObjectURL(url);
> 
> from my on-device testing I'm not sure this is working...
> (don't see the picture, but if I remove the revoke it works)
> 

Works for me but I believe it can be racy since there is simple way to revoke a blob for a background image. I guess I can be dirty and use a setTimeout :/

> @@ +968,5 @@
> > +      };
> > +    }
> > +  },
> > +
> > +  unloadBackgrounImage: function cl_unloadBackgroundImage(element) {
> 
> I think you're missing a 'd'.
> The tests would have told you so :)

I changed the name at the last minute :)

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 35 • 11 years ago

(In reply to Steve Chung [:steveck] from comment #32)
> Comment on attachment 8375789 [details] [diff] [review]
> bug972245.messages.patch
> 
> Review of attachment 8375789 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There is one img style need to be applied for new structure:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/style/sms.css#L1006.
> and here
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/style/sms.css#L193
> 
> That's what I found currently, I will spend more time if there exist any
> image styling need to change or remove.
> 

Thanks for the references. That's what I was looking for.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 36 • 11 years ago

And Travis is green!

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 37 • 11 years ago

Here is the messages patch I sent to Travis.

	Etienne Segonzac (:etienne)
	Comment 38 • 11 years ago

Comment on attachment 8376286 [details] [diff] [review]
bug972245.calllog.patch

Review of attachment 8376286 [details] [diff] [review]:
-----------------------------------------------------------------

All good :)

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 39 • 11 years ago

(In reply to Etienne Segonzac (:etienne) from comment #38)
> Comment on attachment 8376286 [details] [diff] [review]
> bug972245.calllog.patch
> 
> Review of attachment 8376286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All good :)

Call log part landed.

https://github.com/mozilla-b2g/gaia/commit/a921e33b583e87402fec2b369ecca737b880a90b

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 40 • 11 years ago

This is still red on Travis. As it is also unlikely that I can get a review and advices before tomorrow morning, where I will be off for one week, someone else would have to finish this patch on the sms side.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 41 • 11 years ago

Unassigning myself as someone will likely have to finish the sms related part of this patch while I will be in some cold part of the world without any laptop.

	Steve Chung [:steveck] Assignee
	Comment 42 • 11 years ago

take the message part.

	Steve Chung [:steveck] Assignee
	Comment 43 • 11 years ago

Hi Julien, this patch is based on Vivien's patch with addition style and test fixing. It seems fine on current build, but we still need your eagle eyes to verify this patch is safe on master/v1.3. Thanks!

	Julien Wajsberg [:julienw]
	Comment 44 • 11 years ago

Comment on attachment 8376973 [details]
Link to github

r=me with a nit to extract the common code in a function.

makes me quite sad though, will try to find a better solution with platform team.

	Steve Chung [:steveck] Assignee
	Comment 45 • 11 years ago

Suggestion applied and landed in master: 7b37543888457b6cdf8cb8dc766bcd6fd3ba383f

	Steve Chung [:steveck] Assignee
	Comment 46 • 11 years ago

Comment on attachment 8376973 [details]
Link to github

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):bug 972245
[User impact] if declined: Contacts thumbnails will be distorted
[Testing completed]:manual
[Risk to taking this patch] (and alternatives if risky):low risk ,no alternatives
[String changes made]: none

	Julien Wajsberg [:julienw]
	Comment 47 • 11 years ago

Steve, I think you changed the v1.3 status to "wontfix" by mistake.

	Steve Chung [:steveck] Assignee
	Comment 48 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #47)
> Steve, I think you changed the v1.3 status to "wontfix" by mistake.

Oh! thanks for the reminder

	Julien Wajsberg [:julienw]
	Comment 49 • 11 years ago

Comment on attachment 8376973 [details]
Link to github

Removing the approval request because the patch has a bug. Gonna revert, steal, and ask review again.

	Julien Wajsberg [:julienw]
	Comment 50 • 11 years ago

Revert on master: a5b3110d25510a952417ba98a502250f374ef210

Thanks to Tim Pillard [1] :)
[1] https://twitter.com/tpillard/status/435755817220849664

	Julien Wajsberg [:julienw]
	Comment 52 • 11 years ago

Steve, please land yourself and request approval if this looks fine.

	Steve Chung [:steveck] Assignee
	Comment 53 • 11 years ago

Comment on attachment 8377557 [details]
github PR

Thanks for discovering the bug, let's wait for tree reopen.

	Steve Chung [:steveck] Assignee
	Comment 54 • 11 years ago

Comment on attachment 8377557 [details]
github PR

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):bug 972245
[User impact] if declined: Contacts thumbnails will be distorted
[Testing completed]:manual
[Risk to taking this patch] (and alternatives if risky):low risk ,no alternatives
[String changes made]: none

	bhavana bajaj [:bajaj]
	Comment 55 • 11 years ago

Adding verifyme for QA to help test once this lands on master.

	Preeti Raghunath(:Preeti)
	Comment 56 • 11 years ago

Steve,

This will need Fabrice's approval to land on 1.3

	Julien Wajsberg [:julienw]
	Comment 57 • 11 years ago

approval has been already asked to Fabrice.

I'm landing on master
master: b9a5859551ffbe17884de3343f82fc3d98233ec8

	Phil Ringnalda (:philor)
	Comment 58 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/b4e3bf7c62b5

	Salem Elkabule
	Comment 59 • 11 years ago

This issue is fixed on buri v 1.4 Mozilla RIL

Environmental Variables
Device: Buri 1.4 MOZ RIL
Build ID: 20140224040201
Gecko: https://hg.mozilla.org/mozilla-central/rev/31113754db3b
Gaia: ffb527b84594396ed611edf0a8a5a130d60a742f
Platform Version: 30.0a1
Firmware Version: V1.2-device.cfg

The Images display correctly in the Call log and the Message application.

	John Ford [:jhford] CET/CEST Berlin Time
	Comment 60 • 11 years ago

I was not able to uplift this bug to v1.3.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.3, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.3
  git cherry-pick -x -m1 a921e33b583e87402fec2b369ecca737b880a90b
  <RESOLVE MERGE CONFLICTS>
  git commit

	Steve Chung [:steveck] Assignee
	Comment 61 • 11 years ago

call log part land in v1.3 : 6e883bde818d4d53aef7b5b075b4b34267918360

	Steve Chung [:steveck] Assignee
	Comment 62 • 11 years ago

Please note we also need to uplift b9a5859551ffbe17884de3343f82fc3d98233ec8(message) for closing this bug, thanks!

	Ryan VanderMeulen [:RyanVM]
	Comment 63 • 11 years ago

v1.3: 3da675a68d2e4592e0df4a6f470b658f808f7058

	Mike Livingston
	Comment 64 • 11 years ago

Photos appear correctly without distortion in 1.3 and 1.4

1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140525024003
Gaia: 0ce948e378cab7ed3db20231281dd7ca2eb99779
Gecko: 94e367cf6c94
Version: 28.0
Firmware Version: v1.2-device.cfg

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140527000202
Gaia: 0542778892a294d224e75af4a76be5d42938bc90
Gecko: d583ae109f54
Version: 30.0
Firmware Version: v1.2-device.cfg

	Montoya Clemmons (MClemmons)
	Comment 65 • 10 years ago

Photos appear non-distorted on latest 1.3Tarako build.
1.3T Environmental Variables:
Device: Tarako 1.3T
BuildID: 20140530014002
Gaia: e68858693b71d917c9c5ee7e215f7ceea04635f7
Gecko: 1945abae19ff
Version: 28.1
Firmware Version: sp6821a-gonk-4.0-5-12
User Agent: Mozilla/5.0 (Mobile; rv:28.1) Gecko/28.1 Firefox/28.1