Bug 89439

Created attachment 148278 [details] Patch 1

Comment on attachment 148278 [details] Patch 1 Could you review this patch? Thanks in advance. * This is a part of time fields UI.

Comment on attachment 148278 [details] Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=148278&action=review > Source/WebCore/html/TextFieldInputType.h:87 > > + virtual void spinButtonStepDown() OVERRIDE; > + virtual void spinButtonStepUp() OVERRIDE; nit: Add a comment like "// SpinButtonElement::Callback functions." > Source/WebCore/html/shadow/TextControlInnerElements.cpp:301 > if (m_upDownState != Indeterminate) { > - input->stepUpFromRenderer(m_upDownState == Up ? 1 : -1); > + if (m_upDownState == Up) > + m_callback.spinButtonStepUp(); > + else > + m_callback.spinButtonStepDown(); How do you make sure m_callback object is live after spinButtonStepUp/Down calls, which dispatch 'change' event and JavaScript code change the input type. > Source/WebCore/html/shadow/TextControlInnerElements.h:99 > + class Callback { The name 'Callback' sounds too generic. It should be more specific. e.g. ValueUpdater, ValueChangeDelegate, ... > Source/WebCore/html/shadow/TextControlInnerElements.h:102 > + virtual void spinButtonStepDown() = 0; > + virtual void spinButtonStepUp() = 0; Why do you separate the functionality of stepUpFromRenderer() into two functions? It makes the patch larger.

(In reply to comment #3) > > Source/WebCore/html/shadow/TextControlInnerElements.h:102 > > + virtual void spinButtonStepDown() = 0; > > + virtual void spinButtonStepUp() = 0; > > Why do you separate the functionality of stepUpFromRenderer() into two functions? It makes the patch larger. * We support only 1 or -1. ** It seems stepUp(-1) == stepDown(1) isn't so clear. ** We may not want to check amount is zero or not. ** In stepUpFromRenderer, we have ASSERT(n) and if (!n) return * DateTimeNumericFieldElement, we may not want to write https://bugs.webkit.org/attachment.cgi?id=147253&action=prettypatch void DateTimeNumericFieldElement::spinButtonStep(int amount) { if (!amount) return; if (m_hasValue) if (amount > 0) m_value = m_value == m_minimum ? m_maximum : ensureInRange(m_value - 1); else m_value = m_value == m_maximum ? m_minimum : ensureInRange(m_value + 1); else m_value = amount > 0 ? m_maximum : m_minimum; m_hasValue = true; updateText(); }

Created attachment 148290 [details] Patch 2

(In reply to comment #4) > (In reply to comment #3) > > > Source/WebCore/html/shadow/TextControlInnerElements.h:102 > > > + virtual void spinButtonStepDown() = 0; > > > + virtual void spinButtonStepUp() = 0; > > > > Why do you separate the functionality of stepUpFromRenderer() into two functions? It makes the patch larger. > > * We support only 1 or -1. > ** It seems stepUp(-1) == stepDown(1) isn't so clear. > ** We may not want to check amount is zero or not. > ** In stepUpFromRenderer, we have ASSERT(n) and if (!n) return > * DateTimeNumericFieldElement, we may not want to write > https://bugs.webkit.org/attachment.cgi?id=147253&action=prettypatch > > void DateTimeNumericFieldElement::spinButtonStep(int amount) > { > if (!amount) > return; > > if (m_hasValue) > if (amount > 0) > m_value = m_value == m_minimum ? m_maximum : ensureInRange(m_value - 1); > else > m_value = m_value == m_maximum ? m_minimum : ensureInRange(m_value + 1); > > else > m_value = amount > 0 ? m_maximum : m_minimum; > > m_hasValue = true; > updateText(); > } Another option is using StepDirection enum, although we need to use switch with default ASSERT_NOT_REACHED. It is too long.

Comment on attachment 148290 [details] Patch 2 Could you review this patch? Thanks in advance. = Changes since last patch = * Rename SpinButtonElement::Callback to StepActionHandler * SpinButtonElement keeps m_stepActionHandler as pointer and checks it isn't nullptr before calling action. * Add SpinButtonElement::removeStepActionHandler ** This method will be called from TextFieldInputType dtor and destroyShadowSubtree

Comment on attachment 148290 [details] Patch 2 Attachment 148290 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12988019

Comment on attachment 148290 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=148290&action=review > Source/WebCore/html/InputType.h:299 > + void stepUpFromRenderer(int); We should move stepUpFromRenderer to TextFieldInputType, because only TextFieldInputType uses it. Note: We need to expose applyStep as protected for this. > Source/WebCore/html/shadow/TextControlInnerElements.h:101 > + virtual void spinButtonStepDown() = 0; We should have virtual destructor, clang issues warning for this.

Created attachment 148480 [details] Patch 3

Comment on attachment 148480 [details] Patch 3 Could you review this patch? Thanks in advance. * I would like to use separate methods for Step Up and Step Down. Because, ** it isn't Intuitive that stepUp(-1)==stepDown(1) ** We don't want to check parameter isn't zero and to have ASSERT(n). ** I guess if we have stepUpFromRenderer and stepDownFromRenderer, it code could be simpler. = Changes since last patch = * Rename SpinButtonElement::Callback to StepActionHandler * SpinButtonElement keeps m_stepActionHandler as pointer and checks it isn't nullptr before calling action. * Add SpinButtonElement::removeStepActionHandler ** This method will be called from TextFieldInputType dtor and destroyShadowSubtree * Add virtual destructor to StepButtonElement for Mac-clang * Move InputType::stepUpFromRenderer to TextFieldInputType * Expose InputType::applyStep as protected for stepUpFromRenderer

Comment on attachment 148480 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=148480&action=review > Source/WebCore/html/TextFieldInputType.cpp:-155 > - int step = 0; Not using variable step makes patch size bigger, but new code is smaller than current and there is no extra variable initialization, although it will be removed by compiler but readers need to track how variable step is used. > Source/WebCore/html/TextFieldInputType.cpp:-170 > - int step = 0; Not using variable step makes patch size bigger, but new code is smaller than current and there is no extra variable initialization, although it will be removed by compiler but readers need to track how variable step is used.

Comment on attachment 148480 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=148480&action=review > Source/WebCore/ChangeLog:29 > + (WebCore::TextFieldInputType::stepUpFromRenderer): Moved from InputType. I object moving it. Non-textfield UI might use this feature in the future. This feature is not related to "text field" essentially. > Source/WebCore/html/shadow/TextControlInnerElements.cpp:303 > + if (m_stepActionHandler) { > + if (m_upDownState == Up) > + m_stepActionHandler->spinButtonStepUp(); > + else > + m_stepActionHandler->spinButtonStepDown(); > + } We had better share the code with the next part. > Source/WebCore/html/shadow/TextControlInnerElements.cpp:375 > + if (!m_stepActionHandler) > + return; > + > + if (amount > 0) > + m_stepActionHandler->spinButtonStepUp(); > + else if (amount < 0) > + m_stepActionHandler->spinButtonStepDown(); We had better share the code with the previous part. > Source/WebCore/html/shadow/TextControlInnerElements.h:106 > + static PassRefPtr<SpinButtonElement> create(Document*, StepActionHandler&); We should mention that a StepActionHandler implementation must call removeStepActionHandler().

(In reply to comment #13) > (From update of attachment 148480 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148480&action=review > > > Source/WebCore/ChangeLog:29 > > + (WebCore::TextFieldInputType::stepUpFromRenderer): Moved from InputType. > > I object moving it. Non-textfield UI might use this feature in the future. This feature is not related to "text field" essentially. stepUpFromRenderer is implementation of spin button event action and spin button is part of TextFieldInputType at this time. If another feature wants to use stepUpFromRenderer, it will move back into InputType or another good place. If we could have Steppable class, we would move stepUpFromRenderer to there. Although, RangeInputType should be Steppable but it is different from other Steppable classes. I believe　limiting scope of identifier helps us for future modification and maintenance. Is stepUpBySpinButton better name?

Created attachment 148722 [details] Patch 4

Comment on attachment 148722 [details] Patch 4 Could you review this patch? Thanks in advance. == Changes since last review == * Don't move InputType::stepUpFromRenderer => No changes in InputType.{cpp,h} in this patch. * Introduce SpinButtonElement::doStepAction * Added comment for owner of SpinButtonElement must call removeActionHandler.

Comment on attachment 148722 [details] Patch 4 looks good.

Comment on attachment 148722 [details] Patch 4 Clearing flags on attachment: 148722 Committed r120903: <http://trac.webkit.org/changeset/120903>

All reviewed patches have been landed. Closing bug.