Closed
Bug 1351440
Opened 7 years ago
Closed 7 years ago
PaintSVG should not be returning DrawResult
Categories
(Core :: SVG, enhancement)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla55
| Tracking | Status | |
|---|---|---|
| firefox55 | --- | fixed |
People
(Reporter: jwatt, Assigned: u459114)
References
Details
Attachments
(5 files, 2 obsolete files)
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
Bug 1258510 made nsSVGDisplayableFrame's (formerly nsISVGChildFrame's) pure virtual PaintSVG method return a DrawResult. For quite a while I've thought this was just a shiny new return type to indicate more specifically the "result" of "drawing" with PaintSVG, without the possibility of all sorts of nsresults being returned. Now, many months after this change was made, I discover from tnikkel in bug 1345853 comment 5 that this return value is not in fact anything to do with the success or failure of PaintSVG, but actually passes out a very specific piece of information about the success or failure of any *imagelib* drawing that may or may *not* have occurred under the various PaintSVG calls. It seems to me to be quite inappropriate to hijack a function's return value to indicate success or failure of of a small piece of what might happen under the function rather than success or failure of the function as a whole. I'm sure I won't be the only person who will be confused by this, and that as the code changes people will unwittingly return values that they intend to indicate the success/failure of the various PaintSVG methods as a whole. If we need to pass out imagelib information I think that we should be using an out-param here. Call it "aOutImgDrawResult" or something. We should also clearly document why this parameter is needed, and what it is for, in the big comment documenting nsSVGDisplayableFrame::PaintSVG (the original change to return DrawResult didn't add any documentation, let alone mention the above subtleties).
|
Reporter | |
Comment 1•7 years ago
|
||
And I don't think nsSVGDisplayContainerFrame::PaintSVG should stop painting its children if an image fails to render while painting one of them. The rest of the SVG should still paint.
|
||
Comment 2•7 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #1) > And I don't think nsSVGDisplayContainerFrame::PaintSVG should stop painting > its children if an image fails to render while painting one of them. The > rest of the SVG should still paint. Correct. The DrawResult is used to make sure sync decoding in reftests works properly.
|
|
||
Comment 3•7 years ago
|
||
An In/Out parameter passed by reference might make sense then a pure out param since the pattern for DrawResult usage is generally:
DrawResult result = PaintCall();
result &= PaintCall2();
result &= PaintCall3();
where we "update" the DrawResult as we go.The scope of this change, report drawing result by an in/out parameter, may not only include PaintSVG. In nsDisplayList, we use return value to report img lib's drawing result in many paint calls, such as nsDisplayBackgroundImage::Paint. Even inside SVG module, I used return value for reporting at many place, such as: mozilla::Pair<DrawResult, RefPtr<SourceSurface>> GetMaskForMaskedFrame(MaskParams& aParams); I think we should have a unite mechanism for reporting drawing result.
How about wrap DrawResult and imageFlags into a structure, set it up in nsDisplayXXXX::Paint, pass it from PaintSVG(or any other draw calls) to the decoders in imgLib. In this path, all function just pass the object without changing it. Only the functions at both ends read and write this object.
|
|
Reporter | |
Comment 6•7 years ago
|
||
(In reply to C.J. Ku[:cjku](UTC+8) from comment #4) > The scope of this change, report drawing result by an in/out parameter, may > not only include PaintSVG. > In nsDisplayList, we use return value to report img lib's drawing result in > many paint calls, such as nsDisplayBackgroundImage::Paint. Even inside SVG > module, I used return value for reporting at many place, such as: > mozilla::Pair<DrawResult, RefPtr<SourceSurface>> > GetMaskForMaskedFrame(MaskParams& aParams); Yeah, it looks like these places need fixed too. :( (In reply to C.J. Ku[:cjku](UTC+8) from comment #5) > How about wrap DrawResult and imageFlags into a structure, set it up in > nsDisplayXXXX::Paint, pass it from PaintSVG(or any other draw calls) to the > decoders in imgLib. In this path, all function just pass the object without > changing it. Only the functions at both ends read and write this object. As long as you're passing it around an an in/out parameter, that sounds good to me. Check with tnikkel as well though. He also probably has an opinion on naming of that class.
|
|
||
Comment 7•7 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #6) > (In reply to C.J. Ku[:cjku](UTC+8) from comment #4) > > The scope of this change, report drawing result by an in/out parameter, may > > not only include PaintSVG. > > In nsDisplayList, we use return value to report img lib's drawing result in > > many paint calls, such as nsDisplayBackgroundImage::Paint. Even inside SVG > > module, I used return value for reporting at many place, such as: > > mozilla::Pair<DrawResult, RefPtr<SourceSurface>> > > GetMaskForMaskedFrame(MaskParams& aParams); > > Yeah, it looks like these places need fixed too. :( I don't know if the uses outside of SVG are really a problem. They all tend to be functions that have one specific job: to draw an image. So returning a DrawResult for that image makes sense.
(In reply to Timothy Nikkel (:tnikkel) from comment #7) > (In reply to Jonathan Watt [:jwatt] from comment #6) > > (In reply to C.J. Ku[:cjku](UTC+8) from comment #4) > > > The scope of this change, report drawing result by an in/out parameter, may > > > not only include PaintSVG. > > > In nsDisplayList, we use return value to report img lib's drawing result in > > > many paint calls, such as nsDisplayBackgroundImage::Paint. Even inside SVG > > > module, I used return value for reporting at many place, such as: > > > mozilla::Pair<DrawResult, RefPtr<SourceSurface>> > > > GetMaskForMaskedFrame(MaskParams& aParams); > > > > Yeah, it looks like these places need fixed too. :( > > I don't know if the uses outside of SVG are really a problem. They all tend > to be functions that have one specific job: to draw an image. So returning a > DrawResult for that image makes sense. Hmm....ok, let me focus on SVG module only
|
|
Reporter | |
Comment 9•7 years ago
|
||
Awesome, thanks for taking this C.J.
> How about wrap DrawResult and imageFlags into a structure
I think it still makes sense to have such a structure, even if just for the SVG case, and that it could live in DrawResult.h unless Timothy says otherwise.|
|
||
Comment 10•7 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #9) > Awesome, thanks for taking this C.J. > > > How about wrap DrawResult and imageFlags into a structure > > I think it still makes sense to have such a structure, even if just for the > SVG case, and that it could live in DrawResult.h unless Timothy says > otherwise. Yeah, I think this makes sense. Although I'd like to see it in practice before we start using it everywhere. So maybe do a prototype patch converting a small number of places and ask for my feedback?
|
Assignee | |
Comment 11•7 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #10) > (In reply to Jonathan Watt [:jwatt] from comment #9) > > Awesome, thanks for taking this C.J. > > > > > How about wrap DrawResult and imageFlags into a structure > > > > I think it still makes sense to have such a structure, even if just for the > > SVG case, and that it could live in DrawResult.h unless Timothy says > > otherwise. > > Yeah, I think this makes sense. Although I'd like to see it in practice > before we start using it everywhere. So maybe do a prototype patch > converting a small number of places and ask for my feedback? Yes, that's exactly what I am going to do.
|
|
Assignee | |
Comment 12•7 years ago
|
||
Attachment #8859099 -
Flags: feedback?
|
|
Assignee | |
Comment 13•7 years ago
|
||
Comment on attachment 8859099 [details] [diff] [review] WIP Review of attachment 8859099 [details] [diff] [review]: ----------------------------------------------------------------- Hi tnikkel, Please have a look on the draft. I wrap DrawResult and image-flags into DrawPacakge.
Attachment #8859099 -
Flags: feedback? → feedback?(tnikkel)
|
|
Assignee | |
Comment 14•7 years ago
|
||
Hi jwatt, Do we really need to return nsresult from PaintSVG? I don't think we know how to handle error code return from PaintSVG, except just ignore that. If that's the case, may we just return void?
Flags: needinfo?(jwatt)
|
|
Reporter | |
Comment 15•7 years ago
|
||
I think returning void is fine. As I recall we weren't previously using the nsresult.
Flags: needinfo?(jwatt)
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 17•7 years ago
|
||
Comment on attachment 8860479 [details] Bug 1351440 - Return DrawResult by an out paramenter of PaintSVG. In this version, I wrap original paramaters of PaintSVG plus DrawResult into nsSVGDisplayableFrame::PaintSVGParam. Please have a look
Attachment #8860479 -
Flags: feedback?(tnikkel)
Attachment #8860479 -
Flags: feedback?(jwatt)
|
|
Reporter | |
Comment 18•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8860479 [details] Bug 1351440 - Return DrawResult by an out paramenter of PaintSVG. https://reviewboard.mozilla.org/r/132472/#review135434 Thanks for working on this. ::: layout/svg/SVGGeometryFrame.h:103 (Diff revision 1) > const nsDisplayListSet& aLists) override; > > // SVGGeometryFrame methods > gfxMatrix GetCanvasTM(); > protected: > - // nsSVGDisplayableFrame interface: > + virtual void PaintSVG(PaintSVGParam& aParam) override; It's nice to have these comments. Any reason you're removing this one? ::: layout/svg/nsSVGDisplayableFrame.h:58 (Diff revision 1) > typedef mozilla::SVGUserUnitList SVGUserUnitList; > typedef mozilla::image::DrawResult DrawResult; > > NS_DECL_QUERYFRAME_TARGET(nsSVGDisplayableFrame) > > + struct PaintSVGParam { I'm not a fan of collecting all of a functions parameters up into a single struct. If certain parameters logically belong together it makes sense, but when the only thing they have in commen is that they need to be passed to the function I think it just hides/obfuscates what's being passed. In that case I find it can make code more difficult to read and get a handle on. The more code starts to use this pattern the more unobvious it is exactly what is being passed around, and the more time has to be spent going off and looking at what the FunctionXYZParams structs contain (and doing that every single time you need to remind yourself). Furthermore it has resulted in you creating function local references to the parameter's members. In this case I'd personally I'd rather just have something like an ImageParams struct that contains an image flags member and DrawResult member.
|
|
Reporter | |
Comment 19•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8860479 [details] Bug 1351440 - Return DrawResult by an out paramenter of PaintSVG. https://reviewboard.mozilla.org/r/132472/#review135436 ::: layout/svg/nsSVGDisplayableFrame.h:58 (Diff revision 1) > typedef mozilla::SVGUserUnitList SVGUserUnitList; > typedef mozilla::image::DrawResult DrawResult; > > NS_DECL_QUERYFRAME_TARGET(nsSVGDisplayableFrame) > > + struct PaintSVGParam { This can presumably be MOZ_STACK_CLASS.
|
|
Reporter | |
Comment 20•7 years ago
|
||
Comment on attachment 8860479 [details] Bug 1351440 - Return DrawResult by an out paramenter of PaintSVG. Marking f- for now awaiting a response to (or addressing of) the comments in comment 18.
Attachment #8860479 -
Flags: feedback?(jwatt) → feedback-
Attachment #8859099 -
Flags: feedback?(tnikkel)
| Comment hidden (mozreview-request) |
Attachment #8860479 -
Attachment is obsolete: true
Attachment #8860479 -
Flags: feedback?(tnikkel)
Attachment #8859099 -
Attachment is obsolete: true
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
Attachment #8864793 -
Flags: review?(tnikkel)
Attachment #8864793 -
Flags: review?(jwatt)
|
|
Reporter | |
Comment 24•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8864793 [details] Bug 1351440 - Part 2. Encapsulate DrawResult and imgIContainer::FLAG_* into imgDrawingParams, and pass it to PaintSVG. https://reviewboard.mozilla.org/r/136476/#review139870 ::: commit-message-0b255:3 (Diff revision 3) > +Bug 1351440 - Encapsulate DrawResult and imgIContainer::FLAG_* into DrawPackage, and pass it to PaintSVG. > + > +The return value of PaintSVG is not in fact anything to do with the success or s/The Return/The DrawResult return/ s/is not/was not/ (since this commit changes make that the old behavior) ::: commit-message-0b255:4 (Diff revision 3) > +Bug 1351440 - Encapsulate DrawResult and imgIContainer::FLAG_* into DrawPackage, and pass it to PaintSVG. > + > +The return value of PaintSVG is not in fact anything to do with the success or > +failure of it, but actually passes out a very specific piece of information s/failure of it/failure of that method/ s/but actually passes/but was actually passing/ ::: commit-message-0b255:8 (Diff revision 3) > +The return value of PaintSVG is not in fact anything to do with the success or > +failure of it, but actually passes out a very specific piece of information > +about the success or failure of any imagelib drawing that may not have occurred > +under the various PaintSVG calls. > + > +In this patch, I changed the signature of PaintSVG from s/I changed the signature of PaintSVG/the signature of PaintSVG is changed/ ::: gfx/thebes/gfxFont.cpp:2225 (Diff revision 3) > > - bool rv = GetFontEntry()->RenderSVGGlyph(aContext, aGlyphId, > + GetFontEntry()->RenderSVGGlyph(aContext, aGlyphId, aContextPaint); > - aContextPaint); > aContext->Restore(); > aContext->NewPath(); > - return rv; > + return true; Yes, this looks correct to me. Previously we could return false if the referenced SVG element didn't have an nsIFrame, or hypothetically if imagelib drawing happened under this call and failed. Neither should be a reason for our caller (gfxFont::DrawOneGlyph) to go on and try to draw a different way, such as with RenderColorGlyph. It should only do that if the GetFontEntry()->HasSVGGlyph(aGlyphId) call at the begining of this method here were to fail (which it still will). ::: gfx/thebes/gfxSVGGlyphs.cpp:222 (Diff revision 3) > SVGContextPaint* aContextPaint) > { > gfxContextAutoSaveRestore aContextRestorer(aContext); > > Element *glyph = mGlyphIdMap.Get(aGlyphId); > NS_ASSERTION(glyph, "No glyph element. Should check with HasSVGGlyph() first!"); Can you change this to a MOZ_ASSERT, please? ::: image/DrawResult.h:87 (Diff revision 3) > { > aLeft = aLeft & aRight; > return aLeft; > } > > +struct DrawPackage { The term "package" in software is usually used to refer to a collection of files or something similar, so I'm not keen on this term. How about ImgDrawParams or just DrawParams? (Parameers can be both input and output, as they are here.) Before you go to the effort of doing such a rename let's get agreement with Timothy though in case he prefers something else. Also please add a Doxygen compatible comment like: /** * A struct used during painting to provide input flags to determine how * imagelib draw calls should behave, plus an output DrawResult to return * information about the result of any imagelib draw calls that may have * occurred. */ Also we generally put members at the end of structs/classes, and I think it would make sense to put the flags before the result (since that's the order in which they are used - input then output - it's generally "nice" to have them in that order). ::: layout/painting/nsDisplayList.cpp:8579 (Diff revision 3) > nsSVGIntegrationUtils::PaintFramesParams params(*aCtx->ThebesContext(), > mFrame, mVisibleRect, > borderArea, aBuilder, > aManager, > mHandleOpacity, flags); > - > + mozilla::image::DrawPackage package(flags); There's a |using namespace mozilla;| at the top of this file, so at the very least you don't need the 'mozilla::' qualification here. ::: layout/svg/SVGTextFrame.cpp:3133 (Diff revision 3) > gfxMatrix tm = nsSVGUtils::GetCSSPxToDevPxMatrix(mFrame) * > gfxMatrix::Translation(devPixelOffset); > > gfxContext* ctx = aCtx->ThebesContext(); > ctx->Save(); > - DrawResult result = static_cast<SVGTextFrame*>(mFrame)->PaintSVG(*ctx, tm); > + mozilla::image::DrawPackage package(aBuilder->ShouldSyncDecodeImages() Drop the mozilla:: qualification. ::: layout/svg/nsFilterInstance.h:88 (Diff revision 3) > * @param aDirtyArea The area than needs to be painted, in aFilteredFrame's > * frame space (i.e. relative to its origin, the top-left corner of its > * border box). > */ > - static DrawResult PaintFilteredFrame(nsIFrame *aFilteredFrame, > + static void PaintFilteredFrame(nsIFrame *aFilteredFrame, > - DrawTarget* aDrawTarget, > + DrawTarget* aDrawTarget, The indentation seems slightly wrong here. ::: layout/svg/nsFilterInstance.cpp:388 (Diff revision 3) > > RefPtr<DrawTarget> offscreenDT = > gfxPlatform::GetPlatform()->CreateOffscreenContentDrawTarget( > neededRect.Size(), SurfaceFormat::B8G8R8A8); > if (!offscreenDT || !offscreenDT->IsValid()) { > - return DrawResult::TEMPORARY_ERROR; > + return; Good to see mistakes like this being fixed. ::: layout/svg/nsFilterInstance.cpp:498 (Diff revision 3) > aDrawTarget->SetTransform(newTM); > > ComputeNeededBoxes(); > > - DrawResult result = BuildSourceImage(); > - if (result != DrawResult::SUCCESS){ > + BuildSourceImage(aPackage); > + if (aPackage.result != DrawResult::SUCCESS){ Should we be returning here? I thought non-DrawResult::SUCCESS codes were not supposed to abort painting. ::: layout/svg/nsFilterInstance.cpp:502 (Diff revision 3) > - DrawResult result = BuildSourceImage(); > - if (result != DrawResult::SUCCESS){ > - return result; > + BuildSourceImage(aPackage); > + if (aPackage.result != DrawResult::SUCCESS){ > + return; > } > - result = BuildSourcePaints(); > - if (result != DrawResult::SUCCESS){ > + BuildSourcePaints(aPackage); > + if (aPackage.result != DrawResult::SUCCESS){ Ditto. ::: layout/svg/nsSVGClipPathFrame.cpp:246 (Diff revision 3) > static_cast<const nsSVGElement*>(childContent)-> > PrependLocalTransformsTo(mMatrixForChildren, eUserSpaceToParent); > } > > + // XXX cku: we should pass DrawPackage from the caller to this function > + // and change ther return type to void. Actually I think you can change this comment to: // clipPath does not result in any image rendering, so we just use a dummy // DrawPackage instead of requiring our caller to pass one. ::: layout/svg/nsSVGClipPathFrame.cpp:261 (Diff revision 3) > aTarget.PopGroupAndBlend(); > } else if (maskUsage.shouldApplyClipPath) { > aTarget.PopClip(); > } > > - return result; > + return package.result; And therefore I think you can and should make this function's return type 'void'. ::: layout/svg/nsSVGFilterPaintCallback.h:30 (Diff revision 3) > * system. > * @param aDirtyRect the dirty rect *in user space pixels* > * @param aTransformRoot the outermost frame whose transform should be taken > * into account when painting an SVG glyph > */ > - virtual DrawResult Paint(gfxContext& aContext, nsIFrame *aTarget, > + virtual void Paint(gfxContext& aContext, nsIFrame *aTarget, Please add an @param line to the comment with a comment like "imagelib parameters that may be used when painting feImage" to make it clear why we might be passing imagelib params to this function. ::: layout/svg/nsSVGForeignObjectFrame.cpp:280 (Diff revision 3) > PaintFrameFlags flags = PaintFrameFlags::PAINT_IN_TRANSFORM; > if (SVGAutoRenderState::IsPaintingToWindow(aContext.GetDrawTarget())) { > flags |= PaintFrameFlags::PAINT_TO_WINDOW; > } > + if (aPackage.imageFlags & imgIContainer::FLAG_SYNC_DECODE) { > + flags |= PaintFrameFlags::PAINT_SYNC_DECODE_IMAGES; I'm a bit uncomfortable that we're hardcoding this one flag as special for passing on. Should we have an assertion here that imageFlags doesn't contain any other flags that don't currently exist at this time? (In case new flags that may be added in future should be passed on here.) ::: layout/svg/nsSVGImageFrame.cpp:335 (Diff revision 3) > const gfxMatrix& aTransform, > - const nsIntRect *aDirtyRect, > - uint32_t aFlags) > + DrawPackage& aPackage, > + const nsIntRect *aDirtyRect) > { > if (!StyleVisibility()->IsVisible()) > - return DrawResult::SUCCESS; > + return; Can you add curly braces while you're here? ::: layout/svg/nsSVGImageFrame.cpp:414 (Diff revision 3) > appUnitsPerDevPx)); > > // Note: Can't use DrawSingleUnscaledImage for the TYPE_VECTOR case. > // That method needs our image to have a fixed native width & height, > // and that's not always true for TYPE_VECTOR images. > - result = nsLayoutUtils::DrawSingleImage( > + aPackage.result = nsLayoutUtils::DrawSingleImage( This looks like we're stamping over any values that may previously have been stored in aPackage.result. That looks like a bug to me. ::: layout/svg/nsSVGImageFrame.cpp:424 (Diff revision 3) > destRect, > aDirtyRect ? dirtyRect : destRect, > context, > - aFlags); > + aPackage.imageFlags); > } else { // mImageContainer->GetType() == TYPE_RASTER > - result = nsLayoutUtils::DrawSingleUnscaledImage( > + aPackage.result = nsLayoutUtils::DrawSingleUnscaledImage( Ditto. ::: layout/svg/nsSVGMaskFrame.cpp:330 (Diff revision 3) > return MakePair(DrawResult::SUCCESS, RefPtr<SourceSurface>()); > } > > *aParams.maskTransform = ToMatrix(maskSurfaceMatrix); > RefPtr<SourceSurface> surface = destMaskSurface.forget(); > - return MakePair(result, Move(surface)); > + return MakePair(package.result, Move(surface)); It seems like nsSVGMaskFrame::GetMaskForMaskedFrame should be returning the imagelib DrawResult as an out-param too. If you're not going to fix that in this bug, can you file a separate bug and note the bug number in a comment here? ::: layout/svg/nsSVGOuterSVGFrame.cpp:628 (Diff revision 3) > > gfxPoint devPixelOffset = > nsLayoutUtils::PointToGfxPoint(viewportRect.TopLeft(), appUnitsPerDevPixel); > > aContext->ThebesContext()->Save(); > - uint32_t flags = aBuilder->ShouldSyncDecodeImages() > + mozilla::image::DrawPackage package(aBuilder->ShouldSyncDecodeImages() Drop the mozilla:: qualification. ::: layout/svg/nsSVGPatternFrame.cpp:379 (Diff revision 3) > patternWithChildren->mSource = static_cast<SVGGeometryFrame*>(aSource); > } > > // Delay checking NS_FRAME_DRAWING_AS_PAINTSERVER bit until here so we can > // give back a clear surface if there's a loop > - DrawResult result = DrawResult::SUCCESS; > + mozilla::image::DrawPackage package(aFlags); Drop the mozilla:: qualification. ::: layout/svg/nsSVGPatternFrame.cpp:408 (Diff revision 3) > ctx->Restore(); > } > > // caller now owns the surface > RefPtr<SourceSurface> surf = dt->Snapshot(); > - return MakePair(result, Move(surf)); > + return MakePair(package.result, Move(surf)); Again, if you're not going to make the DrawResult an out-param in this bug, please file another bug and note the bug number here. ::: layout/svg/nsSVGUtils.cpp:774 (Diff revision 3) > MixModeBlender blender(aFrame, &aContext); > gfxContext* target = blender.ShouldCreateDrawTargetForBlend() > ? blender.CreateBlendTarget(aTransform) : &aContext; > > if (!target) { > - return DrawResult::TEMPORARY_ERROR; > + aPackage.result = DrawResult::TEMPORARY_ERROR;; Seems like a bug to change aPackage here. ::: layout/svg/nsSVGUtils.cpp:1888 (Diff revision 3) > // SVG user space, so we need to account for any 'transform' attribute: > m = static_cast<nsSVGElement*>(frame->GetContent())-> > PrependLocalTransformsTo(gfxMatrix(), eUserSpaceToParent); > } > - DrawResult result = svgFrame->PaintSVG(*aContext, m); > - return (result == DrawResult::SUCCESS); > + > + // XXX cku: Bug 1358555 The caller should pass DrawPackage to Seems like SVG-in-OpenType should not be allowed to paint exteral resources. If that's the case, is it possible for SVG-in-OT glyphs to return DrawResult failure codes?
Attachment #8864793 -
Flags: review?(jwatt)
|
|
Reporter | |
Updated•7 years ago
|
Attachment #8864793 -
Flags: feedback+
|
|
Assignee | |
Comment 25•7 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8864793 [details] Bug 1351440 - Part 2. Encapsulate DrawResult and imgIContainer::FLAG_* into imgDrawingParams, and pass it to PaintSVG. https://reviewboard.mozilla.org/r/136476/#review139870 > Should we be returning here? I thought non-DrawResult::SUCCESS codes were not supposed to abort painting. No, we should not. Will fix it.
|
|
Assignee | |
Comment 26•7 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8864793 [details] Bug 1351440 - Part 2. Encapsulate DrawResult and imgIContainer::FLAG_* into imgDrawingParams, and pass it to PaintSVG. https://reviewboard.mozilla.org/r/136476/#review139870 > It seems like nsSVGMaskFrame::GetMaskForMaskedFrame should be returning the imagelib DrawResult as an out-param too. If you're not going to fix that in this bug, can you file a separate bug and note the bug number in a comment here? I will fix it in this bug, but in a separate patch.
|
|
Assignee | |
Comment 27•7 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8864793 [details] Bug 1351440 - Part 2. Encapsulate DrawResult and imgIContainer::FLAG_* into imgDrawingParams, and pass it to PaintSVG. https://reviewboard.mozilla.org/r/136476/#review139870 > I'm a bit uncomfortable that we're hardcoding this one flag as special for passing on. Should we have an assertion here that imageFlags doesn't contain any other flags that don't currently exist at this time? (In case new flags that may be added in future should be passed on here.) We only handle FLAG_SYNC_DECODE, since it's the only flag that nsLayoutUtils::PaintFrame care about. Inside nsLayoutUtils::PaintFrame, it will 1. Set sync-decode flag of the builder by calling builder.SetSyncDecodeImages(true) 2. Creating a display list. In the Paint call of each display item will generate a new imgIContainer flags depends on the context of the builder. We do not need to pass any other flags, such as imgIContainer::FLAG_HIGH_QUALITY_SCALING, to PaintFrame since it does not care. PS: We actaully may pass imgIContainer::FLAG_SYNC_DECODE or imgIContainer::FLAG_SYNC_DECODE_IF_FAST. FLAG_SYNC_DECODE_IF_FAST can be discard here.
|
|
Assignee | |
Comment 28•7 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8864793 [details] Bug 1351440 - Part 2. Encapsulate DrawResult and imgIContainer::FLAG_* into imgDrawingParams, and pass it to PaintSVG. https://reviewboard.mozilla.org/r/136476/#review139870 > The term "package" in software is usually used to refer to a collection of files or something similar, so I'm not keen on this term. How about ImgDrawParams or just DrawParams? (Parameers can be both input and output, as they are here.) > > Before you go to the effort of doing such a rename let's get agreement with Timothy though in case he prefers something else. > > Also please add a Doxygen compatible comment like: > > /** > * A struct used during painting to provide input flags to determine how > * imagelib draw calls should behave, plus an output DrawResult to return > * information about the result of any imagelib draw calls that may have > * occurred. > */ > > Also we generally put members at the end of structs/classes, and I think it would make sense to put the flags before the result (since that's the order in which they are used - input then output - it's generally "nice" to have them in that order). DrawParams sounds good for me. Let's wait for tn's opinion first.
|
|
Assignee | |
Comment 29•7 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8864793 [details] Bug 1351440 - Part 2. Encapsulate DrawResult and imgIContainer::FLAG_* into imgDrawingParams, and pass it to PaintSVG. https://reviewboard.mozilla.org/r/136476/#review139870 > Seems like SVG-in-OpenType should not be allowed to paint exteral resources. If that's the case, is it possible for SVG-in-OT glyphs to return DrawResult failure codes? I think you are right, but I am not really fimiliar with these code. Let's remove this comment first. I will look into it deeper in bug 1358555.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 34•7 years ago
|
||
GetPaintServerPattern is another case returns Pair<DrawResult, ..> in SVG module. I think we may fix it in one more patch.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 37•7 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #24) > The term "package" in software is usually used to refer to a collection of > files or something similar, so I'm not keen on this term. How about > ImgDrawParams or just DrawParams? (Parameers can be both input and output, > as they are here.) I talked to jwatt on irc about this. We prefer imgDrawingParams (with a lowercase i).
|
|
Reporter | |
Comment 38•7 years ago
|
||
There are also other places that "package" is used, such as in the aPackage argument name. I think aImgParams would be a good replacement (this argument is for paint methods, so not mentioning "paint" in the argument name should be fine).
|
|
Reporter | |
Comment 39•7 years ago
|
||
And for anyone doing bug archeology in the future, tnikkel and I preferred lowercase "img" as the prefix for the class name because this is consistent with all other imagelib classes that have an "img" prefix.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
Attachment #8865770 -
Flags: review?(tnikkel)
Attachment #8865770 -
Flags: review?(jwatt)
Attachment #8864793 -
Flags: review?(jwatt)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Reporter | |
Comment 54•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8865770 [details] Bug 1351440 - Part 1. Implement imgDrawingParams. https://reviewboard.mozilla.org/r/137392/#review140466 ::: image/DrawResult.h:87 (Diff revision 1) > { > aLeft = aLeft & aRight; > return aLeft; > } > > +/** A struct used during painting to provide input flags to determine how Generally in these Doxygen comments the text starts on the line after the opening "/\*\*" so it would be good if you could do that too. Also, on reflection, can you change ", plus" to just "and". ::: image/DrawResult.h:94 (Diff revision 1) > + * information about the result of any imagelib draw calls that may have > + * occurred. > + */ > +struct imgDrawingParams { > + imgDrawingParams(uint32_t aImageFlags = 0) > + : imageFlags(aImageFlags), result(DrawResult::SUCCESS) {} Please follow the style of the constructor at: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes In other words, make this: imgDrawingParams(uint32_t aImageFlags = 0) : imageFlags(aImageFlags) , result(DrawResult::SUCCESS) {} ::: image/DrawResult.h:98 (Diff revision 1) > + imgDrawingParams(uint32_t aImageFlags = 0) > + : imageFlags(aImageFlags), result(DrawResult::SUCCESS) {} > + > + const uint32_t imageFlags; // Pass image flags of the > + // imgIContainer::FLAG_* variety to the draw > + // calls in image lib. Make this comment "imgIContainer::FLAG_* image flags to pass to image lib draw calls". ::: image/DrawResult.h:99 (Diff revision 1) > + : imageFlags(aImageFlags), result(DrawResult::SUCCESS) {} > + > + const uint32_t imageFlags; // Pass image flags of the > + // imgIContainer::FLAG_* variety to the draw > + // calls in image lib. > + DrawResult result; // Return painting result from image library. And this "To return results from image lib painting"
Attachment #8865770 -
Flags: review?(jwatt) → review+
|
|
Reporter | |
Comment 55•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8864793 [details] Bug 1351440 - Part 2. Encapsulate DrawResult and imgIContainer::FLAG_* into imgDrawingParams, and pass it to PaintSVG. https://reviewboard.mozilla.org/r/136476/#review140470 Awesome! ::: layout/painting/nsDisplayList.cpp:8708 (Diff revision 8) > nsSVGIntegrationUtils::PaintFramesParams params(*aCtx->ThebesContext(), > mFrame, mVisibleRect, > borderArea, aBuilder, > aManager, > mHandleOpacity, flags); > - > + image::imgDrawingParams imgParams(flags); We should avoid the image:: qualification where possible, by using class typedefs or adding: using mozilla::image::imgDrawParams; to source files. ::: layout/svg/SVGTextFrame.cpp:3134 (Diff revision 8) > gfxMatrix tm = nsSVGUtils::GetCSSPxToDevPxMatrix(mFrame) * > gfxMatrix::Translation(devPixelOffset); > > gfxContext* ctx = aCtx->ThebesContext(); > ctx->Save(); > - DrawResult result = static_cast<SVGTextFrame*>(mFrame)->PaintSVG(*ctx, tm); > + image::imgDrawingParams imgParams(aBuilder->ShouldSyncDecodeImages() Avoid image:: qualification. ::: layout/svg/nsSVGClipPathFrame.cpp:247 (Diff revision 8) > PrependLocalTransformsTo(mMatrixForChildren, eUserSpaceToParent); > } > > + // clipPath does not result in any image rendering, so we just use a dummy > + // imgDrawingParams instead of requiring our caller to pass one. > + image::imgDrawingParams imgParams; Avoid image:: qualification. ::: layout/svg/nsSVGClipPathFrame.cpp:248 (Diff revision 8) > } > > + // clipPath does not result in any image rendering, so we just use a dummy > + // imgDrawingParams instead of requiring our caller to pass one. > + image::imgDrawingParams imgParams; > + imgParams.result = result; It doesn't look like there's any reason to set .result on the dummy variable. ::: layout/svg/nsSVGMaskFrame.cpp:256 (Diff revision 8) > MOZ_ASSERT(tmpCtx); // already checked the draw target above > tmpCtx->SetMatrix(maskSurfaceMatrix); > > mMatrixForChildren = GetMaskTransform(aParams.maskedFrame) * > aParams.toUserSpace; > - DrawResult result = DrawResult::SUCCESS; > + mozilla::image::imgDrawingParams imgParams(aParams.flags); Avoid image:: qualification. ::: layout/svg/nsSVGOuterSVGFrame.cpp:628 (Diff revision 8) > > gfxPoint devPixelOffset = > nsLayoutUtils::PointToGfxPoint(viewportRect.TopLeft(), appUnitsPerDevPixel); > > aContext->ThebesContext()->Save(); > - uint32_t flags = aBuilder->ShouldSyncDecodeImages() > + image::imgDrawingParams imgParams(aBuilder->ShouldSyncDecodeImages() Avoid image:: qualification. ::: layout/svg/nsSVGPatternFrame.cpp:379 (Diff revision 8) > patternWithChildren->mSource = static_cast<SVGGeometryFrame*>(aSource); > } > > // Delay checking NS_FRAME_DRAWING_AS_PAINTSERVER bit until here so we can > // give back a clear surface if there's a loop > - DrawResult result = DrawResult::SUCCESS; > + image::imgDrawingParams imgParams(aFlags); Avoid image:: qualification. ::: layout/svg/nsSVGUtils.cpp:1887 (Diff revision 8) > // SVG user space, so we need to account for any 'transform' attribute: > m = static_cast<nsSVGElement*>(frame->GetContent())-> > PrependLocalTransformsTo(gfxMatrix(), eUserSpaceToParent); > } > - DrawResult result = svgFrame->PaintSVG(*aContext, m); > - return (result == DrawResult::SUCCESS); > + > + // SVG-in-OpenType is not be allowed to paint exteral resources, we can s/is not be/is not/ s/we can/so we can/ ::: layout/svg/nsSVGUtils.cpp:1888 (Diff revision 8) > m = static_cast<nsSVGElement*>(frame->GetContent())-> > PrependLocalTransformsTo(gfxMatrix(), eUserSpaceToParent); > } > - DrawResult result = svgFrame->PaintSVG(*aContext, m); > - return (result == DrawResult::SUCCESS); > + > + // SVG-in-OpenType is not be allowed to paint exteral resources, we can > + // just pass a dummy params into PatinSVG. Fix spelling of PaintSVG. Also I'm not sure if SVG-in-OT is allowed to render data: images or not. Did you check that?
Attachment #8864793 -
Flags: review?(jwatt) → review+
|
|
Reporter | |
Comment 56•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8865460 [details] Bug 1351440 - Part 3. Pass imgDrawingParams to nsSVGMaskFrame::GetMaskForMaskedFrame. https://reviewboard.mozilla.org/r/137114/#review140492 This generally looks good, but I'd like to review it again after you've addressed the comments below. ::: commit-message-4b09c:3 (Diff revision 8) > +Bug 1351440 - Part 3. Pass imgDrawingParams to nsSVGMaskFrame::GetMaskForMaskedFrame. > + > +The reason of this change is the same with Part 1. The only difference is we s/same with/same as for/ s/Part 1/Part 2/ And make the second sentence ", except that this commit fixes nsSVGMaskFrame::GetMaskForMaskedFrame rather than PaintSVG." ::: layout/painting/nsDisplayList.cpp:8395 (Diff revision 8) > nsDisplayMask::PaintMask(nsDisplayListBuilder* aBuilder, > gfxContext* aMaskContext) > { > MOZ_ASSERT(aMaskContext->GetDrawTarget()->GetFormat() == SurfaceFormat::A8); > > - uint32_t flags = aBuilder->ShouldSyncDecodeImages() > + image::imgDrawingParams imgParmas(aBuilder->ShouldSyncDecodeImages() Avoid image:: qualification. ::: layout/painting/nsDisplayList.cpp:8512 (Diff revision 8) > gfxContext* context = aCtx->ThebesContext(); > context->Clip(NSRectToSnappedRect(mVisibleRect, > mFrame->PresContext()->AppUnitsPerDevPixel(), > *aCtx->GetDrawTarget())); > > + image::imgDrawingParams imgParams(aBuilder->ShouldSyncDecodeImages() Avoid image:: qualification. ::: layout/painting/nsDisplayList.cpp:8697 (Diff revision 8) > void > nsDisplayFilter::PaintAsLayer(nsDisplayListBuilder* aBuilder, > nsRenderingContext* aCtx, > LayerManager* aManager) > { > - uint32_t flags = aBuilder->ShouldSyncDecodeImages() > + image::imgDrawingParams imgParams(aBuilder->ShouldSyncDecodeImages() Avoid image:: qualification. ::: layout/svg/nsSVGClipPathFrame.h:86 (Diff revision 8) > * aExtraMask. Should be passed when aExtraMask is passed. > */ > - mozilla::Pair<DrawResult, RefPtr<SourceSurface>> > + already_AddRefed<SourceSurface> > GetClipMask(gfxContext& aReferenceContext, nsIFrame* aClippedFrame, > const gfxMatrix& aMatrix, Matrix* aMaskTransform, > + imgDrawingParams& aImgParams, nsSVGClipPathFrame is different to nsSVGMaskFrame. As per my comments for part 2, clipPath doesn't use any <image> elements that it may contain. Therefore we don't need to pass imgDrawingParams to it's methods. Please remove this parameter and use a dummy imgDrawingParams in the clipPath methods that need one to call into generic painting code. ::: layout/svg/nsSVGClipPathFrame.h:110 (Diff revision 8) > * aExtraMask. Should be passed when aExtraMask is passed. > */ > - DrawResult > + void > PaintClipMask(gfxContext& aMaskContext, nsIFrame* aClippedFrame, > const gfxMatrix& aMatrix, Matrix* aMaskTransform, > - SourceSurface* aExtraMask, const Matrix& aExtraMasksTransform); > + imgDrawingParams& aImgParams, SourceSurface* aExtraMask, Same here. ::: layout/svg/nsSVGClipPathFrame.h:162 (Diff revision 8) > already_AddRefed<DrawTarget> CreateClipMask(gfxContext& aReferenceContext, > mozilla::gfx::IntPoint& aOffset); > > void PaintFrameIntoMask(nsIFrame *aFrame, nsIFrame* aClippedFrame, > - gfxContext& aTarget, const gfxMatrix& aMatrix); > + gfxContext& aTarget, const gfxMatrix& aMatrix, > + imgDrawingParams& aImgParams); And here. ::: layout/svg/nsSVGIntegrationUtils.h:136 (Diff revision 8) > * @param aPt in appunits relative to aFrame > */ > static bool > HitTestFrameForEffects(nsIFrame* aFrame, const nsPoint& aPt); > > struct PaintFramesParams { Can you make this MOZ_STACK_CLASS while you're here? ::: layout/svg/nsSVGIntegrationUtils.cpp:481 (Diff revision 8) > - DrawResult result; > bool transparentBlackMask; > bool opacityApplied; > > MaskPaintResult() > - : result(DrawResult::SUCCESS), transparentBlackMask(false), > + : transparentBlackMask(false), opacityApplied(false) Each member on a separate line, please. ::: layout/svg/nsSVGMaskFrame.h:74 (Diff revision 8) > opacity(aOpacity), maskTransform(aMaskTransform), maskMode(aMaskMode), > - flags(aFlags) > + imgParams(aImgParams) > { } > }; > > // nsSVGMaskFrame method: Can you add a line break after this first comment line. ::: layout/svg/nsSVGMaskFrame.h:77 (Diff revision 8) > }; > > // nsSVGMaskFrame method: > - mozilla::Pair<DrawResult, RefPtr<SourceSurface>> > + // The return surface can be null, it's the caller's responsibility to > + // null-check before dereferencing. > + already_AddRefed<SourceSurface> And then use a Doxygen style comment for the comment that actually documents this method.
Attachment #8865460 -
Flags: review?(jwatt) → review-
|
|
Reporter | |
Comment 57•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8865561 [details] Bug 1351440 - Part 4. Pass imgDrawingParams to nsSVGPaintServerFrame::GetPaintServerPattern. https://reviewboard.mozilla.org/r/137176/#review140546 Lovely! ::: commit-message-3de98:3 (Diff revision 5) > +Bug 1351440 - Part 4. Pass imgDrawingParams to nsSVGPaintServerFrame::GetPaintServerPattern. > + > +The reason of this change is the same with Part 1. The only difference is we Fix this up in the same way as I requested for part 3. ::: gfx/thebes/gfxFont.cpp:1644 (Diff revision 5) > if (state.pattern || mFontParams.contextPaint) { > Pattern *pat; > > RefPtr<gfxPattern> fillPattern; > if (mFontParams.contextPaint) { > - mozilla::image::DrawResult result = mozilla::image::DrawResult::SUCCESS; > + mozilla::image::imgDrawingParams imgParams; Avoid the image:: qualification. And maybe call this "dummyImgParams"? ::: layout/svg/SVGContextPaint.h:61 (Diff revision 5) > : mDashOffset(0.0f) > , mStrokeWidth(0.0f) > {} > > public: > typedef image::DrawResult DrawResult; I think you can remove this line now. (Or move it next to the others.) ::: layout/svg/SVGContextPaint.h:156 (Diff revision 5) > { > protected: > typedef mozilla::gfx::DrawTarget DrawTarget; > > public: > typedef mozilla::image::DrawResult DrawResult; Again, is this needed any more? ::: layout/svg/SVGContextPaint.cpp:39 (Diff revision 5) > float& aOpacity, > SVGContextPaint* aOuterContextPaint, > SVGContextPaintImpl::Paint& aTargetPaint, > nsStyleSVGPaint nsStyleSVG::*aFillOrStroke, > - nsSVGEffects::PaintingPropertyDescriptor aProperty) > + nsSVGEffects::PaintingPropertyDescriptor aProperty, > + image::imgDrawingParams& aImgParams) Avoid the image:: qualification. ::: layout/svg/SVGContextPaint.cpp:314 (Diff revision 5) > // The gfxPattern that we create below depends on aFillOpacity, and since > // different elements in the SVG image may pass in different values for > // fill opacities we don't try to cache the gfxPattern that we create. > Color fill = *mFill; > fill.a *= aFillOpacity; > RefPtr<gfxPattern> patern = new gfxPattern(fill); use: return do_AddRef(new gfxPattern(fill)); ::: layout/svg/SVGContextPaint.cpp:329 (Diff revision 5) > if (!mStroke) { > - return MakePair(DrawResult::SUCCESS, RefPtr<gfxPattern>()); > + return nullptr; > } > Color stroke = *mStroke; > stroke.a *= aStrokeOpacity; > RefPtr<gfxPattern> patern = new gfxPattern(stroke); Use do_Addref() ::: layout/svg/nsSVGGradientFrame.cpp:255 (Diff revision 5) > uint32_t nStops = stopFrames.Length(); > > // SVG specification says that no stops should be treated like > // the corresponding fill or stroke had "none" specified. > if (nStops == 0) { > RefPtr<gfxPattern> pattern = new gfxPattern(Color()); Use do_Addref() ::: layout/svg/nsSVGGradientFrame.cpp:268 (Diff revision 5) > nscolor stopColor = stopFrames[nStops-1]->StyleSVGReset()->mStopColor; > > Color stopColor2 = Color::FromABGR(stopColor); > stopColor2.a *= stopOpacity * aGraphicOpacity; > RefPtr<gfxPattern> pattern = new gfxPattern(stopColor2); > - return MakePair(DrawResult::SUCCESS, Move(pattern)); > + return pattern.forget(); Use do_Addref() ::: layout/svg/nsSVGPaintServerFrame.h:59 (Diff revision 5) > > class nsSVGPaintServerFrame : public nsSVGContainerFrame > { > protected: > typedef mozilla::gfx::DrawTarget DrawTarget; > typedef mozilla::image::DrawResult DrawResult; Remove this. ::: layout/svg/nsSVGPaintServerFrame.h:87 (Diff revision 5) > const DrawTarget* aDrawTarget, > const gfxMatrix& aContextMatrix, > nsStyleSVGPaint nsStyleSVG::*aFillOrStroke, > float aOpacity, > - const gfxRect *aOverrideBounds = nullptr, > - uint32_t aFlags = 0) = 0; > + imgDrawingParams& aImgParams, > + const gfxRect *aOverrideBounds = nullptr) = 0; Left-align the * while you're here. ::: layout/svg/nsSVGPatternFrame.cpp:406 (Diff revision 5) > ctx->PopGroupAndBlend(); > ctx->Restore(); > } > > // caller now owns the surface > RefPtr<SourceSurface> surf = dt->Snapshot(); We can just return the return value of Snapshot() directly. ::: layout/svg/nsSVGPatternFrame.cpp:730 (Diff revision 5) > float aGraphicOpacity, > - const gfxRect *aOverrideBounds, > - uint32_t aFlags) > + imgDrawingParams& aImgParams, > + const gfxRect *aOverrideBounds) > { > if (aGraphicOpacity == 0.0f) { > RefPtr<gfxPattern> pattern = new gfxPattern(Color()); Use do_Addref()
Attachment #8865561 -
Flags: review?(jwatt) → review+
|
|
Assignee | |
Comment 58•7 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8864793 [details] Bug 1351440 - Part 2. Encapsulate DrawResult and imgIContainer::FLAG_* into imgDrawingParams, and pass it to PaintSVG. https://reviewboard.mozilla.org/r/136476/#review140470 > Fix spelling of PaintSVG. > > Also I'm not sure if SVG-in-OT is allowed to render data: images or not. Did you check that? I only roughly looked through svg opentype spec and did see image stuff... but I will double check while works on bug 1358555
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
Attachment #8865460 -
Flags: review?(jwatt)
Attachment #8866004 -
Flags: review?(jwatt)
|
|
Reporter | |
Comment 65•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8866004 [details] Bug 1351440 - Part 5. typedef DrawResult in nsDisplayItem to avoid the image:: qualification. https://reviewboard.mozilla.org/r/137600/#review140798 Since DrawResult isn't prefixed with "img" I'm a little less enthusiastic about this than removing the qualifications from imgDrawingParams, but this seems fine. Maybe one day we'll rename it to imgDrawingResult. ;)
Attachment #8866004 -
Flags: review?(jwatt) → review+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Reporter | |
Comment 72•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8865460 [details] Bug 1351440 - Part 3. Pass imgDrawingParams to nsSVGMaskFrame::GetMaskForMaskedFrame. https://reviewboard.mozilla.org/r/137114/#review143958 ::: layout/svg/nsSVGIntegrationUtils.cpp:494 (Diff revision 10) > bool transparentBlackMask; > bool opacityApplied; > > MaskPaintResult() > - : result(DrawResult::SUCCESS), transparentBlackMask(false), > + : transparentBlackMask(false), > opacityApplied(false) Since moving the comma to the beginning of the next line would affect two lines of blame and you're already touching one of these lines, it seems like you might as well move the comma under the colon while you're here.
Attachment #8865460 -
Flags: review?(jwatt) → review+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 87•7 years ago
|
||
I will land these patch first, if tn have any opinion, I will fix them in the follow-up
|
||
Comment 88•7 years ago
|
||
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0065478cc7eb Part 1. Implement imgDrawingParams. r=jwatt https://hg.mozilla.org/integration/autoland/rev/f3460d42d23c Part 2. Encapsulate DrawResult and imgIContainer::FLAG_* into imgDrawingParams, and pass it to PaintSVG. r=jwatt https://hg.mozilla.org/integration/autoland/rev/3240cd94bf4b Part 3. Pass imgDrawingParams to nsSVGMaskFrame::GetMaskForMaskedFrame. r=jwatt https://hg.mozilla.org/integration/autoland/rev/2ea1279576d2 Part 4. Pass imgDrawingParams to nsSVGPaintServerFrame::GetPaintServerPattern. r=jwatt https://hg.mozilla.org/integration/autoland/rev/3ea48d348482 Part 5. typedef DrawResult in nsDisplayItem to avoid the image:: qualification. r=jwatt
|
|
||
Comment 89•7 years ago
|
||
Backed out under suspicion for causing reftest failures on Stylo debug: https://hg.mozilla.org/integration/autoland/rev/0724e572eb406dbe14bb414e287cc29e3f603cce https://hg.mozilla.org/integration/autoland/rev/8cff37651ea2a096d5304b11bd13359f8d117174 https://hg.mozilla.org/integration/autoland/rev/95ea8787d8d7fe2dd6a464da2afd4a66c4d56804 https://hg.mozilla.org/integration/autoland/rev/77bd8b4aa3359b72e5c5b5a9b93d772bb9dbe215 https://hg.mozilla.org/integration/autoland/rev/e10e320c3cb8e27169d48f2f9dd5f8b0474a20e2 Push showing the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0f1ea61951fde4e117d6fb099863a64e78997ef8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=100166091&repo=autoland [task 2017-05-18T17:39:05.828767Z] 17:39:05 INFO - ++DOMWINDOW == 10 (0x7f2f4a869000) [pid = 1004] [serial = 10] [outer = 0x7f2f4ebe3000] [task 2017-05-18T17:39:05.844727Z] 17:39:05 INFO - ++DOMWINDOW == 11 (0x7f2f4a86c800) [pid = 1004] [serial = 11] [outer = 0x7f2f4ebe5000] [task 2017-05-18T17:39:06.042851Z] 17:39:06 INFO - [Parent 1004] WARNING: stylo: No docshell yet, assuming Gecko style system: file /home/worker/workspace/build/src/dom/base/nsDocument.cpp, line 13179 [task 2017-05-18T17:39:06.245529Z] 17:39:06 INFO - [Parent 1004] WARNING: stylo: No docshell yet, assuming Gecko style system: file /home/worker/workspace/build/src/dom/base/nsDocument.cpp, line 13179 [task 2017-05-18T17:39:06.423339Z] 17:39:06 INFO - ++DOCSHELL 0x7f4d65e94000 == 1 [pid = 1059] [id = {571b9e1b-1cb2-4874-9672-067960442853}] [task 2017-05-18T17:39:06.499798Z] 17:39:06 INFO - ++DOMWINDOW == 1 (0x7f4d565d6000) [pid = 1059] [serial = 1] [outer = (nil)] [task 2017-05-18T17:39:06.616899Z] 17:39:06 INFO - [Child 1059] WARNING: stylo: No docshell yet, assuming Gecko style system: file /home/worker/workspace/build/src/dom/base/nsDocument.cpp, line 13179 [task 2017-05-18T17:39:06.618795Z] 17:39:06 INFO - ++DOMWINDOW == 2 (0x7f4d565e9000) [pid = 1059] [serial = 2] [outer = 0x7f4d565d6000] [task 2017-05-18T17:39:06.879977Z] 17:39:06 INFO - ++DOMWINDOW == 3 (0x7f4d6604b000) [pid = 1059] [serial = 3] [outer = 0x7f4d565d6000] [task 2017-05-18T17:39:07.021790Z] 17:39:07 INFO - [Child 1059] WARNING: stylo: cannot get ServoStyleSheets from XBL bindings yet. See bug 1290276.: file /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp, line 2719 [task 2017-05-18T17:39:07.037931Z] 17:39:07 INFO - thread '<unnamed>' panicked at 'assertion failed: !traversal.is_parallel()', /home/worker/workspace/build/src/servo/components/style/sequential.rs:27 [task 2017-05-18T17:39:07.038001Z] 17:39:07 INFO - stack backtrace: [task 2017-05-18T17:39:07.367082Z] 17:39:07 INFO - 0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace [task 2017-05-18T17:39:07.367167Z] 17:39:07 INFO - 1: std::sys_common::backtrace::_print [task 2017-05-18T17:39:07.367215Z] 17:39:07 INFO - 2: std::panicking::default_hook::{{closure}} [task 2017-05-18T17:39:07.367253Z] 17:39:07 INFO - 3: std::panicking::default_hook [task 2017-05-18T17:39:07.367295Z] 17:39:07 INFO - 4: std::panicking::rust_panic_with_hook [task 2017-05-18T17:39:07.367332Z] 17:39:07 INFO - 5: std::panicking::begin_panic [task 2017-05-18T17:39:07.367371Z] 17:39:07 INFO - 6: style::sequential::traverse_dom [task 2017-05-18T17:39:07.367410Z] 17:39:07 INFO - 7: geckoservo::glue::traverse_subtree [task 2017-05-18T17:39:07.367447Z] 17:39:07 INFO - 8: Servo_TraverseSubtree [task 2017-05-18T17:39:07.367525Z] 17:39:07 INFO - 9: _ZN7mozilla13ServoStyleSet25PrepareAndTraverseSubtreeEPKNS_3dom7ElementENS_21TraversalRootBehaviorENS_24TraversalRestyleBehaviorE [task 2017-05-18T17:39:07.375504Z] 17:39:07 INFO - 10: _ZN7mozilla13ServoStyleSet15StyleNewSubtreeEPNS_3dom7ElementE [task 2017-05-18T17:39:07.376071Z] 17:39:07 INFO - 11: _ZN21nsCSSFrameConstructor19GetAnonymousContentEP10nsIContentP8nsIFrameR8nsTArrayIN26nsIAnonymousContentCreator11ContentInfoEE [task 2017-05-18T17:39:07.376643Z] 17:39:07 INFO - 12: _ZN21nsCSSFrameConstructor24BeginBuildingScrollFrameER23nsFrameConstructorStateP10nsIContentP14nsStyleContextP16nsContainerFrameP7nsIAtombRS7_ [task 2017-05-18T17:39:07.377219Z] 17:39:07 INFO - 13: _ZN21nsCSSFrameConstructor30SetUpDocElementContainingBlockEP10nsIContent [task 2017-05-18T17:39:07.377726Z] 17:39:07 INFO - 14: _ZN21nsCSSFrameConstructor24ConstructDocElementFrameEPN7mozilla3dom7ElementEP21nsILayoutHistoryState [task 2017-05-18T17:39:07.378323Z] 17:39:07 INFO - 15: _ZN21nsCSSFrameConstructor20ContentRangeInsertedEP10nsIContentS1_S1_P21nsILayoutHistoryStatebbP16TreeMatchContext [task 2017-05-18T17:39:07.379273Z] 17:39:07 INFO - 16: _ZN21nsCSSFrameConstructor15ContentInsertedEP10nsIContentS1_P21nsILayoutHistoryStateb [task 2017-05-18T17:39:07.384027Z] 17:39:07 INFO - 17: _ZN7mozilla9PresShell10InitializeEii [task 2017-05-18T17:39:07.391484Z] 17:39:07 INFO - 18: _ZN13nsContentSink11StartLayoutEb [task 2017-05-18T17:39:07.392006Z] 17:39:07 INFO - 19: _ZN15HTMLContentSink8OpenBodyEv [task 2017-05-18T17:39:07.392521Z] 17:39:07 INFO - 20: _ZN7CNavDTD10BuildModelEP12nsITokenizerP14nsIContentSink [task 2017-05-18T17:39:07.393001Z] 17:39:07 INFO - 21: _ZN8nsParser10BuildModelEv [task 2017-05-18T17:39:07.393478Z] 17:39:07 INFO - 22: _ZN8nsParser11ResumeParseEbbb [task 2017-05-18T17:39:07.394001Z] 17:39:07 INFO - 23: _ZN8nsParser13OnStopRequestEP10nsIRequestP11nsISupports8nsresult [task 2017-05-18T17:39:07.394521Z] 17:39:07 INFO - 24: _ZN18nsDocumentOpenInfo13OnStopRequestEP10nsIRequestP11nsISupports8nsresult [task 2017-05-18T17:39:07.395067Z] 17:39:07 INFO - 25: _ZN13nsBaseChannel13OnStopRequestEP10nsIRequestP11nsISupports8nsresult [task 2017-05-18T17:39:07.395555Z] 17:39:07 INFO - 26: _ZN17nsInputStreamPump11OnStateStopEv [task 2017-05-18T17:39:07.396068Z] 17:39:07 INFO - 27: _ZN17nsInputStreamPump18OnInputStreamReadyEP19nsIAsyncInputStream [task 2017-05-18T17:39:07.396571Z] 17:39:07 INFO - 28: _ZN23nsInputStreamReadyEvent3RunEv [task 2017-05-18T17:39:07.397063Z] 17:39:07 INFO - 29: _ZN7mozilla14SchedulerGroup8Runnable3RunEv [task 2017-05-18T17:39:07.397606Z] 17:39:07 INFO - 30: _ZN8nsThread16ProcessNextEventEbPb.part.210 [task 2017-05-18T17:39:07.398116Z] 17:39:07 INFO - 31: _Z19NS_ProcessNextEventP9nsIThreadb [task 2017-05-18T17:39:07.398631Z] 17:39:07 INFO - 32: _ZN7mozilla3ipc11MessagePump3RunEPN4base11MessagePump8DelegateE [task 2017-05-18T17:39:07.399122Z] 17:39:07 INFO - 33: _ZN11MessageLoop11RunInternalEv [task 2017-05-18T17:39:07.399592Z] 17:39:07 INFO - 34: _ZN11MessageLoop3RunEv [task 2017-05-18T17:39:07.401681Z] 17:39:07 INFO - 35: _ZN14nsBaseAppShell3RunEv [task 2017-05-18T17:39:07.402203Z] 17:39:07 INFO - 36: _Z15XRE_RunAppShellv [task 2017-05-18T17:39:07.402691Z] 17:39:07 INFO - 37: _ZN7mozilla3ipc26MessagePumpForChildProcess3RunEPN4base11MessagePump8DelegateE [task 2017-05-18T17:39:07.404391Z] 17:39:07 INFO - 38: _ZN11MessageLoop11RunInternalEv [task 2017-05-18T17:39:07.404908Z] 17:39:07 INFO - 39: _ZN11MessageLoop3RunEv [task 2017-05-18T17:39:07.405413Z] 17:39:07 INFO - 40: _Z20XRE_InitChildProcessiPPcPK12XREChildData [task 2017-05-18T17:39:07.405894Z] 17:39:07 INFO - 41: _Z20content_process_mainPN7mozilla9BootstrapEiPPc [task 2017-05-18T17:39:07.406785Z] 17:39:07 INFO - 42: main [task 2017-05-18T17:39:07.408340Z] 17:39:07 INFO - 43: __libc_start_main [task 2017-05-18T17:39:07.408849Z] 17:39:07 INFO - 44: <unknown> [task 2017-05-18T17:39:07.410893Z] 17:39:07 INFO - Redirecting call to abort() to mozalloc_abort [task 2017-05-18T17:39:07.411419Z] 17:39:07 INFO - Hit MOZ_CRASH() at /home/worker/workspace/build/src/memory/mozalloc/mozalloc_abort.cpp:33 [task 2017-05-18T17:39:07.520605Z] 17:39:07 INFO - 1495129147517 Marionette INFO Listening on port 2828 [task 2017-05-18T17:39:07.662298Z] 17:39:07 INFO - [Parent 1004] WARNING: pipe error (75): Connection reset by peer: file /home/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 353 [task 2017-05-18T17:39:07.699107Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.701806Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.703713Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.720052Z] 17:39:07 INFO - ###!!! [Parent][MessageChannel] Error: (msgtype=0x480053,name=PContent::Msg_GMPsChanged) Channel error: cannot send/recv [task 2017-05-18T17:39:07.721595Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.723655Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.727663Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.730265Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.732104Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.734253Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.736113Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.738257Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.742421Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.743786Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.744490Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.745182Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.745870Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.745976Z] 17:39:07 INFO - ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv [task 2017-05-18T17:39:07.751361Z] 17:39:07 INFO - ###!!! [Parent][MessageChannel] Error: (msgtype=0x48001F,name=PContent::Msg_PreferenceUpdate) Channel error: cannot send/recv [task 2017-05-18T17:39:07.766143Z] 17:39:07 INFO - ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpyaOpbN.mozrunner/runreftest_leaks_tab_pid1093.log [task 2017-05-18T17:39:07.768090Z] 17:39:07 INFO - [1093] WARNING: '!compMgr', file /home/worker/workspace/build/src/xpcom/components/nsComponentManagerUtils.cpp, line 63 [task 2017-05-18T17:39:07.784992Z] 17:39:07 INFO - ###!!! [Parent][MessageChannel] Error: (msgtype=0x2C0081,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv [task 2017-05-18T17:39:07.789702Z] 17:39:07 INFO - [Child 1093] WARNING: '!compMgr', file /home/worker/workspace/build/src/xpcom/components/nsComponentManagerUtils.cpp, line 63 [task 2017-05-18T17:39:07.820660Z] 17:39:07 INFO - ++DOCSHELL 0x7f2f43768000 == 6 [pid = 1004] [id = {f12f9651-091a-47f3-925a-8ada5ecd39f5}] [task 2017-05-18T17:39:07.821741Z] 17:39:07 INFO - ++DOMWINDOW == 12 (0x7f2f43768800) [pid = 1004] [serial = 12] [outer = (nil)] [task 2017-05-18T17:39:07.962525Z] 17:39:07 INFO - [Parent 1004] WARNING: stylo: No docshell yet, assuming Gecko style system: file /home/worker/workspace/build/src/dom/base/nsDocument.cpp, line 13179 [task 2017-05-18T17:39:07.963693Z] 17:39:07 INFO - ++DOMWINDOW == 13 (0x7f2f4142d800) [pid = 1004] [serial = 13] [outer = 0x7f2f43768800] [task 2017-05-18T17:39:08.361976Z] 17:39:08 INFO - ++DOMWINDOW == 14 (0x7f2f40333800) [pid = 1004] [serial = 14] [outer = 0x7f2f43768800] [task 2017-05-18T17:39:08.483599Z] 17:39:08 INFO - [Parent 1004] WARNING: stylo: No docshell yet, assuming Gecko style system: file /home/worker/workspace/build/src/dom/base/nsDocument.cpp, line 13179 [task 2017-05-18T17:39:08.485181Z] 17:39:08 INFO - [Parent 1004] WARNING: stylo: No docshell yet, assuming Gecko style system: file /home/worker/workspace/build/src/dom/base/nsDocument.cpp, line 13179 [task 2017-05-18T17:39:08.596408Z] 17:39:08 INFO - [Parent 1004] WARNING: stylo: ServoStyleSheets don't support <style scoped>: file /home/worker/workspace/build/src/layout/base/PresShell.cpp, line 4619 [task 2017-05-18T17:39:08.596554Z] 17:39:08 INFO - [Parent 1004] WARNING: stylo: ServoStyleSheets don't support <style scoped>: file /home/worker/workspace/build/src/layout/base/PresShell.cpp, line 4619 [task 2017-05-18T17:39:08.597505Z] 17:39:08 INFO - [Parent 1004] WARNING: stylo: cannot get ServoStyleSheets from XBL bindings yet. See bug 1290276.: file /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp, line 2719 [task 2017-05-18T17:39:08.604595Z] 17:39:08 INFO - Redirecting call to abort() to mozalloc_abort [task 2017-05-18T17:39:08.605534Z] 17:39:08 INFO - Hit MOZ_CRASH() at /home/worker/workspace/build/src/memory/mozalloc/mozalloc_abort.cpp:33 [task 2017-05-18T17:39:08.609218Z] 17:39:08 INFO - ExceptionHandler::GenerateDump cloned child 1125 [task 2017-05-18T17:39:08.609410Z] 17:39:08 INFO - ExceptionHandler::SendContinueSignalToChild sent continue signal to child [task 2017-05-18T17:39:08.609751Z] 17:39:08 INFO - ExceptionHandler::WaitForContinueSignal waiting for continue signal... [task 2017-05-18T17:39:08.727118Z] 17:39:08 INFO - Hit MOZ_CRASH(Aborting on channel error.) at /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2487 [task 2017-05-18T17:39:08.734519Z] 17:39:08 INFO - Traceback (most recent call last): [task 2017-05-18T17:39:08.735115Z] 17:39:08 INFO - File "/home/worker/workspace/build/tests/reftest/runreftest.py", line 775, in <module> [task 2017-05-18T17:39:08.747204Z] 17:39:08 INFO - sys.exit(run_test_harness(parser, options)) [task 2017-05-18T17:39:08.747464Z] 17:39:08 INFO - File "/home/worker/workspace/build/tests/reftest/runreftest.py", line 769, in run_test_harness [task 2017-05-18T17:39:08.747693Z] 17:39:08 INFO - return reftest.runTests(options.tests, options) [task 2017-05-18T17:39:08.750057Z] 17:39:08 INFO - File "/home/worker/workspace/build/tests/reftest/runreftest.py", line 444, in runTests [task 2017-05-18T17:39:08.751777Z] 17:39:08 INFO - return self.runSerialTests(manifests, options, cmdargs) [task 2017-05-18T17:39:08.754114Z] 17:39:08 INFO - File "/home/worker/workspace/build/tests/reftest/runreftest.py", line 704, in runSerialTests [task 2017-05-18T17:39:08.754237Z] 17:39:08 INFO - debuggerInfo=debuggerInfo) [task 2017-05-18T17:39:08.754379Z] 17:39:08 INFO - File "/home/worker/workspace/build/tests/reftest/runreftest.py", line 639, in runApp [task 2017-05-18T17:39:08.759633Z] 17:39:08 INFO - marionette.start_session(timeout=options.marionette_port_timeout) [task 2017-05-18T17:39:08.760305Z] 17:39:08 INFO - File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/decorators.py", line 28, in _ [task 2017-05-18T17:39:08.760880Z] 17:39:08 INFO - m._handle_socket_failure() [task 2017-05-18T17:39:08.763777Z] 17:39:08 INFO - File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/decorators.py", line 23, in _ [task 2017-05-18T17:39:08.767942Z] 17:39:08 INFO - return func(*args, **kwargs) [task 2017-05-18T17:39:08.768508Z] 17:39:08 INFO - File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/marionette.py", line 1320, in start_session [task 2017-05-18T17:39:08.771900Z] 17:39:08 INFO - self.protocol, _ = self.client.connect() [task 2017-05-18T17:39:08.772466Z] 17:39:08 INFO - File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/transport.py", line 233, in connect [task 2017-05-18T17:39:08.775746Z] 17:39:08 INFO - raw = self.receive(unmarshal=False) [task 2017-05-18T17:39:08.776222Z] 17:39:08 INFO - File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/transport.py", line 184, in receive [task 2017-05-18T17:39:08.779745Z] 17:39:08 INFO - raise socket.error("No data received over socket") [task 2017-05-18T17:39:08.780207Z] 17:39:08 INFO - socket.error: No data received over socket [task 2017-05-18T17:39:27.316838Z] 17:39:27 INFO - #01: mozilla::ipc::ProcessLink::OnChannelError [ipc/glue/MessageLink.cpp:368] [task 2017-05-18T17:39:27.316906Z] 17:39:27 INFO - [task 2017-05-18T17:39:27.316963Z] 17:39:27 INFO - #02: event_process_active_single_queue [ipc/chromium/src/third_party/libevent/event.c:1648] [task 2017-05-18T17:39:27.316996Z] 17:39:27 INFO - [task 2017-05-18T17:39:27.317046Z] 17:39:27 INFO - #03: event_base_loop [ipc/chromium/src/third_party/libevent/event.c:1745] [task 2017-05-18T17:39:27.317076Z] 17:39:27 INFO - [task 2017-05-18T17:39:27.317128Z] 17:39:27 INFO - #04: base::MessagePumpLibevent::Run [ipc/chromium/src/base/message_pump_libevent.cc:381] [task 2017-05-18T17:39:27.317324Z] 17:39:27 INFO - [task 2017-05-18T17:39:27.317783Z] 17:39:27 INFO - #05: MessageLoop::RunInternal [ipc/chromium/src/base/message_loop.cc:239] [task 2017-05-18T17:39:27.318248Z] 17:39:27 INFO - [task 2017-05-18T17:39:27.318858Z] 17:39:27 INFO - #06: MessageLoop::Run [ipc/chromium/src/base/message_loop.cc:505] [task 2017-05-18T17:39:27.319319Z] 17:39:27 INFO - [task 2017-05-18T17:39:27.319910Z] 17:39:27 INFO - #07: base::Thread::ThreadMain [ipc/chromium/src/base/thread.cc:183] [task 2017-05-18T17:39:27.320396Z] 17:39:27 INFO - [task 2017-05-18T17:39:27.321017Z] 17:39:27 INFO - #08: ThreadFunc [ipc/chromium/src/base/platform_thread_posix.cc:40] [task 2017-05-18T17:39:27.321474Z] 17:39:27 INFO - [task 2017-05-18T17:39:27.322051Z] 17:39:27 INFO - #09: libpthread.so.0 + 0x76ba [task 2017-05-18T17:39:27.322529Z] 17:39:27 INFO - [task 2017-05-18T17:39:27.323111Z] 17:39:27 INFO - #10: libc.so.6 + 0x10682d [task 2017-05-18T17:39:27.323636Z] 17:39:27 INFO - [task 2017-05-18T17:39:27.324315Z] 17:39:27 INFO - #11: ??? (???:???) [task 2017-05-18T17:39:28.032734Z] 17:39:28 ERROR - Return code: 1
Flags: needinfo?(cku)
|
|
||
Comment 90•7 years ago
|
||
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/edc2283b1637 Part 1. Implement imgDrawingParams. r=jwatt https://hg.mozilla.org/integration/autoland/rev/bbf16eb4f821 Part 2. Encapsulate DrawResult and imgIContainer::FLAG_* into imgDrawingParams, and pass it to PaintSVG. r=jwatt https://hg.mozilla.org/integration/autoland/rev/4c05d41afa86 Part 3. Pass imgDrawingParams to nsSVGMaskFrame::GetMaskForMaskedFrame. r=jwatt https://hg.mozilla.org/integration/autoland/rev/7995a23f4ace Part 4. Pass imgDrawingParams to nsSVGPaintServerFrame::GetPaintServerPattern. r=jwatt https://hg.mozilla.org/integration/autoland/rev/569c0bf0ddcb Part 5. typedef DrawResult in nsDisplayItem to avoid the image:: qualification. r=jwatt
|
||
Comment 92•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/edc2283b1637 https://hg.mozilla.org/mozilla-central/rev/bbf16eb4f821 https://hg.mozilla.org/mozilla-central/rev/4c05d41afa86 https://hg.mozilla.org/mozilla-central/rev/7995a23f4ace https://hg.mozilla.org/mozilla-central/rev/569c0bf0ddcb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
|
|
Reporter | |
Comment 93•7 years ago
|
||
Oh, this landed. I just wanted to say, thanks so much for fixing this, C.J.! I really appreciate it.
Attachment #8865770 -
Flags: review?(tnikkel)
Attachment #8864793 -
Flags: review?(tnikkel)
You need to log in
before you can comment on or make changes to this bug.
Description
•