Closed
Bug 753021
Opened 12 years ago
Closed 12 years ago
Favicon shortcuts should be centered with a white background
Categories
(Firefox :: Shell Integration, defect)
Tracking
()
RESOLVED
FIXED
Firefox 17
People
(Reporter: bbondy, Assigned: artpar)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
2.65 KB,
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
Bug 110894 introduces favicon shortcuts. UX would like these shortcuts to be centered on a white background like Internet Explorer does instead of having larger pixelated favicons. This bug is to tackle that work.
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → artpar
Assignee | ||
Comment 1•12 years ago
|
||
Hi i looked up nsPresShell::RenderDocument and imgITools Then i looked up other code using this, and wrote some code. I have the following variables now gfxContext context imgIContainer container imgITools imgtool nsIInputStream stream = icondata used stream and imgtool to fill the container. nsIPresShell shell now what should i be doing to make the white background ? #code : gfxImageSurface* surface = new gfxImageSurface(gfxIntSize(32, 32), gfxImageSurface::ImageFormatARGB32); gfxContext context(surface); context.SetOperator(gfxContext::OPERATOR_CLEAR); context.Rectangle(gfxRect(0, 0, 32, 32)); context.Fill(); nsCOMPtr<imgIContainer> container; nsCOMPtr<imgITools> imgtool = do_CreateInstance("@mozilla.org/image/tools;1"); nsCOMPtr<nsIInputStream> stream; nsresult rv = NS_NewByteInputStream(getter_AddRefs(stream), reinterpret_cast<const char*>(aData), aDataLen, NS_ASSIGNMENT_DEPEND); NS_ENSURE_SUCCESS(rv, rv); rv = imgtool->DecodeImageData(stream, aMimeType, getter_AddRefs(container)); NS_ENSURE_SUCCESS(rv, rv); nsCOMPtr<nsIPresShell> shell(do_QueryReferent(aShell)); presShell->RenderDocument(aRect, aFlags, aBackgroundColor, aThebesContext); nsRect r(nsPresContext::CSSPixelsToAppUnits(0), nsPresContext::CSSPixelsToAppUnits(0), nsPresContext::CSSPixelsToAppUnits(32), nsPresContext::CSSPixelsToAppUnits(32)); PRUint32 renderDocFlags = (nsIPresShell::RENDER_IGNORE_VIEWPORT_SCROLLING | nsIPresShell::RENDER_DOCUMENT_RELATIVE); nscolor bgColor; if (!ParseColor(aBGColor, "White")) { return NS_ERROR_FAILURE; } shell->RenderDocument(r, renderDocFlags, bgColor, context);
Reporter | ||
Comment 2•12 years ago
|
||
Do you have a patch that I can apply so I can try to debug and figure out what needs to be changed? Thanks for looking into this :D
Assignee | ||
Comment 3•12 years ago
|
||
I didnt attached a patch since the new variables i introduced arent actually used anywhere. they are just initalised and does not participare anywhere in the code that follows. So right now the icons are as they were. no background. I wanted to ask what do i make use of this context variable now ? The icon data is in "container" and white background is in "context" vars. How to mix them ?
Reporter | ||
Comment 4•12 years ago
|
||
I have never done this before so the below is just a guess. But I CC'ed Joe Drew who is the owner of gfx and imagelib, he can check if those steps are correct, or if not he can school us :) - Create a surface nsRefPtr<gfxASurface> surface = gfxPlatform::GetPlatform()-> CreateOffscreenSurface( - Create a gfx context from that surface nsRefPtr<gfxContext> context = new gfxContext(surface); - Do any additional operations you want directly to the gfx context - Use imgTool->DecodeImageData to get an imgContainer from the favicon image you want to draw - You can then use imgIContainer::Draw to draw to the gfx context above - Then use nsPresShell:RenderDocument passing in the gfx context - You can then dump that to an image using imgtools. See the code in nsPresShell.cpp in the function DumpToPNG to see how
Comment 5•12 years ago
|
||
(In reply to Brian R. Bondy [:bbondy] from comment #4) > - Do any additional operations you want directly to the gfx context Which will be to do SetColor() of white and then a Rectangle(0, 0, width, height). > - You can then use imgIContainer::Draw to draw to the gfx context above Easier would be to do imgIContainer::getFrame(FRAME_FIRST, 0), which gives you a gfxASurface that you can use gfxContext methods to draw directly to the gfxContext. > - Then use nsPresShell:RenderDocument passing in the gfx context Why do you need to do this? If you're just drawing the favicon, there is no document. (I don't know what the requirements are, though.)
Reporter | ||
Comment 6•12 years ago
|
||
> I don't know what the requirements are, though.
So we have as input an imgContainer with a 16x16 image inside of it. We want to generate a 32x32 image with a white background that has the 16x16 image centered in it.
Assignee | ||
Comment 7•12 years ago
|
||
okay, so i have got it upto drawing the surface `s`, which is the first frame of imgContainer, on the context 1. make a gfxContext variable = context 2. context setColor, and Rectangle 3. context.fill() <-- i dont know why but i just did 4. make imgIContainer variable `container` 5. make imgITools variable `imgtool` and `nsIInputStream` = stream 6. get data from aData (which actually contains the data) and put into stream 7. from stream, put into container, using imgtools->decodeImageData 8. new surface s 9. put first frame of container into s 10. draw s on context ? --> context.DrawSurface(s, gfxSize(32, 32)); so if i am right then i suppose i have to convert variable `context` data back to PRUint8 aData variable, so that it will continue. I checked out the code of "DumpToPNG" in "nsPresShell.cpp", there i see an imgIEncoder, which takes in a variable `imgSurface` of type `gfxImageSurface` to init its data. it didnt used the context variable anywhere in the converting back to PRUint8 data form. how do i do this ? #code gfxImageSurface* surface = new gfxImageSurface(gfxIntSize(32, 32), gfxImageSurface::ImageFormatARGB32); gfxContext context(surface); context.SetOperator(gfxContext::OPERATOR_CLEAR); context.SetColor(gfxRGBA(1.0, 1.0, 1.0)); context.Rectangle(gfxRect(0, 0, 32, 32)); context.Fill(); nsCOMPtr<imgIContainer> container; nsCOMPtr<imgITools> imgtool = do_CreateInstance("@mozilla.org/image/tools;1"); nsCOMPtr<nsIInputStream> stream; rv = NS_NewByteInputStream(getter_AddRefs(stream), reinterpret_cast<const char*>(aData), aDataLen, NS_ASSIGNMENT_DEPEND); NS_ENSURE_SUCCESS(rv, rv); rv = imgtool->DecodeImageData(stream, aMimeType, getter_AddRefs(container)); NS_ENSURE_SUCCESS(rv, rv); gfxASurface *s; container->GetFrame(imgIContainer::FRAME_FIRST, 0, &s); context.DrawSurface(s, gfxSize(32, 32));
Reporter | ||
Comment 8•12 years ago
|
||
Hey Parth, I'm not sure but I'll try to figure it out tomorrow.
Reporter | ||
Comment 9•12 years ago
|
||
So I looked into it and the original steps I gave are not correct. I had a conversation a month or so ago with Joe and he had mentioned something about nsPresShell:RenderDocument but I think I had miscommunicated to him what we were doing here. Or I misunderstood what he meant, and so I was trying to fit that into the steps above. In any case that's not needed, and it's simpler than that. The code you have above is almost complete. From there you just need to use the surface called 'surface' w/ surface->Data() to initialize the encoder. See imgTools.cpp at the end of imgTools::EncodeScaledImage for an example of this, and how to get the stream after you have the encoder. You will do that instead of calling encodeScaledImage directly. So basically what you're doing is creating a new surface you can draw to, then you are drawing to it with gfxContext. Then independent of that you are decoding the favicon in a different image container and you are getting the image frame inside of there. Then you are drawing that favicon image frame onto the original new surface. All that's left to do is re-encode that surface and save it to disk.
Assignee | ||
Comment 10•12 years ago
|
||
Okay, thanks a lot for that, i feel i am very close now, i did that you said, it compiles without error now. but no icon is created. this is the part of code i have added for this bug, no need to read it all, i will tell you the important part :: ------ nsRefPtr<gfxImageSurface> surface = new gfxImageSurface(gfxIntSize(32, 32), gfxImageSurface::ImageFormatARGB32); gfxContext context(surface); context.SetOperator(gfxContext::OPERATOR_CLEAR); context.SetColor(gfxRGBA(1.0, 1.0, 1.0)); context.Rectangle(gfxRect(0, 0, 32, 32)); context.Fill(); nsCOMPtr<imgIContainer> container; nsCOMPtr<imgITools> imgtool = do_CreateInstance("@mozilla.org/image/tools;1"); nsCOMPtr<nsIInputStream> stream; rv = NS_NewByteInputStream(getter_AddRefs(stream), reinterpret_cast<const char*>(aData), aDataLen, NS_ASSIGNMENT_DEPEND); NS_ENSURE_SUCCESS(rv, rv); rv = imgtool->DecodeImageData(stream, aMimeType, getter_AddRefs(container)); NS_ENSURE_SUCCESS(rv, rv); nsRefPtr<gfxImageSurface> s; container->CopyFrame(imgIContainer::FRAME_FIRST, 0, getter_AddRefs(s)); PRUint32 aScaledHeight, strideSize; aScaledHeight = s->Height(); strideSize = s->Stride(); context.DrawSurface(s, gfxSize(32, 32)); aData = s->Data(); aDataLen = strideSize * aScaledHeight; ----- If i put all this as comment, then the icon is created, if i put this as code, then icon is not created. I Debugged i get the data in aData variable, aDataLen comes to be 913 (for one instance) ( exactly equal if i mark this block as comment). after this, there is a line memcpy(data, aData, aDataLen); now aData has the data in both cases (with and without above code marked as comment), data looks valid (starts with %%PNG in both cases), but the value of data is still 0 in new case. earlier it was not 0. but no error comes here, AsyncWriteIconToDisk is run, inside that, the following line is there rv = imgtool->DecodeImageData(stream, mMimeTypeOfInputData, getter_AddRefs(container)); NS_ENSURE_SUCCESS(rv, rv) this ensure success fails, and returns the control. wont execute after this. if the above code is marked as comment, then this is success and icon is created in the cache file.
Reporter | ||
Comment 11•12 years ago
|
||
You'll need these changes: > - context.SetOperator(gfxContext::OPERATOR_CLEAR); > + context.SetOperator(gfxContext::OPERATOR_SOURCE); The base image is 32x32, the one we want to put inside it is 16x16, so that means that we need to place it 8 pixels away from the left/right and top/bottom to center it. > + context.Translate(gfxPoint(8, 8)); > - context.DrawSurface(s, gfxSize(32, 32)); > + context.DrawSurface(s, gfxSize(16, 16)); Use GetFrame instead of CopyFrame > - container->CopyFrame ... > + container->GetFrame ... After your DrawSurface call, add this: > + gfxIntSize size; > + size = surface->GetSize(); > + nsCOMPtr<imgIEncoder> encoder = > + do_CreateInstance("@mozilla.org/image/encoder;2?type=image/vnd.microsoft.icon"); > + NS_ENSURE_TRUE(encoder, NS_ERROR_FAILURE); > + encoder->InitFromData(surface->Data(), size.width * size.height * 4, > + size.width, size.height, surface->Stride(), > + imgIEncoder::INPUT_FORMAT_HOSTARGB, EmptyString()); > + > + nsIInputStream* iconStream; > + CallQueryInterface(encoder, &iconStream); Then you can just use the iconStream directly instead of the one in the original code before this task that you used to get from imgtool->EncodeScaledImage. I tried this locally and it generates the icons :D
Assignee | ||
Comment 12•12 years ago
|
||
Sorry I just cant get it right :(
The call for imgtool->EncodeScaledImage happens in AsyncWriteIconToDisk::Run, and this iconStream we make in AsyncFaviconDataReady::OnComplete
I made another member for AsyncWriteIconToDisk to hold this Stream and pass it in the cosntructor, then in the ::run() i check for mURLShortcut and then use it the member stream accodringly
rv = imgtool->EncodeImage(container,
mMimeTypeOfInputData,
NS_LITERAL_STRING("format=bmp;bpp=32"),
getter_AddRefs(mStream));
but this is EncodeImage, you say to use it in EncodeScaledImage, and there is no call to EncodeScaledImage when mURLShortcut is true.
it still tries to write the icon file, but then i get a
###!!! ABORT: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file d:\mozilla-src2\obj-i686-pc-mingw32\dist\include\
.h, line 775
and Crash. The icon file contains no data.
-------------
and also i suppose we dont have to draw the white background when mURLShortcut is false, because its then for jump list icon.
-------------
Use GetFrame instead of CopyFrame
> - container->CopyFrame ...
> + container->GetFrame ...
If i put GetFrame then i have to change
- nsRefPtr<gfxImageSurface> s;
+ nsRefPtr<gfxASurface> s;
now gfxASurface has no member Data() and Stride() which is used in subsequent lines.
Reporter | ||
Comment 13•12 years ago
|
||
Sorry for the delay in getting back to you, I had a holiday Monday and wasn't around a computer. I attached the example code I was testing, which should give you enough info to get you going. The sample code I attached was just applied to the current mozilla-central tip, so you need to modify it to work with the code as is after your work in bug 110894. Regarding the jump list stuff, ya that should remain unchanged in the final patch that you submit. We will need to tweak this code a bit as well, for example: - It currently has a whitebackground but the image that is drawn has a transparent background (I think white should be filled in the transparent parts). - I'm not sure what happens if a website offers a 32x32 favicon. Maybe we will need to also do a context.Scale to ensure we have a 16x16 image. - Add extra error handling wherever needed
Assignee | ||
Comment 14•12 years ago
|
||
totally based on Brian's code, with minor modifications to keep jumplist icons unchanged.
Attachment #639228 -
Flags: review?(netzen)
Reporter | ||
Updated•12 years ago
|
Attachment #638767 -
Attachment is obsolete: true
Reporter | ||
Comment 15•12 years ago
|
||
Comment on attachment 639228 [details] [diff] [review] white background for shortcut icons Review of attachment 639228 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good to me, just some very minor comments. Please implement the nits and then re-request review. Once we get this r+'ed it should be a quick pass on the original bug and we can get it all landed soon :) nit: All of the changes have windows style line endings (\r\n) but they should be unix style line endings (\n). If you use Visual Studio, use this extension: http://www.grebulon.com/software/stripem.php Which allows you to auto-save with whichever line endings you would like. ::: widget/windows/WinUtils.cpp @@ +543,4 @@ > systemIconHeight, > EmptyString(), > getter_AddRefs(iconStream)); > + }else{ nite: } else { (Maybe in the first bug?) @@ +548,5 @@ > + container->GetFrame(imgIContainer::FRAME_FIRST, 0, getter_AddRefs(s)); > + > + gfxImageSurface* surface = > + new gfxImageSurface(gfxIntSize(32, 32), > + gfxImageSurface::ImageFormatARGB32); Let's change this to 48,48. I tried to drag a favicon from both IE and Firefox with this patch and theirs looks like a better resolution. I think 48x48 is the default size so this should fix that problem. @@ +552,5 @@ > + gfxImageSurface::ImageFormatARGB32); > + gfxContext context(surface); > + context.SetOperator(gfxContext::OPERATOR_SOURCE); > + context.SetColor(gfxRGBA(1, 1, 1, 1)); > + context.Rectangle(gfxRect(0, 0, 32, 32)); (0, 0, 48, 48) @@ +555,5 @@ > + context.SetColor(gfxRGBA(1, 1, 1, 1)); > + context.Rectangle(gfxRect(0, 0, 32, 32)); > + context.Fill(); > + > + context.Translate(gfxPoint(8, 8)); (16, 16) @@ +556,5 @@ > + context.Rectangle(gfxRect(0, 0, 32, 32)); > + context.Fill(); > + > + context.Translate(gfxPoint(8, 8)); > + context.DrawSurface(s, gfxSize(16, 16)); Before this call to DrawSurface, please add a call to: context.SetOperator(gfxContext::OPERATOR_OVER); This will ensure that if you are drawing a favicon with a transparent background, the transparent parts will turn white. @@ +563,5 @@ > + nsRefPtr<imgIEncoder> encoder = > + do_CreateInstance("@mozilla.org/image/encoder;2?" > + "type=image/vnd.microsoft.icon"); > + NS_ENSURE_TRUE(encoder, NS_ERROR_FAILURE); > + encoder->InitFromData(surface->Data(), size.width * size.height * 4, For the second parameter I think it's the same since we're using 32bpp but pls change to surface->Stride() * size.height
Attachment #639228 -
Flags: review?(netzen)
Assignee | ||
Comment 16•12 years ago
|
||
Nits fixed. ----------------- recently i formatted my laptop and after that i restored a backup of the source code folder for mozilla. and then i also put the new version of smartmake, but since then i am having trouble with smartmake, and even with direct python make.py i get the following error cl : Command line error D8022 : cannot open 'OS_CPPFLAGS@' d:\mozilla-src2\config\rules.mk:1027:0: command 'd:/mozilla-src2/obj-i686-pc-mingw32/_virtualenv/Scripts/python.exe -O d:/mozilla-src2/ build/cl.py cl -FoWidgetUtils.obj -c -D_HAS_EXCEPTIONS=0 -I../../dist/stl_wrappers -D_IMPL_NS_WIDGET -DMOZILLA_INTERNAL_API -D_IMPL_N S_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DEXCLUDE_SKIA_DEP ENDENCIES -DUNICODE -D_UNICODE -DNOMINMAX -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -D_SECURE_ATL -DCHROMIUM_BUILD -DU_STATIC_I MPLEMENTATION -DOS_WIN=1 -DWIN32 -D_WIN32 -D_WINDOWS -DWIN32_LEAN_AND_MEAN -DCOMPILER_MSVC -Id:/mozilla-src2/ipc/chromium/src -Id:/moz illa-src2/ipc/glue -I../../ipc/ipdl/_ipdlheaders -Id:/mozilla-src2/widget/shared -I. -I../../dist/include -Id:/mozilla-src2/obj-i686- pc-mingw32/dist/include/nspr -Id:/mozilla-src2/obj-i686-pc-mingw32/dist/include/nss @OS_CPPFLAGS@ @OS_CXXFLAGS@ -DDEBUG -D_DEBUG -DTRACING -Zi -O1 -Oy- -MDd @OS_CPPFLAGS@ @OS_COMPILE_CXXFLAGS@ d:/mozilla-src2/widget/shared/WidgetUtils.cpp' failed, retur n code 2 i am sure that this is not related to this code i am editing, ..i am using simple "make" for building since then (that takes a lot of time) so if someone can point out whats wrong here, please tell me.
Attachment #639228 -
Attachment is obsolete: true
Attachment #642200 -
Flags: review?(netzen)
Reporter | ||
Comment 17•12 years ago
|
||
Could you try renaming your objdir out of the way and doing a full build?
Reporter | ||
Comment 18•12 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=502eb26291e0
Reporter | ||
Comment 19•12 years ago
|
||
Comment on attachment 642200 [details] [diff] [review] fixed the nit's : white background for shortcut icons Review of attachment 642200 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! Just needs some extra error checking. ::: widget/windows/WinUtils.cpp @@ +462,2 @@ > nsCOMPtr<nsIFile> icoFile; > nsresult rv = FaviconHelper::GetOutputIconPath(mNewURI, icoFile, mURLShortcut); nit: Add NS_ENSURE_SUCCESS(rv, rv); here @@ +462,5 @@ > nsCOMPtr<nsIFile> icoFile; > nsresult rv = FaviconHelper::GetOutputIconPath(mNewURI, icoFile, mURLShortcut); > > + if (!aData) > + return NS_ERROR_FAILURE; nit: You can remove these 2 lines because a few lines up you already check for !aData and return. @@ +544,5 @@ > EmptyString(), > getter_AddRefs(iconStream)); > } else { > + nsRefPtr<gfxASurface> s; > + container->GetFrame(imgIContainer::FRAME_FIRST, 0, getter_AddRefs(s)); nit: rv = container->GetFrame(... NS_ENSURE_SUCCESS(rv, rv); @@ +566,5 @@ > + "type=image/vnd.microsoft.icon"); > + NS_ENSURE_TRUE(encoder, NS_ERROR_FAILURE); > + encoder->InitFromData(surface->Data(), surface->Stride() * size.height, > + size.width, size.height, surface->Stride(), > + imgIEncoder::INPUT_FORMAT_HOSTARGB, EmptyString()); nit: rv = encoder->InitFromData(... NS_ENSURE_SUCCESS(rv, rv); @@ +567,5 @@ > + NS_ENSURE_TRUE(encoder, NS_ERROR_FAILURE); > + encoder->InitFromData(surface->Data(), surface->Stride() * size.height, > + size.width, size.height, surface->Stride(), > + imgIEncoder::INPUT_FORMAT_HOSTARGB, EmptyString()); > + CallQueryInterface(encoder.get(), getter_AddRefs(iconStream)); Add this after the call to CallQueryInterface: if (!iconStream) { return NS_ERROR_FAILURE; } @@ +573,1 @@ > } nit: remove extra newline above this brace. Add a newline after the brace instead.
Attachment #642200 -
Flags: review?(netzen) → review+
Assignee | ||
Comment 20•12 years ago
|
||
Fixed the nits mentioned.
Attachment #642200 -
Attachment is obsolete: true
Attachment #644673 -
Flags: review?(netzen)
Reporter | ||
Updated•12 years ago
|
Attachment #644673 -
Flags: review?(netzen) → review+
Reporter | ||
Comment 21•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/37c915daed4a
Reporter | ||
Updated•12 years ago
|
Target Milestone: --- → Firefox 17
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/37c915daed4a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•