Bug 54440 – [Refactoring] Make ScheduledEvent on FrameView abstract out to ScheduleAction

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

I need to run post-layout action for <meter> implementation, which occurs only when the is direction (vertical/horizontal) switches.
ScheduledEvent abstraction can be used for it if we make it something abstract.

[Hajime Morrita]

Created attachment 82593 [details]
Patch

[Hajime Morrita]

Hi, could anyone take a look?

I'm planning to use ScheduledAction for <meter>'s post-layout hook.
<meter> need to change its style based on the layout result (horizontal/vertical size)
In practice, the style change rarely happens because <meter> doesn't change its size in normal case.
Though some edge cases need to do it.

[Hajime Morrita]

Created attachment 84019 [details]
Updated to ToT

[Hajime Morrita]

Hi, Could anyone review this? This is a refactoring which just extract 2 classes.
And I need this kind of the post-layout hook to my upcoming change.

[Kent Tamura]

Comment on attachment 84019 [details]
Updated to ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=84019&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25134
> +				0F29C16E1300C2E2002D794E /* AccessibilityAllInOne.cpp in Sources */,
> +				975CA28A130365F800E99AD9 /* Crypto.cpp in Sources */,
> +				975CA2A11303679D00E99AD9 /* JSCrypto.cpp in Sources */,
> +				A1E1154413015C3D0054AC8C /* DistantLightSource.cpp in Sources */,
> +				A1E1154613015C4E0054AC8C /* PointLightSource.cpp in Sources */,
> +				A1E1154813015C5D0054AC8C /* SpotLightSource.cpp in Sources */,
> +				A7B070D2130A409C00A3763C /* FrameActionScheduler.cpp in Sources */,
> +				B8DBDB4B130B0F8A00F5CDB1 /* SetSelectionCommand.cpp in Sources */,
> +				B8DBDB4D130B0F8A00F5CDB1 /* SpellingCorrectionCommand.cpp in Sources */,

Your change adds unrelated files.

> Source/WebCore/page/FrameActionScheduler.h:7
> +/*
> +   Copyright (C) 1997 Martin Jones ([e-mail])
> +             (C) 1998 Waldo Bastian ([e-mail])
> +             (C) 1998, 1999 Torben Weis ([e-mail])
> +             (C) 1999 Lars Knoll ([e-mail])
> +             (C) 1999 Antti Koivisto ([e-mail])
> +   Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.

Does this file contain any code written by these copyright holders?

> Source/WebCore/page/FrameActionScheduler.h:37
> +#include "Event.h"
> +#include "Node.h"
> +#include <wtf/FastAllocBase.h>
> +#include <wtf/Forward.h>
> +#include <wtf/OwnPtr.h>
> +
> +namespace WebCore {
> +
> +class Node;
> +class Event;

We don't need both of "#include <Event.h>" and "class Event".

[Hajime Morrita]

Created attachment 84039 [details]
Patch

[Hajime Morrita]

Kent-san, thank you for your review! 
I updated the patch to address the feedback

(In reply to comment #5)
> (From update of attachment 84019 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84019&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:25134
> > +				0F29C16E1300C2E2002D794E /* AccessibilityAllInOne.cpp in Sources */,
> > +				975CA28A130365F800E99AD9 /* Crypto.cpp in Sources */,
> > +				975CA2A11303679D00E99AD9 /* JSCrypto.cpp in Sources */,
> > +				A1E1154413015C3D0054AC8C /* DistantLightSource.cpp in Sources */,
> > +				A1E1154613015C4E0054AC8C /* PointLightSource.cpp in Sources */,
> > +				A1E1154813015C5D0054AC8C /* SpotLightSource.cpp in Sources */,
> > +				A7B070D2130A409C00A3763C /* FrameActionScheduler.cpp in Sources */,
> > +				B8DBDB4B130B0F8A00F5CDB1 /* SetSelectionCommand.cpp in Sources */,
> > +				B8DBDB4D130B0F8A00F5CDB1 /* SpellingCorrectionCommand.cpp in Sources */,
> 
> Your change adds unrelated files.
Good catch. I failed to merge the patch... Fixed.

