Bug 103697 – Unify IsActiveFlag, IsHoverFlag, and InActiveChainFlag to save bits in Node

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

[Hajime Morrita]

I need another flag so I'd like to make a space for it.
The number of nodes where these flags are on is small and they are tightly related,
we could get them together and manage a collection of enabled nodes in document object.

[Hajime Morrita]

Created attachment 176904 [details]
Patch

[Build Bot]

Comment on attachment 176904 [details]
Patch

Attachment 176904 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15059217

New failing tests:
editing/inserting/insert-div-027.html
accessibility/radio-button-group-members.html
css3/flexbox/flex-flow-margins.html
dom/xhtml/level2/html/HTMLOptionsCollection03.xhtml
dom/xhtml/level2/html/HTMLDocument13.xhtml
dom/html/level2/html/HTMLDocument13.html
dom/html/level2/html/HTMLOptionsCollection03.html
editing/execCommand/4580583-2.html
css3/flexbox/preferred-widths.html
css3/flexbox/flex-flow-orientations.html
dom/html/level2/html/HTMLCollection10.html
editing/inserting/insert-div-018.html
css3/flexbox/flex-flow-margins-auto-size.html
fast/block/block-remove-child-delete-line-box-crash.html
css3/flexbox/flex-flow-auto-margins.html
css3/flexbox/flex-flow-padding.html
animations/body-removal-crash.html
css3/flexbox/flex-align-end.html
dom/xhtml/level2/html/HTMLCollection10.xhtml
css3/flexbox/flex-flow-overflow.html
editing/execCommand/5481523.html
editing/inserting/insert-div-014.html
dom/html/level2/html/HTMLCollection02.html
editing/execCommand/4580583-1.html
css3/flexbox/true-centering.html
css3/flexbox/flex-flow-border.html
editing/inserting/break-blockquote-after-delete.html
editing/pasteboard/paste-RTFD.html
dom/xhtml/level2/html/HTMLCollection02.xhtml

[Hajime Morrita]

Created attachment 176911 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 176911 [details]
Patch

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

> Source/WebCore/dom/Node.h:-695
> -        IsHoveredFlag = 1 << 11,

I'm not sure if we want to move hover flag. It's updated whenever user moves a mouse pointer. Doing hash look ups on every mouse move doesn't seem like a good idea. Focus & active on the other hand only respond to mouse clicks, so they're probably not as crucial.

Also, why don't we just move them to NodeRareData instead of adding a new class & hanging it off of Document?

[WebKit Review Bot]

Comment on attachment 176911 [details]
Patch

Attachment 176911 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15055337

New failing tests:
http/tests/local/drag-over-remote-content.html
css3/flexbox/flex-flow-margins.html
dom/xhtml/level2/html/HTMLOptionsCollection03.xhtml
dom/xhtml/level2/html/HTMLDocument13.xhtml
dom/html/level2/html/HTMLDocument13.html
dom/html/level2/html/HTMLOptionsCollection03.html
editing/execCommand/4580583-2.html
css3/flexbox/preferred-widths.html
css3/flexbox/flex-flow-orientations.html
dom/html/level2/html/HTMLCollection10.html
css3/flexbox/flex-flow-overflow.html
css3/flexbox/flex-flow-margins-auto-size.html
animations/suspend-resume-animation-events.html
http/tests/inspector/console-cd-completions.html
css3/flexbox/flex-flow-auto-margins.html
css3/flexbox/flex-flow-padding.html
http/tests/misc/char-encoding-in-hidden-charset-field-default.html
animations/body-removal-crash.html
css3/flexbox/flex-align-end.html
dom/xhtml/level2/html/HTMLCollection10.xhtml
http/tests/misc/multiple-submit.html
editing/execCommand/5481523.html
dom/html/level2/html/HTMLCollection02.html
http/tests/inspector/console-cd.html
http/tests/misc/form-post-textplain.html
editing/execCommand/4580583-1.html
http/tests/misc/post-submit-button.html
css3/flexbox/true-centering.html
css3/flexbox/flex-flow-border.html
dom/xhtml/level2/html/HTMLCollection02.xhtml

[Hajime Morrita]

Created attachment 176928 [details]
Patch

[Hajime Morrita]

I expect Document::m_focusedNode, m_activeNode and m_hoverNode can also be moved to 
this class. That will make it easy to track lifetime of these nodes.
But it should happen in separate changes.

[Hajime Morrita]

Created attachment 176930 [details]
Patch

[WebKit Review Bot]

Comment on attachment 176930 [details]
Patch

Attachment 176930 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15065167

