[webkit-reviews] review denied: [Bug 90670] Vertically center non-anchored <dialog> elements : [Attachment 158279] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Matt Falkenhagen
<falken at chromium.org>'s request for review:
Bug 90670: Vertically center non-anchored <dialog> elements
https://bugs.webkit.org/show_bug.cgi?id=90670

Attachment 158279: Patch
https://bugs.webkit.org/attachment.cgi?id=158279&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=158279&action=review


Julien, Tony, do you mind taking a look at the LayoutStateDisabler/repaint
stuff? I'm not too familiar with LayoutState.

On second thought, I think the setLogicalTop approach is fine. Sorry for
leading you astray. Unfortunately, WebKit has a ton of build systems, so you'll
need to modify a ton more files (gtk, qt, efl, etc). I recommend looking at
other recent patches that have added/removed new files as a reference.

> Source/WebCore/rendering/RenderDialog.cpp:36
> +using namespace HTMLNames;

What is this for?

> Source/WebCore/rendering/RenderDialog.cpp:45
> +RenderDialog::RenderDialog(Node* node)
> +    : RenderBlock(node)
> +{
> +}
> +
> +RenderDialog::~RenderDialog()
> +{
> +}

You can just put these in the header since they're empty.

> Source/WebCore/rendering/RenderDialog.cpp:64
> +    if (isFloating())
> +	   return "RenderDialog (floating)";
> +    if (isOutOfFlowPositioned())
> +	   return "RenderDialog (positioned)";
> +    if (isAnonymousColumnsBlock())
> +	   return "RenderDialog (anonymous multi-column)";
> +    if (isAnonymousColumnSpanBlock())
> +	   return "RenderDialog (anonymous multi-column span)";
> +    if (isAnonymousBlock())
> +	   return "RenderDialog (anonymous)";
> +    if (isAnonymous())
> +	   return "RenderDialog (generated)";
> +    if (isRelPositioned())
> +	   return "RenderDialog (relative positioned)";
> +    if (isRunIn())
> +	   return "RenderDialog (run-in)";

I don't think you need all this. It's sufficient for now to just return
RenderDialog. IIRC, this is only used for dumping the render tree for testing.

In fact, it'd be fine for you to leave out this method altogether since we
don't even have tests that dump a render tree with dialogs in them.

> Source/WebCore/rendering/RenderDialog.cpp:76
> +    LayoutStateDisabler layoutStateDisabler(view());

I think this might need a LayoutRepainter and a LayoutStateMaintainer instead
(which would also wrap the RenderBlock::layout() method (see, e.g.
RenderFlexibleBox::layoutBlock for an example).

Alternately, maybe you just need to call repaintDuringLayoutIfMoved at the end
instead of repaint.

> Source/WebCore/rendering/RenderDialog.cpp:82
> +    if (logicalHeight() < visibleHeight)
> +	   absolutePoint.move(0, (visibleHeight - logicalHeight()) / 2);

You're mixing logical and physical heights here. The question is what should
happen with a dialog that has -webkit-writing-mode:vertical-rl. It should
probably center horizontally, right? In that case, you should just use
logicalHeights. Would be nice to add a testcase for this as well.

> Source/WebCore/rendering/RenderDialog.cpp:84
> +    LayoutUnit localTop = LayoutSize(localPoint.x(),
localPoint.y()).height();

I'm not sure, but I think you're dealing in physical heights here as well. A
test with vertical writing mode should make it clear whether this is right.

> Source/WebCore/rendering/RenderDialog.h:55
> +inline RenderDialog* toRenderDialog(RenderObject* object)
> +{
> +    ASSERT(!object || object->isDialog());
> +    return static_cast<RenderDialog*>(object);
> +}
> +
> +// This will catch anyone doing an unnecessary cast.
> +void toRenderDialog(const RenderDialog*);

I don't think we need these. If these become common enough usage, that we need
helper functions,we can add them then.

>
LayoutTests/fast/dom/HTMLDialogElement/non-anchored-dialog-positioning.html:22
> +    shouldBeCloseTo('realTop', expectedTop, 1, true);

We should be able to get exact numbers here. shouldBeCloseTo is just for things
like transitions where the actual number is a bit racy.

Might be simpler if you don't use a font-size based sizes below. Just use
pixels.