Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(196)

Issue 68723003: Make chrome/ be documentElement/body agnostic with regards to scrollTop/Left (Closed)

Created:
7 years, 1 month ago by tonikitoo_
Modified:
7 years, 1 month ago
CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, extensions-reviews_chromium.org, dbeam+watch-options_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, estade+watch_chromium.org, chromium-apps-reviews_chromium.org, pedrosimonetti+watch_chromium.org, Julien - ping for review, Peter Beverloo
Base URL:
https://chromium.googlesource.com/chromium/src.git@issue_305800
Visibility:
Public.

Description

Make chrome/ be documentElement/body agnostic with regards to scrollTop/Left Blink is in the process of getting itself in pair with how other Web engines' (Trident,Gecko,Presto) behavior with regards to scrollTop and scrollLeft when called for documentElement and body. This process has been proved itself to be of great impact within Chrome, given that its UI is written in HTML/JS/CSS. See for example bugs 305800, 304753, 305745, 304816, 305742, 305092. This CL tries to prevent Chrome from future similar breakages, as the Blink process of adapting to this change is not yet 100% finalized. It will also allow CLs 69143002 and 51553002 to land without breaking Chrome. CL also fixes driven-by issues like the usage of getElementById instead of $, so CQ can land the CL. Additionally, a PRESUBMIT hook is being considered as well, in order to advice programmers to not user {documentElement.body}.scroll{Top,Left} directly and instead use the helper added to resources/js/util.js BUG=316722 R=dbeam@chromium.org,newt@chromium.org,sky@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235414

Patch Set 1 : Make chrome/ be documentElement/body agnostic with regards to scrollTop/Left #

Total comments: 4

Patch Set 2 : Make chrome/ be documentElement/body agnostic with regards to scrollTop/Left #

Total comments: 2

Patch Set 3 : Make chrome/ be documentElement/body agnostic with regards to scrollTop/Left #

Total comments: 4

Patch Set 4 : #

Total comments: 3

Patch Set 5 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+68 lines, -20 lines) Patch
M chrome/browser/browser_resources.grd View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/about_memory.js View 1 2 3 4 chunks +9 lines, -5 lines 0 comments Download
M chrome/browser/resources/extensions/extension_list.js View 1 2 3 4 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/resources/help/help_base_page.js View 1 2 3 4 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/browser/resources/ntp_android/new_tab.html View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/resources/ntp_android/ntp_android.js View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/options/options_page.js View 1 2 3 4 3 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/resources/uber/uber_utils.js View 3 chunks +4 lines, -3 lines 0 comments Download
M ui/webui/resources/js/util.js View 1 2 1 chunk +40 lines, -0 lines 0 comments Download

Messages

