Closed
Bug 959066
Opened 10 years ago
Closed 10 years ago
hi-res screenshot for getScreenshot API
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(tracking-b2g:backlog)
People
(Reporter: timdream, Assigned: timdream)
References
Details
Attachments
(2 files, 4 obsolete files)
4.26 KB,
patch
|
timdream
:
review+
|
Details | Diff | Splinter Review |
7.22 KB,
patch
|
timdream
:
review+
|
Details | Diff | Splinter Review |
In bug 946596, Peter realized drawWindow() call we rely on could produce hi-res screenshot if we scale() the canvas correctly. I think we should do that for getScreenshot() too. I would need to first investigate Gaia to see if sending back a pixel-dimension image will break Gaia somewhere. Depend bug will be filed and patched for that. To amend getScreenshot(), I will change the math here: https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#716 I will start working on this bug only after bug 878003 lands to avoid merge conflict.
Assignee | ||
Comment 1•10 years ago
|
||
I will test my patch with real mochitests this time.
Assignee | ||
Comment 2•10 years ago
|
||
Gregor, do you need this for Haida sheet switching? It would be faster if I don't own this bug :)
Flags: needinfo?(anygregor)
Comment 3•10 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #2) > Gregor, do you need this for Haida sheet switching? It would be faster if I > don't own this bug :) I am not sure what the hd plans are for 1.4 but we can take it if needed. Or maybe David wants to put this in the backlog of the media team?
Flags: needinfo?(anygregor) → needinfo?(dflanagan)
Whiteboard: [systemsfe]
Assignee | ||
Comment 4•10 years ago
|
||
How is this related to media team?
Comment 5•10 years ago
|
||
Since this is a gecko think, I'd rather that one of the systems teams keeps ownership of it. We've got only a couple of gecko engineers and they're pretty tied up.
Flags: needinfo?(dflanagan)
Comment 6•10 years ago
|
||
(In reply to David Flanagan [:djf] from comment #5) > Since this is a gecko think, I'd rather that one of the systems teams keeps > ownership of it. We've got only a couple of gecko engineers and they're > pretty tied up. Ok we can take it.
Updated•10 years ago
|
blocking-b2g: --- → backlog
Assignee | ||
Comment 7•10 years ago
|
||
I don't think there is a use case where you don't need a screenshot with CSS pixel dimension, so I simply return a screenshot with device pixel dimension. The first two arguments of getScreenshot() is still in CSS pixels. Testcases to come.
Attachment #8359142 -
Attachment is obsolete: true
Attachment #8372032 -
Flags: review?(bugs)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8372033 -
Flags: review?(bugs)
Assignee | ||
Comment 9•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=854843f22aba
Assignee | ||
Comment 10•10 years ago
|
||
We should be looking at m-2 for Desktop, and Emulator m-5.
Updated•10 years ago
|
Attachment #8372032 -
Flags: review?(bugs) → review+
Comment 11•10 years ago
|
||
Comment on attachment 8372033 [details] [diff] [review] Test for patch >+ var iframe1 = document.createElement('iframe'); >+ iframe1.setAttribute('width', cssPixelWidth); >+ iframe1.setAttribute('height', cssPixelHeight); >+ SpecialPowers.wrap(iframe1).mozbrowser = true; >+ document.body.appendChild(iframe1); >+ iframe1.src = 'data:text/html,<html><body>hello</body></html>'; >+ function iframeLoadedHandler() { >+ numLoaded++; >+ if (numLoaded === 2) { >+ SpecialPowers.pushPrefEnv( >+ {'set': [['layout.css.devPixelsPerPx', 1]]}, takeScreenshot); >+ } >+ } >+ >+ iframe1.addEventListener('mozbrowserloadend', iframeLoadedHandler); >+} This is confusing. We wait for the second mozbrowserloadend before we do anything useful. Do we get first one for the about:blank? Could you just set .src of the iframe before appending to body? Or did I miss something? If so, ask review again. But that numLoaded == 2 needs some comment anyhow.
Attachment #8372033 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 12•10 years ago
|
||
You are right. Set the src *before* appending does get rid of the first mozbrowserloadend. I don't know why :jliber did that in other tests. We might need another bug to fix them all? try: https://tbpl.mozilla.org/?tree=Try&rev=1c02e56bdccd Thanks.
Attachment #8372033 -
Attachment is obsolete: true
Attachment #8373791 -
Flags: review?(bugs)
Comment 13•10 years ago
|
||
Ah, it was from other tests. Yes, might be worth to fix them... bug not in this bug.
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #13) > Ah, it was from other tests. Yes, might be worth to fix them... bug not in > this bug. Good first bug filed: bug 970777.
Comment 15•10 years ago
|
||
Comment on attachment 8373791 [details] [diff] [review] Test for patch v2 You don't seem to use var view = image.data; ever.
Attachment #8373791 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8372032 -
Attachment is obsolete: true
Attachment #8374648 -
Flags: review+
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8373791 -
Attachment is obsolete: true
Attachment #8374649 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed,
dev-doc-needed
Assignee | ||
Comment 18•10 years ago
|
||
Let's get this checked-in first and deal with bugs later in Gaia. It's should already be fixed but it's possible some of the layout will still be broken. timdream:~/repo/gaia master$ grep -rl getScreenshot apps apps/browser/js/browser.js apps/system/js/app_window.js apps/system/js/browser_mixin.js apps/system/js/cards_view.js apps/system/test/unit/app_window_test.js apps/system/test/unit/cards_view_test.js timdream:~/repo/gaia master$
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/625e8b95a64e https://hg.mozilla.org/integration/b2g-inbound/rev/fabcf0b6f48d
Flags: in-testsuite+
Keywords: checkin-needed
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/625e8b95a64e https://hg.mozilla.org/mozilla-central/rev/fabcf0b6f48d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S1 (14feb)
Assignee | ||
Comment 21•10 years ago
|
||
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.getScreenshot
Keywords: dev-doc-needed
Whiteboard: [systemsfe]
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•