Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2185493003:
MD Settings: Adding some unit tests for <settings-main>. (Closed)
|
Created:
4 years, 4 months ago by dpapad Modified:
4 years, 4 months ago Reviewers:
michaelpg CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, Dan Beam, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@search_no_results Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Adding some unit tests for <settings-main>.
Specifically testing the "no results" message is shown/hidden as expected.
- Splitting SearchManager to an interface and an implementation.
- Using a TestSearchManager class (implements SearchManager) for testing
<settings-main>.
BUG=630383
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/bb8fba2c2dbc0b02555f11877f4929786eab456c
Cr-Commit-Position: refs/heads/master@{#409615}
Patch Set 1 : Nit. #
Total comments: 28
Patch Set 2 : Address comments. #
Total comments: 7
Patch Set 3 : Address comments. #Patch Set 4 : Resolve conflicts with ToT. #
Messages
Total messages: 53 (38 generated)
Description was changed from ========== MD Settings: Adding some unit tests for <settings-main>. BUG= ========== to ========== MD Settings: Adding some unit tests for <settings-main>. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Adding some unit tests for <settings-main>. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Adding some unit tests for <settings-main>. Specifically testing the the "no results" message is shown/hidden as expected. BUG= 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Patchset #8 (id:140001) has been deleted
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...
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_...)
Description was changed from ========== MD Settings: Adding some unit tests for <settings-main>. Specifically testing the the "no results" message is shown/hidden as expected. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Adding some unit tests for <settings-main>. Specifically testing the "no results" message is shown/hidden as expected. - Splitting SearchManager to an interface and an implementation. - Using a TestSearchManager class (implements SearchManager) for testing <settings-main> BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Patchset #11 (id:220001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Description was changed from ========== MD Settings: Adding some unit tests for <settings-main>. Specifically testing the "no results" message is shown/hidden as expected. - Splitting SearchManager to an interface and an implementation. - Using a TestSearchManager class (implements SearchManager) for testing <settings-main> BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Adding some unit tests for <settings-main>. Specifically testing the "no results" message is shown/hidden as expected. - Splitting SearchManager to an interface and an implementation. - Using a TestSearchManager class (implements SearchManager) for testing <settings-main>. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dpapad@chromium.org changed reviewers: + michaelpg@chromium.org
Description was changed from ========== MD Settings: Adding some unit tests for <settings-main>. Specifically testing the "no results" message is shown/hidden as expected. - Splitting SearchManager to an interface and an implementation. - Using a TestSearchManager class (implements SearchManager) for testing <settings-main>. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Adding some unit tests for <settings-main>. Specifically testing the "no results" message is shown/hidden as expected. - Splitting SearchManager to an interface and an implementation. - Using a TestSearchManager class (implements SearchManager) for testing <settings-main>. BUG=630383 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Thanks for adding tests! If you'd like to discuss the Promise/setTimeout stuff in a separate CL, let me know. Or if you've already heard everything I said from LAX people let me know. https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:494: // 'finisthed', if any, and droping all pending tasks. finished https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:494: // 'finisthed', if any, and droping all pending tasks. dropping https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:195: * @param {string} query @return https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:198: var promiseResolver = new PromiseResolver(); this promise/setTimeout business is scary, if we're adding more I'm gonna review the whole function. https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:198: var promiseResolver = new PromiseResolver(); ok, after looking over it... we're resolving this promise within the same function, why use a PromiseResolver instead of returning a new Promise directly? To me, it doesn't look any messier that way and it's one fewer state to keep track of. Is it just the desire to avoid the extra indent? https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:208: query, assert(this.$$('settings-basic-page'))); We've gotten into trouble by assuming that doing something asynchronously automagically means a property is still the same, or the current DOM reflects the property. In particular, searchContents is triggered by an event which has no correlation with currentRoute. I guess these asserts are fine until they break; then we'll have to decide whether to enforce the page exists here or to stop if the page doesn't exist. https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:210: setTimeout(function() { SearchManager is a singleton, has only one task queue, and calls requestIdleCallback before starting any serious work, which means everything is FIFO and async. So why does settings-main have to use two setTimeouts? We are requesting everything to be rendered, waiting an event loop, and then queueing tasks -- seems like we can call search() twice in a row with no downside. The DOM traversals happen inside separate requestIdleCallbacks which means which means the browser will likely schedule a gap between them[1], and the browser is likely to run both of these setTimeouts before working on the idle task list anyway since the timeout is 0. If I'm missing something, then IMO SearchManager should be updated to work better so users don't have to throttle it manually. [1] https://www.w3.org/TR/requestidlecallback/#dfn-invoke-idle-callbacks-algorithm https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:212: query, assert(this.$$('settings-advanced-page'))).then( this ".then" is okay (I think) because this line actually returns the same Promise object as the basic search() above, but that's only true because: 1. both these setTimeouts execute before any later setTimeouts, since their time is as low as possible (now + 0) and setTimeouts with the same time are executed in order; 2. nobody else calls search() synchronously with a different query anywhere in the entire app in this current event loop (including in any .async(fn) or .async(fn, 0) calls), which you have to grep the rest of the code to know because SearchManager is a singleton anybody could use; 3. the SearchManager happens to key requests by query, not create a duplicate request for the current query, and return a singleton promise owned by the request (none of this is documented in the SearchManager interface) That's a lot for me to reason about on a Friday night. Promise.all([search(query, basic), search(query, advanced)]).then() would be simpler even if it's probably always redundant. https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:218: promiseResolver.resolve(); we should just resolve this before the if so we don't have to have the same line twice. The promise is always resolved regardless of whether this |if| is true. (A promise's callbacks execute asynchronously after it is resolved.) https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/main_page_behavior.js:47: this.scroller = document.body; // Used in unit tests. could this just be assert(this.parentElement)? if we're attached but we're not in a shadow tree we must be a normal DOM child. it can equate to document.body in tests, but it also means we could parent this thing with a <div> to test the scrolling behaviors without messing with the entire document.body. https://codereview.chromium.org/2185493003/diff/240001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_main_test.js (right): https://codereview.chromium.org/2185493003/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_main_test.js:32: function registerTests() { nit: new tests shouldn't use the cr.define/registerTests() paradigm -- just call the suite functions here at the top level (or inside an IIFE if you like), and the TEST_F merely calls mocha.run. see route_tests.js https://codereview.chromium.org/2185493003/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_main_test.js:44: var page = null; nit: page already has too many meaninsg https://codereview.chromium.org/2185493003/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_main_test.js:63: test('NoResultsPageShowsHides', function() { nit... this is just a string, it doesn't have to be CamelCase -- 'no results page shows and hides' is friendlier https://codereview.chromium.org/2185493003/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_main_test.js:85: searchManager.setMatchesFound(false); assert first that noSearchResults is unhidden?
Patchset #3 (id:280001) has been deleted
Patchset #2 (id:260001) has been deleted
https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:494: // 'finisthed', if any, and droping all pending tasks. On 2016/07/30 at 05:38:44, michaelpg wrote: > dropping Done. https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:195: * @param {string} query On 2016/07/30 at 05:38:44, michaelpg wrote: > @return Done. https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:198: var promiseResolver = new PromiseResolver(); On 2016/07/30 at 05:38:44, michaelpg wrote: > ok, after looking over it... we're resolving this promise within the same function, why use a PromiseResolver instead of returning a new Promise directly? > > To me, it doesn't look any messier that way and it's one fewer state to keep track of. Is it just the desire to avoid the extra indent? Removed PromiseResolver. Yes, it was used to avoid the extra indentation. https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:208: query, assert(this.$$('settings-basic-page'))); On 2016/07/30 at 05:38:44, michaelpg wrote: > We've gotten into trouble by assuming that doing something asynchronously automagically means a property is still the same, or the current DOM reflects the property. In particular, searchContents is triggered by an event which has no correlation with currentRoute. > > I guess these asserts are fine until they break; then we'll have to decide whether to enforce the page exists here or to stop if the page doesn't exist. Ack. If the asserts break, we should find a more concrete way of triggering a dom-if, and being notified when the stamped element is ready, but until then, I think setTimeout() is good enough. https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:210: setTimeout(function() { On 2016/07/30 at 05:38:44, michaelpg wrote: > SearchManager is a singleton, has only one task queue, and calls requestIdleCallback before starting any serious work, which means everything is FIFO and async. > > So why does settings-main have to use two setTimeouts? We are requesting everything to be rendered, waiting an event loop, and then queueing tasks -- seems like we can call search() twice in a row with no downside. The DOM traversals happen inside separate requestIdleCallbacks which means which means the browser will likely schedule a gap between them[1], and the browser is likely to run both of these setTimeouts before working on the idle task list anyway since the timeout is 0. > > If I'm missing something, then IMO SearchManager should be updated to work better so users don't have to throttle it manually. > > [1] https://www.w3.org/TR/requestidlecallback/#dfn-invoke-idle-callbacks-algorithm Merged the two setTimeouts to a single call. https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:212: query, assert(this.$$('settings-advanced-page'))).then( On 2016/07/30 at 05:38:44, michaelpg wrote: > this ".then" is okay (I think) because this line actually returns the same Promise object as the basic search() above, but that's only true because: > > 1. both these setTimeouts execute before any later setTimeouts, since their time is as low as possible (now + 0) and setTimeouts with the same time are executed in order; > 2. nobody else calls search() synchronously with a different query anywhere in the entire app in this current event loop (including in any .async(fn) or .async(fn, 0) calls), which you have to grep the rest of the code to know because SearchManager is a singleton anybody could use; > 3. the SearchManager happens to key requests by query, not create a duplicate request for the current query, and return a singleton promise owned by the request (none of this is documented in the SearchManager interface) > > That's a lot for me to reason about on a Friday night. > > Promise.all([search(query, basic), search(query, advanced)]).then() would be simpler even if it's probably always redundant. Cleaned up the code a bit to address your points. I did get a bit confused by your reasoning (mostly about 2). It seems that you are listing too many assumptions on why this code works. The only assumption I think was made (and now is enforced with an assert()), was that SearchManager returns the same Promise if the query text did not change. Let me know if latest version of this function addresses your concerns. https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:218: promiseResolver.resolve(); On 2016/07/30 at 05:38:44, michaelpg wrote: > we should just resolve this before the if so we don't have to have the same line twice. The promise is always resolved regardless of whether this |if| is true. (A promise's callbacks execute asynchronously after it is resolved.) Done.
Friendly ping.
https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/main_page_behavior.js:47: this.scroller = document.body; // Used in unit tests. On 2016/07/30 05:38:44, michaelpg wrote: > could this just be assert(this.parentElement)? if we're attached but we're not > in a shadow tree we must be a normal DOM child. > > it can equate to document.body in tests, but it also means we could parent this > thing with a <div> to test the scrolling behaviors without messing with the > entire document.body. or better, see my new suggestion https://codereview.chromium.org/2185493003/diff/240001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_main_test.js (right): https://codereview.chromium.org/2185493003/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_main_test.js:85: searchManager.setMatchesFound(false); On 2016/07/30 05:38:44, michaelpg wrote: > assert first that noSearchResults is unhidden? ping, did you miss this file? this one is slightly higher priority than a nit :-) to catch a bug where setMatchesFound(false) would hide noSearchResults https://codereview.chromium.org/2185493003/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2185493003/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:194: settings.getSearchManager().search( is this indent ok? https://codereview.chromium.org/2185493003/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:197: whenSearchDone.then( perhaps join with the next line to save precious whitespace (or not) https://codereview.chromium.org/2185493003/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2185493003/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/main_page_behavior.js:44: if (this.domHost && this.domHost.parentNode.tagName == 'PAPER-HEADER-PANEL') Would this.scroller = this.offsetParent work for every case, and be more logical? If so.... should we just remove this.scroller?
Patchset #3 (id:320001) has been deleted
https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2185493003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/main_page_behavior.js:47: this.scroller = document.body; // Used in unit tests. On 2016/08/02 at 20:45:20, michaelpg wrote: > On 2016/07/30 05:38:44, michaelpg wrote: > > could this just be assert(this.parentElement)? if we're attached but we're not > > in a shadow tree we must be a normal DOM child. > > > > it can equate to document.body in tests, but it also means we could parent this > > thing with a <div> to test the scrolling behaviors without messing with the > > entire document.body. > > or better, see my new suggestion Previous suggestion does not work. this.parentElement is null at the time the attached() callback fires. this.parentNode is [object ShadowRoot] (still causes tests to fail). I would rather not spend too much time tweaking this line. Instead the root issue https://bugs.chromium.org/p/chromium/issues/detail?id=631281 should be addressed. See also my response to your newest suggestion. https://codereview.chromium.org/2185493003/diff/240001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_main_test.js (right): https://codereview.chromium.org/2185493003/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_main_test.js:32: function registerTests() { On 2016/07/30 at 05:38:44, michaelpg wrote: > nit: new tests shouldn't use the cr.define/registerTests() paradigm -- just call the suite functions here at the top level (or inside an IIFE if you like), and the TEST_F merely calls mocha.run. > > see route_tests.js Unfortunately removing the registerTests() call, messes up with the initialization timing and ends up in a console error [20719:20719:0802/151025:INFO:CONSOLE(47)] "Uncaught ReferenceError: settings is not defined", source: settings_main_test.js (51) Because route.js is evaled after line 47. https://codereview.chromium.org/2185493003/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_main_test.js:44: var page = null; On 2016/07/30 at 05:38:44, michaelpg wrote: > nit: page already has too many meaninsg Done. https://codereview.chromium.org/2185493003/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_main_test.js:63: test('NoResultsPageShowsHides', function() { On 2016/07/30 at 05:38:44, michaelpg wrote: > nit... this is just a string, it doesn't have to be CamelCase -- 'no results page shows and hides' is friendlier Done. https://codereview.chromium.org/2185493003/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_main_test.js:85: searchManager.setMatchesFound(false); On 2016/07/30 at 05:38:44, michaelpg wrote: > assert first that noSearchResults is unhidden? I don't fully understand your comment "to catch a bug where setMatchesFound(false) would hide noSearchResults" When the searchContents() is invoked with an empty string, there are no matches found, but the "no results" page should still remain hidden. In other words, the behavior you describe as a "bug" is the expected behavior here, and this test ensures it is happening. It really does not matter what the prior state of noSearchResult visibility is. Once searchContents() executes it will decide whether to hide or show the noResultsPage (by updating the showNoResultsFound_ boolean. https://codereview.chromium.org/2185493003/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2185493003/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:194: settings.getSearchManager().search( On 2016/08/02 at 20:45:20, michaelpg wrote: > is this indent ok? Fixed. https://codereview.chromium.org/2185493003/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:197: whenSearchDone.then( On 2016/08/02 at 20:45:20, michaelpg wrote: > perhaps join with the next line to save precious whitespace (or not) Done. I don't mind either way. https://codereview.chromium.org/2185493003/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2185493003/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/main_page_behavior.js:44: if (this.domHost && this.domHost.parentNode.tagName == 'PAPER-HEADER-PANEL') On 2016/08/02 at 20:45:20, michaelpg wrote: > Would > > this.scroller = this.offsetParent > > work for every case, and be more logical? > > If so.... should we just remove this.scroller? I am reluctant of modifying behavior in this CL. Mostly because I don't fully understand what this code does to begin with, and secondarily because this CL's intention is to add new tests without modifying existing behavior. I already filed https://bugs.chromium.org/p/chromium/issues/detail?id=631281 about addressing the |scroller| issue, so if you understand what is going on here, could you send your suggestion in a separate CL? That would be very helpful.
Friendly ping.
lgtm https://codereview.chromium.org/2185493003/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2185493003/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/main_page_behavior.js:44: if (this.domHost && this.domHost.parentNode.tagName == 'PAPER-HEADER-PANEL') On 2016/08/02 22:33:15, dpapad wrote: > On 2016/08/02 at 20:45:20, michaelpg wrote: > > Would > > > > this.scroller = this.offsetParent > > > > work for every case, and be more logical? > > > > If so.... should we just remove this.scroller? > > I am reluctant of modifying behavior in this CL. Mostly because I don't fully > understand what this code does to begin with, and secondarily because this CL's > intention is to add new tests without modifying existing behavior. > > I already filed https://bugs.chromium.org/p/chromium/issues/detail?id=631281 > about addressing the |scroller| issue, so if you understand what is going on > here, could you send your suggestion in a separate CL? That would be very > helpful. Fair points, thank you!
Thanks!
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2185493003/#ps360001 (title: "Resolve conflicts with ToT.")
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:360001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Adding some unit tests for <settings-main>. Specifically testing the "no results" message is shown/hidden as expected. - Splitting SearchManager to an interface and an implementation. - Using a TestSearchManager class (implements SearchManager) for testing <settings-main>. BUG=630383 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Adding some unit tests for <settings-main>. Specifically testing the "no results" message is shown/hidden as expected. - Splitting SearchManager to an interface and an implementation. - Using a TestSearchManager class (implements SearchManager) for testing <settings-main>. BUG=630383 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/bb8fba2c2dbc0b02555f11877f4929786eab456c Cr-Commit-Position: refs/heads/master@{#409615} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bb8fba2c2dbc0b02555f11877f4929786eab456c Cr-Commit-Position: refs/heads/master@{#409615}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:360001) has been created in https://codereview.chromium.org/2217493002/ by peter@chromium.org. The reason for reverting is: Causing various debug bots to fail browser_tests https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2... https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%2... A series of JavaScript errors are visible that preferences were not found for various elements, all in pref_control_behavior.js:38. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