> 
> > Source/WebCore/page/FrameActionScheduler.h:7
> > +/*
> > +   Copyright (C) 1997 Martin Jones ([e-mail])
> > +             (C) 1998 Waldo Bastian ([e-mail])
> > +             (C) 1998, 1999 Torben Weis ([e-mail])
> > +             (C) 1999 Lars Knoll ([e-mail])
> > +             (C) 1999 Antti Koivisto ([e-mail])
> > +   Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
> 
> Does this file contain any code written by these copyright holders?
> 
Cleaned up.


> > Source/WebCore/page/FrameActionScheduler.h:37
> > +#include "Event.h"
> > +#include "Node.h"
> > +#include <wtf/FastAllocBase.h>
> > +#include <wtf/Forward.h>
> > +#include <wtf/OwnPtr.h>
> > +
> > +namespace WebCore {
> > +
> > +class Node;
> > +class Event;
> 
> We don't need both of "#include <Event.h>" and "class Event".
Removed the forward declarations

[Kent Tamura]

Comment on attachment 84039 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84039&action=review

I'll set r+ tomorrow if no one objects.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25126
>  				93F19B0508245E59001E9ABC /* XSLTProcessorLibxslt.cpp in Sources */,
>  				E1BE512D0CF6C512002EA959 /* XSLTUnicodeSort.cpp in Sources */,
>  				977E2E0E12F0FC9C00C13379 /* XSSFilter.cpp in Sources */,
> +				A7B070D2130A409C00A3763C /* FrameActionScheduler.cpp in Sources */,

The new entry should be put into the right place.

[Kent Tamura]

Comment on attachment 84039 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84039&action=review

r- because of some minor issues.

> Source/WebCore/page/FrameActionScheduler.h:69
> +    void scheduleAction(ScheduledAction*);

We should add a comment about the ownership of the specified ScheduledAction object.

> Source/WebCore/page/FrameView.cpp:126
> +    , m_scheduledActions(new FrameActionScheduler())

The name m_scheduledActions is not appropriate.  It should be m_actionScheduler or something.

[Hajime Morrita]

Created attachment 84373 [details]
Patch

[Hajime Morrita]

Kent-san, thank you again for your review!
I updated the patch. Could you have a review again?

> > Source/WebCore/page/FrameActionScheduler.h:69
> > +    void scheduleAction(ScheduledAction*);
> 
> We should add a comment about the ownership of the specified ScheduledAction object.
I change it to RefCounted/RefPtr to remove the lifetime ambiguity.
 
> > Source/WebCore/page/FrameView.cpp:126
> > +    , m_scheduledActions(new FrameActionScheduler())
> 
> The name m_scheduledActions is not appropriate.  It should be m_actionScheduler or something.
Renamed.

[Kent Tamura]

Comment on attachment 84373 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84373&action=review

> > We should add a comment about the ownership of the specified ScheduledAction object.
> I change it to RefCounted/RefPtr to remove the lifetime ambiguity.

Why it isn't OwnPtr? Do you have a plan to use this for cases in which OwnPtr is not enough?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25148
>  				93F19B0508245E59001E9ABC /* XSLTProcessorLibxslt.cpp in Sources */,
>  				E1BE512D0CF6C512002EA959 /* XSLTUnicodeSort.cpp in Sources */,
>  				977E2E0E12F0FC9C00C13379 /* XSSFilter.cpp in Sources */,
> +				A7B070D2130A409C00A3763C /* FrameActionScheduler.cpp in Sources */,

nit: New entry should be put into the right place, not the bottom of the list.

[Hajime Morrita]

Created attachment 84393 [details]
Patch

[Hajime Morrita]

> > > We should add a comment about the ownership of the specified ScheduledAction object.
> > I change it to RefCounted/RefPtr to remove the lifetime ambiguity.
> 
> Why it isn't OwnPtr? Do you have a plan to use this for cases in which OwnPtr is not enough?
That's because the pointers are stored Vector and thus copy can happen.
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:25148
> >  				93F19B0508245E59001E9ABC /* XSLTProcessorLibxslt.cpp in Sources */,
> >  				E1BE512D0CF6C512002EA959 /* XSLTUnicodeSort.cpp in Sources */,
> >  				977E2E0E12F0FC9C00C13379 /* XSSFilter.cpp in Sources */,
> > +				A7B070D2130A409C00A3763C /* FrameActionScheduler.cpp in Sources */,
> 
> nit: New entry should be put into the right place, not the bottom of the list.
Oops. Fixed.

[Kent Tamura]