New failing tests:
fast/html/details-add-summary-10-and-click.html
fast/html/details-add-summary-8-and-click.html
fast/html/details-add-summary-1-and-click.html
fast/html/details-add-summary-2-and-click.html
fast/html/details-remove-summary-6-and-click.html
fast/html/details-add-summary-7-and-click.html
fast/html/details-remove-summary-5-and-click.html
fast/html/details-add-summary-5-and-click.html
fast/html/details-remove-summary-1-and-click.html
fast/html/details-add-summary-6-and-click.html
fast/html/details-remove-summary-4-and-click.html
fast/html/details-add-summary-3-and-click.html
fast/html/details-remove-summary-3-and-click.html
fast/html/details-add-summary-4-and-click.html
fast/html/details-remove-summary-2-and-click.html
fast/html/details-add-summary-9-and-click.html

[Antti Koivisto]

Comment on attachment 176930 [details]
Patch

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

> Source/WebCore/dom/Node.cpp:919
> +bool Node::respondingNodeIs(RespondingNodeFlags flags) const
> +{
> +    ASSERT(responding());
> +    if (Document* doc = document())
> +        return doc->respondingNodes()->hasFlags(this, flags);
> +    return false;
>  }

respondingNodeIs function seems bit unnecessary. Why not just call respondingNodes()->isActive() from Node::active() directly, similar to the setters? Then Node won't have to know about the flags at all.

> Source/WebCore/dom/Node.h:326
> +    bool responding() const { return getFlag(IsRespondingFlag); }

isResponding(). The existing accessors don't follow the pattern but there is not need why new ones wouldn't.

> Source/WebCore/dom/Node.h:334
> +    void setResponding(bool flag) { setFlag(flag, IsRespondingFlag); }
> +
> +    enum RespondingNodeFlags {
> +        IsActiveFlag      = 1 ,
> +        InActiveChainFlag = 1 << 1,
> +        IsHoveredFlag     = 1 << 2,
> +        IsFocusedFlag     = 1 << 3
> +    };

Instead of being in Node, RespondingNodeFlags might be more natural as RespondingNodeSet::Flags.

I'm not sure "responding" really captures these well. Responding to what? I don't have better suggestions though.

> Source/WebCore/dom/Node.h:340
> +    bool respondingNodeIs(RespondingNodeFlags) const;

This should be private.

> Source/WebCore/dom/RespondingNodeSet.h:39
> +class RespondingNodeSet {

Do these flags ever have meaning on non-Elements? If not this could be tightened to be RespondingElementSet.

> Source/WebCore/dom/RespondingNodeSet.h:64
> +    void documentRemovedLastRef() { m_nodes.clear(); }

documentDidRemoveLastRef

[Ojan Vafai]

Comment on attachment 176930 [details]
Patch

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

>> Source/WebCore/dom/Node.h:334
>> +    };
> 
> Instead of being in Node, RespondingNodeFlags might be more natural as RespondingNodeSet::Flags.
> 
> I'm not sure "responding" really captures these well. Responding to what? I don't have better suggestions though.

The CSS spec calls these "User Action pseudo classes". It's a bit wordy, but InteractivePseudoFlags maybe?

[Hajime Morrita]

