Closed
Bug 704558
Opened 13 years ago
Closed 13 years ago
Accept encoder output options in encodeImage and encodeScaledImage
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: TimAbraldes, Assigned: TimAbraldes)
References
Details
(Keywords: addon-compat, dev-doc-needed)
Attachments
(2 files, 7 obsolete files)
17.23 KB,
patch
|
TimAbraldes
:
review+
|
Details | Diff | Splinter Review |
1.05 KB,
patch
|
TimAbraldes
:
review+
|
Details | Diff | Splinter Review |
My specific use case is that I intend to pass "format=bmp" to our ICO encoder in a call to `encodeImage`. I envision being able to do that from js like so (where imgContainer is an image container containing the desired icon image): let imgTools = Cc["@mozilla.org/image/tools;1"] .createInstance(Ci.imgITools); imgTools.encodeImage(imgContainer.value, "image/vnd.microsoft.icon", "format=bmp");
Assignee | ||
Comment 1•13 years ago
|
||
This is what I'm thinking, modulo some nsAString/nsACString differences. Brian, do you mind taking a look and letting me know what you think? I won't be touching this again until next week so no rush.
Attachment #576215 -
Flags: feedback?(netzen)
Comment 2•13 years ago
|
||
Yup, I'll take a look later this week or early next week.
Comment 3•13 years ago
|
||
Comment on attachment 576215 [details] [diff] [review] Patch v1 - Concept This looks good to me, but I have one concern. That there are other products and add-ons using this imgITools function already that this would break. I'm wondering if it would be better to add a new function with the outputOptions parameter instead and have the old functions call into that function for their implementation. Please check with Joe Drew to see if he'd prefer that way or the current way. Also I think you'll have to break up this patch. I can review once you get feedback from Joe Drew for imagelib and widget but I'm not sure if I am allowed to r+ the toolkit/ change even know it is just adding a blank parameter to the existing function. Please r=me once you get the feedback from Joe.
Attachment #576215 -
Flags: feedback?(netzen) → feedback?(joe)
Comment 4•13 years ago
|
||
Comment on attachment 576215 [details] [diff] [review] Patch v1 - Concept Review of attachment 576215 [details] [diff] [review]: ----------------------------------------------------------------- Make outputOptions optional, I think. Also note that you will need to add tests to imglib for this :)
Attachment #576215 -
Flags: feedback?(joe) → feedback+
Comment 5•13 years ago
|
||
I'm not concerned about changes to imgITools because binary addons always need to recompile for every new version of Firefox.
Assignee | ||
Comment 6•13 years ago
|
||
Thanks Joe and Brian for the feedback! I'll proceed as follows: 1) Split the patch into reviewer-specific patches 2) Make outputOptions optional 3) Add tests to imglib
Comment 7•13 years ago
|
||
Sounds good. For #3 by the way you can add them to mozilla/image/test/unit/test_imgtools.js
Assignee | ||
Comment 8•13 years ago
|
||
Summary of changes: - Added 1 test case each for encodeImage and encodeScaledImage - Made outputOptions optional - I changed outputOptions from nsACString to nsAString since we're converting to nsAString anyway before using it I'm running this through tryserver but I thought I'd make it available for review as well. Let me know what you think!
Attachment #576215 -
Attachment is obsolete: true
Attachment #577847 -
Flags: review?(netzen)
Comment 9•13 years ago
|
||
Try run for de4f53ec5ca1 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=de4f53ec5ca1 Results (out of 178 total builds): success: 154 warnings: 20 failure: 4 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tabraldes@mozilla.com-de4f53ec5ca1
Updated•13 years ago
|
Keywords: addon-compat,
dev-doc-needed
Comment 10•13 years ago
|
||
Comment on attachment 577847 [details] [diff] [review] ImageLib & Widget Patch v1 - Adds ImageLib tests, makes outputOptions optional, and avoids excessive string conversion Review of attachment 577847 [details] [diff] [review]: ----------------------------------------------------------------- Good work, just one suggestion for 2 new xpcshell tests. r=? me with the 2 new xpcshell tests. ::: image/test/unit/test_imgtools.js @@ +348,5 @@ > + > +// we'll reuse the container from the previous test > +istream = imgTools.encodeImage(container, > + "image/vnd.microsoft.icon", > + "format=bmp"); Although our current default is PNG format, in case this changes we should have another reftet the same as this one for format=png. @@ +370,5 @@ > +istream = imgTools.encodeScaledImage(container, > + "image/vnd.microsoft.icon", > + 16, > + 16, > + "format=bmp"); Ditto.
Attachment #577847 -
Flags: review?(netzen) → review+
Comment 11•13 years ago
|
||
*reftet -> xpcshell
Comment 12•13 years ago
|
||
Try results looked ok btw.
Assignee | ||
Comment 13•13 years ago
|
||
Attachment #578025 -
Flags: review?(netzen)
Assignee | ||
Comment 14•13 years ago
|
||
Robert, you're listed as a peer for toolkit. Let me know if I should r="someone else"
Attachment #578029 -
Flags: review?(robert.bugzilla)
Comment 15•13 years ago
|
||
Comment on attachment 578029 [details] [diff] [review] Toolkit patch: Modifies consumer of encodeImage/encodeScaledImage to use the new parameter Tim / Brian, would it make sense to specify outputOptions for this case? If you think so please add it and just carry forward my r+
Attachment #578029 -
Flags: review?(robert.bugzilla) → review+
Comment 16•13 years ago
|
||
> Tim / Brian, would it make sense to specify outputOptions for this case?
Nope, should be EmptyString(), so it's all good for that patch.
Comment 17•13 years ago
|
||
Comment on attachment 578025 [details] [diff] [review] Patch with extra tests for imagelib Review of attachment 578025 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, I had to do a double take on both of the PNG ICOs being of size 8214 :). You can qfold the 2 patches I reviewed together if you want before you push.
Attachment #578025 -
Flags: review?(netzen) → review+
Assignee | ||
Comment 18•13 years ago
|
||
qfold complete :)
Attachment #577847 -
Attachment is obsolete: true
Attachment #578025 -
Attachment is obsolete: true
Attachment #578137 -
Flags: review+
Assignee | ||
Comment 19•13 years ago
|
||
Built this locally (clobber build) with the qfolded patch and the toolkit patch. XPCShell tests still work. Running through try one more time just in case, but otherwise it sounds like this is ready for checkin
Comment 20•13 years ago
|
||
Try run for 69ce91f7b8a0 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=69ce91f7b8a0 Results (out of 190 total builds): success: 161 warnings: 25 failure: 4 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tabraldes@mozilla.com-69ce91f7b8a0
Assignee | ||
Comment 21•13 years ago
|
||
I am certainly glad I ran that through try. Every build except "Win debug" and "WinXP debug" failed with this message: FAILED in test #13 -- test encoding an unscaled ICO with format options (png): arrays differ at index 2537 Somehow the "Win debug" and "WinXP debug" builds succeed though
Comment 22•13 years ago
|
||
Hrm strange, I ran the new tests on my machine as well, I'm using win7 x64 (but an x86 build)
Comment 23•13 years ago
|
||
suggested on irc to use bmp and bpp to ensure it's working correctly.
Assignee | ||
Comment 24•13 years ago
|
||
Spoke with bbondy and rs about this. In the interest of getting this functionality into m-c quickly, I'm modifying the test cases to use different format options (no png), and separately I'll track down why png ICO encoding is different on Win Debug builds. Brian - this is currently running through try. If everything looks OK there could you review this patch again?
Attachment #578137 -
Attachment is obsolete: true
Attachment #578384 -
Flags: review?(netzen)
Comment 25•13 years ago
|
||
Try run for 07119f6bccda is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=07119f6bccda Results (out of 190 total builds): exception: 1 success: 152 warnings: 32 failure: 5 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tabraldes@mozilla.com-07119f6bccda
Comment 26•13 years ago
|
||
Try run for 29f78624c640 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=29f78624c640 Results (out of 172 total builds): exception: 2 success: 147 warnings: 19 failure: 4 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tabraldes@mozilla.com-29f78624c640
Comment 27•13 years ago
|
||
> and separately I'll track down why png ICO encoding is different > on Win Debug builds. Thanks I think that would be helpful. The easiest way is probably to try to use a release build or a build using another platform. If you happen to not reproduce on your own machine, one thing you can try with the old patch that is busted on non Win debug (in the context of another bug) is to put in the log a base64 encoded data URI ( http://en.wikipedia.org/wiki/Data_URI_scheme ) for each of the 2 images. Then we can investigate both images from try's Windows Opt vs Windows Debug to see (visually at first) exactly how they differ. I'm not sure if there's an easier way, but you can use do_print to output the 2 URIs and you can use btoa to get the base64 encoded string. Test locally you should be able to paste the URI in your browser's URL bar and see the image.
Comment 28•13 years ago
|
||
Comment on attachment 578384 [details] [diff] [review] ImageLib & Widget Patch v3 - Changes test cases to use different format options (all bmp and bpp; no png) Review of attachment 578384 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I was hoping to r+ this but it's missing the reference icon images. I tried to run the test locally and it failed with missing icons. (not just a display thing in splinter review). For some reason the patch pushed to try does have the icons though. r=me wit the new patch with the images. ::: image/test/unit/test_imgtools.js @@ +345,5 @@ > +/* ========== 11 ========== */ > +testnum++; > +testdesc = "test encoding an unscaled ICO with format options " > + + "(format=bmp;bpp=32)"; > + nit: put plus on the previous line and the 2 strings aligned. Ditto the other 3 testdesc = lines added.
Attachment #578384 -
Flags: review?(netzen) → review-
Assignee | ||
Comment 29•13 years ago
|
||
Oops! I had noticed the issue in tryserver so I pushed an updated patch there but I forgot to update the one here. Thanks for testing/reviewing this Brian!
Attachment #578384 -
Attachment is obsolete: true
Attachment #578623 -
Flags: review?(netzen)
Comment 30•13 years ago
|
||
Comment on attachment 578623 [details] [diff] [review] ImageLib & Widget Patch v4 - Include new reference images Review of attachment 578623 [details] [diff] [review]: ----------------------------------------------------------------- The nits from the previous review are still not implemented from this patch, but after you do those it's good to go to mozilla-inbound.
Attachment #578623 -
Flags: review?(netzen) → review+
Assignee | ||
Comment 31•13 years ago
|
||
I must be under-caffeinated
Attachment #578623 -
Attachment is obsolete: true
Attachment #578653 -
Flags: review+
Comment 32•13 years ago
|
||
Looks good, thanks for fixing that up! Enjoy your coffees :)
Assignee | ||
Comment 33•13 years ago
|
||
Tested locally and seems to work; adding checkin-needed To the committer: Please check in both patches (only checking in 1 will cause bustage). Now acquiring caffeine...
Whiteboard: checkin-needed
Assignee | ||
Comment 34•13 years ago
|
||
Just noticed the path had "r=???" instead of "r=rs" Fixing
Attachment #578029 -
Attachment is obsolete: true
Attachment #578674 -
Flags: review+
Assignee | ||
Comment 35•13 years ago
|
||
(In reply to Brian R. Bondy [:bbondy] from comment #27) > > and separately I'll track down why png ICO encoding is different > > on Win Debug builds. > > Thanks I think that would be helpful. > The easiest way is probably to try to use a release build or a build using > another platform. > > If you happen to not reproduce on your own machine, one thing you can try > with the old patch that is busted on non Win debug (in the context of > another bug) is to put in the log a base64 encoded data URI ( > http://en.wikipedia.org/wiki/Data_URI_scheme ) for each of the 2 images. > Then we can investigate both images from try's Windows Opt vs Windows Debug > to see (visually at first) exactly how they differ. > > I'm not sure if there's an easier way, but you can use do_print to output > the 2 URIs and you can use btoa to get the base64 encoded string. Test > locally you should be able to paste the URI in your browser's URL bar and > see the image. I've filed bug 710079 about WinDebug builds producing .ico files that are different from other builds.
Comment 36•13 years ago
|
||
Thanks for posting!
Assignee | ||
Comment 37•13 years ago
|
||
Is there anything special I need to do to make sure the patches for this bug get into m-i?
Comment 38•13 years ago
|
||
I can push these out for you Tim but the tree is currently closed without special approval. Please ping me again when it's open and I'll push them.
Assignee | ||
Comment 39•13 years ago
|
||
Ah of course, I'm reading about that on the newgroups now. Thanks Brian! Sorry for the unnecessary ping :D
Comment 40•13 years ago
|
||
np glad you pinged me as I didn't know I should push it out. Pls give me a reminder on IRC and I'll get it pushed right away once open.
Comment 41•13 years ago
|
||
Pushed to mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/ff70e251a0f4 http://hg.mozilla.org/integration/mozilla-inbound/rev/4726e2ddf570
Target Milestone: --- → mozilla11
Updated•13 years ago
|
Whiteboard: checkin-needed
Comment 42•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ff70e251a0f4 https://hg.mozilla.org/mozilla-central/rev/4726e2ddf570
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•