Closed
Bug 1246244
Opened 8 years ago
Closed 8 years ago
Save Image from the long-press context menu is broken
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox47 verified, fennec47+)
RESOLVED
FIXED
Firefox 47
People
(Reporter: mconley, Assigned: mconley)
References
Details
Attachments
(2 files)
STR: 1) Browse to some site with an image 2) Long-press the image, choose the "Image" tab, and do "Save Image As" ER: We should get UI for saving the image somewhere. AR: E/GeckoConsole(25669): [JavaScript Error: "Error: saveImageURL couldn't compute private state of content window" {file: "chrome://global/content/contentAreaUtils.js" line: 159}] I broke this in bug 1243643, where I only accounted for the cases where saveImageURL was being handed a CPOW as a Doc, or being handed the aIsContentWindowPrivate bool. I failed to account for the case where we're being handed a Doc that's being rendered in the same process, which is the way things used to be. I suspect this breaks other Gecko apps that rely on ContentAreaUtils, like SeaMonkey and Thunderbird. The solution is to properly support the deprecated API. The long-term solution is for Fennec to pass in the aIsContentWindowPrivate bool.
Updated•8 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/33847/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/33847/
Attachment #8716505 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/33849/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/33849/
Attachment #8716506 -
Flags: review?(margaret.leibovic)
Comment 3•8 years ago
|
||
"Bug 1246466 - Browser: Context Menu 'Save image as ...' does nothing" shows the same error message "chrome://global/content/contentAreaUtils.js" -- Not only FF/Android affected? OS=All for now.
OS: Unspecified → All
See Also: → 1246466
Comment 4•8 years ago
|
||
Comment on attachment 8716505 [details] MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret,jaws https://reviewboard.mozilla.org/r/33847/#review30689 I don't feel confident reviewing contentAreaUtils.js changes, but I'll happily give this a rubber stamp if that's what you're looking for. Fun fact: this is one of our most used menu items. I wonder why I haven't heard anyone else report this bustage.
Attachment #8716505 -
Flags: review?(margaret.leibovic) → review+
Updated•8 years ago
|
Attachment #8716506 -
Flags: review?(margaret.leibovic) → review+
Comment 5•8 years ago
|
||
Comment on attachment 8716506 [details] MozReview Request: Bug 1246244 - Regression test. r=margaret,jaws https://reviewboard.mozilla.org/r/33849/#review30693
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to :Margaret Leibovic from comment #4) > Comment on attachment 8716505 [details] > MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through > saveImageURL properly. r?margaret > > https://reviewboard.mozilla.org/r/33847/#review30689 > > I don't feel confident reviewing contentAreaUtils.js changes, but I'll > happily give this a rubber stamp if that's what you're looking for. I'll take it! :D > > Fun fact: this is one of our most used menu items. I wonder why I haven't > heard anyone else report this bustage. I wouldn't have guessed that, but I guess that's what makes Telemetry fascinating!
Comment 7•8 years ago
|
||
(In reply to :Margaret Leibovic from comment #4) > Fun fact: this is one of our most used menu items. I wonder why I haven't > heard anyone else report this bustage. Probably most users just assume that the image was saved somewhere default :/
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8716505 [details] MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret,jaws Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33847/diff/1-2/
Attachment #8716505 -
Attachment description: MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r?margaret → MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8716506 [details] MozReview Request: Bug 1246244 - Regression test. r=margaret,jaws Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33849/diff/1-2/
Attachment #8716506 -
Attachment description: MozReview Request: Bug 1246244 - Regression test. r?margaret → MozReview Request: Bug 1246244 - Regression test. r=margaret
Assignee | ||
Updated•8 years ago
|
Attachment #8716505 -
Flags: review?(jld)
Assignee | ||
Updated•8 years ago
|
Attachment #8716506 -
Flags: review?(jld)
Comment 10•8 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #7) > (In reply to :Margaret Leibovic from comment #4) > > > Fun fact: this is one of our most used menu items. I wonder why I haven't > > heard anyone else report this bustage. > > Probably most users just assume that the image was saved somewhere default :/ I kept thinking it was gonna be fixed when I updated to the next Nightly and was slow to file a bug.
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10) > (In reply to Richard Newman [:rnewman] from comment #7) > > (In reply to :Margaret Leibovic from comment #4) > > > > > Fun fact: this is one of our most used menu items. I wonder why I haven't > > > heard anyone else report this bustage. > > > > Probably most users just assume that the image was saved somewhere default :/ > > I kept thinking it was gonna be fixed when I updated to the next Nightly and > was slow to file a bug. Cool if I redirect the review request to you? I think jld is busy.
Flags: needinfo?(jaws)
Comment 12•8 years ago
|
||
I've looked at the patches and nothing jumped out at me as bad, but I don't understand frontend-land very well so I wanted to spend some more time on it. I'm fine with handing off the r? if someone else has more time right now.
Updated•8 years ago
|
Assignee: nobody → mconley
tracking-fennec: ? → 47+
Assignee | ||
Updated•8 years ago
|
Attachment #8716505 -
Flags: review?(jld) → review?(jaws)
Assignee | ||
Updated•8 years ago
|
Attachment #8716506 -
Flags: review?(jld) → review?(jaws)
Updated•8 years ago
|
Attachment #8716505 -
Flags: review?(jaws) → review+
Comment 13•8 years ago
|
||
Comment on attachment 8716505 [details] MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret,jaws https://reviewboard.mozilla.org/r/33847/#review31253 ::: toolkit/content/contentAreaUtils.js (Diff revision 2) > - Deprecated.warning("saveImageURL should not be passed document CPOWs. " + > - "The caller should pass in the content type and " + > - "disposition themselves", > - "https://bugzilla.mozilla.org/show_bug.cgi?id=1243643"); > - if (aIsContentWindowPrivate == undefined) { Shouldn't we still keep the deprecated warning around for when saveImageURL is passed a CPOW, just outside of us computing the state of aIsContentWindowPrivate?
Updated•8 years ago
|
Attachment #8716506 -
Flags: review?(jaws) → review+
Comment 14•8 years ago
|
||
Comment on attachment 8716506 [details] MozReview Request: Bug 1246244 - Regression test. r=margaret,jaws https://reviewboard.mozilla.org/r/33849/#review31255
Assignee | ||
Comment 15•8 years ago
|
||
https://reviewboard.mozilla.org/r/33847/#review31253 > Shouldn't we still keep the deprecated warning around for when saveImageURL is passed a CPOW, just outside of us computing the state of aIsContentWindowPrivate? Yeah, that's a good call. I'll put that back, and just make the test temporarily flip the forbid CPOWs pref for now (since Cu.isCrossProcessWrapper will get called outside of the addon compartment).
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8716505 [details] MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret,jaws Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33847/diff/2-3/
Attachment #8716505 -
Attachment description: MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret → MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret,jaws
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8716506 [details] MozReview Request: Bug 1246244 - Regression test. r=margaret,jaws Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33849/diff/2-3/
Attachment #8716506 -
Attachment description: MozReview Request: Bug 1246244 - Regression test. r=margaret → MozReview Request: Bug 1246244 - Regression test. r=margaret,jaws
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/61eec2456b97 https://hg.mozilla.org/integration/mozilla-inbound/rev/24cd0b6089b2
Assignee | ||
Comment 19•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c13ff8c5a97a
Assignee | ||
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ecd788a20f1de92f075b3984406c7f556e1b473 Bug 1246244 - Follow-up - clean up MockFilePicker once test is done. r=me
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/61eec2456b97 https://hg.mozilla.org/mozilla-central/rev/24cd0b6089b2 https://hg.mozilla.org/mozilla-central/rev/7ecd788a20f1
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Updated•8 years ago
|
Flags: needinfo?(jaws)
Comment 22•8 years ago
|
||
Long press on the image, go to the "Image" tab and choose "Save Image As" -> A snackbar is displayed at the bottom of the page and a 'Download complete' notification is present in the android notification bar. Verified as fixed using: Device: One A2001 (Android 5.1.1) Build: Firefox for Android 47 Beta 1
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•