(In reply to comment #11)
> (From update of attachment 176930 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176930&action=review
> 
> >> Source/WebCore/dom/Node.h:334
> >> +    };
> > 
> > Instead of being in Node, RespondingNodeFlags might be more natural as RespondingNodeSet::Flags.
> > 
> > I'm not sure "responding" really captures these well. Responding to what? I don't have better suggestions though.
> 
> The CSS spec calls these "User Action pseudo classes". It's a bit wordy, but InteractivePseudoFlags maybe?
So these nodes are interacting. InteractingNodeSet might be better name then.
I'll update the patch that way. Thanks for suggestions, and reviews!

[Ojan Vafai]

> > > Instead of being in Node, RespondingNodeFlags might be more natural as RespondingNodeSet::Flags.
> > > 
> > > I'm not sure "responding" really captures these well. Responding to what? I don't have better suggestions though.
> > 
> > The CSS spec calls these "User Action pseudo classes". It's a bit wordy, but InteractivePseudoFlags maybe?
> So these nodes are interacting. InteractingNodeSet might be better name then.
> I'll update the patch that way. Thanks for suggestions, and reviews!

Yeah. I think that's better. Although, "interacting" sounds to me like the nodes are interacting with each other. So, I think InteractiveNodeSet would be a little less confusing.

It's still not a very clear name. I'm having trouble thinking of anything better.

[Ryosuke Niwa]

(In reply to comment #13)
> > > > Instead of being in Node, RespondingNodeFlags might be more natural as RespondingNodeSet::Flags.
> > > > 
> > > > I'm not sure "responding" really captures these well. Responding to what? I don't have better suggestions though.
> > > 
> > > The CSS spec calls these "User Action pseudo classes". It's a bit wordy, but InteractivePseudoFlags maybe?
> > So these nodes are interacting. InteractingNodeSet might be better name then.
> > I'll update the patch that way. Thanks for suggestions, and reviews!
> 
> Yeah. I think that's better. Although, "interacting" sounds to me like the nodes are interacting with each other. So, I think InteractiveNodeSet would be a little less confusing.

How about NodesInUserInteraction or NodesWithUserAction?

[Hajime Morrita]

Created attachment 177178 [details]
Patch

[Hajime Morrita]

Comment on attachment 176930 [details]
Patch

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

Thanks for the review, Antti! I updated the patch.

>> Source/WebCore/dom/Node.cpp:919
>>  }
> 
> respondingNodeIs function seems bit unnecessary. Why not just call respondingNodes()->isActive() from Node::active() directly, similar to the setters? Then Node won't have to know about the flags at all.

I tried to hide RespondingNodeSet.h in Node.cpp. But it might have been overkill.
I moved it to Node.h and get rid of this indirection.

>> Source/WebCore/dom/Node.h:326
>> +    bool responding() const { return getFlag(IsRespondingFlag); }
> 
> isResponding(). The existing accessors don't follow the pattern but there is not need why new ones wouldn't.

Right done.

>>>> Source/WebCore/dom/Node.h:334
>>>> +    };
>>> 
>>> Instead of being in Node, RespondingNodeFlags might be more natural as RespondingNodeSet::Flags.
>>> 
>>> I'm not sure "responding" really captures these well. Responding to what? I don't have better suggestions though.
>> 
>> The CSS spec calls these "User Action pseudo classes". It's a bit wordy, but InteractivePseudoFlags maybe?
> 
> So these nodes are interacting. InteractingNodeSet might be better name then.
> I'll update the patch that way. Thanks for suggestions, and reviews!

I moved these flags to UserActionElementSet (yes, this is new name).

>> Source/WebCore/dom/RespondingNodeSet.h:39
>> +class RespondingNodeSet {
> 
> Do these flags ever have meaning on non-Elements? If not this could be tightened to be RespondingElementSet.

Right. These for selector matching thus makes sense only for Elements. Updated patch reflects this fact.

>> Source/WebCore/dom/RespondingNodeSet.h:64
>> +    void documentRemovedLastRef() { m_nodes.clear(); }
> 
> documentDidRemoveLastRef

Renamed.

[Darin Adler]

I would not call this “cleanup”. I retitled the bug.

[Adam Barth]

Comment on attachment 177178 [details]
Patch

Causing many failures in fast/html/details-*

[Hajime Morrita]

Created attachment 177868 [details]
Patch

[Hajime Morrita]

> Created an attachment (id=177868) [details]
> Patch
Fixed broken tests, addressed misleading title.

[Antti Koivisto]

Comment on attachment 177868 [details]
Patch

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

> Source/WebCore/dom/Node.h:712
> +        IsInUserActionFlag = 1 << 10,

IsUserActionElementFlag/IsInUserActionElementSetFlag or similar.

> Source/WebCore/dom/UserActionElementSet.h:48
> +public:
> +    enum ElementFlags {
> +        IsActiveFlag      = 1 ,
> +        InActiveChainFlag = 1 << 1,
> +        IsHoveredFlag     = 1 << 2,
> +        IsFocusedFlag     = 1 << 3
> +    };

I think this enum can be private.

> Source/WebCore/dom/UserActionElementSet.h:58
> +    static PassOwnPtr<UserActionElementSet> create() { return adoptPtr(new UserActionElementSet()); }
> +    static bool isFocused(const Node* node) { return hasFlags(node, IsFocusedFlag); }
> +    static void setFocused(Node* node, bool enable) { setFlags(node, enable, IsFocusedFlag); }
> +    static bool isActive(const Node* node) { return hasFlags(node, IsActiveFlag); }
> +    static void setActive(Node* node, bool enable) { setFlags(node, enable, IsActiveFlag); }
> +    static bool isInActiveChain(const Node* node) { return hasFlags(node, InActiveChainFlag); }
> +    static void setInActiveChain(Node* node, bool enable) { setFlags(node, InActiveChainFlag, enable); }
> +    static bool isHovered(const Node* node) { return hasFlags(node, IsHoveredFlag); }
> +    static void setHovered(Node* node, bool enable) { setFlags(node, enable, IsHoveredFlag); }

Jumping trough these static functions seems strange and unnecessary. Why is this done?

[Hajime Morrita]

Comment on attachment 177868 [details]
Patch

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

>> Source/WebCore/dom/Node.h:712
>> +        IsInUserActionFlag = 1 << 10,
> 
> IsUserActionElementFlag/IsInUserActionElementSetFlag or similar.

Sure. Will fix.

>> Source/WebCore/dom/UserActionElementSet.h:48
>> +    };
> 
> I think this enum can be private.

