Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2022893003:
[MD settings] scroll to section find host (Closed)
|
Created:
4 years, 6 months ago by dschuyler Modified:
4 years, 6 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] scroll to section find host
This CL fixes an issue with the scroll to section not finding its host
element. A unit test has also been added to detect the issue.
BUG=616186
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/5f6ada4bb2d2c650ed6c4ffb3e81a33cf8f2b368
Cr-Commit-Position: refs/heads/master@{#398089}
Patch Set 1 #
Total comments: 12
Patch Set 2 : review changes #
Total comments: 2
Patch Set 3 : setInterval #Patch Set 4 : closure fix #
Total comments: 5
Messages
Total messages: 27 (11 generated)
Description was changed from ========== [MD settings] scroll to section find host This CL fixes an issue with the scroll to section not finding its host element. A unit test has also been added to detect the issue. BUG=616186 ========== to ========== [MD settings] scroll to section find host This CL fixes an issue with the scroll to section not finding its host element. A unit test has also been added to detect the issue. BUG=616186 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
dschuyler@chromium.org changed reviewers: + dbeam@chromium.org
please update chrome/browser/resources/settings/settings_page/compiled_resources2.gyp#main_page_behavior to depend on '<(DEPTH)/ui/webui/resources/js/compiled_resources2.gyp:util', https://codereview.chromium.org/2022893003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2022893003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:338: /** @param? https://codereview.chromium.org/2022893003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:339: * @return {Object} The enclosing (shadow dom) host of element. this should not be an object https://codereview.chromium.org/2022893003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:345: }).host; wrong indent https://codereview.chromium.org/2022893003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:345: }).host; can this be: return findAncestor(el, function(n) { return n.host; }).host; https://codereview.chromium.org/2022893003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:358: or maybe: var host = findAncestor(el, function(n) { return n.host; }).host; https://codereview.chromium.org/2022893003/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/basic_page_browsertest.js (right): https://codereview.chromium.org/2022893003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/basic_page_browsertest.js:66: // This timeout should be longer than the one in main_page_behavior, can you just make scrollToSection_ return a Promise or dispatch an event for testing or something?
While I don't like polling, I'd also like to not change the code solely for testing. https://codereview.chromium.org/2022893003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2022893003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:338: /** On 2016/05/31 21:27:20, Dan Beam wrote: > @param? Acknowledged. https://codereview.chromium.org/2022893003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:339: * @return {Object} The enclosing (shadow dom) host of element. On 2016/05/31 21:27:20, Dan Beam wrote: > this should not be an object Acknowledged. https://codereview.chromium.org/2022893003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:345: }).host; On 2016/05/31 21:27:20, Dan Beam wrote: > can this be: > > return findAncestor(el, function(n) { return n.host; }).host; Acknowledged. https://codereview.chromium.org/2022893003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:345: }).host; On 2016/05/31 21:27:20, Dan Beam wrote: > wrong indent Acknowledged. https://codereview.chromium.org/2022893003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:358: On 2016/05/31 21:27:20, Dan Beam wrote: > or maybe: > > var host = findAncestor(el, function(n) { return n.host; }).host; Done. https://codereview.chromium.org/2022893003/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/basic_page_browsertest.js (right): https://codereview.chromium.org/2022893003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/basic_page_browsertest.js:66: // This timeout should be longer than the one in main_page_behavior, On 2016/05/31 21:27:20, Dan Beam wrote: > can you just make scrollToSection_ return a Promise or dispatch an event for > testing or something? In the 'or something' category I made the check independent of the timeout value in scrollToSection_(). Done.
eh, i don't know if setInterval() or other little things make your code much better lgtm https://codereview.chromium.org/2022893003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/basic_page_browsertest.js (right): https://codereview.chromium.org/2022893003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/basic_page_browsertest.js:74: pollForVisibility(); arguably: var intervalId = window.setInterval(function() { var page = self.getPage('basic'); var section = self.getSection(page, page.currentRoute.section); if (!section) return; window.clearInterval(intervalId); resolve(); }, 55); or setTimeout(function pollForVisibility() { var page = self.getPage('basic') var section = self.getSection(page, page.currentRoute.section); if (!section) setTimeout(pollForVisibility, 55); else resolve(); });
On 2016/05/31 22:10:30, Dan Beam wrote: > eh, i don't know if setInterval() or other little things make your code much > better > > lgtm > > https://codereview.chromium.org/2022893003/diff/20001/chrome/test/data/webui/... > File chrome/test/data/webui/settings/basic_page_browsertest.js (right): > > https://codereview.chromium.org/2022893003/diff/20001/chrome/test/data/webui/... > chrome/test/data/webui/settings/basic_page_browsertest.js:74: > pollForVisibility(); > arguably: > > var intervalId = window.setInterval(function() { > var page = self.getPage('basic'); > var section = self.getSection(page, page.currentRoute.section); > if (!section) > return; > > window.clearInterval(intervalId); > resolve(); > }, 55); > > or > > setTimeout(function pollForVisibility() { > var page = self.getPage('basic') > var section = self.getSection(page, page.currentRoute.section); > if (!section) > setTimeout(pollForVisibility, 55); > else > resolve(); > }); I went went with a variation on setInterval.
https://codereview.chromium.org/2022893003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/basic_page_browsertest.js (right): https://codereview.chromium.org/2022893003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/basic_page_browsertest.js:74: pollForVisibility(); On 2016/05/31 22:10:29, Dan Beam wrote: > arguably: > > var intervalId = window.setInterval(function() { > var page = self.getPage('basic'); > var section = self.getSection(page, page.currentRoute.section); > if (!section) > return; > > window.clearInterval(intervalId); > resolve(); > }, 55); > > or > > setTimeout(function pollForVisibility() { > var page = self.getPage('basic') > var section = self.getSection(page, page.currentRoute.section); > if (!section) > setTimeout(pollForVisibility, 55); > else > resolve(); > }); Done.
The CQ bit was checked by dschuyler@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/2022893003/#ps40001 (title: "setInterval")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022893003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2022893003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: closure_compilation on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Added dependency for closure.
The CQ bit was checked by dschuyler@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/2022893003/#ps60001 (title: "closure fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022893003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2022893003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
Sorry, I didn't notice this and made my own: https://codereview.chromium.org/2039603002/ The trick is that the MainPageBehavior element *is* the host.
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022893003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [MD settings] scroll to section find host This CL fixes an issue with the scroll to section not finding its host element. A unit test has also been added to detect the issue. BUG=616186 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] scroll to section find host This CL fixes an issue with the scroll to section not finding its host element. A unit test has also been added to detect the issue. BUG=616186 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/5f6ada4bb2d2c650ed6c4ffb3e81a33cf8f2b368 Cr-Commit-Position: refs/heads/master@{#398089} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5f6ada4bb2d2c650ed6c4ffb3e81a33cf8f2b368 Cr-Commit-Position: refs/heads/master@{#398089}
Message was sent while issue was closed.
I didn't expect this to land without addressing my comment here or the CL I linked to. Anyway, here were my issues with the CL. https://codereview.chromium.org/2022893003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2022893003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/main_page_behavior.js:349: var host = findAncestor(element, function(n) { return n.host; }).host; This is more complicated than necessary (this.scrollHeight would have sufficed). If this is due to changes "coming down the pipeline" it would've been cool to have a comment or TODO or something, or even a comment in the CL at the very least. Also curious why Polymer.Base.domHost doesn't suffice. https://codereview.chromium.org/2022893003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/basic_page_browsertest.js (right): https://codereview.chromium.org/2022893003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/basic_page_browsertest.js:28: TEST_F('SettingsBasicPageBrowserTest', 'MAYBE_Load', function() { Load is no longer an appropriate name for this test. https://codereview.chromium.org/2022893003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/basic_page_browsertest.js:59: self.getPage('basic').currentRoute = { Since this is a test of the entire page, why not trigger the actual interaction being tested (tapping an item in the settings-menu)? https://codereview.chromium.org/2022893003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/basic_page_browsertest.js:68: if (self.getSection(page, page.currentRoute.section)) { Why not actually *check* page.scroller.scrollTop to confirm the page has scrolled? https://codereview.chromium.org/2022893003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/basic_page_browsertest.js:72: }, 55); This is an unnecessary use of setInterval when setTimeout with a 0 timeout would suffice. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