(In reply to comment #14)
> > > > We should add a comment about the ownership of the specified ScheduledAction object.
> > > I change it to RefCounted/RefPtr to remove the lifetime ambiguity.
> > 
> > Why it isn't OwnPtr? Do you have a plan to use this for cases in which OwnPtr is not enough?
> That's because the pointers are stored Vector and thus copy can happen.

Vector.h and VectorTraits.h correctly handle OwnPtr<> elements.

[Hajime Morrita]

Created attachment 84403 [details]
Patch

[Hajime Morrita]

> 
> Vector.h and VectorTraits.h correctly handle OwnPtr<> elements.
Wow, I didn't know that. Thank you for pointing this!

Anyway, I found binding/js/ScheduledAction whose name conflicts ScheduledAction here.
So I renamed it to FrameAction and renamed ScheduledEvent to EventFrameAction respectively.

[Kent Tamura]

Comment on attachment 84403 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84403&action=review

> Source/WebCore/page/FrameActionScheduler.h:60
> +    // passed action objects are deleted by FrameAction
> +    void scheduleAction(FrameAction*);

I like making the parameter type PassOwnPtr<FrameAction> and delete the comment.

[Hajime Morrita]

Created attachment 84406 [details]
Fixed

[Kent Tamura]

Comment on attachment 84406 [details]
Fixed

ok

[Hajime Morrita]

Committed r80210: <http://trac.webkit.org/changeset/80210>

[Dimitri Glazkov (Google)]

(In reply to comment #21)
> Committed r80210: <http://trac.webkit.org/changeset/80210>

I apologize for not seeing this earlier! I just now realized this is related to meter.

[Dimitri Glazkov (Google)]

(In reply to comment #0)
> I need to run post-layout action for <meter> implementation, which occurs only when the is direction (vertical/horizontal) switches.
> ScheduledEvent abstraction can be used for it if we make it something abstract.

I am not so sure about us actually needing this. Can you explain how this will work a bit? The reason I am asking is because progress/meter aren't unique in this -- input[type=range] will also need to adopt this logic.

To me, it seemed like we need a new rendering primitive, the one that listens to the dimensions it's given and makes decisions on how to lay out the children accordingly. Right?

How does the ability to run post-layout actions buy us anything new?

[Hajime Morrita]

(In reply to comment #23)
> (In reply to comment #0)
> > I need to run post-layout action for <meter> implementation, which occurs only when the is direction (vertical/horizontal) switches.
> > ScheduledEvent abstraction can be used for it if we make it something abstract.
> 
> I am not so sure about us actually needing this. Can you explain how this will work a bit? The reason I am asking is because progress/meter aren't unique in this -- input[type=range] will also need to adopt this logic.
> 
> To me, it seemed like we need a new rendering primitive, the one that listens to the dimensions it's given and makes decisions on how to lay out the children accordingly. Right?
> 
> How does the ability to run post-layout actions buy us anything new?

Hi, Dimitri.
<meter> need this because it uses the layout result (whether it is vertically or horizontally long)
to determine its rendering (whether to show a vertical or horizontal bar.)
Current implementation has shadow nodes for both horizontal and vertical bars,
and hide on of them just after the layout.

This trick works. but we need to have manual layout code for it
because CSS doesn't allow overlapping node (except using position: absolute.)

Instead of this, I'm thinking we can switch the style (pseudo-class)
if the dominance direction (vertical/horizontal) changes by the layout.
Because style change during the layout isn't allowed, we need to
defer the style change after the layout.  this scheduler is for such deferring. 
Giving the pseudo-class dynamically based on the layout,
we can get rid of large part of manual layout logic from the <meter> code.
Such kind of layout code is hard to write correctly and causes potential security hole.
(Actually I had several bugs around there....)

This technique can cause an extra layout, but it doesn't matter in practice because
the size of <meter> doesn't change in most cases.

[Dimitri Glazkov (Google)]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #0)
> > > I need to run post-layout action for <meter> implementation, which occurs only when the is direction (vertical/horizontal) switches.
> > > ScheduledEvent abstraction can be used for it if we make it something abstract.
> > 
> > I am not so sure about us actually needing this. Can you explain how this will work a bit? The reason I am asking is because progress/meter aren't unique in this -- input[type=range] will also need to adopt this logic.
> > 
> > To me, it seemed like we need a new rendering primitive, the one that listens to the dimensions it's given and makes decisions on how to lay out the children accordingly. Right?
> > 
> > How does the ability to run post-layout actions buy us anything new?
> 
> Hi, Dimitri.
> <meter> need this because it uses the layout result (whether it is vertically or horizontally long)
> to determine its rendering (whether to show a vertical or horizontal bar.)
> Current implementation has shadow nodes for both horizontal and vertical bars,
> and hide on of them just after the layout.

Right. The problem you're dealing with is pretty fundamental. I don't think we should be using tricks. I think we should fix the problem itself :)

The gist of the problem is that we need this ability to change what selector the element matches _after_ computing its logical width. I really don't think we should do this.

I am going to post to whatwg to start the discussion on this. In the meantime, I highly recommend going with what we've done with input[type=range] -- just have explicit selector for horizontal and vertical orientation.

Engaging in double-layout and other stuff like this is the road to hell.