Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
881243003:
Add designer-stage (Closed)
|
Created:
5 years, 10 months ago by justinfagnani Modified:
5 years, 10 months ago Reviewers:
imac Base URL:
https://github.com/PolymerLabs/designer2.git@master Visibility:
Public. |
DescriptionAdd designer-stage
BUG=
Patch Set 1 #
Total comments: 42
Patch Set 2 : Review comments #
Messages
Total messages: 6 (1 generated)
justinfagnani@google.com changed reviewers: + imac@google.com
FYI: This has a little bit of demo content for testing until I get some content loading functionality ready.
https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/demo... File elements/designer-stage/demo.html (right): https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/demo... elements/designer-stage/demo.html:5: --> Go w/ the big license blurb (e.g. https://github.com/Polymer/wct-local/blob/master/lib/plugin.js#L1-9): * It points to the authors/contributors/patents lists. * You don't need a separate LICENSE file w/ this guy. * @license makes various tools happy (uglify will preserve it). https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/demo... elements/designer-stage/demo.html:10: body { Probably want html, body { here so you have height: 100% on html too https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... File elements/designer-stage/designer-stage.html (right): https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:11: <link rel="import" href="../designer-selection/designer-selection.html"> I think you forgot to include this in the patch https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:13: <body> IMO going w/ the `html`-less and `body`-less style makes way more sense for imports: * It makes it pretty clear that this file is not to be used directly as a master document. * Less boilerplate per import. * Also, it's just the norm for all our import. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:42: } You may want pointer-events: none; on this guy (but it's not clear to me w/o designer-selection's source) https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:68: frame.onload = function() { Hey, use `addEventListener` here! I don't have a rational reason other than the old style feels wrong https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:68: frame.onload = function() { Seems like there's a potential race here too: couldn't the frame have loaded by now (e.g. polyfill land)? https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:73: }; Don't forget to handle the `error` event! https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:83: }, Are you aiming for a UUID here, or just something reasonably unique? Might want to use crypto.getRandomValues() when it's available (e.g. everything but IE 10) (being on the same machine makes RNG attacks a bit more real, I think) https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:87: throw new Error('Invalid token'); Stick the name of the token in that error message so it's more actionable (log injection concerns be damned, IMO). Or maybe console.log it in addition to the throw https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:97: } As this grows, consider moving to a lookup table https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:110: style.height = bounds.height; Probably want to clamp these to stay within the stage (or maybe use overflow: hidden to accomplish that) https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:116: var bounds = data.bounds; `data` can be null here https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:120: selection.directions = ResizeDirection.width_height; Where is `ResizeDirection` coming from? https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:125: // Possibly implement message batching or extension. Damn right you'll factor that out! https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:143: }, '*'); Might be good to use an actual origin. What if an element `window.location`s on you? https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:151: message_type: 'selectElement', IMO you should do the dreaded ##ms debounce here, otherwise every time someone clicks the element is going to briefly select. Aka, have selection begin after 300ms, OR the mouse moved 10px or so. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:153: y: e.clientY You should probably translate these relative to `frame`. (Also, I could have sworn there was a coordinate translation helper somewhere in the depths of the DOM API, but I can't find it...)
PTAL https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/demo... File elements/designer-stage/demo.html (right): https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/demo... elements/designer-stage/demo.html:5: --> On 2015/02/05 01:54:06, imac wrote: > Go w/ the big license blurb (e.g. > https://github.com/Polymer/wct-local/blob/master/lib/plugin.js#L1-9): > > * It points to the authors/contributors/patents lists. > * You don't need a separate LICENSE file w/ this guy. > * @license makes various tools happy (uglify will preserve it). I grabbed the version out of an .html file, which doesn't have @license https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/demo... elements/designer-stage/demo.html:10: body { On 2015/02/05 01:54:06, imac wrote: > Probably want html, body { here so you have height: 100% on html too Done. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... File elements/designer-stage/designer-stage.html (right): https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:11: <link rel="import" href="../designer-selection/designer-selection.html"> On 2015/02/05 01:54:07, imac wrote: > I think you forgot to include this in the patch It's in the other CL https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:13: <body> On 2015/02/05 01:54:06, imac wrote: > IMO going w/ the `html`-less and `body`-less style makes way more sense for > imports: > > * It makes it pretty clear that this file is not to be used directly as a master > document. > * Less boilerplate per import. > * Also, it's just the norm for all our import. Done. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:42: } On 2015/02/05 01:54:07, imac wrote: > You may want pointer-events: none; on this guy (but it's not clear to me w/o > designer-selection's source) no, the selector needs events. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:68: frame.onload = function() { On 2015/02/05 01:54:06, imac wrote: > Hey, use `addEventListener` here! I don't have a rational reason other than the > old style feels wrong Done. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:68: frame.onload = function() { On 2015/02/05 01:54:07, imac wrote: > Seems like there's a potential race here too: couldn't the frame have loaded by > now (e.g. polyfill land)? In that case I'm unsure how to know the content is loaded (remember that I can't access the frame contents). https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:73: }; On 2015/02/05 01:54:06, imac wrote: > Don't forget to handle the `error` event! Done. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:83: }, On 2015/02/05 01:54:06, imac wrote: > Are you aiming for a UUID here, or just something reasonably unique? Might want > to use crypto.getRandomValues() when it's available (e.g. everything but IE 10) > > (being on the same machine makes RNG attacks a bit more real, I think) Just reasonably unique. I can use crypto soon I guess. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:87: throw new Error('Invalid token'); On 2015/02/05 01:54:07, imac wrote: > Stick the name of the token in that error message so it's more actionable (log > injection concerns be damned, IMO). Or maybe console.log it in addition to the > throw Done. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:97: } On 2015/02/05 01:54:06, imac wrote: > As this grows, consider moving to a lookup table Acknowledged. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:110: style.height = bounds.height; On 2015/02/05 01:54:07, imac wrote: > Probably want to clamp these to stay within the stage (or maybe use overflow: > hidden to accomplish that) This is just a very rough start, I also need to validate that left > top, for instance. Will do more soon. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:116: var bounds = data.bounds; On 2015/02/05 01:54:07, imac wrote: > `data` can be null here I'm going to trust messages from myself for now, especially since with the token I can reasonably trust they're from my code. I can live with the NPE if something weird happens, since I'm not sure how I could recover from that error. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:120: selection.directions = ResizeDirection.width_height; On 2015/02/05 01:54:07, imac wrote: > Where is `ResizeDirection` coming from? Loaded with designer-selection. I'll modularize soon. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:125: // Possibly implement message batching or extension. On 2015/02/05 01:54:07, imac wrote: > Damn right you'll factor that out! Acknowledged! :) https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:143: }, '*'); On 2015/02/05 01:54:06, imac wrote: > Might be good to use an actual origin. I'm not sure what origin to put, since a sandboxed iframe get a unique origin. The other direction maybe. I'm unsure how these messages could be intercepted though, since I create and have a reference to the actual iframe. >What if an element `window.location`s on you? I don't follow. Any 3rd party element is in the sandboxed iframe and doesn't have navigation capability. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:151: message_type: 'selectElement', On 2015/02/05 01:54:07, imac wrote: > IMO you should do the dreaded ##ms debounce here, otherwise every time someone > clicks the element is going to briefly select. > > Aka, have selection begin after 300ms, OR the mouse moved 10px or so. What wrong with selecting on every click? https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:153: y: e.clientY On 2015/02/05 01:54:07, imac wrote: > You should probably translate these relative to `frame`. (Also, I could have > sworn there was a coordinate translation helper somewhere in the depths of the > DOM API, but I can't find it...) Adding todo. I need some test where client bounds != stage bounds.
lgtm https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... File elements/designer-stage/designer-stage.html (right): https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:68: frame.onload = function() { On 2015/02/05 22:15:45, justinfagnani wrote: > On 2015/02/05 01:54:07, imac wrote: > > Seems like there's a potential race here too: couldn't the frame have loaded > by > > now (e.g. polyfill land)? > > In that case I'm unsure how to know the content is loaded (remember that I can't > access the frame contents). Hmm, could post a message to it and hope for a response - syn/ack or something https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:143: }, '*'); On 2015/02/05 22:15:45, justinfagnani wrote: > On 2015/02/05 01:54:06, imac wrote: > > Might be good to use an actual origin. > > I'm not sure what origin to put, since a sandboxed iframe get a unique origin. > The other direction maybe. I'm unsure how these messages could be intercepted > though, since I create and have a reference to the actual iframe. > > >What if an element `window.location`s on you? > > I don't follow. Any 3rd party element is in the sandboxed iframe and doesn't > have navigation capability. Oh, sandboxed iframes prevent navigation? Neat! https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:151: message_type: 'selectElement', On 2015/02/05 22:15:45, justinfagnani wrote: > On 2015/02/05 01:54:07, imac wrote: > > IMO you should do the dreaded ##ms debounce here, otherwise every time someone > > clicks the element is going to briefly select. > > > > Aka, have selection begin after 300ms, OR the mouse moved 10px or so. > > What wrong with selecting on every click? That works too! I was expecting the selection visuals to be different from dragging
Thanks! https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... File elements/designer-stage/designer-stage.html (right): https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:68: frame.onload = function() { On 2015/02/06 20:29:49, imac wrote: > On 2015/02/05 22:15:45, justinfagnani wrote: > > On 2015/02/05 01:54:07, imac wrote: > > > Seems like there's a potential race here too: couldn't the frame have loaded > > by > > > now (e.g. polyfill land)? > > > > In that case I'm unsure how to know the content is loaded (remember that I > can't > > access the frame contents). > > Hmm, could post a message to it and hope for a response - syn/ack or something I'll test in polyfill land. I suspect the setup and potential races will change when I add support for loading files. https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:143: }, '*'); On 2015/02/06 20:29:49, imac wrote: > On 2015/02/05 22:15:45, justinfagnani wrote: > > On 2015/02/05 01:54:06, imac wrote: > > > Might be good to use an actual origin. > > > > I'm not sure what origin to put, since a sandboxed iframe get a unique origin. > > The other direction maybe. I'm unsure how these messages could be intercepted > > though, since I create and have a reference to the actual iframe. > > > > >What if an element `window.location`s on you? > > > > I don't follow. Any 3rd party element is in the sandboxed iframe and doesn't > > have navigation capability. > > Oh, sandboxed iframes prevent navigation? Neat! And popups, pointer-locks and a few other things... :) https://codereview.chromium.org/881243003/diff/1/elements/designer-stage/desi... elements/designer-stage/designer-stage.html:151: message_type: 'selectElement', On 2015/02/06 20:29:49, imac wrote: > On 2015/02/05 22:15:45, justinfagnani wrote: > > On 2015/02/05 01:54:07, imac wrote: > > > IMO you should do the dreaded ##ms debounce here, otherwise every time > someone > > > clicks the element is going to briefly select. > > > > > > Aka, have selection begin after 300ms, OR the mouse moved 10px or so. > > > > What wrong with selecting on every click? > > That works too! I was expecting the selection visuals to be different from > dragging Acknowledged. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
