Closed
Bug 878003
Opened 11 years ago
Closed 11 years ago
Add mime type argument to getScreenshot API to control transparency of resulting screenshot
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
1.3 C2/1.4 S2(17jan)
People
(Reporter: alive, Assigned: timdream)
References
Details
Attachments
(3 files, 8 obsolete files)
(deleted),
patch
|
timdream
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
timdream
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
timdream
:
review+
|
Details | Diff | Splinter Review |
Currently the mozbrowser API: getScreenshot generate a JPG image thus it's not transparent.
But in some case(like homescreen) we may need a PNG version to avoid showing a gray background in the screenshot image.
Reporter | ||
Comment 1•11 years ago
|
||
WIP v1(not tested)
Reporter | ||
Comment 2•11 years ago
|
||
patch v1
Reporter | ||
Comment 3•11 years ago
|
||
Unfortunately the patch doesn't work.
It always gives me a black-background homescreen screenshot. Strange.
Reporter | ||
Comment 4•11 years ago
|
||
Deassign myself first.
Assignee: alive → nobody
blocking-b2g: --- → leo?
Comment 5•11 years ago
|
||
Triage - blocking as it blocks a leo+ blocker bug 880572. Whether or not 880572 is a real blocker should be discussed again with partner as it is an attempt to fix bug 871919 more correctly.
Updated•11 years ago
|
blocking-b2g: leo? → leo+
Updated•11 years ago
|
Blocks: browser-api
Assignee | ||
Comment 6•11 years ago
|
||
Gonna give it a try when I have the time to do so. Feel free to steal it.
Assignee: nobody → timdream
No longer blocks: browser-api
Updated•11 years ago
|
Blocks: browser-api
Assignee | ||
Updated•11 years ago
|
Attachment #760268 -
Attachment is obsolete: true
Comment 7•11 years ago
|
||
bug 880572 is no longer a blocker.
Please renom if this alone is considered blocking.
blocking-b2g: leo+ → ---
Assignee | ||
Comment 8•11 years ago
|
||
Kanru, are you eligible of reviewing mozbrowser API patch?
This is a follow-up to Alive's work -- I somehow came across the |mozOpaque| attribute we need to escape here. I tested locally and this works.
Attachment #761902 -
Attachment is obsolete: true
Attachment #8342938 -
Flags: review?(kchen)
Comment 9•11 years ago
|
||
Comment on attachment 8342938 [details] [diff] [review]
transparent.patch
Review of attachment 8342938 [details] [diff] [review]:
-----------------------------------------------------------------
See also https://bugzilla.mozilla.org/show_bug.cgi?id=801676 to understand why the screenshot was in JPEG format.
Could we test it the screenshot is actually transparent? Currently the getScreenshot test does not verify the captured image though..
::: dom/browser-element/BrowserElementChildPreload.js
@@ +721,5 @@
> // - Crop the viewport so its width is no larger than maxWidth and its
> // height is no larger than maxHeight.
> //
> + // - Transparent tells us to output a PNG or JPG.
> + //
I'm not sure if it's a good idea to mention this. The image format could be anything capable encoding the alpha channel.
Attachment #8342938 -
Flags: review?(kchen) → review?(bugs)
Comment 10•11 years ago
|
||
Comment on attachment 8342938 [details] [diff] [review]
transparent.patch
What if we had mimetype as paramater? (image/jpeg or image/png)
Then we can in the future add more types, of which some may support
transparency? Also, this way it becomes clear what kind of data will be generated.
Attachment #8342938 -
Flags: review?(bugs) → review-
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(timdream)
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8342938 [details] [diff] [review]
> transparent.patch
>
> What if we had mimetype as paramater? (image/jpeg or image/png)
> Then we can in the future add more types, of which some may support
> transparency? Also, this way it becomes clear what kind of data will be
> generated.
Thanks for the review!
If we are using mimetype here, IMHO we should expose the third argument too (quality of JPEG). What do you think?
Also, even though toBlob() is default to image/png, I would still in favor of using JPEG for this method by default since I was told by :jlebar that results lower memory footprints. Besides, changing API default will make this Gecko bug blocked by another Gaia bug and force us to do yet another gaia/gecko repo sync landing dance, something I would like to avoid. Does that work for you too?
Flags: needinfo?(timdream) → needinfo?(bugs)
Comment 12•11 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #11)
> If we are using mimetype here, IMHO we should expose the third argument too
> (quality of JPEG). What do you think?
That could be added later if needed, right?
> Also, even though toBlob() is default to image/png, I would still in favor
> of using JPEG for this method by default since I was told by :jlebar that
> results lower memory footprints. Besides, changing API default will make
> this Gecko bug blocked by another Gaia bug and force us to do yet another
> gaia/gecko repo sync landing dance, something I would like to avoid. Does
> that work for you too?
Yeah, defaulting to jpeg if not mimetype is passed sounds still ok.
It is a bit inconsistent with toBlob but taking a screenshot is quite different anyway.
Flags: needinfo?(bugs)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8342938 -
Attachment is obsolete: true
Attachment #8345200 -
Flags: review?(bugs)
Assignee | ||
Updated•11 years ago
|
Summary: Add third argument to getScreenshot API: transparent or not → Add mime type argument to getScreenshot API to control transparency of resulting screenshot
Assignee | ||
Comment 14•11 years ago
|
||
I have test almost ready and will push to try tomorrow.
Updated•11 years ago
|
Attachment #8345200 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 15•11 years ago
|
||
I will set review after verifying the test on try and/or locally.
Assignee | ||
Comment 16•11 years ago
|
||
I modify the test and try not to change it's exec flow (which is a bit confusing to be honest)
-- The first screenshot will now called with image/jpeg
-- The second screenshot will now called with image/png
-- when the screenshots are received, the image will be passed to a canvas and we will read the 4th value of the image data array (the alpha channel of the first pixel) to see if the image is indeed opaque/transparent.
I have verified the test passes locally. Took me some time to figure out how to run mochitest locally (I tried to run it with B2G/Desktop and I eventually give up and build Firefox Desktop).
Try server submission:
https://tbpl.mozilla.org/?tree=Try&rev=f25e37636140
Attachment #8345765 -
Flags: review?(bugs)
Assignee | ||
Updated•11 years ago
|
Attachment #8345674 -
Attachment is obsolete: true
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #8345765 -
Flags: review?(bugs)
Assignee | ||
Comment 18•11 years ago
|
||
OK, I didn't look into the tests closely enough. What passes locally was the inproc test. For oop test it will fail on my machine as well. Strangely, I could visually confirm the screenshots are indeed transparent on the phone even if all the app frames are oop.
Could there be something different between oop'd frame in Fx Desktop and B2G? I am going to run the tests (inproc and oop) with emulator to see if it failed on B2G too first.
Assignee | ||
Comment 19•11 years ago
|
||
I am a little confused by TryServer coz it looks like emulator run doesn't cover dom/ tests?
https://tbpl.mozilla.org/?tree=Try&rev=9a8fafd373ad
linux64 build do, however:
https://tbpl.mozilla.org/?tree=Try&rev=5acd05b2ccde
Assignee | ||
Comment 20•11 years ago
|
||
After consulting with Kanru and appendChild(img) to see what's really going on, we found that on Firefox desktop, an oop frame in Firefox desktop will always painted with an opaque solid color -- the default background color of user's choice -- browser.display.background_color. Attempt to set the background color to transparent will not work -- something we need to find a fix or it will forever block testing on Desktop. :mattwoodrow, Kanru told me you might have some idea on this, could you help out?
Also, it is appeared that emulator does not run dom/browser-elements/ tests at all. I checked fulllog of 1-9. One idea of mime is to figure out how to make TBPL only run this oop tests in emulator but not other tests, and skip this expected failed test in Desktop builds. Need to figure out how to do it. But first I would like to see if :smaug thinks this is a good idea.
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bugs)
Assignee | ||
Comment 21•11 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #20)
> Also, it is appeared that emulator does not run dom/browser-elements/ tests
> at all. I checked fulllog of 1-9. One idea of mime is to figure out how to
> make TBPL only run this oop tests in emulator but not other tests, and skip
> this expected failed test in Desktop builds. Need to figure out how to do
> it. But first I would like to see if :smaug thinks this is a good idea.
BTW I still need to make sure the oop test passes on emulator first.
Comment 22•11 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #20)
> Also, it is appeared that emulator does not run dom/browser-elements/ tests
> at all. I checked fulllog of 1-9. One idea of mime is to figure out how to
> make TBPL only run this oop tests in emulator but not other tests, and skip
> this expected failed test in Desktop builds. Need to figure out how to do
> it. But first I would like to see if :smaug thinks this is a good idea.
Perhaps don't skip but mark the test expected fail.
Flags: needinfo?(bugs)
Assignee | ||
Comment 23•11 years ago
|
||
Thanks for the reply.
So I did some digging on tbpl again and I found the mochitest numbers have changed since. We no longer run browser_element tests on Desktop Firefox (these builds run "M (1 2 3 4 5 bc oth)" and no test results show up in any of the full logs.
Emulator now shows "M (1 2 3 4 5 6 7 8 9) and browser_element tests did pop-up on mochitest-4.
Try is currently closed. I have rebase'd my local tree and will push a "try: -b o -p emulator -u mochitest-4 -t none" try again and see if the original test passes. If so I think the tests can be checked-in w/o being marked as expected failure.
Assignee | ||
Comment 24•11 years ago
|
||
Wait, I was mistaken on the previous comment:
We do run those tests on Desktop in mochitest-2, and Android on m-4, e.g.,
https://tbpl.mozilla.org/?rev=5c548fcd09f9
https://tbpl.mozilla.org/php/getParsedLog.php?id=32678618&tree=Mozilla-Central&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=32680305&tree=Mozilla-Central&full=1
And we still don't run browser-element tests anywhere for B2G ICS Emulator Opt, e.g.,
https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=8b40a99f24cf
https://tbpl.mozilla.org/php/getParsedLog.php?id=32665570&tree=B2g-Inbound&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=32665362&tree=B2g-Inbound&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=32665779&tree=B2g-Inbound&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=32665621&tree=B2g-Inbound&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=32665111&tree=B2g-Inbound&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=32664969&tree=B2g-Inbound&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=32664997&tree=B2g-Inbound&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=32665098&tree=B2g-Inbound&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=32666745&tree=B2g-Inbound&full=1
So expected failure is the way to go here.
Assignee | ||
Comment 25•11 years ago
|
||
Ouch, I found this:
https://mxr.mozilla.org/mozilla-central/source/testing/mochitest/b2g.json#284
hg blame showed emulator was NEVER configured to run mozbrowser tests when it was being enabled in TPBL. I will push a try to enable everything and see what will happen.
Assignee | ||
Comment 26•11 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #25)
> hg blame showed emulator was NEVER configured to run mozbrowser tests when
> it was being enabled in TPBL. I will push a try to enable everything and see
> what will happen.
Bug 830523
Assignee | ||
Comment 27•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=2c80ede259b6
try: -b o -p emulator -u mochitest-1,mochitest-2,mochitest-3,mochitest-4,mochitest-5,mochitest-6,mochitest-7,mochitest-8 -t none
Assignee | ||
Comment 28•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=231d290c1f7c
try: -b o -p android -u mochitest-4 -t none
Assignee | ||
Comment 29•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=0d2f5c4e9730
try: -b o -p linux64,macosx64,win32 -u mochitest-2 -t none
Assignee | ||
Comment 30•11 years ago
|
||
Verified browser-elements tests are being run on m-3 for emulator (comment 27). Gonna to disable the failed tests and push again. oop tests are not being run; need to investigate why.
The inproc test on Android passes in comment 28 (oop is not tested).
There was an logic error on my patch on |if (!isB2G ...| so test failed on comment 29 (desktop builds), so re-push to try here:
https://tbpl.mozilla.org/?tree=Try&rev=17f8462f1aef
Assignee | ||
Comment 31•11 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #30)
> The inproc test on Android passes in comment 28 (oop is not tested).
It looks like we disabled the tests because of bug 774939
> There was an logic error on my patch on |if (!isB2G ...| so test failed on
> comment 29 (desktop builds), so re-push to try here:
>
> https://tbpl.mozilla.org/?tree=Try&rev=17f8462f1aef
These tests now passes with expected failure.
Assignee | ||
Comment 32•11 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #30)
> Verified browser-elements tests are being run on m-3 for emulator (comment
> 27). Gonna to disable the failed tests and push again. oop tests are not
> being run; need to investigate why.
So according to
https://tbpl.mozilla.org/php/getParsedLog.php?id=32687440&tree=Try&full=1
Emulators are built with MOZ_ANDROID_OMTC=1 too.
Assignee | ||
Comment 33•11 years ago
|
||
These patches disabled failed tests in comment 27 and enable oop tests by removing |ifndef MOZ_ANDROID_OMTC|.
https://tbpl.mozilla.org/?tree=Try&rev=9d1bbf5cb350
Assignee | ||
Comment 34•11 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #32)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #30)
> > Verified browser-elements tests are being run on m-3 for emulator (comment
> > 27). Gonna to disable the failed tests and push again. oop tests are not
> > being run; need to investigate why.
>
> So according to
>
> https://tbpl.mozilla.org/php/getParsedLog.php?id=32687440&tree=Try&full=1
>
> Emulators are built with MOZ_ANDROID_OMTC=1 too.
I was looking at the wrong log. Emulators are NOT built with MOZ_ANDROID_OMTC=1.
https://tbpl.mozilla.org/php/getParsedLog.php?id=32709504&tree=Try&full=1#error49
It simply gave up trying because there were too many timeouts in the inproc part of the tests. Gonna try to disable that and make try run a full run and disable all failed tests.
Assignee | ||
Comment 35•11 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #34)
> It simply gave up trying because there were too many timeouts in the inproc
> part of the tests. Gonna try to disable that and make try run a full run and
> disable all failed tests.
https://tbpl.mozilla.org/?tree=Try&rev=0acb71eebccf
This try disables all the failed tsts in comment 27 and comment 33. Also it make TestRunner not to give up.
Assignee | ||
Comment 36•11 years ago
|
||
\O/
The oop test passes on comment 35. I am disabling the failed tests and request for review.
Assignee | ||
Comment 37•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=4f8a1daca701
All proposed final patches.
Assignee | ||
Comment 38•11 years ago
|
||
The tests will now check for transparency of the second screenshot.
It is being marked expected failure on Fx Desktop builds on all platform.
Attachment #8345765 -
Attachment is obsolete: true
Attachment #8357646 -
Flags: review?(bugs)
Assignee | ||
Comment 39•11 years ago
|
||
Since the test only passes on B2G Emulator, I need it to be run on TPBL.
This patch enables the entire dom/browser-elements tests to be run in B2G Emulator, but disables tests I found failing with try.
Attachment #8357651 -
Flags: review?(jgriffin)
Comment 40•11 years ago
|
||
Comment on attachment 8357651 [details] [diff] [review]
Patch for enabling some mozbrowser tests on B2G emulator
Review of attachment 8357651 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks!
Attachment #8357651 -
Flags: review?(jgriffin) → review+
Comment 41•11 years ago
|
||
Comment on attachment 8357646 [details] [diff] [review]
Test for patch
>+ function screenshotTaken(screenshotImageData) {
>+ screenshotImageDatas.push(screenshotImageData);
>+ if (screenshotImageDatas.length === 1) {
> ok(true, 'Got initial non blank screenshot');
> iframe1.src = 'data:text/html,<html>' +
>- '<body style="background:blue">hello</body></html>';
>+ '<body style="background:transparent">hello</body></html>';
Could you move this src load to be after you have checked that
screenshotImageData.data is what is expected
Attachment #8357646 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 42•11 years ago
|
||
Patch for commit, r=smaug
Attachment #8345200 -
Attachment is obsolete: true
Attachment #8359041 -
Flags: review+
Assignee | ||
Comment 43•11 years ago
|
||
Part 2, tests, r=smaug with comment addressed.
Attachment #8357646 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8359042 -
Flags: review+
Assignee | ||
Comment 44•11 years ago
|
||
Part 3 enables the tests on B2G emulator, r=jgriffin
Attachment #8357651 -
Attachment is obsolete: true
Attachment #8359043 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 45•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/8fc3d2e02f47
https://hg.mozilla.org/integration/b2g-inbound/rev/9c97bd22aab2
https://hg.mozilla.org/integration/b2g-inbound/rev/1f13f7342ceb
Flags: in-testsuite+
Keywords: checkin-needed
Comment 46•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8fc3d2e02f47
https://hg.mozilla.org/mozilla-central/rev/9c97bd22aab2
https://hg.mozilla.org/mozilla-central/rev/1f13f7342ceb
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
Assignee | ||
Comment 47•11 years ago
|
||
I've added the parameter to
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.getScreenshot
Updated•10 years ago
|
Flags: needinfo?(matt.woodrow)
You need to log in
before you can comment on or make changes to this bug.
Description
•