Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(74)

Issue 948373002: Revert of Simplify form validation handling (Closed)

Created:
5 years, 10 months ago by hiroshige
Modified:
5 years, 10 months ago
Reviewers:
keishi, tkent
CC:
blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, spartha
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Revert of Simplify form validation handling (patchset #1 id:20001 of https://codereview.chromium.org/943303002/) Reason for revert: This CL breaks browser test: AllForms_FormStructureBrowserTest.DataDrivenHeuristics_108 http://build.chromium.org/p/chromium.webkit/builders/Mac10.8%20Tests/builds/12230 http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/42755 http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tests%20%281%29/builds/10647 http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/6767 Bisected locally on Linux: ./out/Release/browser_tests --gtest_filter=AllForms/FormStructureBrowserTest.DataDrivenHeuristics/108 Blink 190722 / Chromium 317759: OK Blink 190723 / Chromium 317759: NG So Blink 190723 is reverted. Original issue's description: > Simplify form validation handling > > This CL contains some part of https://codereview.chromium.org/460343004/ by > sudarshan.p, which was reverted by a performance regression. > > This CL simplifies form validation state handling code by > - lazy update of m_isValid > setNeedsValidityCheck just makes m_isValid dirty. > - m_isValid takes care of willValidate state too. > m_isValid = !willValidate() || valid() > > We switched to invalidation sets for :valid and :invalid by Blink > r184392. Calling pseudoStateChanged() for unchanged validity state won't have so > bad performance. So, we can evaluate m_isValid lazily. > > We need to change the meaning of m_isValid slightly so that it takes care of > willValidate(). This is necessary to clean m_validityIsDirty of associated > controls in HTMLFormElement::checkInvalidControlsAndCollectUnhandled. > Without this change, there would be a case where willValidate state > change won't invalidate :valid :invalid style of <form>. > > This CL will have a few percent regression on blink_perf.dom:textarea-edit. > A fix for the performance regression will be landed soon. > > BUG= > > Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190723 TBR=keishi@chromium.org,tkent@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190744

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+77 lines, -35 lines) Patch
M Source/core/html/HTMLFieldSetElement.h View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/html/HTMLFieldSetElement.cpp View 1 chunk +9 lines, -0 lines 0 comments Download
M Source/core/html/HTMLFormControlElement.h View 3 chunks +5 lines, -2 lines 0 comments Download
M Source/core/html/HTMLFormControlElement.cpp View 4 chunks +21 lines, -27 lines 0 comments Download
M Source/core/html/HTMLFormElement.h View 2 chunks +6 lines, -0 lines 0 comments Download
M Source/core/html/HTMLFormElement.cpp View 4 chunks +34 lines, -6 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
hiroshige
Created Revert of Simplify form validation handling
5 years, 10 months ago (2015-02-24 11:27:04 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/948373002/1
5 years, 10 months ago (2015-02-24 11:27:50 UTC) #2
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://src.chromium.org/viewvc/blink?view=rev&revision=190744
5 years, 10 months ago (2015-02-24 11:28:31 UTC) #3
tkent
5 years, 10 months ago (2015-02-24 22:00:14 UTC) #4
Message was sent while issue was closed.
lgtm.  thank you for gardening.

Powered by Google App Engine
This is Rietveld 408576698