Total messages: 39 (0 generated)
tonikitoo_
7 years, 1 month ago (2013-11-11 15:27:43 UTC) #1
tonikitoo_
There is a big open question in this CL: where is the best place to ...
7 years, 1 month ago (2013-11-11 16:19:21 UTC) #2
tonikitoo_
7 years, 1 month ago (2013-11-11 16:20:06 UTC) #3
Dan Beam
https://codereview.chromium.org/68723003/diff/40001/ui/webui/resources/js/util.js File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/68723003/diff/40001/ui/webui/resources/js/util.js#newcode327 ui/webui/resources/js/util.js:327: * @param {Node} doc The document node where information ...
7 years, 1 month ago (2013-11-11 19:44:03 UTC) #4
tonikitoo_
> https://codereview.chromium.org/68723003/diff/40001/ui/webui/resources/js/util.js#newcode327 > ui/webui/resources/js/util.js:327: * @param {Node} doc The document node where > information will ...
7 years, 1 month ago (2013-11-11 20:27:48 UTC) #5
Dan Beam
are you sure util.js is being included on all these pages? https://chromiumcodereview.appspot.com/68723003/diff/110001/chrome/browser/resources/extensions/extension_list.js File chrome/browser/resources/extensions/extension_list.js (right): ...
7 years, 1 month ago (2013-11-11 23:46:45 UTC) #6
tonikitoo_
On 2013/11/11 23:46:45, Dan Beam wrote: > are you sure util.js is being included on ...
7 years, 1 month ago (2013-11-12 18:36:21 UTC) #7
tonikitoo_
On 2013/11/12 18:36:21, tonikitoo_ wrote: > On 2013/11/11 23:46:45, Dan Beam wrote: > > are ...
7 years, 1 month ago (2013-11-12 19:15:56 UTC) #8
Dan Beam
if you manually tried all the pages/bugs, that's fine. we have no particularly good dependency ...
7 years, 1 month ago (2013-11-12 22:53:39 UTC) #9
Dan Beam
please just fix up the 80 col wrap issues (i.e. that the code should wrap ...
7 years, 1 month ago (2013-11-12 22:54:08 UTC) #10
commit-bot: I haz the power
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an ...
7 years, 1 month ago (2013-11-13 03:37:30 UTC) #11
tonikitoo_
On 2013/11/13 03:37:30, I haz the power (commit-bot) wrote: > No LGTM from a valid ...
7 years, 1 month ago (2013-11-13 03:42:31 UTC) #12
Dan Beam
lgtm
7 years, 1 month ago (2013-11-13 04:39:38 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/200001
7 years, 1 month ago (2013-11-13 04:39:57 UTC) #14
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=35881
7 years, 1 month ago (2013-11-13 04:55:38 UTC) #15
Dan Beam
https://codereview.chromium.org/68723003/diff/200001/chrome/browser/resources/about_memory.js File chrome/browser/resources/about_memory.js (right): https://codereview.chromium.org/68723003/diff/200001/chrome/browser/resources/about_memory.js#newcode6 chrome/browser/resources/about_memory.js:6: if (document.getElementById('helpTooltip')) ^ if util.js really is included on ...
7 years, 1 month ago (2013-11-13 06:06:31 UTC) #16
tonikitoo_
On 2013/11/13 06:06:31, Dan Beam wrote: > https://codereview.chromium.org/68723003/diff/200001/chrome/browser/resources/about_memory.js > File chrome/browser/resources/about_memory.js (right): > > https://codereview.chromium.org/68723003/diff/200001/chrome/browser/resources/about_memory.js#newcode6 ...
7 years, 1 month ago (2013-11-13 14:56:25 UTC) #17
tonikitoo_
Update: > Files that I am not able to test: > - chrome/browser/resources/help/help_base_page.js Ok, I ...
7 years, 1 month ago (2013-11-13 15:35:11 UTC) #18
Dan Beam
On 2013/11/13 14:56:25, tonikitoo_ wrote: > On 2013/11/13 06:06:31, Dan Beam wrote: > > > ...
7 years, 1 month ago (2013-11-13 17:38:40 UTC) #19
tonikitoo_
On 2013/11/13 17:38:40, Dan Beam wrote: > On 2013/11/13 14:56:25, tonikitoo_ wrote: > > > ...
7 years, 1 month ago (2013-11-13 17:43:31 UTC) #20
tonikitoo_
I managed to fix about_memory.js by adding an <include src=> and editing browser_resources.grd so its ...
7 years, 1 month ago (2013-11-13 18:40:25 UTC) #21
Dan Beam
+newt@ for chrome/browser/resources/ntp_android (could you try this patch and verify it works?)
7 years, 1 month ago (2013-11-13 19:27:13 UTC) #22
newt (away)
verified that this works for the Android NTP. lgtm for chrome/browser/resources/ntp_android
7 years, 1 month ago (2013-11-13 22:49:06 UTC) #23
Dan Beam
thanks newt@! lgtm w/nits https://codereview.chromium.org/68723003/diff/460001/chrome/browser/resources/extensions/extension_list.js File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/68723003/diff/460001/chrome/browser/resources/extensions/extension_list.js#newcode64 chrome/browser/resources/extensions/extension_list.js:64: $(idToHighlight).clientHeight; nit: var scrollTop = ...
7 years, 1 month ago (2013-11-13 23:31:06 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001
7 years, 1 month ago (2013-11-14 03:47:03 UTC) #25
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=36138
7 years, 1 month ago (2013-11-14 04:04:15 UTC) #26
tonikitoo_
adding @sky for the grd bits.
7 years, 1 month ago (2013-11-14 05:48:58 UTC) #27
sky
LGTM
7 years, 1 month ago (2013-11-14 15:37:13 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001
7 years, 1 month ago (2013-11-14 15:42:30 UTC) #29
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=224386
7 years, 1 month ago (2013-11-14 19:09:19 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001
7 years, 1 month ago (2013-11-14 19:21:11 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001
7 years, 1 month ago (2013-11-14 22:16:17 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001
7 years, 1 month ago (2013-11-15 02:05:04 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001
7 years, 1 month ago (2013-11-15 04:09:46 UTC) #34
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=224877
7 years, 1 month ago (2013-11-15 08:24:17 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001
7 years, 1 month ago (2013-11-15 14:37:44 UTC) #36
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=225186
7 years, 1 month ago (2013-11-15 19:11:25 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001
7 years, 1 month ago (2013-11-15 19:24:28 UTC) #38
commit-bot: I haz the power
7 years, 1 month ago (2013-11-15 20:34:35 UTC) #39
Message was sent while issue was closed.
Change committed as 235414

Powered by Google App Engine
This is Rietveld 408576698