Print Preview: Making margin lines draggable.

Print Preview: Making margin lines draggable.

BUG=69337
TEST=Select custom margins. Click and drag on a margin line to adjust the value.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106042

[Evan Stade, 2011-10-14 19:04:49]

what happens when the user drags the mouse outside of the mainview (or outside
the browser completely) before releasing? Do we still get a mouseup on the
mainview?

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_textbox.js (right):

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:195: y =  0.5 -
offsetY;
extra space

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui.js (right):

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:134:
this.onMarginLineMouseUp.bind(this));
I don't think this is necessary. You are double-handling mouse ups on margin
line.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:147:
window.document.onmousemove = this.onMouseMove_.bind(this);
use addEventListener

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:159:
this.lastClickedMarginsUIPair.box_.onBlur_();
don't do this -- it's a hack. Make a helper function with an appropriate name
and call that from here and from onBlur_.

Also, the underscore means private, yet you are accessing the variable anyway
(twice in this line)

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:161:
window.document.onmousemove = null;
instead of attaching and detaching, always leave the handler attached. Do
nothing if not in dragging mode (which is toggled by mousedown and mouseup).

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui_pair.js (right):

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:21:
marginsUIPair.classList.add('top-bottom');
ternary operator

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:203: if (this.isTop_()
|| this.isBottom_()) {
no curlies

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:210: /**
function explanation.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:218: dragDistance =
(this.originBeforeDragging.distanceYFrom(rhs) -
I don't think distanceYFrom is useful. It's actually shorter and clearer to do

rhs.y - this.originBeforeDragging.y

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:226: dragDistance *=
-1;
I don't think that multiplying by -1 makes sense here if the @return comment is
correct (?)

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:242: if (validity ==
0) {
no curlies

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:259: if
(this.isTop_())
when some of the conditional blocks in the stanza have curlies, all of them must

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:279: */
@private

[dpapad, 2011-10-14 20:57:58]

I am listening for mousemove and mouseup in window.document, not mainview
anymore. Therefore they are still handled even if the mouse is outside the
browser window.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_textbox.js (right):

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:195: y =  0.5 -
offsetY;
On 2011/10/14 19:04:49, Evan Stade wrote:
> extra space

Done.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui.js (right):

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:134:
this.onMarginLineMouseUp.bind(this));
On 2011/10/14 19:04:49, Evan Stade wrote:
> I don't think this is necessary. You are double-handling mouse ups on margin
> line.

Removed it since it is not necessary. It was not being double-handled though
since the mouseup event was not propagated to window.document.onmouseup so
onMarginLineMouseUp was executed once regardless.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:147:
window.document.onmousemove = this.onMouseMove_.bind(this);
On 2011/10/14 19:04:49, Evan Stade wrote:
> use addEventListener

Done.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:159:
this.lastClickedMarginsUIPair.box_.onBlur_();
On 2011/10/14 19:04:49, Evan Stade wrote:
> don't do this -- it's a hack. Make a helper function with an appropriate name
> and call that from here and from onBlur_.
> 
> Also, the underscore means private, yet you are accessing the variable anyway
> (twice in this line)

Working on removing this hack. I had in mind to do it but missed it somehow.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:161:
window.document.onmousemove = null;
On 2011/10/14 19:04:49, Evan Stade wrote:
> instead of attaching and detaching, always leave the handler attached. Do
> nothing if not in dragging mode (which is toggled by mousedown and mouseup).

I changed this for onmouseup only. Having the onmousemove handler always
attached has an impact in performace. It will execute repeatedly, checking for
(this.lastClickedMarginsUIPair == null) to decide whether to act or not. I don't
see what is the problem with detaching and attaching when necessary.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui_pair.js (right):

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:21:
marginsUIPair.classList.add('top-bottom');
On 2011/10/14 19:04:49, Evan Stade wrote:
> ternary operator

