Closed Bug 838042 Opened 7 years ago Closed 7 years ago

[Browser][User Story] Image save from webpage

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:leo+, b2g18 fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 --- fixed

People

(Reporter: pdol, Assigned: fabrice)

References

Details

(Keywords: feature, relnote, Whiteboard: relnote-b2g:1.1+)

Attachments

(1 file, 2 obsolete files)

UCID: Browser-112

User Story:
As a user, I want the ability to save an image from the webpage (currently supported MIME types) so that I can later access that image from my Gallery application.
Keywords: feature
Summary: [B2G][Browser][User Story] Image save from webpage → [Browser][User Story] Image save from webpage
Justin, can you comment on how we might implement this feature? Presumably we would need to extend the Browser API to make this possible?
I think we'd just have to add a bit of information to the contextmenu event.
Whiteboard: u=user c=browser s=v1.1-sprint-2
Assignee: nobody → bfrancis
We already set the image url on the contextmenu event from there:
https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChild.js#497
Whiteboard: u=user c=browser s=v1.1-sprint-2 → u=rmacdonald@mozilla.com c=browser s=v1.1-sprint-2
Whiteboard: u=rmacdonald@mozilla.com c=browser s=v1.1-sprint-2
Depends on: 844304
Attached patch patch (obsolete) — Splinter Review
That works fine in my tests. I did not add any UI feedback notifying the user about success or failure, because a desktop notification seems a bit too heavy, and I'm not sure what else we can use.
Assignee: bfrancis → fabrice
Attachment #720208 - Flags: review?(bfrancis)
Hopefully this flow doesn't come too late, but I've posted a draft to dropbox:

https://www.dropbox.com/s/bdgx8ofucuzc3vj/browser-saveimage-130225-DRAFT.pdf

This is subject to review by the browser UI folks who have been at MWC but I don't anticipate any changes as this is a fairly simple requirement.

Also, re Comment 4, there is a simple banner that appears for a few seconds as confirmation.

Let me know if you have any questions.
Attached patch patch v2 (obsolete) — Splinter Review
Now with a 3 seconds banner displaying error or success.
Attachment #720208 - Attachment is obsolete: true
Attachment #720208 - Flags: review?(bfrancis)
Attachment #721345 - Flags: review?(bfrancis)
Attachment #721619 - Flags: review?(bfrancis)
Attachment #721345 - Attachment is obsolete: true
Attachment #721345 - Flags: review?(bfrancis)
This is an awesome feature, thanks Fabrice! Now I can save my boarding pass to the gallery when I fly home :)

Just some cleanup work to do I think, see my comments on the pull request.

Maybe this could benefit from a security review as we're saving data from the world wild web onto device storage?

Do we need latel10n for 1.1 features?
Comment on attachment 721619 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8479

r+me with comments addressed (see comments in pull request).
Attachment #721619 - Flags: review?(bfrancis) → review+
Ben, I updated the pull request to address your comments and Kevin's. I also fixed the issue with saving twice an image with the same name. It's now prepending the file name with the current date ms and even allows retries.
This looks great now, thanks! It can't currently be applied cleanly, maybe while you merge with upstream you could rebase and squash into one commit before merging :)

Thanks Fabrice
Pushed on master:
https://github.com/mozilla-b2g/gaia/commit/1b1fb4a6637e6cd1254d419057bbe6875d52770c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
and https://github.com/mozilla-b2g/gaia/commit/cedc21afea33ba230b75debdb251ea4b5931ac96 because the linter doesn't like all the good things like double quotes!
I was not able to uplift this bug to v1-train.  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-train, 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-train
  git cherry-pick -x  1b1fb4a6637e6cd1254d419057bbe6875d52770c
  <RESOLVE MERGE CONFLICTS>
  git commit
  git checkout v1-train
  git cherry-pick -x  cedc21afea33ba230b75debdb251ea4b5931ac96
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(fabrice)
It has no dependencies that I know of.
Flags: needinfo?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #16)
> It has no dependencies that I know of.

in that case, could you please resolve the merge conflict?
pushed:
commit 777dc79736e081f24ebc413da5fc97ff2658407f

commit 1168b5f92e3262ea3c492105fa53a085bff6ce49
jford2:~/b2g/uplifting/gaia $ git branch --contains 777dc79736e081f24ebc413da5fc97ff2658407f
  v1-train
jford2:~/b2g/uplifting/gaia $ git branch --contains 1168b5f92e3262ea3c492105fa53a085bff6ce49
  v1-train

Thanks for the merge conflict resolutions!
I believe we've got test case coverage in MozTrap for this, right?
Flags: needinfo?(nhirata.bugzilla)
Flags: in-moztrap?
Flags: needinfo?(nhirata.bugzilla)
Flags: in-moztrap?(nhirata.bugzilla)
Flags: in-moztrap?
#6889
Flags: in-moztrap?(nhirata.bugzilla) → in-moztrap+
Duplicate of this bug: 831235
Keywords: relnote
Whiteboard: relnote-b2g:1.1+
You need to log in before you can comment on or make changes to this bug.