Right. will do.

>> Source/WebCore/dom/UserActionElementSet.h:58
>> +    static void setHovered(Node* node, bool enable) { setFlags(node, enable, IsHoveredFlag); }
> 
> Jumping trough these static functions seems strange and unnecessary. Why is this done?

Yeah, ideally we could write document()->userActionElementSet().isFocused() or something
but we need some kind of indirection like this
since neither Node.h and UserActionElement.h cannot depend on Document which owns UserActionElement.
We can access Document::userActionElementSet() only inside .cpp files due to this dependency restriction.

But I agree that having many static functions looks strange. Will try to move them to non-statics.

[WebKit Review Bot]

Comment on attachment 177868 [details]
Patch

Attachment 177868 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15158538

New failing tests:
touchadjustment/touch-links-active.html
fast/events/touch/gesture/gesture-tap-active-state.html
editing/selection/editable-links.html
fast/css/hover-active-drag.html
fast/forms/search-cancel-button-mouseup.html
fast/events/touch/gesture/gesture-tap-active-state-iframe.html

[WebKit Review Bot]

Comment on attachment 177868 [details]
Patch

Attachment 177868 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15157612

New failing tests:
touchadjustment/touch-links-active.html
fast/events/touch/gesture/gesture-tap-active-state.html
editing/selection/editable-links.html
fast/css/hover-active-drag.html
fast/forms/search-cancel-button-mouseup.html
fast/events/touch/gesture/gesture-tap-active-state-iframe.html

[Hajime Morrita]

Created attachment 177918 [details]
Patch

[Hajime Morrita]

(In reply to comment #25)
> Created an attachment (id=177918) [details]
> Patch
Addressed Antti's point, fixed broken test (which was caused by a bad copy-n-paste)

[Build Bot]

Comment on attachment 177918 [details]
Patch

Attachment 177918 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15170594

[Hajime Morrita]

Created attachment 178126 [details]
Patch

[Hajime Morrita]

Bots are all green!
Antti, could you take another look when you have time?

[Ryosuke Niwa]

Comment on attachment 178126 [details]
Patch

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

I’ve started to think that maybe it’s better to make NodeRareData deletable.

> Source/WebCore/ChangeLog:13
> +        aElementFlags.

Typo: aElementFlags.

> Source/WebCore/ChangeLog:22
> +        flags. However, there are certain differences, that are why such a
> +        data structure is worth adding:

This sentence doesn’t read smoothly. How about something like "however, the following characteristics of these flags make adding a new data structure on Document an attractive alternative".

[Darin Adler]

Comment on attachment 178126 [details]
Patch

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

> Source/WebCore/dom/UserActionElementSet.cpp:38
> +    if (Document* doc = node->document())

Please don’t abbreviate document to doc in new code. Just use the whole word.

[Hajime Morrita]

Created attachment 178450 [details]
Patch

[Hajime Morrita]

Comment on attachment 178126 [details]
Patch

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

Ryosuke, Dari, Thanks for the review!
I updated the patch to address these points.

>> Source/WebCore/ChangeLog:13
>> +        aElementFlags.
> 
> Typo: aElementFlags.

Fixed.

>> Source/WebCore/ChangeLog:22
>> +        data structure is worth adding:
> 
> This sentence doesn’t read smoothly. How about something like "however, the following characteristics of these flags make adding a new data structure on Document an attractive alternative".

Sounds good. I copy-pasted this.

>> Source/WebCore/dom/UserActionElementSet.cpp:38
>> +    if (Document* doc = node->document())
> 
> Please don’t abbreviate document to doc in new code. Just use the whole word.

Right. I should get rid of my bad habit :-/

[Hajime Morrita]

Created attachment 178696 [details]
Patch

[Hajime Morrita]

I talked Antti at IRC and updated the patch to take his advice.

[Ryosuke Niwa]

Comment on attachment 178696 [details]
Patch

Looks fine to me.

[Hajime Morrita]

Comment on attachment 178696 [details]
Patch

Thanks! Landing.

[Antti Koivisto]

Yeh, looks good.

[WebKit Review Bot]

Comment on attachment 178696 [details]
Patch

Clearing flags on attachment: 178696

Committed r137277: <http://trac.webkit.org/changeset/137277>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.