dgozman@chromium.org changed reviewers: + pfeldman@chromium.org
Take a look please.
A screenshot please? https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/c... File Source/devtools/front_end/common/Geometry.js (right): https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/c... Source/devtools/front_end/common/Geometry.js:384: Insets.prototype.isEqual = function(insets) Please follow prototype definition code style. https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/e... File Source/devtools/front_end/emulated_devices/module.json (right): https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/e... Source/devtools/front_end/emulated_devices/module.json:179: "page-rect": { Should we define rects on one line for readability? It also could be [t, r, b, l] to save the bytes. https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/e... File Source/devtools/front_end/emulation/EmulatedDevices.js (right): https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/e... Source/devtools/front_end/emulation/EmulatedDevices.js:289: this.modes.push({title: "", orientation: WebInspector.EmulatedDevice.Vertical, pageRect: {top: 0, left: 0, width: this.horizontal.width, height: this.horizontal.height}, images: null}); defining rects as array would help readability here as well. https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/e... Source/devtools/front_end/emulation/EmulatedDevices.js:289: this.modes.push({title: "", orientation: WebInspector.EmulatedDevice.Vertical, pageRect: {top: 0, left: 0, width: this.horizontal.width, height: this.horizontal.height}, images: null}); Why would you rotate the tablet and not the phone? Can we format our legacy data? https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/e... Source/devtools/front_end/emulation/EmulatedDevices.js:297: if (this.type === WebInspector.EmulatedDevice.Type.Phone || this.type === WebInspector.EmulatedDevice.Type.Tablet || this.type === WebInspector.EmulatedDevice.Type.Unknown) I don't think there should be a code that would check for Phone or Tablet at all. They should all be the same. https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/m... File Source/devtools/front_end/main/Main.js (right): https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/m... Source/devtools/front_end/main/Main.js:130: Runtime.experiments.register("deviceArt", "Device art in Device Mode", true); It is rather a deviceInsets at this point. Lets separate insets from art and implement it first.
PTAL https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/c... File Source/devtools/front_end/common/Geometry.js (right): https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/c... Source/devtools/front_end/common/Geometry.js:384: Insets.prototype.isEqual = function(insets) On 2015/06/13 06:48:59, pfeldman wrote: > Please follow prototype definition code style. Done. https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/e... File Source/devtools/front_end/emulated_devices/module.json (right): https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/e... Source/devtools/front_end/emulated_devices/module.json:179: "page-rect": { On 2015/06/13 06:48:59, pfeldman wrote: > Should we define rects on one line for readability? It also could be [t, r, b, > l] to save the bytes. Will do in a follow-up. https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/e... File Source/devtools/front_end/emulation/EmulatedDevices.js (right): https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/e... Source/devtools/front_end/emulation/EmulatedDevices.js:289: this.modes.push({title: "", orientation: WebInspector.EmulatedDevice.Vertical, pageRect: {top: 0, left: 0, width: this.horizontal.width, height: this.horizontal.height}, images: null}); On 2015/06/13 06:48:59, pfeldman wrote: > Why would you rotate the tablet and not the phone? Can we format our legacy > data? We don't have legacy data anymore. It's all in descriptors. This code will be removed once experiment is over. https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/e... Source/devtools/front_end/emulation/EmulatedDevices.js:297: if (this.type === WebInspector.EmulatedDevice.Type.Phone || this.type === WebInspector.EmulatedDevice.Type.Tablet || this.type === WebInspector.EmulatedDevice.Type.Unknown) On 2015/06/13 06:48:59, pfeldman wrote: > I don't think there should be a code that would check for Phone or Tablet at > all. They should all be the same. Added TODO, will remove in next patch. https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/m... File Source/devtools/front_end/main/Main.js (right): https://codereview.chromium.org/1178643004/diff/1/Source/devtools/front_end/m... Source/devtools/front_end/main/Main.js:130: Runtime.experiments.register("deviceArt", "Device art in Device Mode", true); On 2015/06/13 06:48:59, pfeldman wrote: > It is rather a deviceInsets at this point. Lets separate insets from art and > implement it first. Renamed to device modes.
lgtm
https://codereview.chromium.org/1178643004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/emulation/OverridesUI.js (right): https://codereview.chromium.org/1178643004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/emulation/OverridesUI.js:45: function deviceSelected(apply) extract method https://codereview.chromium.org/1178643004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/emulation/OverridesUI.js:66: modeSelectElement.classList.add("hidden"); .toggle https://codereview.chromium.org/1178643004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/emulation/OverridesUI.js:90: for (var index = 0; index < modes.length; index++) { no {}
PTAL https://codereview.chromium.org/1178643004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/emulation/OverridesUI.js (right): https://codereview.chromium.org/1178643004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/emulation/OverridesUI.js:45: function deviceSelected(apply) On 2015/06/16 18:32:01, pfeldman wrote: > extract method Done. https://codereview.chromium.org/1178643004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/emulation/OverridesUI.js:66: modeSelectElement.classList.add("hidden"); On 2015/06/16 18:32:01, pfeldman wrote: > .toggle Done. https://codereview.chromium.org/1178643004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/emulation/OverridesUI.js:90: for (var index = 0; index < modes.length; index++) { On 2015/06/16 18:32:01, pfeldman wrote: > no {} Done.
The CQ bit was checked by dgozman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/1178643004/#ps80001 (title: "Turn into class")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1178643004/80001
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197375
Chromium Code Reviews
