Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
246203004:
Fixed unnecessary Paste Popup showing issue when long press on ReadOnly/Disabled elements. (Closed)
|
Created:
6 years, 8 months ago by AKVT Modified:
6 years, 2 months ago CC:
blink-reviews, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, pdr., ojan, rune+blink Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionPaste Popup showing is blocked for ReadOnly/Disabled input and textarea elements.
As per isContentEditable() semantics, it should return true for editable elements,
not for READ-ONLY and DISABLED elements.
Editable element check is not taken care of disabled and readonly elements
after getting the hittest result. Hence causing this issue. In this patch we
are checking whether the element is readonly or disabled and not allowing
paste popup to show.
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183874
Patch Set 1 #
Total comments: 1
Patch Set 2 : Updated code based on review comments. #
Total comments: 2
Patch Set 3 : Added Layout Test cases for this change #
Total comments: 19
Patch Set 4 : Updated LayoutTest HTML based on review comments. #
Total comments: 4
Patch Set 5 : Updated Layout Test case HTML based on review comments. #
Total comments: 3
Patch Set 6 : #Patch Set 7 : Fixed the review comments and refined Layout Test case. #Patch Set 8 : Refined the test case and added expected.txt #Patch Set 9 : Refined the Layout test case. #
Total comments: 14
Patch Set 10 : Fixed the review comments and optimized the Layout test. #Patch Set 11 : Fixed the typo in Layout test. #
Messages
Total messages: 50 (4 generated)
The CQ bit was checked by ajith.v@samsung.com
The CQ bit was unchecked by ajith.v@samsung.com
This change can solve the issue of showing Paste Popup window when we do long press on disabled/readonly input/textarea/contenteditable elements. PTAL this patch.
Added more reviewers.
PTAL
Blink convention is to select the minimal set of reviewers needed for your change. I've removed all current reviewers. I'd suggest picking someone who's familiar with editing (perhaps tkent@chromium.org).
On 2014/04/23 08:38:07, dcheng wrote: > Blink convention is to select the minimal set of reviewers needed for your > change. I've removed all current reviewers. I'd suggest picking someone who's > familiar with editing (perhaps mailto:tkent@chromium.org). Also, I think you still need to sign the individual CLA (see http://dev.chromium.org/developers/contributing-code/external-contributor-che... for directions).
PTAL
Delegate to yosin and yutak because editing-related.
Hi Yoshi and Yuta Kitamura, Please review this change.
The change looks legitimate, but could you please write a test?
https://codereview.chromium.org/246203004/diff/1/Source/core/rendering/HitTes... File Source/core/rendering/HitTestResult.cpp (right): https://codereview.chromium.org/246203004/diff/1/Source/core/rendering/HitTes... Source/core/rendering/HitTestResult.cpp:383: if (innerElement->isFormControlElement()) { It seems description and this change aren't matched, description says for contenteditable, but change for form control. Readonly and disabled text field should be checked within L390 and L393, which check text area and text field.
On 2014/04/25 01:07:19, Yoshi wrote: > https://codereview.chromium.org/246203004/diff/1/Source/core/rendering/HitTes... > File Source/core/rendering/HitTestResult.cpp (right): > > https://codereview.chromium.org/246203004/diff/1/Source/core/rendering/HitTes... > Source/core/rendering/HitTestResult.cpp:383: if > (innerElement->isFormControlElement()) { > It seems description and this change aren't matched, description says for > contenteditable, but change for form control. > > Readonly and disabled text field should be checked within L390 and L393, which > check text area and text field. @Yoshi Thanks for reviewing. I have fixed the review comments, and will upload as next patch set.
On 2014/04/25 00:23:32, Yuta Kitamura wrote: > The change looks legitimate, but could you please > write a test? I have prepared the layout test cases but while verifying on Android build, I am facing some font installation error. So I am not able to verify it yet. Once it's done I will upload it as next patch set with review comment fixes.
@Yoshi PTAL I have updated the review comments. @Yuta, Could you pls point out an existing test case which I can refer for writing test case for this change.
Could you write layout test for this? https://codereview.chromium.org/246203004/diff/20001/Source/core/rendering/Hi... File Source/core/rendering/HitTestResult.cpp (right): https://codereview.chromium.org/246203004/diff/20001/Source/core/rendering/Hi... Source/core/rendering/HitTestResult.cpp:381: if (isHTMLTextAreaElement(*m_innerNonSharedNode) && !toHTMLFormControlElement(m_innerNonSharedNode)->isDisabledOrReadOnly()) nit: Can we move |isDisabledOrReadOnly()| call to |return| statement? We don't need to call |isHTMLInputELement()| and |rendereIsEditable()|. https://codereview.chromium.org/246203004/diff/20001/Source/core/rendering/Hi... Source/core/rendering/HitTestResult.cpp:384: if (isHTMLInputElement(*m_innerNonSharedNode) && !toHTMLFormControlElement(m_innerNonSharedNode)->isDisabledOrReadOnly()) nit: Can we move |isDisabledOrReadOnly()| call to |return| statement? We don't need to call and |rendereIsEditable()|.
See "LayoutTests/fast/events", |window.eventSender| can fire "mousemove", "mousedown", etc.
@Yosi and Yuta PTAL. I have uploaded layout test cases. Please review and let me know your comments.
Please use function abstraction for each step. Many statements are hard to read. https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... File LayoutTests/editing/selection/readonly-disabled-hittest.html (right): https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:3: <body onload="test();"> nit: Please decrease indentation. https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:50: var readOnlyText = document.getElementById("readOnlyText"); nit: Please fix indentation. https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:66: //child = document.createTextNode("<br/>"); nit: Please remove commented unused statement. https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:70: //Disabled Text nit: Please insert a space after "//". https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:71: var disabledText = document.getElementById("disabledText"); nit: Please fix indentation. https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:90: var readOnlyDisabledText = document.getElementById("readOnlyDisabledText"); nit: Could you fix indentation? https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:106: //child = document.createTextNode("<br/>"); nit: Could you remove comment out statement? https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:108: document.getElementById('result').appendChild(child); nit: Please use one kind of quotation mark. I recommend to use single-quote in JS embedded in HTML. https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Strings
C++ change looks okay but the test needs more work. https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... File LayoutTests/editing/selection/readonly-disabled-hittest.html (right): https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:2: <html> Indentation and formatting are horribly broken in this file. We generally require the test files to follow the Blink coding style, so please correct the formatting so that people can easily read the tests. As an optional suggestion, I recommend taking a look at and using our JS test framework; we have two flavors and you can use either of them: resources/js-test.js (WebKit in-house utility) and resources/testharness.js (W3C test suite format). https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:4: <input id="normalText" type="text" value="Normal input" /> "/>" notation is for XML. As long as you use HTML5 doctype you don't have to use this notation. Just write the open tag.
@Yuta and Yosi, I have updated formatting in HTML. PTAL https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... File LayoutTests/editing/selection/readonly-disabled-hittest.html (right): https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:3: <body onload="test();"> On 2014/05/29 03:34:11, yosi wrote: > nit: Please decrease indentation. Done. https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:4: <input id="normalText" type="text" value="Normal input" /> On 2014/05/29 06:12:30, Yuta Kitamura wrote: > "/>" notation is for XML. As long as you use HTML5 doctype > you don't have to use this notation. Just write the open tag. Done. https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:50: var readOnlyText = document.getElementById("readOnlyText"); On 2014/05/29 03:34:11, yosi wrote: > nit: Please fix indentation. Done. https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:66: //child = document.createTextNode("<br/>"); On 2014/05/29 03:34:11, yosi wrote: > nit: Please remove commented unused statement. Done. https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:70: //Disabled Text On 2014/05/29 03:34:11, yosi wrote: > nit: Please insert a space after "//". Done. https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:71: var disabledText = document.getElementById("disabledText"); On 2014/05/29 03:34:11, yosi wrote: > nit: Please fix indentation. Done. https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:90: var readOnlyDisabledText = document.getElementById("readOnlyDisabledText"); On 2014/05/29 03:34:11, yosi wrote: > nit: Could you fix indentation? Done. https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:106: //child = document.createTextNode("<br/>"); On 2014/05/29 03:34:11, yosi wrote: > nit: Could you remove comment out statement? Done. https://codereview.chromium.org/246203004/diff/60001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:108: document.getElementById('result').appendChild(child); On 2014/05/29 03:34:11, yosi wrote: > nit: Please use one kind of quotation mark. > I recommend to use single-quote in JS embedded in HTML. > https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Strings Done.
https://codereview.chromium.org/246203004/diff/80001/LayoutTests/editing/sele... File LayoutTests/editing/selection/readonly-disabled-hittest.html (right): https://codereview.chromium.org/246203004/diff/80001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:3: <body onload="test();"> Since test files are governed by Blink coding style, you should use 4-space indent in test files. Generally I recommend doing the following: - Don't indent for nested HTML elements, and - Use 4-space indent for JavaScript indentations. https://codereview.chromium.org/246203004/diff/80001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:5: <br/> This is XML-style, too.
@Yosi & Yuta, PTAL. I have updated review comments. Please review the contextClick() calls are sufficient to verify this code change. Or do we have any other way to replicate long press on individual element ? https://codereview.chromium.org/246203004/diff/80001/LayoutTests/editing/sele... File LayoutTests/editing/selection/readonly-disabled-hittest.html (right): https://codereview.chromium.org/246203004/diff/80001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:3: <body onload="test();"> On 2014/05/29 07:56:09, Yuta Kitamura wrote: > Since test files are governed by Blink coding style, > you should use 4-space indent in test files. > > Generally I recommend doing the following: > - Don't indent for nested HTML elements, and > - Use 4-space indent for JavaScript indentations. Done. https://codereview.chromium.org/246203004/diff/80001/LayoutTests/editing/sele... LayoutTests/editing/selection/readonly-disabled-hittest.html:5: <br/> On 2014/05/29 07:56:09, Yuta Kitamura wrote: > This is XML-style, too. Done.
@Yosi & Yuta, PTAL
Function |test| is too long. Please break it up. https://codereview.chromium.org/246203004/diff/100001/LayoutTests/editing/sel... File LayoutTests/editing/selection/readonly-disabled-hittest.html (right): https://codereview.chromium.org/246203004/diff/100001/LayoutTests/editing/sel... LayoutTests/editing/selection/readonly-disabled-hittest.html:37: if (items.length > 0) { Why do we need to check result of eventSender.contextClick()? It always return same value. https://codereview.chromium.org/246203004/diff/100001/LayoutTests/editing/sel... LayoutTests/editing/selection/readonly-disabled-hittest.html:51: if (!window.eventSender) nit: We don't need to check |window.eventSender| again. This is already done at L32.
I see too much duplicate contents in tests, please consider grouping some common operations into a function. Also, as I suggested earlier, consider using one of the existing JS test facilities. You don't have to invent the equality test by yourself. https://codereview.chromium.org/246203004/diff/100001/Source/core/rendering/H... File Source/core/rendering/HitTestResult.cpp (right): https://codereview.chromium.org/246203004/diff/100001/Source/core/rendering/H... Source/core/rendering/HitTestResult.cpp:387: return (!toHTMLFormControlElement(m_innerNonSharedNode)->isDisabledOrReadOnly() && toHTMLInputElement(*m_innerNonSharedNode).isTextField()); nit: Outermost parentheses are not necessary
On 2014/06/03 07:01:26, Yuta Kitamura wrote: > I see too much duplicate contents in tests, please consider > grouping some common operations into a function. > > Also, as I suggested earlier, consider using one of > the existing JS test facilities. You don't have to invent > the equality test by yourself. > > https://codereview.chromium.org/246203004/diff/100001/Source/core/rendering/H... > File Source/core/rendering/HitTestResult.cpp (right): > > https://codereview.chromium.org/246203004/diff/100001/Source/core/rendering/H... > Source/core/rendering/HitTestResult.cpp:387: return > (!toHTMLFormControlElement(m_innerNonSharedNode)->isDisabledOrReadOnly() && > toHTMLInputElement(*m_innerNonSharedNode).isTextField()); > nit: Outermost parentheses are not necessary @Yuta - Can I write Unit test case instead of Layout Test case ? If Layout test is mandatory I will do that.
For layout tests, - Please use LayoutTests/resources/js-test.js; it offers testing framework. - Please use function abstraction to encapsulate each mouse click; current test script is hard to maintain.
On 2014/06/18 15:56:29, AJITH KUMAR V wrote: > @Yuta - Can I write Unit test case instead of Layout Test case ? > If Layout test is mandatory I will do that. Unit tests and layout tests serve for different purposes; a unit test basically checks a small functionality of the code in question, while a layout test exercises the entire codebase and tests user-visible functionality. Historically we have been requiring only layout tests, so it's fine not to have unit tests. It'd be awesome if you can add some, though.
@Yosin and Yuta, Could you please help me to write the relevant content for executing this code. I modified HitTestResult.isContentEditable() function, which is not able to get invoked using Javascript. I will take care of modularizing the HTML, once I get the JS method which will invoke this method.
On 2014/07/24 14:32:03, AKV wrote: > @Yosin and Yuta, Could you please help me to write the relevant content for > executing this code. I modified HitTestResult.isContentEditable() function, > which is not able to get invoked using Javascript. I will take care of > modularizing the HTML, once I get the JS method which will invoke this method. I'm sorry but I could not understand what you meant...
On 2014/07/25 09:49:36, Yuta Kitamura wrote: > On 2014/07/24 14:32:03, AKV wrote: > > @Yosin and Yuta, Could you please help me to write the relevant content for > > executing this code. I modified HitTestResult.isContentEditable() function, > > which is not able to get invoked using Javascript. I will take care of > > modularizing the HTML, once I get the JS method which will invoke this method. > > I'm sorry but I could not understand what you meant... Sorry for the ambiguity. While I was writing the layout test case for this change, I am not able to find an equivalent javascript which will internally invoke this function on the DOM side. I am planning to do eventSender.mouseMoveTo(x, y); eventSender.mouseDown(x, y); wait for a period of some seconds and do eventSender.mouseUp(x, y). But still my code change will not get invoked with this change. It just do a hitTest only on DOM. Checking of isContentEditable() won't be triggered. So I am stuck while writing the test case. I hope now my point is clear to you.
@Yosin and Yutak, I have incorporated the review comments with this Patch Set. PTAL. Sorry for the delay in submitting the patch, I was learning the Layout Test writing tricks.
It seems you forget to upload LayoutTests/editing/selection/readonly-disabled-hittest-expected.txt
On 2014/09/04 00:32:45, Yosi_UTC9 wrote: > It seems you forget to upload > LayoutTests/editing/selection/readonly-disabled-hittest-expected.txt @Yosi - While running full Layout test case, I found some failures, so I am correcting it and will upload expected.txt and refined test case soon. Sorry for the delay.
tkent@chromium.org changed reviewers: - tkent@chromium.org
@Yosi & Yuta PTAL
Looks mostly fine to me. Mostly stylistic comments. https://codereview.chromium.org/246203004/diff/200001/LayoutTests/editing/sel... File LayoutTests/editing/selection/readonly-disabled-hittest.html (right): https://codereview.chromium.org/246203004/diff/200001/LayoutTests/editing/sel... LayoutTests/editing/selection/readonly-disabled-hittest.html:5: <script src="../../touchadjustment/resources/touchadjustment.js"></script> Using this code sounds like an abuse to me, because this test has nothing to do with touch adjustments. Seems like the dependency to this js file is tiny, so you can just implement the necessary function here. https://codereview.chromium.org/246203004/diff/200001/LayoutTests/editing/sel... LayoutTests/editing/selection/readonly-disabled-hittest.html:15: debug("gestureLongPress not implemented by this platform."); It would be nice if you can add some instruction text indicating how to run the test manually. https://codereview.chromium.org/246203004/diff/200001/LayoutTests/editing/sel... LayoutTests/editing/selection/readonly-disabled-hittest.html:56: <body onload="test();"> I would recommend setting jsTestIsAsync to true (and calling finishJSTest() in the end). If you do so, the test prints "TEST COMPLETED" line after the "PASS" lines. https://codereview.chromium.org/246203004/diff/200001/LayoutTests/platform/an... File LayoutTests/platform/android/editing/selection/readonly-disabled-hittest-expected.txt (right): https://codereview.chromium.org/246203004/diff/200001/LayoutTests/platform/an... LayoutTests/platform/android/editing/selection/readonly-disabled-hittest-expected.txt:1: PASS successfullyParsed is true IIUC, gestureLongPress() is defined on every platform we support (note that we have touch supports on desktops nowadays). Therefore, this -expected.txt file should reside next to the test file. https://codereview.chromium.org/246203004/diff/200001/LayoutTests/platform/an... LayoutTests/platform/android/editing/selection/readonly-disabled-hittest-expected.txt:4: PASS adjusted node was INPUT#normalText. As mentioned in the test file, I don't think the wording "adjusted node" makes sense for this test. https://codereview.chromium.org/246203004/diff/200001/Source/core/rendering/H... File Source/core/rendering/HitTestResult.cpp (right): https://codereview.chromium.org/246203004/diff/200001/Source/core/rendering/H... Source/core/rendering/HitTestResult.cpp:374: return !toHTMLFormControlElement(m_innerNonSharedNode)->isDisabledOrReadOnly(); nit: Since we know m_innerNodeSharedNode is non-null, it's better to use a reference rather than a pointer, i.e.: !toHTMLFormControlElement(*m_innerNonSharedNode).isDisabledOrReadOnly() nit: toHTMLTextAreaElement() may be slightly better than toHTMLFormControlElement(), because it matches the type check immediately before this line. https://codereview.chromium.org/246203004/diff/200001/Source/core/rendering/H... Source/core/rendering/HitTestResult.cpp:377: return !toHTMLFormControlElement(m_innerNonSharedNode)->isDisabledOrReadOnly() && toHTMLInputElement(*m_innerNonSharedNode).isTextField(); nit: Doing the type cast twice for the same object sounds a bit silly. You can save the reference to HTMLInputElement in a local variable, and then use it to grab the result, i.e.: HTMLInputElement& inputElement = toHTMLInputElement(*m_innerNonSharedNode); return inputElement.isDisabledOrReadOnly() && inputElement.isTextField();
@Yuta & Yosi PTAL. https://codereview.chromium.org/246203004/diff/200001/LayoutTests/editing/sel... File LayoutTests/editing/selection/readonly-disabled-hittest.html (right): https://codereview.chromium.org/246203004/diff/200001/LayoutTests/editing/sel... LayoutTests/editing/selection/readonly-disabled-hittest.html:5: <script src="../../touchadjustment/resources/touchadjustment.js"></script> On 2014/10/16 04:01:23, Yuta Kitamura wrote: > Using this code sounds like an abuse to me, because > this test has nothing to do with touch adjustments. > > Seems like the dependency to this js file is tiny, so > you can just implement the necessary function here. Done. https://codereview.chromium.org/246203004/diff/200001/LayoutTests/editing/sel... LayoutTests/editing/selection/readonly-disabled-hittest.html:15: debug("gestureLongPress not implemented by this platform."); On 2014/10/16 04:01:23, Yuta Kitamura wrote: > It would be nice if you can add some instruction text > indicating how to run the test manually. Done. Thanks https://codereview.chromium.org/246203004/diff/200001/LayoutTests/editing/sel... LayoutTests/editing/selection/readonly-disabled-hittest.html:56: <body onload="test();"> On 2014/10/16 04:01:23, Yuta Kitamura wrote: > I would recommend setting jsTestIsAsync to true (and > calling finishJSTest() in the end). If you do so, > the test prints "TEST COMPLETED" line after the > "PASS" lines. Done. Thank you https://codereview.chromium.org/246203004/diff/200001/LayoutTests/platform/an... File LayoutTests/platform/android/editing/selection/readonly-disabled-hittest-expected.txt (right): https://codereview.chromium.org/246203004/diff/200001/LayoutTests/platform/an... LayoutTests/platform/android/editing/selection/readonly-disabled-hittest-expected.txt:1: PASS successfullyParsed is true On 2014/10/16 04:01:23, Yuta Kitamura wrote: > IIUC, gestureLongPress() is defined on every platform > we support (note that we have touch supports on desktops > nowadays). > > Therefore, this -expected.txt file should reside next to > the test file. Done. Thanks https://codereview.chromium.org/246203004/diff/200001/LayoutTests/platform/an... LayoutTests/platform/android/editing/selection/readonly-disabled-hittest-expected.txt:4: PASS adjusted node was INPUT#normalText. On 2014/10/16 04:01:23, Yuta Kitamura wrote: > As mentioned in the test file, I don't think the wording > "adjusted node" makes sense for this test. Done. Thanks https://codereview.chromium.org/246203004/diff/200001/Source/core/rendering/H... File Source/core/rendering/HitTestResult.cpp (right): https://codereview.chromium.org/246203004/diff/200001/Source/core/rendering/H... Source/core/rendering/HitTestResult.cpp:374: return !toHTMLFormControlElement(m_innerNonSharedNode)->isDisabledOrReadOnly(); On 2014/10/16 04:01:23, Yuta Kitamura wrote: > nit: Since we know m_innerNodeSharedNode is non-null, it's > better to use a reference rather than a pointer, i.e.: > > !toHTMLFormControlElement(*m_innerNonSharedNode).isDisabledOrReadOnly() > > nit: toHTMLTextAreaElement() may be slightly better than > toHTMLFormControlElement(), because it matches the type check > immediately before this line. Done. Thank you https://codereview.chromium.org/246203004/diff/200001/Source/core/rendering/H... Source/core/rendering/HitTestResult.cpp:377: return !toHTMLFormControlElement(m_innerNonSharedNode)->isDisabledOrReadOnly() && toHTMLInputElement(*m_innerNonSharedNode).isTextField(); On 2014/10/16 04:01:23, Yuta Kitamura wrote: > nit: Doing the type cast twice for the same object sounds > a bit silly. > > You can save the reference to HTMLInputElement in a local > variable, and then use it to grab the result, i.e.: > > HTMLInputElement& inputElement = toHTMLInputElement(*m_innerNonSharedNode); > return inputElement.isDisabledOrReadOnly() && inputElement.isTextField(); Done. Thank you
@Yuta & Yosi PTAL PS11
LGTM!
The CQ bit was checked by ajith.v@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/246203004/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/27443)
@Yuta, mac_blink failure logs appears independent of this change. Could you please check it.
Maybe a flake but I'm not sure. You can try to check CQ again to confirm (you don't need my approval to check it again for the same patchset). If the same test fails again, you need to take a look. To check the actual results, follow the link of the red box above, and click the link labeled as "(zip)" in "archive_webkit_tests_results" section. If you need to update your patch to fix the failure, please let me know before CQ'ing again.
The CQ bit was checked by ajith.v@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/246203004/240001
Message was sent while issue was closed.
Committed patchset #11 (id:240001) as 183874 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
