Closed Bug 959066 Opened 7 years ago Closed 7 years ago

hi-res screenshot for getScreenshot API

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
1.4 S1 (14feb)
tracking-b2g backlog

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Attached patch WIP (untested) (obsolete) — Splinter Review
I will test my patch with real mochitests this time.
Gregor, do you need this for Haida sheet switching? It would be faster if I don't own this bug :)
Flags: needinfo?(anygregor)
(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]
How is this related to media team?
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)
(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.
blocking-b2g: --- → backlog
Attached patch Patch v1 (obsolete) — Splinter Review
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)
Attached patch Test for patch (obsolete) — Splinter Review
Attachment #8372033 - Flags: review?(bugs)
We should be looking at m-2 for Desktop, and Emulator m-5.
Attachment #8372032 - Flags: review?(bugs) → review+
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-
Attached patch Test for patch v2 (obsolete) — Splinter Review
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)
Ah, it was from other tests. Yes, might be worth to fix them... bug not in this bug.
(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 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+
Attachment #8372032 - Attachment is obsolete: true
Attachment #8374648 - Flags: review+
Attachment #8373791 - Attachment is obsolete: true
Attachment #8374649 - Flags: review+
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$
https://hg.mozilla.org/mozilla-central/rev/625e8b95a64e
https://hg.mozilla.org/mozilla-central/rev/fabcf0b6f48d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S1 (14feb)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.