Closed
Bug 764125
Opened 13 years ago
Closed 12 years ago
[Azure] Get Azure/Cairo canvas passing tests
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: nrc, Assigned: nrc)
References
(Depends on 1 open bug, Blocks 3 open bugs)
Details
Attachments
(18 files, 24 obsolete files)
950 bytes,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
1.24 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
11.67 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
7.10 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
2.23 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
2.87 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
2.41 KB,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
1011 bytes,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
12.71 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
1.98 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
4.33 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
13.58 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
6.18 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
2.17 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
3.21 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
9.82 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
20.97 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
15.63 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•13 years ago
|
||
The attached patch should fix the 2d.toDataURL.* by implementing DrawTargetCairo::CopySurface()
Comment 2•13 years ago
|
||
Two similarly named patches. Let's try for the right one this time.
Attachment #632382 -
Attachment is obsolete: true
Comment 4•13 years ago
|
||
Comment 5•13 years ago
|
||
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #636212 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #636213 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #636212 -
Attachment is obsolete: true
Attachment #636212 -
Flags: review?(bas.schouten)
Attachment #636214 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #636215 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #636216 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #636217 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #636218 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #636219 -
Flags: review?(joe)
Updated•12 years ago
|
Attachment #636214 -
Flags: review?(bas.schouten) → review+
Updated•12 years ago
|
Attachment #636213 -
Flags: review?(bas.schouten) → review+
Comment 14•12 years ago
|
||
Comment on attachment 636215 [details] [diff] [review]
patch 3: pass around the size of a gfxASurface when creating a DrawTarget
Review of attachment 636215 [details] [diff] [review]:
-----------------------------------------------------------------
How will this work for Cairo surfaces that don't have a finite size? (i.e. PDF and such)
Comment 15•12 years ago
|
||
Attachment #633021 -
Attachment is obsolete: true
Attachment #637004 -
Flags: review?(bas.schouten)
Updated•12 years ago
|
Attachment #637004 -
Attachment description: part 1.5: implement DrawTargetCairo::CopySurface → patch 1.5: implement DrawTargetCairo::CopySurface
Comment 16•12 years ago
|
||
Attachment #633022 -
Attachment is obsolete: true
Attachment #637006 -
Flags: review?(karlt)
Comment 17•12 years ago
|
||
Attachment #637007 -
Flags: review?(bas.schouten)
Comment 18•12 years ago
|
||
Comment 19•12 years ago
|
||
Comment on attachment 637006 [details] [diff] [review]
patch 8: Fix canvas 2d.drawImage.5arg and 2d.drawImage.9arg surface scaling tests
> Bug 764125; Fix canvas 2d.drawImage.5arg and 2d.drawImage.9arg surface scaling tests. r=karl
The comment should describe how the patch changes behavior.
e.g. "switch from EXTEND_NONE to EXTEND_PAD for drawing surfaces to avoid sampling beyond the surface"
Attachment #637006 -
Flags: review?(karlt) → review+
Comment 20•12 years ago
|
||
Comment on attachment 637004 [details] [diff] [review]
patch 1.5: implement DrawTargetCairo::CopySurface
Review of attachment 637004 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/2d/DrawTargetCairo.cpp
@@ +473,5 @@
> AutoPrepareForDrawing prep(this, mContext);
> +
> + cairo_matrix_t src_mat;
> + cairo_matrix_init_scale(&src_mat, 1, 1);
> + cairo_matrix_translate(&src_mat, aSource.X(), aSource.Y());
nit: It's easier to use the members directly rather than the getters (i.e. aSource.x, aSource.y)
@@ +480,5 @@
> + if (aSurface->GetType() == SURFACE_CAIRO) {
> + surf = static_cast<SourceSurfaceCairo*>(aSurface)->GetSurface();
> + }
> +
> + cairo_pattern_t* pat = cairo_pattern_create_for_surface(surf);
What will this do with a NULL surface? Should we have an else { gfxWarning() above?
@@ +482,5 @@
> + }
> +
> + cairo_pattern_t* pat = cairo_pattern_create_for_surface(surf);
> + cairo_pattern_set_matrix(pat, &src_mat);
> + cairo_pattern_set_filter(pat, CAIRO_FILTER_BILINEAR);
We can use point filtering as there's never any resizing going on here. I also believe cairo has a set_source_surface that can directly set a source surface with an offset. That should make this function a lot simpler.
What you'd do is totally ignore matrices or anything (except set the identity matrix on the context), i.e. as far as I can see something like this would work (peudo-code):
cairo_save(mContext);
cairo_identity_matrix(mContext);
cairo_set_source_surface(mContext, surf, aDest - aSource);
cairo_set_operator(mContext, SOURCE);
cairo_reset_clip(mContext);
cairo_new_path(mContext);
cairo_rectangle(aDest.x, aDest.y, aSource.width, aSource.height);
cairo_clip(mContext);
cairo_paint(mContext);
cairo_restore(mContext);
@@ +498,5 @@
> + cairo_clip(mContext);
> + cairo_set_source(mContext, pat);
> +
> + cairo_set_operator(mContext, CAIRO_OPERATOR_SOURCE);
> + cairo_paint_with_alpha(mContext, 1.0F);
We should just be able to use cairo_paint();
Attachment #637004 -
Flags: review?(bas.schouten) → review-
Comment 21•12 years ago
|
||
Comment on attachment 637007 [details] [diff] [review]
patch 9: fixed crash test fail on gtk caused by null
Review of attachment 637007 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/thebes/gfxPlatform.cpp
@@ +633,5 @@
> if (backend == BACKEND_CAIRO) {
> nsRefPtr<gfxASurface> surf = CreateOffscreenSurface(ThebesIntSize(aSize),
> ContentForFormat(aFormat));
> + if (!surf)
> + return NULL;
nit: { } also for single line ifs.
Attachment #637007 -
Flags: review?(bas.schouten) → review+
Comment 22•12 years ago
|
||
Comment on attachment 636216 [details] [diff] [review]
patch 4: store a reference to the backing surface in DrawTargetCairo
Review of attachment 636216 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/2d/SourceSurfaceCairo.cpp
@@ +105,5 @@
> cairo_destroy(ctx);
>
> // Swap in this new surface.
> cairo_surface_destroy(mSurface);
> + cairo_pattern_destroy(pat);
This is actually fixing a leak as far as I can see. It should technically be in a separate patch.
Attachment #636216 -
Flags: review?(bas.schouten) → review+
Updated•12 years ago
|
Attachment #636218 -
Flags: review?(bas.schouten) → review+
Comment 23•12 years ago
|
||
I've made the suggested changes in comment 20 and now the patch looks suspiciously like the given pseudo-code.
Attachment #637004 -
Attachment is obsolete: true
Attachment #638865 -
Flags: review?(bas.schouten)
Comment 24•12 years ago
|
||
(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #23)
> Created attachment 638865 [details] [diff] [review]
> patch 1.5: implement DrawTargetCairo::CopySurface [v2]
>
> I've made the suggested changes in comment 20 and now the patch looks
> suspiciously like the given pseudo-code.
I assume you checked if it worked? ;)
Comment 25•12 years ago
|
||
(In reply to Bas Schouten (:bas) from comment #24)
> I assume you checked if it worked? ;)
Your assumption is correct.
Updated•12 years ago
|
Attachment #637009 -
Attachment description: patch 10: Fixed build break for b2g/qt → patch 10: Fixed build break for qt
Attachment #637009 -
Flags: review?(gwright)
Comment 26•12 years ago
|
||
Comment on attachment 633023 [details] [diff] [review]
Possible logic error affecting Cairo win32
I haven't been able to figure out the problem fixed by this patch (possible logic error...). It seems it will not fail gracefully if it hits a later error condition. Anyway the logic appears inverted.
Attachment #633023 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 27•12 years ago
|
||
Attachment #639196 -
Flags: review?(joe)
Assignee | ||
Comment 28•12 years ago
|
||
Attachment #636217 -
Attachment is obsolete: true
Attachment #636217 -
Flags: review?(bas.schouten)
Attachment #639197 -
Flags: review?(bas.schouten)
Assignee | ||
Updated•12 years ago
|
Attachment #636219 -
Flags: review?(joe) → review?(bas.schouten)
Assignee | ||
Comment 29•12 years ago
|
||
Comment on attachment 639196 [details] [diff] [review]
patch 9.5 test tweaks
Looks like Joe is away, so I've asked Bas for reviews on these two patches too, not sure if there is a better person for the Canvas stuff, but at least Bas has seen the other patches.
Attachment #639196 -
Flags: review?(joe) → review?(bas.schouten)
Comment 30•12 years ago
|
||
Comment on attachment 636219 [details] [diff] [review]
patch 7: changes to nsCanvasRenderingContext2DAzure
Review of attachment 636219 [details] [diff] [review]:
-----------------------------------------------------------------
Please explain why we need the mNewPath stuff, it's not clear to me.
Assignee | ||
Comment 31•12 years ago
|
||
(In reply to Bas Schouten (:bas) from comment #30)
> Comment on attachment 636219 [details] [diff] [review]
> patch 7: changes to nsCanvasRenderingContext2DAzure
>
> Review of attachment 636219 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Please explain why we need the mNewPath stuff, it's not clear to me.
It was to meet the spec for quadratic curves (tests: http://philip.html5.org/tests/canvas/suite/tests/index.2d.path.quadraticCurveTo.ensuresubpath.html) - if there is no existing path then the first control point is used as the start of the path.
Assignee | ||
Comment 32•12 years ago
|
||
rebased and fixed a bug
Attachment #636219 -
Attachment is obsolete: true
Attachment #636219 -
Flags: review?(bas.schouten)
Attachment #640100 -
Flags: review?(bas.schouten)
Comment 33•12 years ago
|
||
Comment on attachment 637009 [details] [diff] [review]
patch 10: Fixed build break for qt
Review of attachment 637009 [details] [diff] [review]:
-----------------------------------------------------------------
jfkthame is better suited to review this I think.
::: gfx/thebes/gfxQtPlatform.cpp
@@ +514,3 @@
> return gPlatformFTLibrary;
> +#else
> + return nsnull;
Jonathan Kew wasn't too fond of this. Maybe we can NS_ASSERTION that we're not using Pango here?
@@ +518,3 @@
> }
>
> +#ifndef MOZ_PANGO
Where is the matching endif for this? Why is this necessary?
Attachment #637009 -
Flags: review?(gwright) → review?(jfkthame)
Updated•12 years ago
|
Attachment #638865 -
Flags: review?(bas.schouten) → review?(jmuizelaar)
Updated•12 years ago
|
Attachment #636215 -
Flags: review?(bas.schouten) → review?(jmuizelaar)
Updated•12 years ago
|
Attachment #639197 -
Flags: review?(bas.schouten) → review?(jmuizelaar)
Updated•12 years ago
|
Attachment #640100 -
Flags: review?(bas.schouten) → review?(jmuizelaar)
Updated•12 years ago
|
Attachment #639196 -
Flags: review?(bas.schouten) → review?(jmuizelaar)
Comment 34•12 years ago
|
||
Comment on attachment 638865 [details] [diff] [review]
patch 1.5: implement DrawTargetCairo::CopySurface [v2]
It seems like this should use cairo_fill instead of clip and paint. Otherwise this looks ok.
Attachment #638865 -
Flags: review?(jmuizelaar) → review-
Comment 35•12 years ago
|
||
(In reply to Nick Cameron [:nrc] from comment #31)
> (In reply to Bas Schouten (:bas) from comment #30)
> > Comment on attachment 636219 [details] [diff] [review]
> > patch 7: changes to nsCanvasRenderingContext2DAzure
> >
> > Review of attachment 636219 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > Please explain why we need the mNewPath stuff, it's not clear to me.
>
> It was to meet the spec for quadratic curves (tests:
> http://philip.html5.org/tests/canvas/suite/tests/index.2d.path.
> quadraticCurveTo.ensuresubpath.html) - if there is no existing path then the
> first control point is used as the start of the path.
Does this affect other backends then? If so it would probably be better in another bug.
Updated•12 years ago
|
Attachment #640100 -
Flags: review?(jmuizelaar) → review?(bas.schouten)
Comment 36•12 years ago
|
||
Comment on attachment 636215 [details] [diff] [review]
patch 3: pass around the size of a gfxASurface when creating a DrawTarget
Review of attachment 636215 [details] [diff] [review]:
-----------------------------------------------------------------
This needs Bas's comment about cairo surfaces not having sizes addressed
Attachment #636215 -
Flags: review?(jmuizelaar) → review-
Comment 37•12 years ago
|
||
cairo_fill instead of cairo_clip and cairo_paint
Attachment #638865 -
Attachment is obsolete: true
Attachment #640848 -
Flags: review?(jmuizelaar)
Updated•12 years ago
|
Attachment #640848 -
Flags: review?(jmuizelaar) → review+
Comment 38•12 years ago
|
||
Comment on attachment 639196 [details] [diff] [review]
patch 9.5 test tweaks
Review of attachment 639196 [details] [diff] [review]:
-----------------------------------------------------------------
Do we understand the cause of these regressions? i.e. why it worked before with cairo but not with azure? The comment near the test talks about cairo limitations but isn't clear about what limitations and why we hit them now and not before.
Attachment #639196 -
Flags: review?(jmuizelaar) → review-
Assignee | ||
Comment 39•12 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #35)
> (In reply to Nick Cameron [:nrc] from comment #31)
> > (In reply to Bas Schouten (:bas) from comment #30)
> > > Comment on attachment 636219 [details] [diff] [review]
> > > patch 7: changes to nsCanvasRenderingContext2DAzure
> > >
> > > Review of attachment 636219 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > >
> > > Please explain why we need the mNewPath stuff, it's not clear to me.
> >
> > It was to meet the spec for quadratic curves (tests:
> > http://philip.html5.org/tests/canvas/suite/tests/index.2d.path.
> > quadraticCurveTo.ensuresubpath.html) - if there is no existing path then the
> > first control point is used as the start of the path.
>
> Does this affect other backends then? If so it would probably be better in
> another bug.
It does not seem to affect D2D, I do not know why, presumably because it is taken into account in the DrawTarget somehow. I don't know about other backends. It might be possible to move the logiv into the DrawTarget, but that seemed much more difficult. I believe this test fails for Thebes/Cairo canvas anyway, so might not be necessary to pass immediately.
Assignee | ||
Comment 40•12 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #36)
> Comment on attachment 636215 [details] [diff] [review]
> patch 3: pass around the size of a gfxASurface when creating a DrawTarget
>
> Review of attachment 636215 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> This needs Bas's comment about cairo surfaces not having sizes addressed
We discussed this on IRC, and iirc, we are not able to deal with size-less surfaces at the moment and it there is no easy way to sort this, so it didn't need addressing, but Bas was going to have a think about this before confirming.
Assignee | ||
Comment 41•12 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #38)
> Comment on attachment 639196 [details] [diff] [review]
> patch 9.5 test tweaks
>
> Review of attachment 639196 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Do we understand the cause of these regressions? i.e. why it worked before
> with cairo but not with azure? The comment near the test talks about cairo
> limitations but isn't clear about what limitations and why we hit them now
> and not before.
The limitation is that Cairo draws arcs by stroking perpendicular to the arc, and at very large stroke thicknesses, this becomes a fan. Where exactly the 'blades' of the fan appear seems to depend on exactly how the arc is defined and the platform. So if the blades of the fan are where pixels are tested it passes the test, if the testing pixels fall in between the blades, then we fail. Before, we were rendering wrong, but got lucky with the test, now we are not so lucky.
Comment 42•12 years ago
|
||
Comment on attachment 639197 [details] [diff] [review]
patch 5: drawing changes to DrawTargetCairo
Review of attachment 639197 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/2d/DrawTargetCairo.cpp
@@ +220,5 @@
> return pat;
> }
>
> static bool
> +OperatorAffectsUncoveredAreas(CompositionOp op)
Please copy the comment from nsCanvasRenderingContext2D.cpp as well
@@ +327,5 @@
> + cairo_new_path(mContext);
> + cairo_rectangle(mContext, 0, 0, aDest.Width(), aDest.Height());
> + cairo_clip(mContext);
> + cairo_set_source(mContext, pat);
> + cairo_paint(mContext);
We should be able to use fill() instead of clip() and paint()
@@ +418,5 @@
> + cairo_new_path(mContext);
> + cairo_rectangle(mContext, 0, 0, width, height);
> + cairo_clip(mContext);
> + cairo_set_source(mContext, pat);
> + cairo_paint(mContext);
Same here. fill() instead of clip() and paint()
@@ +727,5 @@
> + unsigned char* surfData = cairo_image_surface_get_data(surf);
> + const int32_t pixelWidth = BytesPerPixel(aFormat);
> + for (size_t y = 0; y < aSize.height; ++y) {
> + memcpy(surfData + y*aSize.width*pixelWidth, aData + y*aStride, aSize.width*pixelWidth);
> + }
I think it would probably be helpful to add a helper method that does this strided copy. We could use it the CoreGraphics backend too.
Attachment #639197 -
Flags: review?(jmuizelaar) → review+
Comment 43•12 years ago
|
||
Comment on attachment 639196 [details] [diff] [review]
patch 9.5 test tweaks
Review of attachment 639196 [details] [diff] [review]:
-----------------------------------------------------------------
It would be nice to know more about bug 764108 before pushing this, but I guess it doesn't need to block.
::: content/canvas/test/test_canvas.html
@@ +11309,5 @@
> isPixel(ctx, 1,48, 0,255,0,255, 0);
> +// Fails on Linux with Azure/Cairo only
> +// The arc is drawn badly due to Cairo limitations, the error only becomes
> +// apparent on Linux because of anti-aliasing, probably due to X
> +// Bug 764125
Move the description you gave into this comment to make it more useful. Also we should only disable the test when using Azure/Cairo
Attachment #639196 -
Flags: review- → review+
Assignee | ||
Comment 44•12 years ago
|
||
Added comment, carrying r=jrmuizel
Attachment #639196 -
Attachment is obsolete: true
Attachment #642528 -
Flags: review+
Assignee | ||
Comment 45•12 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #43)
> Move the description you gave into this comment to make it more useful. Also
> we should only disable the test when using Azure/Cairo
I added the comment, but the test only fails on Linux, it passes on other platforms. We do not have the facility for to mark a test passing on specific platforms, the policy elsewhere in the file seems to be to comment out tests which are platform-specific pass/fail.
Comment 46•12 years ago
|
||
(In reply to Nick Cameron [:nrc] from comment #45)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #43)
>
> > Move the description you gave into this comment to make it more useful. Also
> > we should only disable the test when using Azure/Cairo
>
> I added the comment, but the test only fails on Linux, it passes on other
> platforms. We do not have the facility for to mark a test passing on
> specific platforms, the policy elsewhere in the file seems to be to comment
> out tests which are platform-specific pass/fail.
You can, and you should. I object to commenting out any test.
Assignee | ||
Comment 47•12 years ago
|
||
with a conditional test, rather than commenting it out
Attachment #642533 -
Flags: review+
Assignee | ||
Comment 48•12 years ago
|
||
(In reply to :Ms2ger from comment #46)
> (In reply to Nick Cameron [:nrc] from comment #45)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #43)
> >
> > > Move the description you gave into this comment to make it more useful. Also
> > > we should only disable the test when using Azure/Cairo
> >
> > I added the comment, but the test only fails on Linux, it passes on other
> > platforms. We do not have the facility for to mark a test passing on
> > specific platforms, the policy elsewhere in the file seems to be to comment
> > out tests which are platform-specific pass/fail.
>
> You can, and you should. I object to commenting out any test.
OK, we can (very small bit of extra code, it turns out), but in this case I think we probably shouldn't, because whether we pass or not is basically non-deterministic, and we have established that we don't care (at the moment) whether we pass or not. I believe that is expressed by commenting out the test (we don't delete it because we might change our mind in the future). Making the test conditional says that it passes on one platform and fails on another, and we expect it to remain that way. It is easy to imagine some change somewhere which changes this situation and then a conditional test would be confusing.
Anyway, I have put up alternative versions of the patch, I can land whichever.
Updated•12 years ago
|
Attachment #633023 -
Flags: review?(bas.schouten) → review+
Comment 49•12 years ago
|
||
Comment on attachment 637009 [details] [diff] [review]
patch 10: Fixed build break for qt
Review of attachment 637009 [details] [diff] [review]:
-----------------------------------------------------------------
I'd like to know what makes this appear necessary; is there something in one of the other patches here that's requiring it?
::: gfx/thebes/gfxQtPlatform.cpp
@@ +518,1 @@
> }
I'm not clear why we need to do this - what purpose does it serve (in the MOZ_PANGO case) if it's just going to return null?
If we do have to provide this as a stub in the MOZ_PANGO case, we should assert NS_NOTREACHED because it shouldn't ever be called (I presume).
Assignee | ||
Comment 50•12 years ago
|
||
made one test conditional and added an IsLinux method to do so. Carrying r=jrmuizel
Attachment #642528 -
Attachment is obsolete: true
Attachment #642533 -
Attachment is obsolete: true
Attachment #643330 -
Flags: review+
Assignee | ||
Comment 51•12 years ago
|
||
changes as requested; carrying r=jrmuizel
Attachment #643342 -
Flags: review+
Assignee | ||
Comment 52•12 years ago
|
||
removed the mNewPath stuff (to Bug 775063).
Attachment #640100 -
Attachment is obsolete: true
Attachment #640100 -
Flags: review?(bas.schouten)
Attachment #643351 -
Flags: review?
Assignee | ||
Updated•12 years ago
|
Attachment #643351 -
Flags: review? → review?(bas.schouten)
Updated•12 years ago
|
Attachment #636215 -
Flags: review- → review+
Comment 53•12 years ago
|
||
Comment on attachment 637009 [details] [diff] [review]
patch 10: Fixed build break for qt
this patch is no longer needed because the patch causing the original build break is no longer being used.
Attachment #637009 -
Attachment is obsolete: true
Attachment #637009 -
Flags: review?(jfkthame)
Comment 54•12 years ago
|
||
So, for exceptions, I think we want different messages for the two places where we call onFailure. The one in wrapObjectTemplate should say something about the value not being an object, and the one next to the xpc_qsUnwrapArg call something about it not implementing descriptor.interface.identifier. (The latter never deals with non-object values.)
Assignee | ||
Comment 55•12 years ago
|
||
removed all the bits Bas didn't like and fixed the bits which needed fixing
Attachment #644796 -
Flags: review?
Assignee | ||
Updated•12 years ago
|
Attachment #643351 -
Attachment is obsolete: true
Attachment #643351 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 56•12 years ago
|
||
Attachment #644797 -
Flags: review?(bas.schouten)
Assignee | ||
Updated•12 years ago
|
Attachment #644797 -
Attachment is obsolete: true
Attachment #644797 -
Flags: review?(bas.schouten)
Assignee | ||
Updated•12 years ago
|
Attachment #644796 -
Flags: review? → review?(bas.schouten)
Assignee | ||
Comment 57•12 years ago
|
||
Attachment #644831 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 58•12 years ago
|
||
Attachment #644832 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 59•12 years ago
|
||
I'm a bit unsure about this one - is using the backend type as a bitmask really nasty? Is there a better way to do this?
Attachment #644833 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 60•12 years ago
|
||
Attachment #644834 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 61•12 years ago
|
||
core -> cg
Attachment #644832 -
Attachment is obsolete: true
Attachment #644832 -
Flags: review?(bas.schouten)
Attachment #644836 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 62•12 years ago
|
||
core -> cg
Attachment #644833 -
Attachment is obsolete: true
Attachment #644833 -
Flags: review?(bas.schouten)
Attachment #644837 -
Flags: review?(bas.schouten)
Comment on attachment 644833 [details] [diff] [review]
patch 12: enable Azure canvas pt 2
Review of attachment 644833 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/thebes/gfxPlatform.cpp
@@ +730,5 @@
> + if (strcmp(aName, "skia") == 0)
> + return BACKEND_SKIA;
> + if (strcmp(aName, "direct2d") == 0)
> + return BACKEND_DIRECT2D;
> + if (strcmp(aName, "core") == 0)
Better to just take const nsCString& and call EqualsLiteral here. char*s are somewhat evil and unsafe so the more we can avoid using them, the better, especially when (like here) the nsCString code is just as simple.
I think "cg" is better then "core", which is a bit ambiguous.
::: gfx/thebes/gfxPlatform.h
@@ +467,5 @@
> +
> + // returns the first backend named in the pref gfx.canvas.azure.backends
> + // which is a component of aMask, a bitmask of backend types
> + static mozilla::gfx::BackendType GetBackendPref(mozilla::gfx::BackendType aMask);
> + static mozilla::gfx::BackendType BackendTypeForName(const char* aName);
GetBackendPref/BackendTypeForName are not good names for functions which are canvas-specific. Nor are mFallbackDrawTargetBackend and mPreferredDrawTargetBackend good names for data which are canvas-specific.
Either we keep these names, and make the pref cover content as well as canvas, or we parameterize these over "content" and "canvas".
Attachment #644833 -
Attachment is obsolete: false
What about different kinds of cairo backends? Should we split cairo into cairo-image, cairo-xlib, cairo-gdi, etc?
Attachment #644833 -
Attachment is obsolete: true
Assignee | ||
Comment 65•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #64)
> What about different kinds of cairo backends? Should we split cairo into
> cairo-image, cairo-xlib, cairo-gdi, etc?
I'd be inclined to use a different pref for that, so that the azure.backends pref is one-to-one with the actual Azure backends.
What do we need to be able to switch Cairo surface type for?
Assignee | ||
Comment 66•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #63)
> Comment on attachment 644833 [details] [diff] [review]
> patch 12: enable Azure canvas pt 2
>
> Review of attachment 644833 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Either we keep these names, and make the pref cover content as well as
> canvas, or we parameterize these over "content" and "canvas".
Is it worth having different backend preferences for canvas and content? I guess we don't need to fallback for content; do we? I had assumed we'd use the same pref for canvas/content, but maybe we don't want to.
Comment 67•12 years ago
|
||
Comment on attachment 644834 [details] [diff] [review]
patch 13: enable Azure canvas pt 3
Review of attachment 644834 [details] [diff] [review]:
-----------------------------------------------------------------
::: widget/xpwidgets/GfxInfoBase.cpp
@@ +731,5 @@
> + static bool azureInited = false;
> +
> + if (!azureInited) {
> + nsresult rv = mozilla::Preferences::GetBool("gfx.canvas.azure.enabled", &azure);
> + azure = NS_SUCCEEDED(rv) && azure;
Maybe 'azure = Preferences::GetBool("gfx.canvas.azure.enabled", false);'
Comment 68•12 years ago
|
||
Comment on attachment 644836 [details] [diff] [review]
patch 11: enabling Azure Canvas pt 1
Review of attachment 644836 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +569,5 @@
> nsresult
> NS_NewCanvasRenderingContext2DAzure(nsIDOMCanvasRenderingContext2D** aResult)
> {
> + // XXX[nrc] remove this check when Thebes canvas is removed
> + // (because we will always support Azure)
*shrug*. This entire function will be able to go by then.
Comment 69•12 years ago
|
||
Comment on attachment 644831 [details] [diff] [review]
patch 10: mochitest for isPointInPath and multiple transforms
Review of attachment 644831 [details] [diff] [review]:
-----------------------------------------------------------------
Now also <http://w3c-test.org/html/tests/submission/PhilipTaylor/canvas/2d.path.isPointInPath.transform.4.html>.
Assignee | ||
Comment 70•12 years ago
|
||
Attachment #644836 -
Attachment is obsolete: true
Attachment #644836 -
Flags: review?(bas.schouten)
Attachment #645191 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 71•12 years ago
|
||
Attachment #644837 -
Attachment is obsolete: true
Attachment #644837 -
Flags: review?(bas.schouten)
Attachment #645193 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 72•12 years ago
|
||
All three patches updated for comments by roc and Ms2ger (thanks for the comments!)
Attachment #644834 -
Attachment is obsolete: true
Attachment #644834 -
Flags: review?(bas.schouten)
Attachment #645194 -
Flags: review?(bas.schouten)
Updated•12 years ago
|
Attachment #644796 -
Flags: review?(bas.schouten) → review+
Updated•12 years ago
|
Attachment #644831 -
Flags: review?(bas.schouten) → review+
Updated•12 years ago
|
Attachment #645191 -
Flags: review?(bas.schouten) → review+
Comment 73•12 years ago
|
||
Comment on attachment 645193 [details] [diff] [review]
patch 12: enable Azure canvas pt 2
Review of attachment 645193 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/d3d10/LayerManagerD3D10.cpp
@@ +488,4 @@
> if ((aFormat != FORMAT_B8G8R8A8 &&
> + aFormat != FORMAT_B8G8R8X8) ||
> + !gfxPlatform::GetPlatform()->SupportsAzure(backend) ||
> + backend != BACKEND_DIRECT2D) {
This really can't happen with D3D10 layers.
::: gfx/thebes/gfxAndroidPlatform.h
@@ -32,5 @@
> virtual already_AddRefed<gfxASurface>
> CreateOffscreenSurface(const gfxIntSize& size,
> gfxASurface::gfxContentType contentType);
>
> - virtual bool SupportsAzure(mozilla::gfx::BackendType& aBackend) { aBackend = mozilla::gfx::BACKEND_SKIA; return true; }
Why was this removed?
::: gfx/thebes/gfxPlatform.h
@@ +469,5 @@
> +
> + /**
> + * initialise preferred and fallback canvas backends
> + * both backends will be contained in aBackendBitmask, the order is
> + * determined by the gfx.canvas.azure.backends pref
This comment isn't very clear in my opinion.
Comment 74•12 years ago
|
||
(In reply to Bas Schouten (:bas) from comment #73)
> Comment on attachment 645193 [details] [diff] [review]
> patch 12: enable Azure canvas pt 2
>
> Review of attachment 645193 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: gfx/thebes/gfxAndroidPlatform.h
> @@ -32,5 @@
> > virtual already_AddRefed<gfxASurface>
> > CreateOffscreenSurface(const gfxIntSize& size,
> > gfxASurface::gfxContentType contentType);
> >
> > - virtual bool SupportsAzure(mozilla::gfx::BackendType& aBackend) { aBackend = mozilla::gfx::BACKEND_SKIA; return true; }
>
> Why was this removed?
I should be more clear here, it was removed, does that mean support in gfxAndroidPlatform is identical to that in gfxPlatform?
Comment 75•12 years ago
|
||
Comment on attachment 645194 [details] [diff] [review]
patch 13: enable Azure canvas pt 3
Review of attachment 645194 [details] [diff] [review]:
-----------------------------------------------------------------
It would probably be worth making sure the fallback backend is also reported here, but we can do that later.
Attachment #645194 -
Flags: review?(bas.schouten) → review+
Assignee | ||
Comment 76•12 years ago
|
||
(In reply to Bas Schouten (:bas) from comment #74)
> (In reply to Bas Schouten (:bas) from comment #73)
> > Comment on attachment 645193 [details] [diff] [review]
> > patch 12: enable Azure canvas pt 2
> >
> > Review of attachment 645193 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > ::: gfx/thebes/gfxAndroidPlatform.h
> > @@ -32,5 @@
> > > virtual already_AddRefed<gfxASurface>
> > > CreateOffscreenSurface(const gfxIntSize& size,
> > > gfxASurface::gfxContentType contentType);
> > >
> > > - virtual bool SupportsAzure(mozilla::gfx::BackendType& aBackend) { aBackend = mozilla::gfx::BACKEND_SKIA; return true; }
> >
> > Why was this removed?
>
> I should be more clear here, it was removed, does that mean support in
> gfxAndroidPlatform is identical to that in gfxPlatform?
The behaviour is fixed to that of gfxPlatform, but the behaviour is to return mPreferredCanvasBackend, which is set differently for different platforms. Right now, we will return cairo for Android because Skia is kind of broken, but hopefully we'll fix that and switch to Skia, if it proves to have better perf (which it probably will).
Assignee | ||
Comment 77•12 years ago
|
||
(In reply to Bas Schouten (:bas) from comment #75)
> Comment on attachment 645194 [details] [diff] [review]
> patch 13: enable Azure canvas pt 3
>
> Review of attachment 645194 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> It would probably be worth making sure the fallback backend is also reported
> here, but we can do that later.
What do you mean by report? I filed bug 776802 about providing better info from gfxPlatform about the Azure backend.
Assignee | ||
Comment 78•12 years ago
|
||
(In reply to Bas Schouten (:bas) from comment #73)
> Comment on attachment 645193 [details] [diff] [review]
> patch 12: enable Azure canvas pt 2
>
> Review of attachment 645193 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: gfx/layers/d3d10/LayerManagerD3D10.cpp
> @@ +488,4 @@
> > if ((aFormat != FORMAT_B8G8R8A8 &&
> > + aFormat != FORMAT_B8G8R8X8) ||
> > + !gfxPlatform::GetPlatform()->SupportsAzure(backend) ||
> > + backend != BACKEND_DIRECT2D) {
>
> This really can't happen with D3D10 layers.
>
It can happen if the user sets the pref to use a different backend, and without this change, that pref would not be respected.
Assignee | ||
Comment 79•12 years ago
|
||
improved comment, changed static variable with destructor to pointer
Attachment #645193 -
Attachment is obsolete: true
Attachment #645193 -
Flags: review?(bas.schouten)
Attachment #645632 -
Flags: review?(bas.schouten)
Updated•12 years ago
|
Attachment #645632 -
Flags: review?(bas.schouten) → review+
Assignee | ||
Comment 80•12 years ago
|
||
Attachment #646007 -
Flags: review?(roc)
Comment on attachment 646007 [details] [diff] [review]
patch 14: remove AzureEnabled from gfxInfo
Review of attachment 646007 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/thebes/gfxPlatform.cpp
@@ +1177,5 @@
> + static bool azureEnabled;
> + if (!checkedPref) {
> + azureEnabled = Preferences::GetBool("gfx.canvas.azure.enabled", false);
> + checkedPref = true;
> + }
Why not just always check this pref? No need to cache it AFAICT.
@@ +1181,5 @@
> + }
> +
> + if (!azureEnabled) {
> + mPreferredCanvasBackend = BACKEND_NONE;
> + mFallbackCanvasBackend = BACKEND_NONE;
and return? Because you overwrite these below.
Assignee | ||
Comment 82•12 years ago
|
||
fixed roc's comments and changed GetAzureBackendInfo slightly
Attachment #646007 -
Attachment is obsolete: true
Attachment #646007 -
Flags: review?(roc)
Attachment #646013 -
Flags: review?(roc)
Attachment #646013 -
Flags: review?(roc) → review+
Assignee | ||
Comment 83•12 years ago
|
||
removed an unnecessary pref check from AzureCanvasEnabled in nsCanvasRenderingContext2DAzure and fixed some typo bugs. Carrying r=roc.
Attachment #646013 -
Attachment is obsolete: true
Attachment #646031 -
Flags: review+
Assignee | ||
Comment 84•12 years ago
|
||
Assignee | ||
Comment 85•12 years ago
|
||
Assignee | ||
Comment 86•12 years ago
|
||
Comment 87•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9b9acad7f6d0
https://hg.mozilla.org/mozilla-central/rev/06a5feea546f
https://hg.mozilla.org/mozilla-central/rev/f4c2417994aa
https://hg.mozilla.org/mozilla-central/rev/f5a58948203f
https://hg.mozilla.org/mozilla-central/rev/ad4139ad6247
https://hg.mozilla.org/mozilla-central/rev/f731bb5b22b8
https://hg.mozilla.org/mozilla-central/rev/e2deb8b45529
https://hg.mozilla.org/mozilla-central/rev/1260e5f010c0
https://hg.mozilla.org/mozilla-central/rev/0223c2ea0033
https://hg.mozilla.org/mozilla-central/rev/cce58a8050f3
https://hg.mozilla.org/mozilla-central/rev/0f8c4f6e6248
https://hg.mozilla.org/mozilla-central/rev/0278d5acd5f2
https://hg.mozilla.org/mozilla-central/rev/fcc82e42815f
https://hg.mozilla.org/mozilla-central/rev/03e169cce29a
https://hg.mozilla.org/mozilla-central/rev/79a562f4b12d
https://hg.mozilla.org/mozilla-central/rev/148d57e5d062
https://hg.mozilla.org/mozilla-central/rev/3c73222e6e1c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•