Bug 54440

Created attachment 82593 [details] Patch

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.

Created attachment 84019 [details] Updated to ToT

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 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".

Created attachment 84039 [details] Patch

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 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 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.

Created attachment 84373 [details] Patch

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 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.

Created attachment 84393 [details] Patch

> > > 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.

(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.

Created attachment 84403 [details] Patch

> > 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 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.

Created attachment 84406 [details] Fixed

Comment on attachment 84406 [details] Fixed ok

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

(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.

(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?

(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.

(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.