Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1923843003:
DevTools: Update 3D device in accelerometer emulation (Closed)
|
Created:
4 years, 7 months ago by luoe Modified:
4 years, 7 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chowse, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, maxwalker, pfeldman+blink_chromium.org, pfeldman, sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Update 3D device in accelerometer emulation
BUG=604833
Committed: https://crrev.com/2543cd77d5ecb9f37fa0b59b4c1bf2e6b3a7d0fb
Cr-Commit-Position: refs/heads/master@{#391863}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address comments in patch 1 #Patch Set 3 : Restaged commit #Patch Set 4 : Reset should revert back to portrait in select menu #
Total comments: 4
Patch Set 5 : Address comments #Patch Set 6 : Rebased #
Total comments: 4
Messages
Total messages: 47 (23 generated)
Description was changed from ========== DevTools: Update 3D device in accelerometer emulation BUG=604833 ========== to ========== DevTools: Update 3D device in accelerometer emulation BUG=604833 ==========
luoe@chromium.org changed reviewers: + lushnikov@chromium.org
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923843003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923843003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1923843003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js (right): https://codereview.chromium.org/1923843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js:9: * @param {boolean} error wait.. I've already seen this in a different patch. Should you rebaseline? https://codereview.chromium.org/1923843003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/sensors.css (right): https://codereview.chromium.org/1923843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/sensors.css:141: .accelerometer-bottom::after maybe add accelerometer-element class and just add it to the desired elements?
Geolocation.js and any others *should* be gone from this CL. https://codereview.chromium.org/1923843003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js (right): https://codereview.chromium.org/1923843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js:9: * @param {boolean} error On 2016/04/28 17:51:28, lushnikov wrote: > wait.. I've already seen this in a different patch. Should you rebaseline? Ok, I did a rebase in patch 2 https://codereview.chromium.org/1923843003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/sensors.css (right): https://codereview.chromium.org/1923843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/sensors.css:141: .accelerometer-bottom::after On 2016/04/28 17:51:28, lushnikov wrote: > maybe add accelerometer-element class and just add it to the desired elements? Done.
can we have a screenshot for this as well?
Description was changed from ========== DevTools: Update 3D device in accelerometer emulation BUG=604833 ========== to ========== DevTools: Update 3D device in accelerometer emulation Screenshot: http://imgur.com/PLR6qac BUG=604833 ==========
Screenshot link added to description.
Description was changed from ========== DevTools: Update 3D device in accelerometer emulation Screenshot: http://imgur.com/PLR6qac BUG=604833 ========== to ========== DevTools: Update 3D device in accelerometer emulation BUG=604833 ==========
On 2016/05/02 21:38:40, luoe wrote: > Screenshot link added to description. http://imgur.com/PLR6qac
lgtm https://codereview.chromium.org/1923843003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/devtools.gypi (right): https://codereview.chromium.org/1923843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/devtools.gypi:817: 'front_end/Images/accelerometer-back.png', let's sort these https://codereview.chromium.org/1923843003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/Images/src/search.svg (left): https://codereview.chromium.org/1923843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/Images/src/search.svg:1: <?xml version="1.0" encoding="UTF-8" standalone="no"?> why do you remove this file?
Comments addressed. https://codereview.chromium.org/1923843003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/devtools.gypi (right): https://codereview.chromium.org/1923843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/devtools.gypi:817: 'front_end/Images/accelerometer-back.png', On 2016/05/04 16:18:25, lushnikov wrote: > let's sort these Done. https://codereview.chromium.org/1923843003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/Images/src/search.svg (left): https://codereview.chromium.org/1923843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/Images/src/search.svg:1: <?xml version="1.0" encoding="UTF-8" standalone="no"?> On 2016/05/04 16:18:25, lushnikov wrote: > why do you remove this file? Re-added. My fault, this change is not related to this CL.
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923843003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923843003/60001
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923843003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923843003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by luoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/1923843003/#ps80001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923843003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923843003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
The CQ bit was unchecked by luoe@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923843003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923843003/80001
The CQ bit was checked by luoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/1923843003/#ps100001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923843003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923843003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923843003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923843003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923843003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923843003/100001
Message was sent while issue was closed.
Description was changed from ========== DevTools: Update 3D device in accelerometer emulation BUG=604833 ========== to ========== DevTools: Update 3D device in accelerometer emulation BUG=604833 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: Update 3D device in accelerometer emulation BUG=604833 ========== to ========== DevTools: Update 3D device in accelerometer emulation BUG=604833 Committed: https://crrev.com/2543cd77d5ecb9f37fa0b59b4c1bf2e6b3a7d0fb Cr-Commit-Position: refs/heads/master@{#391863} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/2543cd77d5ecb9f37fa0b59b4c1bf2e6b3a7d0fb Cr-Commit-Position: refs/heads/master@{#391863}
Message was sent while issue was closed.
phistuck@gmail.com changed reviewers: + phistuck@gmail.com
Message was sent while issue was closed.
This change list adds some new prefixed feature usage. I think they can (and should) be dropped. :( https://codereview.chromium.org/1923843003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js (right): https://codereview.chromium.org/1923843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:322: this._boxElement.style.webkitTransform = this._boxMatrix.toString(); I understand you did not touch this, but can this be changed from webkitTransform to transform? https://codereview.chromium.org/1923843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:349: var mat90 = new WebKitCSSMatrix(); It is so crucial to use WebKitCSSMatrix? Can this be implemented without it? https://codereview.chromium.org/1923843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:351: this._boxElement.style.webkitTransform = mat90.multiply(this._currentMatrix); webkitTransform to transform https://codereview.chromium.org/1923843003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/sensors.css (right): https://codereview.chromium.org/1923843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/sensors.css:113: background: -webkit-linear-gradient(#E1F5FE 0%, #E1F5FE 64%, #b0Ebf3 64%, #DEF6F9 100%); Why -webkit-linear-gradient and not just linear-gradient?
Message was sent while issue was closed.
On 2016/05/06 11:25:44, PhistucK wrote: > This change list adds some new prefixed feature usage. I think they can (and > should) be dropped. :( > > https://codereview.chromium.org/1923843003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js > (right): > > https://codereview.chromium.org/1923843003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:322: > this._boxElement.style.webkitTransform = this._boxMatrix.toString(); > I understand you did not touch this, but can this be changed from > webkitTransform to transform? > > https://codereview.chromium.org/1923843003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:349: var > mat90 = new WebKitCSSMatrix(); > It is so crucial to use WebKitCSSMatrix? Can this be implemented without it? > > https://codereview.chromium.org/1923843003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:351: > this._boxElement.style.webkitTransform = mat90.multiply(this._currentMatrix); > webkitTransform to transform > > https://codereview.chromium.org/1923843003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/devtools/front_end/emulation/sensors.css (right): > > https://codereview.chromium.org/1923843003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/sensors.css:113: > background: -webkit-linear-gradient(#E1F5FE 0%, #E1F5FE 64%, #b0Ebf3 64%, > #DEF6F9 100%); > Why -webkit-linear-gradient and not just linear-gradient? You're right, we should be avoiding webkit prefixes if they are not necessary. I had not considered removing prefixes, but taking out the webkit in front of "transform" and "linear-gradient" is something I will do in a separate CL. Comment added to https://bugs.chromium.org/p/chromium/issues/detail?id=604833 I don't know of a clean way of implementing the orientation transformations without a Matrix interface like WebKitCSSMatrix. Since there isn't an unprefixed CSSMatrix, I'd say we can use it for now. There are a few other places in our code that use WebKitCSSMatrix, so this isn't just a one-off either. |
