Bug 92602

Summary: Change Element::isReadOnlyFormControl to Element::shouldMatchReadOnlySelector/shouldMatchReadWriteSelector or HTMLFormControlElement::readOnly
Product: WebKit Reporter: yosin
Component: DOMAssignee: yosin
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, eric, macpherson, menard, mifenton, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 92607, 92612, 92712    
Bug Blocks: 92473    
Attachments:
Description Flags
Patch 1
tkent: review-
Patch 2
none
Patch 3
none
Patch 4 none

Description yosin 2012-07-29 19:16:36 PDT
This bug is preparation of bug 92473, :read-only and :read-write selector should match none-isFormisFormControlElement.
Comment 1 yosin 2012-07-29 23:41:30 PDT
*** Bug 92607 has been marked as a duplicate of this bug. ***
Comment 2 yosin 2012-07-30 00:30:49 PDT
Created attachment 155224 [details]
Patch 1
Comment 3 yosin 2012-07-30 00:32:37 PDT
Comment on attachment 155224 [details]
Patch 1

Could you review this patch?
Thanks in advance.
Comment 4 Kent Tamura 2012-07-30 00:47:40 PDT
Comment on attachment 155224 [details]
Patch 1

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

> Source/WebCore/dom/Element.h:354
> +    virtual bool shouldMatchReadWriteSelector() const { return false; }

We should replace isReadOnlyFormControl() with shouldMatchReadWriteSelector in Element.h.
If we have other isReadOnlyFormControl() callsites, we should remove them before this patch.

> Source/WebCore/html/HTMLInputElement.cpp:1479
> +bool HTMLInputElement::shouldMatchReadWriteSelector() const
> +{
> +    return m_inputType->shouldMatchReadWriteSelector();
> +}

We have no reasons to forward it to InputType::shouldMatchReadWriteSelector() for now.
This change just wastes the code size and CPU time.
Comment 5 yosin 2012-07-30 19:16:39 PDT
Element::isReadOnlyFormControl() is used for :read-only and :read-write CSS pseudo class matching. We would like to rename to shouldMatchReadWriteSelector() to make intention of this function explicitly.
Comment 6 yosin 2012-07-30 20:59:01 PDT
Created attachment 155431 [details]
Patch 2
Comment 7 yosin 2012-07-30 21:00:04 PDT
Comment on attachment 155431 [details]
Patch 2

Could you review this patch?
Thanks in advance.
Comment 8 Kent Tamura 2012-07-30 21:07:54 PDT
Comment on attachment 155431 [details]
Patch 2

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

> Source/WebCore/dom/Element.h:353
> +    virtual bool shouldMatchReadWriteSelector() const { return false; }

nit: we had better avoid to add the function body here for a virtual function. It wastes compile-time resources.
Comment 9 yosin 2012-07-30 21:16:59 PDT
Created attachment 155432 [details]
Patch 3
Comment 10 yosin 2012-07-30 21:19:43 PDT
Comment on attachment 155432 [details]
Patch 3

Clearing flags on attachment: 155432

Committed r124171: <http://trac.webkit.org/changeset/124171>
Comment 11 yosin 2012-07-30 21:19:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Review Bot 2012-07-30 22:42:44 PDT
Re-opened since this is blocked by 92712
Comment 13 yosin 2012-07-31 00:16:41 PDT
Created attachment 155452 [details]
Patch 4
Comment 14 yosin 2012-07-31 00:20:19 PDT
Comment on attachment 155452 [details]
Patch 4

Could you review this patch?
Thanks in advance.

= Changes since the last patch =
* Introduce Element::shouldMatchReadOnlySelector, because !shouldMatchReadWriteSelector isn't equal to read only control.
** We found this by failure of fast/css/square-button-appearance.html. This test uses "div" element for rendering square button.
Comment 15 Kent Tamura 2012-07-31 00:47:46 PDT
Comment on attachment 155452 [details]
Patch 4

Looks good.
Comment 16 yosin 2012-07-31 00:59:01 PDT
Comment on attachment 155452 [details]
Patch 4

Clearing flags on attachment: 155452

Committed r124180: <http://trac.webkit.org/changeset/124180>
Comment 17 yosin 2012-07-31 00:59:08 PDT
All reviewed patches have been landed.  Closing bug.