Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2458113003:
Prevent bogus 'search-changed' event firing from cr_search_field_behavior.js. (Closed)
|
Created:
4 years, 1 month ago by dpapad Modified:
4 years, 1 month ago Reviewers:
Dan Beam CC:
chromium-reviews, dbeam+watch-elements_chromium.org, michaelpg+watch-elements_chromium.org, oshima+watch_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent bogus 'search-changed' event firing from cr_search_field_behavior.js.
BUG=660434
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6799603e6ec837f72dee0443f54de1e857457749
Cr-Commit-Position: refs/heads/master@{#428518}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments. #
Total comments: 2
Patch Set 3 : string -> boolean. #Patch Set 4 : ? -> undefined #
Messages
Total messages: 18 (10 generated)
Description was changed from ========== Prevent bogus 'search-changed' event firing from cr_search_field_behavior.js. BUG= ========== to ========== Prevent bogus 'search-changed' event firing from cr_search_field_behavior.js. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Prevent bogus 'search-changed' event firing from cr_search_field_behavior.js. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Prevent bogus 'search-changed' event firing from cr_search_field_behavior.js. BUG=660434 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm but i still don't love clobbering document.body in tests (but it's so common in all our tests now it's probably too late to change...) https://codereview.chromium.org/2458113003/diff/1/chrome/test/data/webui/cr_e... File chrome/test/data/webui/cr_elements/cr_toolbar_search_field_tests.js (right): https://codereview.chromium.org/2458113003/diff/1/chrome/test/data/webui/cr_e... chrome/test/data/webui/cr_elements/cr_toolbar_search_field_tests.js:37: // Test that no bogus 'search-changed' event is fired during construction s/bogus/initial https://codereview.chromium.org/2458113003/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js (right): https://codereview.chromium.org/2458113003/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:95: /** @private */ can we annotate @params here? https://codereview.chromium.org/2458113003/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:97: // Prevent bogus 'search-changed' event from firing on startup. s/bogus/initial
Regarding clobbering document.body, yes we've been doing this from the very beginning AFAIU (https://cs.chromium.org/chromium/src/chrome/test/data/webui/polymer_browser_t...). Also, personally I don't find anything wrong with it. We use browserPreload URL as a way to load all dependencies of a unit test, and then we clear document.body to populate the DOM programmatically (either with appendChild() or with innerHTML). The alternative is to have an HTML file accommodating every test, so that no programmatic DOM manipulation is necessary, but I don't see how this would be much better. https://codereview.chromium.org/2458113003/diff/1/chrome/test/data/webui/cr_e... File chrome/test/data/webui/cr_elements/cr_toolbar_search_field_tests.js (right): https://codereview.chromium.org/2458113003/diff/1/chrome/test/data/webui/cr_e... chrome/test/data/webui/cr_elements/cr_toolbar_search_field_tests.js:37: // Test that no bogus 'search-changed' event is fired during construction On 2016/10/28 at 20:54:06, Dan Beam wrote: > s/bogus/initial Done. https://codereview.chromium.org/2458113003/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js (right): https://codereview.chromium.org/2458113003/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:95: /** @private */ On 2016/10/28 at 20:54:06, Dan Beam wrote: > can we annotate @params here? Done. https://codereview.chromium.org/2458113003/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:97: // Prevent bogus 'search-changed' event from firing on startup. On 2016/10/28 at 20:54:06, Dan Beam wrote: > s/bogus/initial Done, actually s/bogus/unnecessary, I think this better reflects the intention than "initial"
https://codereview.chromium.org/2458113003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js (right): https://codereview.chromium.org/2458113003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:97: * @param {?string} previous string|undefined
https://codereview.chromium.org/2458113003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js (right): https://codereview.chromium.org/2458113003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:97: * @param {?string} previous On 2016/10/28 at 21:14:08, Dan Beam wrote: > string|undefined Done. Also it should be boolean not string.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2458113003/#ps60001 (title: "? -> undefined")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Prevent bogus 'search-changed' event firing from cr_search_field_behavior.js. BUG=660434 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Prevent bogus 'search-changed' event firing from cr_search_field_behavior.js. BUG=660434 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6799603e6ec837f72dee0443f54de1e857457749 Cr-Commit-Position: refs/heads/master@{#428518} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6799603e6ec837f72dee0443f54de1e857457749 Cr-Commit-Position: refs/heads/master@{#428518} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
