Bug 92602 - Change Element::isReadOnlyFormControl to Element::shouldMatchReadOnlySelector/shouldMatchReadWriteSelector or HTMLFormControlElement::readOnly
Summary: Change Element::isReadOnlyFormControl to Element::shouldMatchReadOnlySelector...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL:
Keywords:
: 92607 (view as bug list)
Depends on: 92607 92612 92712
Blocks: 92473
  Show dependency treegraph
 
Reported: 2012-07-29 19:16 PDT by yosin
Modified: 2012-07-31 00:59 PDT (History)
7 users (show)

See Also:


Attachments
Patch 1 (8.08 KB, patch)
2012-07-30 00:30 PDT, yosin
tkent: review-
Details | Formatted Diff | Diff
Patch 2 (11.23 KB, patch)
2012-07-30 20:59 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 3 (11.85 KB, patch)
2012-07-30 21:16 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 4 (14.03 KB, patch)
2012-07-31 00:16 PDT, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.