Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1650243004:
[DevTools] Option to show device frames in emulation mode (Closed)
|
Created:
4 years, 10 months ago by mmccoy Modified:
4 years, 8 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman, samli, sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Initial implementation for an overflow option for displaying a device art outline in emulation mode.
This patch implements the initial view logic and exposed overflow UI to toggle device art for all flagship/default device profiles _except_ Samsung Galaxy S5 (pending assets).
BUG=411892
Committed: https://crrev.com/d1a7ca28d7d40c3fa23568249d14dc378b1ecc9c
Cr-Commit-Position: refs/heads/master@{#388278}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Positioning and context menu fixes, addressing review comments #
Total comments: 10
Patch Set 3 : #Patch Set 4 : Scaling insets #Patch Set 5 : Allow top inset for device frame when there is one, add transition for revealing outline #Patch Set 6 : Use new "in-house" vector assets, merge and polish #Patch Set 7 : Merge changes from samli patch, Moarrrr devices. #Patch Set 8 : Trimming some dead code #
Total comments: 23
Patch Set 9 : Cleanup, addressing dgozman comments #
Total comments: 6
Patch Set 10 : Code cleaup, remove unused code and cl comments #
Total comments: 6
Patch Set 11 : Place device frame art options behind experiment #
Total comments: 9
Patch Set 12 : Disallow rulers and device frame to be show simultaneously #
Total comments: 11
Patch Set 13 : Remove reference to maxLength attribute #Patch Set 14 : Don't toggle settings in toolbar automatically, instead don't show rulers if device outline is enab… #
Total comments: 2
Patch Set 15 : Remove unused code #Patch Set 16 : Add device frame SVG assets to devtools gypi #Messages
Total messages: 58 (10 generated)
mmccoy@chromium.org changed reviewers: + dgozman@chromium.org
Description was changed from ========== [DevTools] Initial implementation for showing device art (Nexus 5X only) This patch implements the initial view logic to high/show device art for one device (N5X). Subsequent patches will be for positioning fix and smoothing out the revealing of the frame. BUG= ========== to ========== [DevTools] Initial implementation for an overflow option for displaying device art in device mode. (Nexus 5X only) This patch implements the initial view logic to high/show device art for one device (N5X). Subsequent patches will be for positioning fix and smoothing out the revealing of the frame. BUG= ==========
Description was changed from ========== [DevTools] Initial implementation for an overflow option for displaying device art in device mode. (Nexus 5X only) This patch implements the initial view logic to high/show device art for one device (N5X). Subsequent patches will be for positioning fix and smoothing out the revealing of the frame. BUG= ========== to ========== [DevTools] Initial implementation for an overflow option for displaying device art in device mode. (Nexus 5X only) This patch implements the initial view logic to toggle device art for one device (N5X). Subsequent patches will be for positioning fix and smoothing out the revealing of the frame. BUG= ==========
Description was changed from ========== [DevTools] Initial implementation for an overflow option for displaying device art in device mode. (Nexus 5X only) This patch implements the initial view logic to toggle device art for one device (N5X). Subsequent patches will be for positioning fix and smoothing out the revealing of the frame. BUG= ========== to ========== [DevTools] Initial implementation for an overflow option for displaying device art in device mode. (Nexus 5X only) This patch implements the initial view logic to toggle device art for one device (N5X). Subsequent patches will be for positioning fix and smoothing out the revealing of the frame. BUG= ==========
The direction is good. Keep going. https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json (right): https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json:311: "insets" : { "left": 0, "top": 0, "right": 0, "bottom": 0 }, Zero insets are probably wrong. https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:246: * @return {Insets|string|null} ?Insets https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:454: Revert this one. https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js (right): https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:245: if (this._model.outlineImage()) { Instead oh hiding, disable the item (last parameter in appendCheckboxItem). https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:260: this._positionOutline(); I think should have been handled in lines 203-208. https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js (right): https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js:15: this.vertical = {width: 0, height: 0, outlineInsets: null, outlineImage: null, outlineWidth: 0, outlineHeight: 0}; I think outlineWidth === width + insets.left + insets.right, so there is no need to expose it. Same with height. https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js:329: * @return {Insets|string} ?Insets https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js:335: return ""; return null https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/deviceModeView.css (right): https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/deviceModeView.css:130: z-index: 2; I'd prefer to avoid z-index. Can we instead rearrange things in the DOM?
On 2016/02/01 at 17:18:28, dgozman wrote: > The direction is good. Keep going. > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json (right): > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json:311: "insets" : { "left": 0, "top": 0, "right": 0, "bottom": 0 }, > Zero insets are probably wrong. > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:246: * @return {Insets|string|null} > ?Insets > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:454: > Revert this one. > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js (right): > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:245: if (this._model.outlineImage()) { > Instead oh hiding, disable the item (last parameter in appendCheckboxItem). > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:260: this._positionOutline(); > I think should have been handled in lines 203-208. > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js (right): > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js:15: this.vertical = {width: 0, height: 0, outlineInsets: null, outlineImage: null, outlineWidth: 0, outlineHeight: 0}; > I think outlineWidth === width + insets.left + insets.right, so there is no need to expose it. > Same with height. > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js:329: * @return {Insets|string} > ?Insets > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js:335: return ""; > return null > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/emulation/deviceModeView.css (right): > > https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/emulation/deviceModeView.css:130: z-index: 2; > I'd prefer to avoid z-index. Can we instead rearrange things in the DOM? Thanks for all the comments, Dmitry. Sorry about the lag in updating, was on vacation last week. Took another, pass -- can you have a look? One more thing I need to add is to re-position (top margin needed) the root view element when outline art is toggled. -mm
Now that this starts working, could you please attach a screenshot of how it looks? > One more thing I need to add is to re-position (top margin needed) the root view element when outline art is toggled. Take a look at how media inspector or top ruler is implemented. https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:250: return (this._device && this._mode) ? this._device.outlineInsets(this._mode) : ""; I believe you have to scale insets by this._scale. https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:205: var showOutline = this._showOutlineSetting.get() && this._model.type() !== WebInspector.DeviceModeModel.Type.None; type === Device, as you don't have an outline for responsive mode. https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:216: var outlineImageDimensions = this._model.outlineDimensions(); I believe you don't need this. https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:221: this._outlineArea.style.width = outlineImageDimensions.width + "px"; What about |style.right = outlineInsets.right + "px"| and positioning outlineArea relative to screenArea? https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js (right): https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js:324: * @return {?Insets|null} just ?Insets
Thanks again, here's a screenshot in current form: https://x20web.corp.google.com/users/mm/mmccoy/ss/device-mode-frame-demo.png Still working on additional top inset when device frame is active. Am I on the right track with that? I'm thinking something like.. if outlineImage, add its top inset to the contentArea top inset. https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json (right): https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json:311: "insets" : { "left": 0, "top": 0, "right": 0, "bottom": 0 }, On 2016/02/01 at 17:18:27, dgozman wrote: > Zero insets are probably wrong. Done. https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:246: * @return {Insets|string|null} On 2016/02/01 at 17:18:27, dgozman wrote: > ?Insets Done. https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js (right): https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:245: if (this._model.outlineImage()) { On 2016/02/01 at 17:18:27, dgozman wrote: > Instead oh hiding, disable the item (last parameter in appendCheckboxItem). Done. https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:260: this._positionOutline(); On 2016/02/01 at 17:18:27, dgozman wrote: > I think should have been handled in lines 203-208. Removed! https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js (right): https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js:15: this.vertical = {width: 0, height: 0, outlineInsets: null, outlineImage: null, outlineWidth: 0, outlineHeight: 0}; On 2016/02/01 at 17:18:27, dgozman wrote: > I think outlineWidth === width + insets.left + insets.right, so there is no need to expose it. > Same with height. Done! https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js:329: * @return {Insets|string} On 2016/02/01 at 17:18:27, dgozman wrote: > ?Insets Done https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js:335: return ""; On 2016/02/01 at 17:18:27, dgozman wrote: > return null Done. https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/deviceModeView.css (right): https://codereview.chromium.org/1650243004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/deviceModeView.css:130: z-index: 2; On 2016/02/01 at 17:18:27, dgozman wrote: > I'd prefer to avoid z-index. Can we instead rearrange things in the DOM? Done! https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:250: return (this._device && this._mode) ? this._device.outlineInsets(this._mode) : ""; On 2016/02/09 at 21:15:10, dgozman wrote: > I believe you have to scale insets by this._scale. It works well for all cases I tested, but is it okay to do this transform here? I don't see something similar being done for screenImage for example? https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:205: var showOutline = this._showOutlineSetting.get() && this._model.type() !== WebInspector.DeviceModeModel.Type.None; On 2016/02/09 at 21:15:10, dgozman wrote: > type === Device, as you don't have an outline for responsive mode. Done. https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:216: var outlineImageDimensions = this._model.outlineDimensions(); On 2016/02/09 at 21:15:10, dgozman wrote: > I believe you don't need this. Ah yes, removed throughout. https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:221: this._outlineArea.style.width = outlineImageDimensions.width + "px"; On 2016/02/09 at 21:15:10, dgozman wrote: > What about |style.right = outlineInsets.right + "px"| and positioning outlineArea relative to screenArea? Hrm, can you elaborate a little. outlineArea should be a child of screenArea? That doesn't seem right. https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js (right): https://codereview.chromium.org/1650243004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/EmulatedDevices.js:324: * @return {?Insets|null} On 2016/02/09 at 21:15:10, dgozman wrote: > just ?Insets Done.
Made some other adjustments based no your comments, made affordance for inset top when there is outline art available. Here's a lil screencast of how its working atm: https://x20web.corp.google.com/users/mm/mmccoy/ss/dm-outline-demo.mov -mm
On 2016/02/10 21:58:10, mmccoy wrote: > Made some other adjustments based no your comments, made affordance for inset > top when there is outline art available. > > Here's a lil screencast of how its working atm: > https://x20web.corp.google.com/users/mm/mmccoy/ss/dm-outline-demo.mov > > -mm Sorry, cannot access this url - 403 Forbidden.
On 2016/02/10 at 22:50:45, dgozman wrote: > On 2016/02/10 21:58:10, mmccoy wrote: > > Made some other adjustments based no your comments, made affordance for inset > > top when there is outline art available. > > > > Here's a lil screencast of how its working atm: > > https://x20web.corp.google.com/users/mm/mmccoy/ss/dm-outline-demo.mov > > > > -mm > > Sorry, cannot access this url - 403 Forbidden. Apologies! Fixed the perms. -mm
On 2016/02/11 15:18:11, mmccoy wrote: > On 2016/02/10 at 22:50:45, dgozman wrote: > > On 2016/02/10 21:58:10, mmccoy wrote: > > > Made some other adjustments based no your comments, made affordance for > inset > > > top when there is outline art available. > > > > > > Here's a lil screencast of how its working atm: > > > https://x20web.corp.google.com/users/mm/mmccoy/ss/dm-outline-demo.mov > > > > > > -mm > > > > Sorry, cannot access this url - 403 Forbidden. > > Apologies! Fixed the perms. > > -mm Sorry, still can't access it.
On 2016/02/11 at 18:23:47, dgozman wrote: > On 2016/02/11 15:18:11, mmccoy wrote: > > On 2016/02/10 at 22:50:45, dgozman wrote: > > > On 2016/02/10 21:58:10, mmccoy wrote: > > > > Made some other adjustments based no your comments, made affordance for > > inset > > > > top when there is outline art available. > > > > > > > > Here's a lil screencast of how its working atm: > > > > https://x20web.corp.google.com/users/mm/mmccoy/ss/dm-outline-demo.mov > > > > > > > > -mm > > > > > > Sorry, cannot access this url - 403 Forbidden. > > > > Apologies! Fixed the perms. > > > > -mm > > Sorry, still can't access it. fail! :) I'm resorting to drive -- I shared an .mov with you.
On 2016/02/11 at 20:37:05, mmccoy wrote: > On 2016/02/11 at 18:23:47, dgozman wrote: > > On 2016/02/11 15:18:11, mmccoy wrote: > > > On 2016/02/10 at 22:50:45, dgozman wrote: > > > > On 2016/02/10 21:58:10, mmccoy wrote: > > > > > Made some other adjustments based no your comments, made affordance for > > > inset > > > > > top when there is outline art available. > > > > > > > > > > Here's a lil screencast of how its working atm: > > > > > https://x20web.corp.google.com/users/mm/mmccoy/ss/dm-outline-demo.mov > > > > > > > > > > -mm > > > > > > > > Sorry, cannot access this url - 403 Forbidden. > > > > > > Apologies! Fixed the perms. > > > > > > -mm > > > > Sorry, still can't access it. > > fail! :) I'm resorting to drive -- I shared an .mov with you. mmccoy: I didn't realise you were working on this and I did the same work (https://codereview.chromium.org/1688113002). You may want to take the code I've written or I could take the image you've used? I used the facebook device images but I don't think we have permission to use them. Let me know.
> I used the facebook device images but I don't think we have permission to use them. Let me know. I'm looking into the licensing.
Managed to apply the patch locally and try it out. Screenshot: http://i.imgur.com/E3st0Uu.png The licensing bits will require some time. I'll keep yall in the loop.
Thanks for digging into the licensing questions Paul, this patch needs a little more spit and polish too so I'll work on that. -mm On Wed, Feb 17, 2016 at 5:54 PM <paulirish@chromium.org> wrote: > > Managed to apply the patch locally and try it out. > Screenshot: http://i.imgur.com/E3st0Uu.png > > > The licensing bits will require some time. I'll keep yall in the loop. > https://codereview.chromium.org/1650243004/ > > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Thanks for digging into the licensing questions Paul, this patch needs a little more spit and polish too so I'll work on that. -mm On Wed, Feb 17, 2016 at 5:54 PM <paulirish@chromium.org> wrote: > > Managed to apply the patch locally and try it out. > Screenshot: http://i.imgur.com/E3st0Uu.png > > > The licensing bits will require some time. I'll keep yall in the loop. > https://codereview.chromium.org/1650243004/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
@paulirish/@dgozman Can you guys have another look? I'm now using "in-house" SVGs made by Chris Conover from the MD team; free of licensing issues and actually seem to somehow perform a little snappier than the facebook versions I was using. Here are some screenshots of the current state: https://drive.google.com/a/google.com/folderview?id=0B-kYjigWy8c_QUl0SlNuNzZV... Thanks! -mm On 2016/02/18 14:35:50, chromium-reviews wrote: > Thanks for digging into the licensing questions Paul, this patch needs a > little more spit and polish too so I'll work on that. > > -mm > > On Wed, Feb 17, 2016 at 5:54 PM <mailto:paulirish@chromium.org> wrote: > > > > > Managed to apply the patch locally and try it out. > > Screenshot: http://i.imgur.com/E3st0Uu.png > > > > > > The licensing bits will require some time. I'll keep yall in the loop. > > https://codereview.chromium.org/1650243004/ > > > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== [DevTools] Initial implementation for an overflow option for displaying device art in device mode. (Nexus 5X only) This patch implements the initial view logic to toggle device art for one device (N5X). Subsequent patches will be for positioning fix and smoothing out the revealing of the frame. BUG= ========== to ========== [DevTools] Initial implementation for an overflow option for displaying a device art outline in emulation mode. (Nexus 5X + Nexus 6P only) This patch implements the initial view logic and exposed overflow UI to toggle device art for flagship devices (N5X, N6P). Subsequent patches will add more art for the default device profiles. BUG= ==========
Description was changed from ========== [DevTools] Initial implementation for an overflow option for displaying a device art outline in emulation mode. (Nexus 5X + Nexus 6P only) This patch implements the initial view logic and exposed overflow UI to toggle device art for flagship devices (N5X, N6P). Subsequent patches will add more art for the default device profiles. BUG= ========== to ========== [DevTools] Initial implementation for an overflow option for displaying a device art outline in emulation mode. (Nexus 5X + Nexus 6P only) This patch implements the initial view logic and exposed overflow UI to toggle device art for flagship devices (N5X, N6P). Subsequent patches will add more art for the default device profiles. BUG=411892 ==========
On 2016/03/14 at 21:16:21, mmccoy wrote: > @paulirish/@dgozman > > Can you guys have another look? I'm now using "in-house" SVGs made by Chris Conover from the MD team; free of licensing issues and actually seem to somehow perform a little snappier than the facebook versions I was using. Nice, things are looking good so far. Seems like even the artifact of the speaker ovals are now fixed. I've left some comments about remaining device art on the attached bug: https://bugs.chromium.org/p/chromium/issues/detail?id=411892 But IMO we can land with just these while we work on the rest.
You probably want to take the code in https://codereview.chromium.org/1688113002 since I dealt with cases like rulers & zoom. On 2016/03/14 at 21:16:21, mmccoy wrote: > @paulirish/@dgozman > > Can you guys have another look? I'm now using "in-house" SVGs made by Chris Conover from the MD team; free of licensing issues and actually seem to somehow perform a little snappier than the facebook versions I was using. > > Here are some screenshots of the current state: > https://drive.google.com/a/google.com/folderview?id=0B-kYjigWy8c_QUl0SlNuNzZV... > > Thanks! > > -mm > > On 2016/02/18 14:35:50, chromium-reviews wrote: > > Thanks for digging into the licensing questions Paul, this patch needs a > > little more spit and polish too so I'll work on that. > > > > -mm > > > > On Wed, Feb 17, 2016 at 5:54 PM <mailto:paulirish@chromium.org> wrote: > > > > > > > > Managed to apply the patch locally and try it out. > > > Screenshot: http://i.imgur.com/E3st0Uu.png > > > > > > > > > The licensing bits will require some time. I'll keep yall in the loop. > > > https://codereview.chromium.org/1650243004/ > > > > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an email > > to mailto:chromium-reviews+unsubscribe@chromium.org.
Hi all, please TAL; * Merged changes from Sam's patch * Added more device profiles, the only current default device that isn't included in this patch is the Galaxy S5
Description was changed from ========== [DevTools] Initial implementation for an overflow option for displaying a device art outline in emulation mode. (Nexus 5X + Nexus 6P only) This patch implements the initial view logic and exposed overflow UI to toggle device art for flagship devices (N5X, N6P). Subsequent patches will add more art for the default device profiles. BUG=411892 ========== to ========== [DevTools] Initial implementation for an overflow option for displaying a device art outline in emulation mode. This patch implements the initial view logic and exposed overflow UI to toggle device art for all flagship/default device profiles _except_ Samsung Galaxy S5 (pending assets). BUG=411892 ==========
Nice, this looks good from a product side.
This looks really good! A couple of comments, and we are done. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json (right): https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json:425: "insets" : { "left": 94, "top": 48, "right": 97, "bottom": 48 } Coordinate changes with Paul here: https://codereview.chromium.org/1822573002/ https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:407: nit: extra blank line https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:437: var outline = this._deviceOutlineSetting.get() ? orientation.outlineInsets : null; Use new Insets(0, 0, 0, 0) instead of null. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:438: var width = outline ? outline.left + orientation.width + outline.right : orientation.width; You don't really use |width| and |height| variables. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:533: this._screenRect.left - outline.left * scale, Math.max(0, .....) https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js (right): https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:274: contextMenu.appendCheckboxItem(WebInspector.UIString("Show device frame"), deviceOutlineSetting.set.bind(deviceOutlineSetting, !deviceOutlineSetting.get()), deviceOutlineSetting.get() && this._model.outlineImage() !== "", !this._model.outlineImage()); Do not alter the disabled/checked state based on the current device mode - it will confuse users. Copy disabled check from the line above. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:236: applyRect(this._outlineImage, this._model.outlineRect().scale(1 / zoomFactor)); Just |outlineRect|. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:242: nit: extra blank lines https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:321: _onScreenImageLoaded: function(element, success) nit: rename to _onImageLoaded for consistency. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:418: this._maxLength = maxLength; I'm not completely sure why do we need this. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/deviceModeView.css (right): https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/deviceModeView.css:121: box-shadow: hsl(0, 0%, 88%) 1px 1px 0 1px, hsla(0, 0%, 80%, 0.6) 0 0 16px; Why this change?
https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json (right): https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulated_devices/module.json:425: "insets" : { "left": 94, "top": 48, "right": 97, "bottom": 48 } On 2016/03/21 at 21:08:38, dgozman wrote: > Coordinate changes with Paul here: https://codereview.chromium.org/1822573002/ Merged and adjusted insets. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:407: On 2016/03/21 at 21:08:38, dgozman wrote: > nit: extra blank line Done. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:437: var outline = this._deviceOutlineSetting.get() ? orientation.outlineInsets : null; On 2016/03/21 at 21:08:38, dgozman wrote: > Use new Insets(0, 0, 0, 0) instead of null. Done. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:533: this._screenRect.left - outline.left * scale, On 2016/03/21 at 21:08:38, dgozman wrote: > Math.max(0, .....) Done. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js (right): https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:274: contextMenu.appendCheckboxItem(WebInspector.UIString("Show device frame"), deviceOutlineSetting.set.bind(deviceOutlineSetting, !deviceOutlineSetting.get()), deviceOutlineSetting.get() && this._model.outlineImage() !== "", !this._model.outlineImage()); On 2016/03/21 at 21:08:38, dgozman wrote: > Do not alter the disabled/checked state based on the current device mode - it will confuse users. Copy disabled check from the line above. Done. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:236: applyRect(this._outlineImage, this._model.outlineRect().scale(1 / zoomFactor)); On 2016/03/21 at 21:08:38, dgozman wrote: > Just |outlineRect|. Done. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:242: On 2016/03/21 at 21:08:38, dgozman wrote: > nit: extra blank lines Done. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:321: _onScreenImageLoaded: function(element, success) On 2016/03/21 at 21:08:38, dgozman wrote: > nit: rename to _onImageLoaded for consistency. Done. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:418: this._maxLength = maxLength; On 2016/03/21 at 21:08:38, dgozman wrote: > I'm not completely sure why do we need this. @samli I blindly poached this from your patch.. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/deviceModeView.css (right): https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/deviceModeView.css:121: box-shadow: hsl(0, 0%, 88%) 1px 1px 0 1px, hsla(0, 0%, 80%, 0.6) 0 0 16px; On 2016/03/21 at 21:08:39, dgozman wrote: > Why this change? Not sure exactly! I think I c+p the wrong style from elsewhere in the file. :) Reverted.
https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js (right): https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:274: contextMenu.appendCheckboxItem(WebInspector.UIString("Show device frame"), deviceOutlineSetting.set.bind(deviceOutlineSetting, !deviceOutlineSetting.get()), deviceOutlineSetting.get() && this._model.outlineImage() !== "", !this._model.outlineImage()); On 2016/03/21 22:03:05, mmccoy wrote: > On 2016/03/21 at 21:08:38, dgozman wrote: > > Do not alter the disabled/checked state based on the current device mode - it > will confuse users. Copy disabled check from the line above. > > Done. You should not check this._model.outlineImage() here at all. The checkbox should not depend on the selected device. https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:418: this._maxLength = maxLength; On 2016/03/21 22:03:05, mmccoy wrote: > On 2016/03/21 at 21:08:38, dgozman wrote: > > I'm not completely sure why do we need this. > > @samli > I blindly poached this from your patch.. Let's revert and see what's broken. Have you tested with all UI options: rulers, media queries, etc? https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:442: this._applyDeviceMetrics(new Size(orientation.width, orientation.height), this._mode.insets, outline || new Insets(0, 0, 0, 0), this._scaleSetting.get(), this._device.deviceScaleFactor, this._device.mobile(), this._mode.orientation == WebInspector.EmulatedDevice.Horizontal ? "landscapePrimary" : "portraitPrimary", resetPageScaleFactor); No need for "|| new Insets()" anymore. https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:240: this._outlineImage.classList.toggle("device-frame-visible", (this._model.deviceOutlineSetting().get() && this._model.outlineImage() !== "")); Do not compare with empty string. It will be casted to bool automatically: ... && this._model.outlineImage() https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:292: var showingDevice = this._model.type() === WebInspector.DeviceModeModel.Type.Device; Does this code try to hide rulers when showing outline? There is |showRulers| variable at the top, could just update it's value then.
On 2016/03/22 at 20:30:50, dgozman wrote: > https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js (right): > > https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:274: contextMenu.appendCheckboxItem(WebInspector.UIString("Show device frame"), deviceOutlineSetting.set.bind(deviceOutlineSetting, !deviceOutlineSetting.get()), deviceOutlineSetting.get() && this._model.outlineImage() !== "", !this._model.outlineImage()); > On 2016/03/21 22:03:05, mmccoy wrote: > > On 2016/03/21 at 21:08:38, dgozman wrote: > > > Do not alter the disabled/checked state based on the current device mode - it > > will confuse users. Copy disabled check from the line above. > > > > Done. > > You should not check this._model.outlineImage() here at all. The checkbox should not depend on the selected device. Without this check, the checkbox for "Show device frame" is toggleable even if we don't ship an outline image for the currently selected device -- which results in a noop. I think its better UX to disable the option if we don't have a frame asset for the device, no? > > https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): > > https://codereview.chromium.org/1650243004/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:418: this._maxLength = maxLength; > On 2016/03/21 22:03:05, mmccoy wrote: > > On 2016/03/21 at 21:08:38, dgozman wrote: > > > I'm not completely sure why do we need this. > > > > @samli > > I blindly poached this from your patch.. > > Let's revert and see what's broken. Have you tested with all UI options: rulers, media queries, etc? This code truncates the ruler so that it is constrained by the width/height of the device frame (when visible). > > https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): > > https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:442: this._applyDeviceMetrics(new Size(orientation.width, orientation.height), this._mode.insets, outline || new Insets(0, 0, 0, 0), this._scaleSetting.get(), this._device.deviceScaleFactor, this._device.mobile(), this._mode.orientation == WebInspector.EmulatedDevice.Horizontal ? "landscapePrimary" : "portraitPrimary", resetPageScaleFactor); > No need for "|| new Insets()" anymore. > > https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): > > https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:240: this._outlineImage.classList.toggle("device-frame-visible", (this._model.deviceOutlineSetting().get() && this._model.outlineImage() !== "")); > Do not compare with empty string. It will be casted to bool automatically: > > ... && this._model.outlineImage() > > https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:292: var showingDevice = this._model.type() === WebInspector.DeviceModeModel.Type.Device; > Does this code try to hide rulers when showing outline? There is |showRulers| variable at the top, could just update it's value then.
https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:442: this._applyDeviceMetrics(new Size(orientation.width, orientation.height), this._mode.insets, outline || new Insets(0, 0, 0, 0), this._scaleSetting.get(), this._device.deviceScaleFactor, this._device.mobile(), this._mode.orientation == WebInspector.EmulatedDevice.Horizontal ? "landscapePrimary" : "portraitPrimary", resetPageScaleFactor); On 2016/03/22 at 20:30:49, dgozman wrote: > No need for "|| new Insets()" anymore. Done. https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:240: this._outlineImage.classList.toggle("device-frame-visible", (this._model.deviceOutlineSetting().get() && this._model.outlineImage() !== "")); On 2016/03/22 at 20:30:49, dgozman wrote: > Do not compare with empty string. It will be casted to bool automatically: > > ... && this._model.outlineImage() Done. https://codereview.chromium.org/1650243004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:292: var showingDevice = this._model.type() === WebInspector.DeviceModeModel.Type.Device; On 2016/03/22 at 20:30:49, dgozman wrote: > Does this code try to hide rulers when showing outline? There is |showRulers| variable at the top, could just update it's value then. No, rulers and outline can both be visible simultaneously. I think this block just calculates the added geometry of the outline to position the rules correctly.
pingsies!
Sorry for delay, slipped through reviews. https://codereview.chromium.org/1650243004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js (right): https://codereview.chromium.org/1650243004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:274: contextMenu.appendCheckboxItem(WebInspector.UIString("Show device frame"), deviceOutlineSetting.set.bind(deviceOutlineSetting, !deviceOutlineSetting.get()), deviceOutlineSetting.get() && this._model.outlineImage() !== "", (this._model.type() === WebInspector.DeviceModeModel.Type.None || this._model.outlineImage() === "")); I see your point about this checkbox doing nothing when there is no outline image. I think we should do something similar to the modes switch - show the control when it makes sense in a separate menu. Let's land this behind an experiment (register one in Main.js, check that it's enabled here and in _calculateAndEmulate). I will figure out the final UI later. https://codereview.chromium.org/1650243004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:266: // this._topRuler.show(this._contentClip, this._contentArea); Commented code? https://codereview.chromium.org/1650243004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:408: render: function(offset, scale, maxLength) Please revert anything related to maxLength. Neither me nor you understand it, so it should not be in the code.
samli@chromium.org changed reviewers: + samli@chromium.org
https://codereview.chromium.org/1650243004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:408: render: function(offset, scale, maxLength) On 2016/04/07 at 00:54:43, dgozman wrote: > Please revert anything related to maxLength. Neither me nor you understand it, so it should not be in the code. mmccoy correctly determined what this is: "This code truncates the ruler so that it is constrained by the width/height of the device frame (when visible)." Looks very off with rulers on top of device art when the size cannot be changed, so this fixes the ruler to only show within the device.
On 2016/04/07 01:09:55, samli wrote: > https://codereview.chromium.org/1650243004/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js > (right): > > https://codereview.chromium.org/1650243004/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:408: > render: function(offset, scale, maxLength) > On 2016/04/07 at 00:54:43, dgozman wrote: > > Please revert anything related to maxLength. Neither me nor you understand it, > so it should not be in the code. > > mmccoy correctly determined what this is: "This code truncates the ruler so that > it is constrained by the width/height of the device frame (when visible)." > > Looks very off with rulers on top of device art when the size cannot be changed, > so this fixes the ruler to only show within the device. Here's a screenshot illustrating that: https://drive.google.com/open?id=0B-kYjigWy8c_TVRTT2FUWVd2LW8 I noticed after updating this patch that the loading the experiments pane is crashing my build, throwing the following: [58822:1295:0411/103247:FATAL:Range.h(72)] Check failed: false. TypeError should not be thrown. [58797:1295:0411/103247:FATAL:render_widget_host_impl.cc(1628)] Check failed: !g_check_for_pending_resize_ack || resize_ack_pending_. It would seem even without my changes of simply registering the new experiment in Main.js; its still crashing when I click on the experiments sidebar item in settings. Do I need to do anything special in-build to make that bit work? -mm
https://codereview.chromium.org/1650243004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js (right): https://codereview.chromium.org/1650243004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:274: contextMenu.appendCheckboxItem(WebInspector.UIString("Show device frame"), deviceOutlineSetting.set.bind(deviceOutlineSetting, !deviceOutlineSetting.get()), deviceOutlineSetting.get() && this._model.outlineImage() !== "", (this._model.type() === WebInspector.DeviceModeModel.Type.None || this._model.outlineImage() === "")); On 2016/04/07 00:54:43, dgozman wrote: > I see your point about this checkbox doing nothing when there is no outline > image. I think we should do something similar to the modes switch - show the > control when it makes sense in a separate menu. > > Let's land this behind an experiment (register one in Main.js, check that it's > enabled here and in _calculateAndEmulate). I will figure out the final UI later. Acknowledged. https://codereview.chromium.org/1650243004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:266: // this._topRuler.show(this._contentClip, this._contentArea); On 2016/04/07 00:54:43, dgozman wrote: > Commented code? Done.
Last round of comments. https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:238: return (this._device && this._mode) ? this._device.outlineImage(this._mode) : ""; Should return empty string when experiment is off. https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:436: var outline = this._deviceOutlineSetting.get() ? orientation.outlineInsets : new Insets(0,0,0,0); Should be zero insets when experiment is off. https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:266: this._topRuler.show(this._contentArea); You show topRuler twice. Let's revert. https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:453: render: function(offset, scale, maxLength) Thanks for screenshot, now it's clear. I still don't like how rulers and outline play together. Let's just hide rulers when outline is visible and remove maxLength handling. https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/main/Main.js (right): https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/main/Main.js:109: Runtime.experiments.register("deviceFrames", "Show device frame outline option", true); We try to keep them alphabetical: "Device frame"
I was able to successfully test the experiment by manually enabling it in code with 'setEnabled', but when I try to navigate to the experiments tab my build crashes -- should I be worried about that? :) In fact, the same crash happens even without my changes to Main.js, is there some other trick to get experiments to work with local builds? Thanks for the great comments and help getting this landed. -mm https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:238: return (this._device && this._mode) ? this._device.outlineImage(this._mode) : ""; On 2016/04/11 at 17:24:52, dgozman wrote: > Should return empty string when experiment is off. Done. https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:436: var outline = this._deviceOutlineSetting.get() ? orientation.outlineInsets : new Insets(0,0,0,0); On 2016/04/11 at 17:24:52, dgozman wrote: > Should be zero insets when experiment is off. Done. https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:266: this._topRuler.show(this._contentArea); On 2016/04/11 at 17:24:52, dgozman wrote: > You show topRuler twice. Let's revert. Done. https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/main/Main.js (right): https://codereview.chromium.org/1650243004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/main/Main.js:109: Runtime.experiments.register("deviceFrames", "Show device frame outline option", true); On 2016/04/11 at 17:24:52, dgozman wrote: > We try to keep them alphabetical: "Device frame" Done.
https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js (right): https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:275: // contextMenu.appendCheckboxItem(WebInspector.UIString("Show device frame"), this._deviceOutlineSetting.set.bind(this._deviceOutlineSetting, !this._deviceOutlineSetting.get()), this._deviceOutlineSetting.get() && this._model.outlineImage() !== "", (this._model.type() === WebInspector.DeviceModeModel.Type.None || this._model.outlineImage() === "")); nit: remove commented code https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:291: this._toggleRulers(); We should instead do this in DeviceModeView and avoid toggling settings on users' behalf. https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:309: this._toggleDeviceFrames(); ditto https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:213: var showRulers = this._showRulersSetting.get() && this._model.type() !== WebInspector.DeviceModeModel.Type.None; Just set |showRulers| to false here if we have outline. https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:291: var showingDevice = this._model.type() === WebInspector.DeviceModeModel.Type.Device; Unused. https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:477: if (this._maxLength) This is not needed anymore.
On 2016/04/13 at 22:52:06, dgozman wrote: > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js (right): > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:275: // contextMenu.appendCheckboxItem(WebInspector.UIString("Show device frame"), this._deviceOutlineSetting.set.bind(this._deviceOutlineSetting, !this._deviceOutlineSetting.get()), this._deviceOutlineSetting.get() && this._model.outlineImage() !== "", (this._model.type() === WebInspector.DeviceModeModel.Type.None || this._model.outlineImage() === "")); > nit: remove commented code > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:291: this._toggleRulers(); > We should instead do this in DeviceModeView and avoid toggling settings on users' behalf. > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:309: this._toggleDeviceFrames(); > ditto > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:213: var showRulers = this._showRulersSetting.get() && this._model.type() !== WebInspector.DeviceModeModel.Type.None; > Just set |showRulers| to false here if we have outline. > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:291: var showingDevice = this._model.type() === WebInspector.DeviceModeModel.Type.Device; > Unused. > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:477: if (this._maxLength) > This is not needed anymore. Done.
On 2016/04/14 at 18:57:40, mmccoy wrote: > On 2016/04/13 at 22:52:06, dgozman wrote: > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js (right): > > > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:275: // contextMenu.appendCheckboxItem(WebInspector.UIString("Show device frame"), this._deviceOutlineSetting.set.bind(this._deviceOutlineSetting, !this._deviceOutlineSetting.get()), this._deviceOutlineSetting.get() && this._model.outlineImage() !== "", (this._model.type() === WebInspector.DeviceModeModel.Type.None || this._model.outlineImage() === "")); > > nit: remove commented code > > > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:291: this._toggleRulers(); > > We should instead do this in DeviceModeView and avoid toggling settings on users' behalf. > > > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:309: this._toggleDeviceFrames(); > > ditto > > > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): > > > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:213: var showRulers = this._showRulersSetting.get() && this._model.type() !== WebInspector.DeviceModeModel.Type.None; > > Just set |showRulers| to false here if we have outline. > > > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:291: var showingDevice = this._model.type() === WebInspector.DeviceModeModel.Type.Device; > > Unused. > > > > https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... > > third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:477: if (this._maxLength) > > This is not needed anymore. > > Done. @dgozman Is this what you mean in terms of implementation of disallowing frames + rulers to be visible at the same time? It works, but I find it strange. Eg, If you have rulers enabled, and enable frames, the frame will supplant the ruler. However, if you have frames enabled and attempt to enable rulers on top of it, its a no-op. Is that acceptable for now, until the final UI is determined? Thanks!
> @dgozman > Is this what you mean in terms of implementation of disallowing frames + rulers > to be visible at the same time? It works, but I find it strange. > > Eg, > If you have rulers enabled, and enable frames, the frame will supplant the > ruler. However, if you have frames enabled and attempt to enable rulers on top > of it, its a no-op. Is that acceptable for now, until the final UI is > determined? I think that's fine for now. We'll play with it and see how it goes.
ping! what's the next step here?
On 2016/04/19 00:31:39, pbakaus wrote: > ping! what's the next step here? I'm waiting for all comments to be addressed.
On 2016/04/19 at 00:57:18, dgozman wrote: > On 2016/04/19 00:31:39, pbakaus wrote: > > ping! what's the next step here? > > I'm waiting for all comments to be addressed. I believe I've addressed all comments, apologies if my reply didn't indicate that. Is there something specific that I may have left out? -mm
https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js (right): https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:275: // contextMenu.appendCheckboxItem(WebInspector.UIString("Show device frame"), this._deviceOutlineSetting.set.bind(this._deviceOutlineSetting, !this._deviceOutlineSetting.get()), this._deviceOutlineSetting.get() && this._model.outlineImage() !== "", (this._model.type() === WebInspector.DeviceModeModel.Type.None || this._model.outlineImage() === "")); On 2016/04/13 at 22:52:06, dgozman wrote: > nit: remove commented code Done. https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:291: this._toggleRulers(); On 2016/04/13 at 22:52:06, dgozman wrote: > We should instead do this in DeviceModeView and avoid toggling settings on users' behalf. Ack. https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeToolbar.js:309: this._toggleDeviceFrames(); On 2016/04/13 at 22:52:06, dgozman wrote: > ditto Ack. https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:291: var showingDevice = this._model.type() === WebInspector.DeviceModeModel.Type.Device; On 2016/04/13 at 22:52:06, dgozman wrote: > Unused. Done. https://codereview.chromium.org/1650243004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:477: if (this._maxLength) On 2016/04/13 at 22:52:06, dgozman wrote: > This is not needed anymore. Removed.
https://codereview.chromium.org/1650243004/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:291: var showingDevice = this._model.type() === WebInspector.DeviceModeModel.Type.Device; This is still unused.
https://codereview.chromium.org/1650243004/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/1650243004/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:291: var showingDevice = this._model.type() === WebInspector.DeviceModeModel.Type.Device; On 2016/04/19 at 17:14:42, dgozman wrote: > This is still unused. Doh! Gone now.
lgtm
On 2016/04/19 at 17:29:12, dgozman wrote: > lgtm Thanks a ton Dmitry! One last question before I attempt to land this.. should I be concerned that in my build clicking the 'Experiments' tab in devtools would crash the process? Were you able to patch this change, and did that occur? It was occuring even without any changes to Main.js, so I'm guessing it might have something to do with my particular build environment? I tested the functionally by (temporarily) manually enabling the experiment via code, rather than in the UI. -mm
On 2016/04/19 17:40:48, mmccoy wrote: > On 2016/04/19 at 17:29:12, dgozman wrote: > > lgtm > > Thanks a ton Dmitry! > > One last question before I attempt to land this.. should I be concerned that in > my build clicking the 'Experiments' tab in devtools would crash the process? > Were you able to patch this change, and did that occur? It was occuring even > without any changes to Main.js, so I'm guessing it might have something to do > with my particular build environment? I tested the functionally by > (temporarily) manually enabling the experiment via code, rather than in the UI. > > -mm I don't think your changes could cause a crash. Let's land this. In the worst case we'd revert, but I doubt it.
On 2016/04/19 at 17:42:46, dgozman wrote: > On 2016/04/19 17:40:48, mmccoy wrote: > > On 2016/04/19 at 17:29:12, dgozman wrote: > > > lgtm > > > > Thanks a ton Dmitry! > > > > One last question before I attempt to land this.. should I be concerned that in > > my build clicking the 'Experiments' tab in devtools would crash the process? > > Were you able to patch this change, and did that occur? It was occuring even > > without any changes to Main.js, so I'm guessing it might have something to do > > with my particular build environment? I tested the functionally by > > (temporarily) manually enabling the experiment via code, rather than in the UI. > > > > -mm > > I don't think your changes could cause a crash. Let's land this. In the worst case we'd revert, but I doubt it. Roger that, thanks for confirming! Landing.
The CQ bit was checked by mmccoy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1650243004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1650243004/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Initial implementation for an overflow option for displaying a device art outline in emulation mode. This patch implements the initial view logic and exposed overflow UI to toggle device art for all flagship/default device profiles _except_ Samsung Galaxy S5 (pending assets). BUG=411892 ========== to ========== [DevTools] Initial implementation for an overflow option for displaying a device art outline in emulation mode. This patch implements the initial view logic and exposed overflow UI to toggle device art for all flagship/default device profiles _except_ Samsung Galaxy S5 (pending assets). BUG=411892 Committed: https://crrev.com/d1a7ca28d7d40c3fa23568249d14dc378b1ecc9c Cr-Commit-Position: refs/heads/master@{#388278} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/d1a7ca28d7d40c3fa23568249d14dc378b1ecc9c Cr-Commit-Position: refs/heads/master@{#388278} |
