Closed Bug 891908 Opened 7 years ago Closed 7 years ago

[MMS] Replacing an attachment with a large image won't trigger resizing

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect, P1)

defect

Tracking

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

RESOLVED FIXED
1.1 QE4 (15jul)
blocking-b2g leo+
Tracking Status
b2g18 --- fixed
b2g-v1.1hd --- fixed

People

(Reporter: gnarf, Assigned: gnarf)

Details

(Whiteboard: [leo-triage], [LeoVB+] )

Attachments

(1 file, 1 obsolete file)

STR:
  * Attach any image
  * Tap on image, choose replace, pick a very large image

Expected:
  Image is resized

Actual:
  Image is not resized

-------

I consider this a release blocker
Flags: needinfo?(dietrich)
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #774133 - Flags: review?(felash)
large as in file size or dimensions?

do the images shot on the camera trigger this?

if not from the phone, where would these large images come from?

please re-nom if it's easy to hit.
blocking-b2g: leo? → -
Flags: needinfo?(dietrich)
blocking-b2g: - → leo?
Priority: -- → P1
Whiteboard: [leo-triage]
Target Milestone: --- → 1.1 QE5
Comment on attachment 774133 [details] [diff] [review]
patch v1

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

small things here and there

::: apps/sms/js/compose.js
@@ +38,5 @@
>      type: 'sms'
>    };
>  
>    // handler for 'input' in contentEditable
> +  function composeCheck(duck) {

I'd like to rename this function then. What about "onContentAdded(duck)" ?

also, I think we should separate what comes as an keyboard event and what is directly called in 2 functions. Sooner or later we'll parse also video and not only images, therefore it's probably cleaner to just separate this in 2 functions now.

@@ +40,5 @@
>  
>    // handler for 'input' in contentEditable
> +  function composeCheck(duck) {
> +
> +    if (duck instanceof Attachment && duck.type === 'img') {

// does it quack like a duck ?

@@ +68,5 @@
>        compose.disable(true);
>        state.empty = true;
>      }
>  
> +    trigger('input', {type: 'input'});

can't we create a real Event ?

@@ +382,5 @@
>          // insert element at the end of the Compose area
>          dom.message.insertBefore(fragment, dom.message.lastChild);
>          this.scrollToTarget(dom.message.lastChild);
>        }
> +      composeCheck(item);

you need to call this in prepend too.
Attachment #774133 - Flags: review?(felash)
(In reply to Dietrich Ayala (:dietrich) from comment #2)
> large as in file size or dimensions?

as in file size

> 
> do the images shot on the camera trigger this?

not on unagi but I suppose it's device dependent.
unless the camera app already resizes the picture.

> 
> if not from the phone, where would these large images come from?

email, web, wherever :)

> 
> please re-nom if it's easy to hit.

it's especially frustrating for a user when it happens, for after all a small patch.
Yes, images shot from the camera can trigger this.  a >150k image is all you need - attach three of the same image, and replace one, it will not be properly resized.
Attached patch patch v2Splinter Review
Updates from review
Attachment #774133 - Attachment is obsolete: true
Attachment #774764 - Flags: review?(felash)
Target Milestone: 1.1 QE5 → 1.1 QE4 (15jul)
Comment on attachment 774764 [details] [diff] [review]
patch v2

Redirecting the final review to Steve
Attachment #774764 - Flags: review?(felash) → review?(schung)
Leo would like to have a discussion in Triage to mark it as blocker. As per comment number 5 it is easily reproducible.
Discussed with Mozilla. Comment 5 clears the STR. Marking Leo+. - Veeresh
blocking-b2g: leo? → leo+
Comment on attachment 774764 [details] [diff] [review]
patch v2

The latest patch looks good to me without lint/unit test error. r=me even the duck doesn't quack :)
Attachment #774764 - Flags: review?(schung) → review+
Whiteboard: [leo-triage] → [leo-triage], [LeoVB+]
v1.1.0hd: c945c4f8ce2a3e50b58b571a188b7c75d24890e6
You need to log in before you can comment on or make changes to this bug.