[Webkit-unassigned] [Bug 124325] Consolidate and expose Frame/Node/Selection screenshot capabilities

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=124325





--- Comment #60 from Darin Adler <darin at apple.com>  2013-12-04 18:31:42 PST ---
(In reply to comment #57)
> > > Source/WebCore/page/FrameSnapshotting.cpp:95
> > > +    std::unique_ptr<ImageBuffer> buffer = ImageBuffer::create(usedRect.size(), deviceScaleFactor, ColorSpaceDeviceRGB);
> > > +    if (!buffer)
> > > +        return nullptr;
> > 
> > Does ImageBuffer::create really return null when it fails? I am surprised? Normally if we can’t allocate memory we crash instead of returning null unless the function is named “try”.
> 
> Yes, it does. Should this be added to the style guide?

Doesn’t seem to me like something to be mentioned in the style guide, but maybe.

We have plenty of functions that return null when they fail, but generally that is for failures other than memory exhaustion.

> > > Source/WebCore/platform/DragImage.cpp:80
> > > +        frame.document()->updateLayout();
> > 
> > It doesn’t make sense to do this after using node.renderer(). It needs to be done *before* calling node.renderer().
> 
> Perhaps the original author was confused as to whether setting drag state entailed any layout invalidation. I was under the impression that it would trigger changes because of -webkit-drag styles. Is this incorrect?

Good point. We do need to update layout after setting the drag state. I now see that this was mentioned in an existing comment that this patch deletes.

    // forces style recalc - needed since changing the drag state might
    // imply new styles, plus JS could have changed other things

Not a great comment, but did answer the question that came to mind when I read the code. It’s good to have “why” comments--those are the ones we encourage in WebKit. I think a comment that explicitly mentions that drag state can affect style would be good to add. It’s actually quite strange to have a flag in a renderer that can affect style, because normally the architecture is that DOM plus style is used to create renderers. Having the renderer state also affect style is a sort of “circular” concept that doesn’t fit too well and because of that I think this is something that eventually we’ll want to fix by moving the state out of the renderer.

But it’s not right to call node.renderer() without first calling updateLayout(), so I we need additional calls to updateLayout() in a few places:

1) at the start of the snapshotNode function
2) here in ScopedNodeDragEnabler’s constructor, just before calling node.renderer()
3) in createDragImageFromSnapshot, in the CSS_IMAGE_ORIENTATION code
4) at the start of the createDragImageForImage, although maybe we can just rely on the side effect from ScopedNodeDragEnabler

> > > Source/WebCore/platform/DragImage.cpp:144
> > > +        if (RenderView* root = frame.contentRenderer())
> > > +            root->setSelection(startRenderer, startOffset, endRenderer, endOffset, RenderView::RepaintNothing);
> > 
> > This doesn’t seem right. What if contentRenderer now returns non-null but in the constructor it returned null? Then we’ll be setting the selection to random values!
> 
> Why would it become null? There is no layout run between construction and destruction. (I'm not aware of any other way to serialize the RenderView's selection.)

I think the issue here is that the guarantee that there is no layout is far away from this helper class. It’s fragile to have the assumptions relatively far away from the code that makes the assumption valid. But I think it’s just a coding style issue, not an actual bug.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.