Done.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:203: if (this.isTop_()
|| this.isBottom_()) {
On 2011/10/14 19:04:49, Evan Stade wrote:
> no curlies

Done.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:210: /**
On 2011/10/14 19:04:49, Evan Stade wrote:
> function explanation.

The explanation here would be a copy/paste of the @return comment, so I left it
out.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:218: dragDistance =
(this.originBeforeDragging.distanceYFrom(rhs) -
On 2011/10/14 19:04:49, Evan Stade wrote:
> I don't think distanceYFrom is useful. It's actually shorter and clearer to do
> 
> rhs.y - this.originBeforeDragging.y

Done.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:226: dragDistance *=
-1;
On 2011/10/14 19:04:49, Evan Stade wrote:
> I don't think that multiplying by -1 makes sense here if the @return comment
is
> correct (?)

My wording was not correct, since distance can't be negative. I meant
displacement which can be anything.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:242: if (validity ==
0) {
On 2011/10/14 19:04:49, Evan Stade wrote:
> no curlies

Done.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:259: if
(this.isTop_())
On 2011/10/14 19:04:49, Evan Stade wrote:
> when some of the conditional blocks in the stanza have curlies, all of them
must

Done.

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:279: */
On 2011/10/14 19:04:49, Evan Stade wrote:
> @private

Done.

[Evan Stade, 2011-10-14 21:58:36]

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui.js (right):

http://codereview.chromium.org/8233030/diff/28001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:161:
window.document.onmousemove = null;
On 2011/10/14 20:57:58, dpapad wrote:
> On 2011/10/14 19:04:49, Evan Stade wrote:
> > instead of attaching and detaching, always leave the handler attached. Do
> > nothing if not in dragging mode (which is toggled by mousedown and mouseup).
> 
> I changed this for onmouseup only. Having the onmousemove handler always
> attached has an impact in performace. It will execute repeatedly, checking for
> (this.lastClickedMarginsUIPair == null) to decide whether to act or not.

this will not measurably impact performance

> I don't
> see what is the problem with detaching and attaching when necessary.

it's more code

but I guess I don't really care that much

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_textbox.js (right):

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:61: return
print_preview.extractMarginValue(this.value);
@type

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:178: * Calculates the
coordinates where |this| should be displayed.
why are you not doing this in CSS? remember how I mentioned that you could do
50% with some x/y offset via margins, padding, or -webkit-transform: translate

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:180: *     should be
drawn relative to the upper left corner of the plugin.
(in percent, from 0 to 1)

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:190: if (this.isTop_())
{
for your consideration:

var x = 0.5, y = 0.5;
if (this.isTop_() || this.isBottom_())
  x -= offsetX / 2;
else
  y -= offsetY / 2;

if (this.isRight_())
  x -= offsetX;
else if (this.isBottom_())
  y -= offsetY;

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_utils.js (right):

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:47: * @param {sting} text
The text to check (in inches).
can you file a bug to i18n-ize the measurements

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:86: *     certain
precision and back.
@return

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:95: function Point(x, y)
{
once again, not a useful class

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:132: 
@return docs for all these functions

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:141: validateMarginValue:
validateMarginValue,
keep the list alphabetical

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui.js (right):

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:172: * Brings |uiPair| in
front of the other pairs.
a note about why this is necessary would be helpful

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:176:
this.pairsAsList.forEach(function(pair) { pair.style.zIndex = 0; });
instead of editing style from js, apply a CSS class

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui_pair.js (right):

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:213: dragDisplacement
*= -1;
my point was that it seems like if you take the bottom line and drag up, versus
if you take the top line and drag up, the signs will be different. So you're
using an inverted coordinate system in each case. I think the comment should
mention this somehow.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:286: 
don't need this newline

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.css (right):

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:750:
.margins-ui-pair.left-right {
vertical seems like a better name than left-right, however I think

.margins-ui-pair.left,
.margins-ui-pair.right {

will be what you'll want after you make the textbox positioning more CSSy.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:824: }
824 line css file? you should break this up to match the .js files (can be
another patch)

[dpapad, 2011-10-14 23:14:38]

Addressed comments.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_textbox.js (right):

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:61: return
print_preview.extractMarginValue(this.value);
On 2011/10/14 21:58:36, Evan Stade wrote:
> @type

Done.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:178: * Calculates the
coordinates where |this| should be displayed.
On 2011/10/14 21:58:36, Evan Stade wrote:
> why are you not doing this in CSS? remember how I mentioned that you could do
> 50% with some x/y offset via margins, padding, or -webkit-transform: translate

I will try this on a separate CL once this lands.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:180: *     should be
drawn relative to the upper left corner of the plugin.
On 2011/10/14 21:58:36, Evan Stade wrote:
> (in percent, from 0 to 1)

Done.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:190: if (this.isTop_())
{
On 2011/10/14 21:58:36, Evan Stade wrote:
> for your consideration:
> 
> var x = 0.5, y = 0.5;
> if (this.isTop_() || this.isBottom_())
>   x -= offsetX / 2;
> else
>   y -= offsetY / 2;
> 
> if (this.isRight_())
>   x -= offsetX;
> else if (this.isBottom_())
>   y -= offsetY;

Done, initializing x,y to 0.5, but the rest seems less readable since you need
to look in two places to figure out what is the final x and y.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_utils.js (right):

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:47: * @param {sting} text
The text to check (in inches).
On 2011/10/14 21:58:36, Evan Stade wrote:
> can you file a bug to i18n-ize the measurements

Done, filed bug 100416.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:86: *     certain
precision and back.
On 2011/10/14 21:58:36, Evan Stade wrote:
> @return

Done.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:95: function Point(x, y)
{
On 2011/10/14 21:58:36, Evan Stade wrote:
> once again, not a useful class

I was using distanceXFrom, distanceYFrom before addressing your previous
comments. In any case I see it as a typedef in C++, not necessary but helps.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:132: 
On 2011/10/14 21:58:36, Evan Stade wrote:
> @return docs for all these functions

Done.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:141: validateMarginValue:
validateMarginValue,
On 2011/10/14 21:58:36, Evan Stade wrote:
> keep the list alphabetical

Done.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui.js (right):

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:172: * Brings |uiPair| in
front of the other pairs.
On 2011/10/14 21:58:36, Evan Stade wrote:
> a note about why this is necessary would be helpful

Done.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:176:
this.pairsAsList.forEach(function(pair) { pair.style.zIndex = 0; });
On 2011/10/14 21:58:36, Evan Stade wrote:
> instead of editing style from js, apply a CSS class

Done.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui_pair.js (right):

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:213: dragDisplacement
*= -1;
On 2011/10/14 21:58:36, Evan Stade wrote:
> my point was that it seems like if you take the bottom line and drag up,
versus
> if you take the top line and drag up, the signs will be different. So you're
> using an inverted coordinate system in each case. I think the comment should
> mention this somehow.

Done.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui_pair.js:286: 
On 2011/10/14 21:58:36, Evan Stade wrote:
> don't need this newline

Done.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.css (right):

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:750:
.margins-ui-pair.left-right {
On 2011/10/14 21:58:36, Evan Stade wrote:
> vertical seems like a better name than left-right, however I think
> 
> .margins-ui-pair.left,
> .margins-ui-pair.right {
> 
> will be what you'll want after you make the textbox positioning more CSSy.

Done.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:824: }
On 2011/10/14 21:58:36, Evan Stade wrote:
> 824 line css file? you should break this up to match the .js files (can be
> another patch)

I will keep this in mind (and file a bug to keep track of it).

[Evan Stade, 2011-10-17 21:36:45]

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_textbox.js (right):

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:178: * Calculates the
coordinates where |this| should be displayed.
On 2011/10/14 23:14:38, dpapad wrote:
> On 2011/10/14 21:58:36, Evan Stade wrote:
> > why are you not doing this in CSS? remember how I mentioned that you could
do
> > 50% with some x/y offset via margins, padding, or -webkit-transform:
translate
> 
> I will try this on a separate CL once this lands.

doing so now would actually make this CL simpler and I don't think it would take
a lot of time. Overly complex code is more likely to have bugs; also if you land
code right before a branch, then change it right after, finding bugs and merging
fixes becomes harder/less likely (for example, you wouldn't get the benefit of
canary user testing). I would rather we land the code we want to keep, then (if
necessary) find and fix the bugs in it.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_utils.js (right):

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:95: function Point(x, y)
{
On 2011/10/14 23:14:38, dpapad wrote:
> On 2011/10/14 21:58:36, Evan Stade wrote:
> > once again, not a useful class
> 
> I was using distanceXFrom, distanceYFrom before addressing your previous
> comments.

but now you aren't

> In any case I see it as a typedef in C++, not necessary but helps.

we've already had this discussion and I don't see what has changed that would
lead us to a different conclusion. The point of a typedef is to abstract away
the implementation details (which you haven't done here as users of this class
must know its internals); to create a shorthand name for a complex templated
type definition (also not the same thing); or to create a compatibility layer
for different build configurations.

http://codereview.chromium.org/8233030/diff/38001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins.css (right):

http://codereview.chromium.org/8233030/diff/38001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins.css:1:
input[type='text'].margin-box {
license header

http://codereview.chromium.org/8233030/diff/38001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins.css:55: 
can you make sure you don't have extra newlines at end of files (I don't know if
you have 2 \ns here or if rietveld is displaying it wrong)

[dpapad, 2011-10-17 23:28:06]

Addressed comments. Also changed calls to the plugin so that we only query for
the page location when the onPreviewPositionHasChanged() is fired for 2 reasons
1) Avoid unnecessary calls to the plugin.
2) It is related to a crash, seemingly because the page location was queried
when no page was shown.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_textbox.js (right):

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:178: * Calculates the
coordinates where |this| should be displayed.
On 2011/10/17 21:36:45, Evan Stade wrote:
> On 2011/10/14 23:14:38, dpapad wrote:
> > On 2011/10/14 21:58:36, Evan Stade wrote:
> > > why are you not doing this in CSS? remember how I mentioned that you could
> do
> > > 50% with some x/y offset via margins, padding, or -webkit-transform:
> translate
> > 
> > I will try this on a separate CL once this lands.
> 
> doing so now would actually make this CL simpler and I don't think it would
take
> a lot of time. Overly complex code is more likely to have bugs; also if you
land
> code right before a branch, then change it right after, finding bugs and
merging
> fixes becomes harder/less likely (for example, you wouldn't get the benefit of
> canary user testing). I would rather we land the code we want to keep, then
(if
> necessary) find and fix the bugs in it.

Done.

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_utils.js (right):

http://codereview.chromium.org/8233030/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:95: function Point(x, y)
{
On 2011/10/17 21:36:45, Evan Stade wrote:
> On 2011/10/14 23:14:38, dpapad wrote:
> > On 2011/10/14 21:58:36, Evan Stade wrote:
> > > once again, not a useful class
> > 
> > I was using distanceXFrom, distanceYFrom before addressing your previous
> > comments.
> 
> but now you aren't
> 
> > In any case I see it as a typedef in C++, not necessary but helps.
> 
> we've already had this discussion and I don't see what has changed that would
> lead us to a different conclusion. The point of a typedef is to abstract away
> the implementation details (which you haven't done here as users of this class
> must know its internals); to create a shorthand name for a complex templated
> type definition (also not the same thing); or to create a compatibility layer
> for different build configurations.

Done.

http://codereview.chromium.org/8233030/diff/38001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins.css (right):

http://codereview.chromium.org/8233030/diff/38001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins.css:1:
input[type='text'].margin-box {
On 2011/10/17 21:36:45, Evan Stade wrote:
> license header

Done.

http://codereview.chromium.org/8233030/diff/38001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins.css:55: 
On 2011/10/17 21:36:45, Evan Stade wrote:
> can you make sure you don't have extra newlines at end of files (I don't know
if
> you have 2 \ns here or if rietveld is displaying it wrong)

Done.

[Evan Stade, 2011-10-18 01:04:40]

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_utils.js (right):

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:52: *     minimum margin,
1 if the value is larger than the maximum margin.
dang, this is getting complicated. You should create an enum for all the
possible return states

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins.css (right):

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins.css:8: color: #fff;
white

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui.js (right):

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:178:
pair.classList.remove('front');
nit: rename 'front' to 'dragging'. (js/css separation of function and
appearance)

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1115: <include
src="margins_ui_pair.js"/>
this should go after margins_ui

[dpapad, 2011-10-18 02:18:11]

Addressed comments

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_utils.js (right):

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:52: *     minimum margin,
1 if the value is larger than the maximum margin.
On 2011/10/18 01:04:40, Evan Stade wrote:
> dang, this is getting complicated. You should create an enum for all the
> possible return states

Done.

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins.css (right):

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins.css:8: color: #fff;
On 2011/10/18 01:04:40, Evan Stade wrote:
> white

Done.

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui.js (right):

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:178:
pair.classList.remove('front');
On 2011/10/18 01:04:40, Evan Stade wrote:
> nit: rename 'front' to 'dragging'. (js/css separation of function and
> appearance)

Done.

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8233030/diff/43001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1115: <include
src="margins_ui_pair.js"/>
On 2011/10/18 01:04:40, Evan Stade wrote:
> this should go after margins_ui

Done.

[Evan Stade, 2011-10-18 03:56:07]

lgtm

[commit-bot: I haz the power, 2011-10-18 05:23:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpapad@chromium.org/8233030/44013

[commit-bot: I haz the power, 2011-10-18 05:23:34]

Can't apply patch for file
chrome/browser/resources/print_preview/margin_settings.js.
While running patch -p1 --forward --force;
patching file chrome/browser/resources/print_preview/margin_settings.js
Hunk #2 FAILED at 160.
Hunk #3 succeeded at 205 (offset 1 line).
Hunk #4 succeeded at 288 (offset 2 lines).
Hunk #5 succeeded at 326 (offset 2 lines).
Hunk #6 succeeded at 393 (offset 2 lines).
Hunk #7 succeeded at 408 (offset 2 lines).
Hunk #8 succeeded at 419 (offset 2 lines).
Hunk #9 succeeded at 433 (offset 2 lines).
Hunk #10 succeeded at 497 (offset 2 lines).
Hunk #11 succeeded at 523 (offset 2 lines).
Hunk #12 succeeded at 541 (offset 2 lines).
Hunk #13 succeeded at 572 (offset 2 lines).
1 out of 13 hunks FAILED -- saving rejects to file
chrome/browser/resources/print_preview/margin_settings.js.rej

[commit-bot: I haz the power, 2011-10-18 05:41:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpapad@chromium.org/8233030/46002

[commit-bot: I haz the power, 2011-10-18 07:28:48]

Change committed as 106042