Bug 54440 - [Refactoring] Make ScheduledEvent on FrameView abstract out to ScheduleAction
Summary: [Refactoring] Make ScheduledEvent on FrameView abstract out to ScheduleAction
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks: 50661
  Show dependency treegraph
 
Reported: 2011-02-15 00:37 PST by Hajime Morrita
Modified: 2011-03-04 09:36 PST (History)
5 users (show)

See Also:


Attachments
Patch (22.35 KB, patch)
2011-02-15 22:56 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Updated to ToT (22.71 KB, patch)
2011-02-27 22:42 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (22.57 KB, patch)
2011-02-28 04:19 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (22.44 KB, patch)
2011-03-01 23:35 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (22.56 KB, patch)
2011-03-02 03:15 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (22.43 KB, patch)
2011-03-02 04:58 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Fixed (22.42 KB, patch)
2011-03-02 05:17 PST, Hajime Morrita
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-02-15 00:37:48 PST
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.
Comment 1 Hajime Morrita 2011-02-15 22:56:20 PST
Created attachment 82593 [details]
Patch
Comment 2 Hajime Morrita 2011-02-15 22:59:34 PST
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.
Comment 3 Hajime Morrita 2011-02-27 22:42:33 PST
Created attachment 84019 [details]
Updated to ToT
Comment 4 Hajime Morrita 2011-02-27 22:45:01 PST
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.
Comment 5 Kent Tamura 2011-02-28 00:28:01 PST
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 (mjones@kde.org)
> +             (C) 1998 Waldo Bastian (bastian@kde.org)
> +             (C) 1998, 1999 Torben Weis (weis@kde.org)
> +             (C) 1999 Lars Knoll (knoll@kde.org)
> +             (C) 1999 Antti Koivisto (koivisto@kde.org)
> +   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".
Comment 6 Hajime Morrita 2011-02-28 04:19:52 PST
Created attachment 84039 [details]
Patch
Comment 7 Hajime Morrita 2011-02-28 04:22:13 PST
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 (mjones@kde.org)
> > +             (C) 1998 Waldo Bastian (bastian@kde.org)
> > +             (C) 1998, 1999 Torben Weis (weis@kde.org)
> > +             (C) 1999 Lars Knoll (knoll@kde.org)
> > +             (C) 1999 Antti Koivisto (koivisto@kde.org)
> > +   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
Comment 8 Kent Tamura 2011-03-01 01:03:44 PST
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.
Comment 9 Kent Tamura 2011-03-01 20:31:24 PST
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.
Comment 10 Hajime Morrita 2011-03-01 23:35:10 PST
Created attachment 84373 [details]
Patch
Comment 11 Hajime Morrita 2011-03-01 23:38:53 PST
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.
Comment 12 Kent Tamura 2011-03-01 23:43:06 PST
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.
Comment 13 Hajime Morrita 2011-03-02 03:15:33 PST
Created attachment 84393 [details]
Patch
Comment 14 Hajime Morrita 2011-03-02 03:18:46 PST
> > > 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.
Comment 15 Kent Tamura 2011-03-02 03:22:15 PST
(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.
Comment 16 Hajime Morrita 2011-03-02 04:58:40 PST
Created attachment 84403 [details]
Patch
Comment 17 Hajime Morrita 2011-03-02 05:02:26 PST
> 
> 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.
Comment 18 Kent Tamura 2011-03-02 05:08:02 PST
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.
Comment 19 Hajime Morrita 2011-03-02 05:17:10 PST
Created attachment 84406 [details]
Fixed
Comment 20 Kent Tamura 2011-03-02 05:40:40 PST
Comment on attachment 84406 [details]
Fixed

ok
Comment 21 Hajime Morrita 2011-03-02 22:29:21 PST
Committed r80210: <http://trac.webkit.org/changeset/80210>
Comment 22 Dimitri Glazkov (Google) 2011-03-03 07:54:43 PST
(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.
Comment 23 Dimitri Glazkov (Google) 2011-03-03 08:49:12 PST
(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?
Comment 24 Hajime Morrita 2011-03-04 01:43:52 PST
(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.
Comment 25 Dimitri Glazkov (Google) 2011-03-04 09:36:24 PST
(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.