Refactor print preview web ui

I've separated out the model from the UI widgets into a separate "data" folder. Some of the classes in the "data" folder are event targets that dispatch events when their state changes. UI widgets listen to these state changes and update their UI accordingly. The two most important "data" classes are PrintTicketStore and DestinationStore. These two manage the state of the print ticket (aka print settings) and the list of all of the print destinations respectively.

The UI widgets are divided into top-level components, settings components, and previewarea components.

Top-level components just include the outer most container widget (called PrintPreview) which handles global events and PrintHeader which renders the print and cancel buttons and some statistics on the document. If you were to start reviewing, PrintPreview is where I would start.

Settings components include all of the UI widgets that can be used to change the state of the PrintTicketStore. They live in the "settings" folder and include "CopiesSettings" and "PageSettings".

Preview area components include all of the UI widgets involved on the main part of the preview UI. PreviewArea and CustomMargins are the two main widgets in the "previewarea" folder.

It's important to note that almost all of the UI widgets have a dependency on PrintTicketStore. Crucially, this allows all UI widgets to be independent from each other and just depend on a common object.

Previously, the state of the print ticket (aka print settings) were distributed in the various UI controls. For example, custom margins were stored both in the preview area and in the margin settings and when it came time to print, these UI components had to be queried for that information, or the question on whether to show the header-footer setting depended on the state of the preview area. Now the state is consolidated into PrintTicketStore. This allows modification of each of the UI widgets without the worry of breaking other widgets.

BUG=114206
TEST=

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

[Albert Bodenhamer, 2012-04-16 19:15:01]

Here's the initial patch you sent me uploaded to the code review server.  Please
skim it.  I'll look it over today as well.  Once we're somewhat happy with it,
I'll add some of the SMO folks to take a look.

[Albert Bodenhamer, 2012-04-16 19:25:06]

http://codereview.chromium.org/10108001/diff/1/chrome/browser/browser_resourc...
File chrome/browser/browser_resources.grd (right):

http://codereview.chromium.org/10108001/diff/1/chrome/browser/browser_resourc...
chrome/browser/browser_resources.grd:107: <include
name="IDR_PRINT_PREVIEW_IMAGES_CLASSIC_PRINTER_24"
Why 2 different color depths?  Keep in mind, space is at a premium.  Just pick
one and stick with it.

[Albert Bodenhamer, 2012-04-17 22:02:49]

Hey folks,

Robert has been working hard on the new print preview UI.  The CL has become
quite large though and I'd like to get folks to take a look at where he's going
with it sooner rather than later.

Can I get folks to take a look and provide feedback when you get a chance?

Thanks.

[kmadhusu, 2012-04-17 22:13:25]

You also need to update the corresponding WebUI tests. Ref:
src/chrome/test/data/webui/print_preview.js.

[Robert Toscano, 2012-04-17 22:15:19]

On 2012/04/17 22:13:25, kmadhusu wrote:
> You also need to update the corresponding WebUI tests. Ref:
> src/chrome/test/data/webui/print_preview.js.

This CL is not complete. There are lots of TODOs with corner cases I haven't
touched yet, but the overall structure is there. Consequently, none of the unit
tests have been updated in this CL.

[dpapad1, 2012-04-17 22:16:21]

On 2012/04/17 22:13:25, kmadhusu wrote:
> You also need to update the corresponding WebUI tests. Ref:
> src/chrome/test/data/webui/print_preview.js.

Also a more detailed description of this CL would help getting usefull comments
faster. What is it trying to do? it seems as if most of the UI is rewritten.

[Robert Toscano, 2012-04-19 21:28:45]

I've uploaded a new patch which has removed the new search functionality. Now
this CL just consist of the new design.

[Robert Toscano, 2012-04-19 22:23:38]

Hey Albert,

I see there are two files: chrome/browser/ui/webui/print_preview/base.html and
chrome/browser/ui/webui/print_preview/popup.html that have snuck into this CL.
Are these from your local client? They were introduced in patch #2.

[dpapad1, 2012-04-19 22:45:00]

Hey Robert,

I have started looking through the code, sending some initial comments (mostly
nits as I am getting familiar with the new code).

In general I like this change, by reading your description. We did indeed have
state spread across various Settings objects which made querying for state break
encapsulation and coupled things that should not be coupled. I will keep reading
and send more comments. Thank you for taking up such a heavy and important task.

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/chromium_capabilities.js
(right):

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/chromium_capabilities.js:8: function
ChromiumPrintTicket(capabilities) {
Could you add documentation (@type,@param,@returs for all members/methods)?

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/coordinate2d.js (right):

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/coordinate2d.js:8: function
Coordinate2d(x, y) {
Document this class.

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/destination.js (right):

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination.js:17: * @param
{Array.<string>} tags Tags associated with the destination.
If this is optional (seems like it is judging from line 54) use
@param{Array.<string>=}

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination.js:43: * Whether the
destination is local or cloud-based.
Nit (optional): "{@code true}  if destination is local, {@code false}  if
cloud-based."

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination.js:110: get
isGooglePromoted() {
I believe is better to use the "get foo() {...}" syntax only if you are directly
returning a member var, so in this case i would use regular method syntax (foo:
function() {...}), also for "get location()". You don't have to adhere just a
personal preference.

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/margins.js (right):

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/margins.js:56: * @return {boolean}
Whether this margins object is equivalent to another.
Nit: s/equivalent/equal

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/page_number_set.js (right):

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/page_number_set.js:11: * @param
{Array.<number>!} pageNumberList A list of page numbers to include
Exclamation marks should be at the start
!Array.<number>
here and elsewhere.

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/page_number_set.js:18: * @type
{Array.<number>}
!Array.<number>

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/page_number_set.js:81: equals:
function(other) {
Document |other|.

[Robert Toscano, 2012-04-23 23:00:09]

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/chromium_capabilities.js
(right):

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/chromium_capabilities.js:8: function
ChromiumPrintTicket(capabilities) {
On 2012/04/19 22:45:01, dpapad1 wrote:
> Could you add documentation (@type,@param,@returs for all members/methods)?

Done.

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/coordinate2d.js (right):

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/coordinate2d.js:8: function
Coordinate2d(x, y) {
On 2012/04/19 22:45:01, dpapad1 wrote:
> Document this class.

Done.

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/destination.js (right):

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination.js:17: * @param
{Array.<string>} tags Tags associated with the destination.
On 2012/04/19 22:45:01, dpapad1 wrote:
> If this is optional (seems like it is judging from line 54) use
> @param{Array.<string>=}

Done.

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination.js:43: * Whether the
destination is local or cloud-based.
I'll leave as is. FWIW, "Whether ...." is a bit more concise to me.

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination.js:110: get
isGooglePromoted() {
I agree with you on this point, but I'm also worried that it would be
inconsistent since most of the members of this class are simple getters. I was
struggling with this idea and decided it would make using the class easier if I
tried to keep the usage of getters (or lack of) consistent within a class.

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/margins.js (right):

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/margins.js:56: * @return {boolean}
Whether this margins object is equivalent to another.
On 2012/04/19 22:45:01, dpapad1 wrote:
> Nit: s/equivalent/equal

Done.

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/page_number_set.js (right):

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/page_number_set.js:11: * @param
{Array.<number>!} pageNumberList A list of page numbers to include
On 2012/04/19 22:45:01, dpapad1 wrote:
> Exclamation marks should be at the start
> !Array.<number>
> here and elsewhere.

Done.

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/page_number_set.js:18: * @type
{Array.<number>}
On 2012/04/19 22:45:01, dpapad1 wrote:
> !Array.<number>

Done.

http://codereview.chromium.org/10108001/diff/12001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/page_number_set.js:81: equals:
function(other) {
On 2012/04/19 22:45:01, dpapad1 wrote:
> Document |other|.

Done.

[dpapad, 2012-04-24 01:24:56]

Posting some more comments, mostly nits, but also some questions.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/cloud_print_interface.js:166: * @param
{object} params HTTP parameters to include in the request.
Why change this? I believe Object is correct
(https://developers.google.com/closure/compiler/docs/js-for-compiler).

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/chromium_capabilities.js
(right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/chromium_capabilities.js:10: *
Nit: Unnecessary blank line.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/chromium_capabilities.js:73: * @type
{?print_preview.Margins.Type}
I think that "?" makes sense only for primitives. From the link in another
comment "All object types are nullable by default whether or not they are
declared with the Nullable operator."

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/coordinate2d.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/coordinate2d.js:47: *     translated
along the X and Y dimensions.
Nit: "translated" might fit in previous line, not sure.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/coordinate2d.js:56: *     by the
given factor.
Nit: "by" can fit in previous line.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/coordinate2d.js:63: * @param
{print_preview.Coordinate2d} other The other two-dimensional
Nit: Shorter comment
The point to compare against.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/margins.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/margins.js:52: DEFAULT: 'default',
Why changing these to strings?

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/measurement_system.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/measurement_system.js:22:
MeasurementSystem.UnitType.IMPERIAL : unitType;
Isn't this equivalent to
this.unitType_ = unitType || MeasurementSystem.UnitType.IMPERIAL;

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:593:
this.pageSize = new print_preview.Size(612, 792);
Where do these values come from?

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/native_layer.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:71:
NativeLayer.MarginsTypeMap_[print_preview.Margins.Type.DEFAULT] = 0;
This mapping was not needed when Margins.Type was an enum of numbers (see
related comment in margins.js). What is the benefit of using strings?

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:425: var nativeLayer =
new print_preview.NativeLayer();
Global var? Should all global vars be initialized in one place, so that they are
easily spotted?

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:431: //
localStrings.getStringF('printPreviewTitleFormat', initiatorTabTitle)
IIRC, we use the tab title to pre-populate the filename to save in the "Save
File As" dialog when saving to PDF.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:458: var marginsType;
This also got more complicated by converting MarginsType to an enum of strings,
otherwise it could be a simple assignment, right?

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:477: var
initialSettingsSetEvt =
Nit: An abbreviation for 2 saving characters not really worth it IMO.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/preview_generator.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/preview_generator.js:197: *     shown in
the preview. E.g an index of 0 might be the fifth page in
Took me a while to parse this comment. I think it would be slightly more clear
as follows
"An index of 0 is the first displayed page but not the first original document
page necessarily."

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/layout_settings.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/layout_settings.js:34:
LANDSCAPE_RADIO: 'layout-settings-landscape-radio',
Why using classes and not id's for unique elements?

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/layout_settings.js:53: return
this.getElement().getElementsByClassName(
This could be getElementById() combined with previous comment.

[Robert Toscano, 2012-04-24 22:29:55]

Here are my comment responses. I'm still waiting on Albert to upload my new
patch, but the answers to some of your questions are in here.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/cloud_print_interface.js:166: * @param
{object} params HTTP parameters to include in the request.
On 2012/04/24 01:24:56, dpapad wrote:
> Why change this? I believe Object is correct
> (https://developers.google.com/closure/compiler/docs/js-for-compiler).

Done.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/chromium_capabilities.js
(right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/chromium_capabilities.js:10: *
On 2012/04/24 01:24:56, dpapad wrote:
> Nit: Unnecessary blank line.

Done.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/chromium_capabilities.js:73: * @type
{?print_preview.Margins.Type}
It's not explicitly mentioned in the js doc whether this applies to typedefs of
a primitive. "print_preview.Margins.Type" is a typedef of "string". So I tend to
treat typedefs as the types they are aliasing.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/coordinate2d.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/coordinate2d.js:47: *     translated
along the X and Y dimensions.
3 chars too long.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/coordinate2d.js:56: *     by the
given factor.
No more room :(

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/coordinate2d.js:63: * @param
{print_preview.Coordinate2d} other The other two-dimensional
Nice, saved me a couple of lines.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/margins.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/margins.js:52: DEFAULT: 'default',
Good question. The only reason why numbers were used before was because they
matched values in the native (C++) layer. In an effort to isolate the native
layer from the javascript layer, I tried to move all dependencies on
native-related constants into nativelayer.js. So the Margins class should not
assign any semantic meaning to these enum items. That said, I chose strings
because easy to use in boolean conditionals. Compare:

marginType = null;
if (marginType) // false

marginType = 'foo';
if (marginType) // true

marginType = 3;
if (marginType) // true

marginType = 0;
if (marginType) // false, but in this case we want it to be true since 0 would
be a valid value for a margin type.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/measurement_system.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/measurement_system.js:22:
MeasurementSystem.UnitType.IMPERIAL : unitType;
On 2012/04/24 01:24:56, dpapad wrote:
> Isn't this equivalent to
> this.unitType_ = unitType || MeasurementSystem.UnitType.IMPERIAL;

Done.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:593:
this.pageSize = new print_preview.Size(612, 792);
8.5"x11" paper size. I've move the initialization and defaults to the
PrintTicketStore.initialize method, where it should make more sense.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/native_layer.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:71:
NativeLayer.MarginsTypeMap_[print_preview.Margins.Type.DEFAULT] = 0;
I explain the benefit in my comment response in margins.js.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:425: var nativeLayer =
new print_preview.NativeLayer();
Yea I'm not sure where I should put them. My goal was to have no global vars,
but because of the way that the native layer calls into javascript, all of the
callbacks below need to work on some global instance. An alternative would be to
set the global vars in the NativeLayer's constructor. Something like:

  print_preview.PrintPreview = function() {
    ...
    this.nativeLayer_ = new print_preview.NativeLayer();
    ...
  };

and then in NativeLayer constructor I would add all of the global functions:

  print_preview.NativeLayer = function() {
    ...
    global['setInitialSettings'] = this.setInitialSettings_.bind(this);
    ...
  };

  print_preview.NativeLayer.prototype = {
    ...
    setInitialSettings_: function(initialSettings) {
      ...
      this.dispatchEvent(event);
    },
    ...
  }

What do you think?

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:431: //
localStrings.getStringF('printPreviewTitleFormat', initiatorTabTitle)
This must happen in the native layer then, because all I saw before was that
this method set the print preview's document title to the tab title. The
document's title was never subsequently used in the javascript layer.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:458: var marginsType;
It could be, but then native semantics of these integer values would break
encapsulation, since they would need to be used in the rest of the app (namely
MarginSettings).

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:477: var
initialSettingsSetEvt =
On 2012/04/24 01:24:56, dpapad wrote:
> Nit: An abbreviation for 2 saving characters not really worth it IMO.

Done.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/preview_generator.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/preview_generator.js:197: *     shown in
the preview. E.g an index of 0 might be the fifth page in
On 2012/04/24 01:24:56, dpapad wrote:
> Took me a while to parse this comment. I think it would be slightly more clear
> as follows
> "An index of 0 is the first displayed page but not the first original document
> page necessarily."

Done.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/layout_settings.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/layout_settings.js:34:
LANDSCAPE_RADIO: 'layout-settings-landscape-radio',
Using class names will allow the component to be more modular. I.e. you will be
able to have multiple layoutsettings on the page. This might be useful for unit
testing or demoing. But if you use IDs within this component, then you create an
implicit contract between this component and the HTML document it's being used
in.

What do you think?

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/layout_settings.js:53: return
this.getElement().getElementsByClassName(
See previous comment.

[dpapad, 2012-04-24 22:50:19]

FYI, I am sheriffing today and tomorrow, will try to take a look at the rest of
the comments some time tomorrow.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/chromium_capabilities.js
(right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/chromium_capabilities.js:73: * @type
{?print_preview.Margins.Type}
On 2012/04/24 22:29:56, rltoscano wrote:
> It's not explicitly mentioned in the js doc whether this applies to typedefs
of
> a primitive. "print_preview.Margins.Type" is a typedef of "string". So I tend
to
> treat typedefs as the types they are aliasing. 

Hm, I guess I never think of this as a typedef, but as a new type therefore not
a primitive type.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/margins.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/margins.js:52: DEFAULT: 'default',
On 2012/04/24 22:29:56, rltoscano wrote:
> Good question. The only reason why numbers were used before was because they
> matched values in the native (C++) layer. In an effort to isolate the native
> layer from the javascript layer, I tried to move all dependencies on
> native-related constants into nativelayer.js. So the Margins class should not
> assign any semantic meaning to these enum items. That said, I chose strings
> because easy to use in boolean conditionals. Compare:
> 
> marginType = null;
> if (marginType) // false
> 
> marginType = 'foo';
> if (marginType) // true
> 
> marginType = 3;
> if (marginType) // true
> 
> marginType = 0;
> if (marginType) // false, but in this case we want it to be true since 0 would
> be a valid value for a margin type.

Alternatively you could add one more enum constant UNINITIALIZED = -1, and make
sure that whenever you declare a marginsType var you give it that value, then
you can replace
if (marginsType)
with
if (marginsType != Margins.Type.UNINITIALIZED)

I know is not as convenient but it would eliminate the need for a map, given
that you follow the enum order in the C++ code (and mention this in a comment
for future devs).

[dpapad1, 2012-04-26 16:04:44]

Some of the comments might be outdated (they were made from another account and
forgot to publish).

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:73:
PrintTicketStore.Event = {
Nit: Should this be PrintTicketStore.EventType (like goog.events.EventType)?

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:244:
isCopiesValidForValue: function(value) {
This method should be static (or a util method in a separate file), it does not
access any of the |this| object variables.

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/margin_settings.js (right):

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/margin_settings.js:135: get
select_() {
"get foo()" syntax for private functions seems unnecessary.

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/page_settings.js (right):

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/page_settings.js:57: set
isEnabled(isEnabled) {
I would name this setEnabled() instead of set isEnabled(). is*** implies that
this is a function with no side effects that returns a boolean. Also I prefer
avoiding the "set foo()" syntax if there is no corresponding get function.

[Robert Toscano, 2012-04-28 01:41:36]

Here are the responses to your latest comments. I've also sent abodenha@ my
patch, so he'll upload at earliest convenience.

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:73:
PrintTicketStore.Event = {
On 2012/04/26 16:04:44, dpapad1 wrote:
> Nit: Should this be PrintTicketStore.EventType (like goog.events.EventType)?

Done.

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:244:
isCopiesValidForValue: function(value) {
On 2012/04/26 16:04:44, dpapad1 wrote:
> This method should be static (or a util method in a separate file), it does
not
> access any of the |this| object variables.

Done.

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/margin_settings.js (right):

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/margin_settings.js:135: get
select_() {
What do you suggest? a "getSelect_" method?

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/page_settings.js (right):

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/page_settings.js:57: set
isEnabled(isEnabled) {
I wanted to model this after dom elements that have just properties (not
methods). E.g. disabled: "domEl.disabled = true;". But if you think the
getter/setter is confusing I can change it to use a regular method. But what
about the name setIsEnabled?

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/chromium_capabilities.js
(right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/chromium_capabilities.js:73: * @type
{?print_preview.Margins.Type}
I'm pretty sure the closure compiler will treat them as the base primitive type.
And I did find that the explanation of the @enum tag hints at it:
https://developers.google.com/closure/compiler/docs/js-for-compiler#tags

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/margins.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/margins.js:52: DEFAULT: 'default',
Ok, I'll go with what you suggest except for the UNINITIALIZED enum value. Seems
easier just to check for null instead.

[dpapad, 2012-04-30 16:19:25]

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/margin_settings.js (right):

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/margin_settings.js:135: get
select_() {
On 2012/04/28 01:41:37, rltoscano wrote:
> What do you suggest? a "getSelect_" method?

Either a getSelect_, or adding a select_ member variable and initialize it in
enterDocument(). I prefer the second option beacause 
 - you don't have to search the DOM every time you need to access the drop down
and
 - you don't even need a getter function, just access select_ directly.

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/page_settings.js (right):

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/page_settings.js:57: set
isEnabled(isEnabled) {
On 2012/04/28 01:41:37, rltoscano wrote:
> I wanted to model this after dom elements that have just properties (not
> methods). E.g. disabled: "domEl.disabled = true;". But if you think the
> getter/setter is confusing I can change it to use a regular method. But what
> about the name setIsEnabled?

Ok, so I see two possible namings.
 - If you prefer modeling this after DOM Elements, rename it to "enabled" and
keep the set syntax as is.
 - Otherwise rename to "setIsEnabled" or "setEnabled" both seem fine to me.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/chromium_capabilities.js
(right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/chromium_capabilities.js:73: * @type
{?print_preview.Margins.Type}
On 2012/04/28 01:41:37, rltoscano wrote:
> I'm pretty sure the closure compiler will treat them as the base primitive
type.
> And I did find that the explanation of the @enum tag hints at it:
> https://developers.google.com/closure/compiler/docs/js-for-compiler#tags

Sg.

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/margins.js (right):

http://codereview.chromium.org/10108001/diff/25001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/margins.js:52: DEFAULT: 'default',
On 2012/04/28 01:41:37, rltoscano wrote:
> Ok, I'll go with what you suggest except for the UNINITIALIZED enum value.
Seems
> easier just to check for null instead.

Ok, just make sure to initialize variables to null (or handle null and undefined
properly), specifically

var hello;
hello == null // true
hello === null // false
hello == undefined // true
hello === undefined // true

hello = null;
hello == null // true
hello === null // true
hello == undefined // true
hello === undefined // false

[Robert Toscano, 2012-05-05 00:01:33]

With this latest patch, all functionality is back up and running. Only thing
that's left is to start making the unit tests work again.

[Robert Toscano, 2012-05-05 00:02:50]

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/margin_settings.js (right):

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/margin_settings.js:135: get
select_() {
On 2012/04/30 16:19:25, dpapad wrote:
> On 2012/04/28 01:41:37, rltoscano wrote:
> > What do you suggest? a "getSelect_" method?
> 
> Either a getSelect_, or adding a select_ member variable and initialize it in
> enterDocument(). I prefer the second option beacause 
>  - you don't have to search the DOM every time you need to access the drop
down
> and
>  - you don't even need a getter function, just access select_ directly.

Done.

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/page_settings.js (right):

http://codereview.chromium.org/10108001/diff/16001/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/page_settings.js:57: set
isEnabled(isEnabled) {
On 2012/04/30 16:19:25, dpapad wrote:
> On 2012/04/28 01:41:37, rltoscano wrote:
> > I wanted to model this after dom elements that have just properties (not
> > methods). E.g. disabled: "domEl.disabled = true;". But if you think the
> > getter/setter is confusing I can change it to use a regular method. But what
> > about the name setIsEnabled?
> 
> Ok, so I see two possible namings.
>  - If you prefer modeling this after DOM Elements, rename it to "enabled" and
> keep the set syntax as is.
>  - Otherwise rename to "setIsEnabled" or "setEnabled" both seem fine to me.

Done.

[Robert Toscano, 2012-05-08 21:33:35]

All unit tests should be working now.

[dpapad, 2012-05-09 15:48:56]

Besides the margins code (as discussed per email), I noticed that the "Loading
preview..." message only appears the first time, it does not reappear in
subsequent regenerations of the document. To reproduce

1) go to http://en.wikipedia.org/wiki/August_2010_in_sports and hit ctrl+p
2) wait till the document loads
3) change portrait to landscape
The "Preview loading..." message should appear. IIRC the behavior was to show
that messge for any preview request that takes more than 200ms to complete.

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/destination.js (right):

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination.js:201:
state['capabilities'] = this.capabilities_.serialize();
Could you get the capabilities object unserialized and rely on line 203 to
serialize? It is passing twice through JSON.stringify now, right?

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/document_info.js (right):

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/document_info.js:32: * Size of the
pages of the document.
Document the unit of measurement.

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/document_info.js:38: * Printable
area of the document.
Same here.

[dpapad, 2012-05-09 22:34:18]

One more behavioral change I noticed is the way the settings on the left are
animated (fade in/out). For example take the following steps
 1) change margins to NONE
 2) Observe header and footer option fade out
 3) change margins back to minimum or default
 4) Observe header and footer option fade in

It feels very jumpy in both cases compared to ToT.

[Robert Toscano, 2012-05-09 22:35:42]

On 2012/05/09 22:34:18, dpapad wrote:
> One more behavioral change I noticed is the way the settings on the left are
> animated (fade in/out). For example take the following steps
>  1) change margins to NONE
>  2) Observe header and footer option fade out
>  3) change margins back to minimum or default
>  4) Observe header and footer option fade in
> 
> It feels very jumpy in both cases compared to ToT.

Could the jumpiness be due to running in Debug mode?

[dpapad, 2012-05-09 22:37:18]

On 2012/05/09 22:35:42, rltoscano wrote:
> On 2012/05/09 22:34:18, dpapad wrote:
> > One more behavioral change I noticed is the way the settings on the left are
> > animated (fade in/out). For example take the following steps
> >  1) change margins to NONE
> >  2) Observe header and footer option fade out
> >  3) change margins back to minimum or default
> >  4) Observe header and footer option fade in
> > 
> > It feels very jumpy in both cases compared to ToT.
> 
> Could the jumpiness be due to running in Debug mode?

I am running both in Release mode, don't know if it is just my idea, I think is
not.

[dpapad, 2012-05-09 23:18:09]

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:21: function
PrintTicketStore() {
It seems that all classes under settings/ have a reference to a PrintTicketStore
object? I am assuming that they all have reference to the same object
(singleton) and that only one exists at a time. If that is the case you could
add the following line after the constructor
cr.addSingletonGetter(PrintTickeStore);

and then get a reference to it wherever necessary by doing
print_preview.PrintTicketStore.getInstance();

This way you would not have to pass it to each constructor (and also document
the @param). Same for DestinationStore.

[Robert Toscano, 2012-05-10 00:12:05]

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:21: function
PrintTicketStore() {
You're right there is a singleton (it's created in PrintPreview). This is the
dependency-injection style of design, where a given class's depedencies are
injected via the javascript contructor. This paradigm makes unit testing way
easier than having to override static method calls. It also makes reasoning
about the code much easier since by looking at a classes constructor, you can
understand what other object it depends on.

[dpapad, 2012-05-10 00:15:32]

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:21: function
PrintTicketStore() {
On 2012/05/10 00:12:05, rltoscano wrote:
> You're right there is a singleton (it's created in PrintPreview). This is the
> dependency-injection style of design, where a given class's depedencies are
> injected via the javascript contructor. This paradigm makes unit testing way
> easier than having to override static method calls. It also makes reasoning
> about the code much easier since by looking at a classes constructor, you can
> understand what other object it depends on.

Sg. it does makes things cleaner. I have been using
goog.testing.PropertyReplacer lately which makes swapping methods (static or
not) very easy.

[Robert Toscano, 2012-05-10 00:23:27]

Albert will upload a patch with the changes soon. I've addressed all but one of
the custom margins UI changes. Namely the one about leaving the margin control
in an error state when the user moves focus away from the control. I really
don't like the idea of erasing a user's input (by replacing with last value) if
the user accidentally clicks outside of the box. Would you play around with the
current behavior and see if my argument makes/does not make sense? If not, I'll
put the old behavior back.

I've also restored that loading screen behavior and gotten rid of the
serialization code for now.

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/destination.js (right):

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/destination.js:201:
state['capabilities'] = this.capabilities_.serialize();
Got rid of the serialization code for now.

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/document_info.js (right):

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/document_info.js:32: * Size of the
pages of the document.
On 2012/05/09 15:48:56, dpapad wrote:
> Document the unit of measurement.

Done.

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/document_info.js:38: * Printable
area of the document.
On 2012/05/09 15:48:56, dpapad wrote:
> Same here.

Done.

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:21: function
PrintTicketStore() {
On 2012/05/10 00:15:32, dpapad wrote:
> On 2012/05/10 00:12:05, rltoscano wrote:
> > You're right there is a singleton (it's created in PrintPreview). This is
the
> > dependency-injection style of design, where a given class's depedencies are
> > injected via the javascript contructor. This paradigm makes unit testing way
> > easier than having to override static method calls. It also makes reasoning
> > about the code much easier since by looking at a classes constructor, you
can
> > understand what other object it depends on.
> 
> Sg. it does makes things cleaner. I have been using
> goog.testing.PropertyReplacer lately which makes swapping methods (static or
> not) very easy.

We have access to the closure library from print preview? If so that would be
awesome. I'd also like to build a round of unit test to take advantage of the
new dependcy-injection-style of the classes--in another CL.

[Robert Toscano, 2012-05-10 00:25:32]

Ah I forgot to mention: I moved some code around regarding the invalid
offsetting of the custom margin controls, but I'm not 100% sure it fixed the
margin offsetting problem you described in your email since I'm not able to
repro on my end. Would you let me know if this is still occurring for you?

[Robert Toscano, 2012-05-10 00:26:15]

Patch is uploaded.

[dpapad, 2012-05-10 02:15:02]

Yes the margin lines are still wrongly positioned (seems like an offset and also
wrong scale).

Found two more issues.
1) When clikcing on a margin line to start dragging the margin line does jumps
to a specific offset from the mouse pointer and then starts moving, it should
start dragging right away without jumping.
2) Related to not correcting invalid values on focus out. Try entering an
invalid value and then start dragging the line (it causes a flood of javascript
errors).

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:21: function
PrintTicketStore() {
On 2012/05/10 00:23:27, rltoscano wrote:
> On 2012/05/10 00:15:32, dpapad wrote:
> > On 2012/05/10 00:12:05, rltoscano wrote:
> > > You're right there is a singleton (it's created in PrintPreview). This is
> the
> > > dependency-injection style of design, where a given class's depedencies
are
> > > injected via the javascript contructor. This paradigm makes unit testing
way
> > > easier than having to override static method calls. It also makes
reasoning
> > > about the code much easier since by looking at a classes constructor, you
> can
> > > understand what other object it depends on.
> > 
> > Sg. it does makes things cleaner. I have been using
> > goog.testing.PropertyReplacer lately which makes swapping methods (static or
> > not) very easy.
> 
> We have access to the closure library from print preview? If so that would be
> awesome. I'd also like to build a round of unit test to take advantage of the
> new dependcy-injection-style of the classes--in another CL.

No, we don't. I am using it outside chromium code, sorry for not clarifying.

http://codereview.chromium.org/10108001/diff/69005/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/native_layer.js (right):

http://codereview.chromium.org/10108001/diff/69005/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:188:
chrome.send('saveLastPrinter', [destination.id, '' /*TODO*/);
Closing bracket missing, causing uncaught js error.

[Robert Toscano, 2012-05-10 03:36:55]

On 2012/05/10 02:15:02, dpapad wrote:
> Yes the margin lines are still wrongly positioned (seems like an offset and
also
> wrong scale).
Talking offline about this issue.

> 
> Found two more issues.
> 1) When clikcing on a margin line to start dragging the margin line does jumps
> to a specific offset from the mouse pointer and then starts moving, it should
> start dragging right away without jumping.
Yea this was a regression, sorry about that. Now fixed.

> 2) Related to not correcting invalid values on focus out. Try entering an
> invalid value and then start dragging the line (it causes a flood of
javascript
> errors).
> 
Done.

[Robert Toscano, 2012-05-10 03:37:18]

Patch on its way for latest fixes.

[Robert Toscano, 2012-05-10 03:37:41]

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

http://codereview.chromium.org/10108001/diff/61004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:21: function
PrintTicketStore() {
No prob.

http://codereview.chromium.org/10108001/diff/69005/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/native_layer.js (right):

http://codereview.chromium.org/10108001/diff/69005/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:188:
chrome.send('saveLastPrinter', [destination.id, '' /*TODO*/);
On 2012/05/10 02:15:03, dpapad wrote:
> Closing bracket missing, causing uncaught js error.

Done.

[dpapad, 2012-05-10 15:49:26]

http://codereview.chromium.org/10108001/diff/69005/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/native_layer.js (right):

http://codereview.chromium.org/10108001/diff/69005/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:188:
chrome.send('saveLastPrinter', [destination.id, '' /*TODO*/);
On 2012/05/10 03:37:41, rltoscano wrote:
> On 2012/05/10 02:15:03, dpapad wrote:
> > Closing bracket missing, causing uncaught js error.
> 
> Done.

Does not seem fixed. Is the latest patch uploaded (as of now latest is patch11)?
Trying to test again to answer questions in your latest email.

[Robert Toscano, 2012-05-10 16:21:43]

Patch 12 just uploaded

[dpapad, 2012-05-10 16:37:51]

On 2012/05/10 16:21:43, rltoscano wrote:
> Patch 12 just uploaded

While testing for answering to your email noticed one more issue, repro steps
1) Choose save as pdf.
2) Click "save"
3) Click cancel on the save as dialog
4) Change color setting.
5) Click save again --> error thrown in the js console, save as dialog does not
reopen.

Also I noticed that when loading the first preview, I saw a colored preview for
a split second and then it automatically changed to black and white.

[Robert Toscano, 2012-05-10 23:55:46]

This latest patch (13) reverts the margins to their old behavior and does a
little performance optimization for CSS animations. PTAL

[dpapad, 2012-05-11 01:55:17]

On 2012/05/10 23:55:46, rltoscano wrote:
> This latest patch (13) reverts the margins to their old behavior and does a
> little performance optimization for CSS animations. PTAL

Thanks for addressing all the comments. A few more issues listed, hopefully easy
to fix.

 -  My default printer is black and white only. When opening google.com in
preview, the page is colored for a split second and then it becomes black and
white. We should not display anything until the plugin is in its final state
(already b&w).
 - Custom margins are not preserved, try the following. Select custom margin
(set some values) then print. Now open a different page in preview, the first
displayed preview should use the last used margin values. Custom is indeed
pre-selected but the values are wrong (0 in my case). Did not find time to test
all the other sticky settings.
 - The first time custom margins are selected (first since the browser was
started), they should be initialized to the Default margins values (e.g. no
regenerating the preview needed), I am seeing different than the Default values
(tried http://en.wikipedia.org/wiki/August_2010_in_sports).
 - Switching from any margin option to "Custom" on large docs, results in the
margin lines being displayed, then the "preview loading" animation. The margin
lines should only be displayed after the new preview is displayed.

[Robert Toscano, 2012-05-15 18:01:35]

Ok issues should be fixed with latest patch.

[dpapad, 2012-05-15 20:09:33]

On 2012/05/15 18:01:35, rltoscano wrote:
> Ok issues should be fixed with latest patch.

I cant repro the color/b&w flickering anymore, thanks for fixing.

Managed to get the following error
Uncaught TypeError: Object #<Object> has no method 'updateValueInPts'
print_preview.js:3032

repro steps.
 - Select custom margins, drag some margin, then print.
 - Reopen print preview, the error appears and preview never shows up.

Also found the following change of behavior.
  - Select custom margins, drag some margin.
  - Change margin selection to None/Minimum/Default
  - Change again to Custom

The last used (even if not used for printing) custom margins value should be
used.
Instead the custom margins appear depending on what is currently selected (so if
None was selected custom margins appear at 0, etc).

The previous behavior showed up the custom margins based on what is currently
selected only when there are no last used custom margins (either printing or
even just previewing).

[Robert Toscano, 2012-05-15 22:20:43]

Latest patch should fix these issues. The custom margins behavior is subtle.
Should be reverted to old behavior now. Sorry about the syntax error, was doing
some minor refactoring, and forgot to look at console output

[dpapad, 2012-05-16 15:46:09]

On 2012/05/15 22:20:43, rltoscano wrote:
> Latest patch should fix these issues. The custom margins behavior is subtle.
> Should be reverted to old behavior now. Sorry about the syntax error, was
doing
> some minor refactoring, and forgot to look at console output

Thanks for fixing. I think we are getting pretty close to landing this. A few
more issues (2nd one more serious).

 - Margin lines dont appear when save to pdf is selected (they only appear when
the user hovers over the preview area, which is confusing.
Repro steps
  -- Open preview
  -- select save to pdf
  -- select custom margins
  -- observer that custom margin lines do not appear.

 - Custom margins should be reset (to default) when switching layouts
(landscape/portrait). I was able to make the preview fail. Repro steps
  -- Open print preview
  -- select custom margins
  -- drag the top margin as close to the bottom margin as possible
  -- switch layouts
  -- see error message

[Robert Toscano, 2012-05-16 16:45:19]

Addressed. Please take another look.

[Robert Toscano, 2012-05-16 18:18:24]

Ok Albert uploaded latest patch that fixes margin textbox issues.

[dpapad, 2012-05-16 20:41:28]

On 2012/05/16 18:18:24, rltoscano wrote:
> Ok Albert uploaded latest patch that fixes margin textbox issues.

LGTM!!

There might be stuff that I have missed, hopefully nothing big, I tried to test
as much as possible, if anything comes up should be addressed in follow up CLs.

[Albert Bodenhamer, 2012-05-16 20:44:03]

Thanks Demetrios.

It looks like we have a couple of issues with browser_tests and
https://chromiumcodereview.appspot.com/10083060 just landed ahead of us so we'll
have to resolve that.  We'll have at least 1 more patch coming. :-/

On 2012/05/16 20:41:28, dpapad wrote:
> On 2012/05/16 18:18:24, rltoscano wrote:
> > Ok Albert uploaded latest patch that fixes margin textbox issues.
> 
> LGTM!!
> 
> There might be stuff that I have missed, hopefully nothing big, I tried to
test
> as much as possible, if anything comes up should be addressed in follow up
CLs.

[kmadhusu, 2012-05-16 20:45:17]

On 2012/05/16 20:41:28, dpapad wrote:
> On 2012/05/16 18:18:24, rltoscano wrote:
> > Ok Albert uploaded latest patch that fixes margin textbox issues.
> 
> LGTM!!
> 
> There might be stuff that I have missed, hopefully nothing big, I tried to
test
> as much as possible, if anything comes up should be addressed in follow up
CLs.

rltoscano(repeating our conversation for reference): I just committed the chrome
print preview ui changes to support auto fit to page. Please include those
changes in this CL. I will be very happy to review all your changes that are
related to auto fit to page. Sorry for the trouble it might have caused you.

[kmadhusu, 2012-05-18 02:50:21]

rltoscano: I found some regressions in the refactored code. As I said in our
chat conversation, I have specified the test case repro steps, expected and
observed behavior for reference. 

Case 1: 
0. Preview http://www.corp.google.com/~kmadhusu/no_crawl/test_html/css_4_4.html
1. Select a physical printer.
2. Select custom margins.
3. Change the top margin to 6.50"
4. Select Save as PDF

Expected behavior: Margin option should be reset to Default and preview data
should be displayed.
Observed behavior: Loading animation screen is displayed for ever.

Case 2:
1. Preview http://www.corp.google.com/~kmadhusu/no_crawl/test_html/css_4_4.html.
2. In Page range text box, enter the page number "2"
3. Select "All" radio button.
4. Notice the text of page range textbox.

Expected behavior: We should retain the selected page range and we should not
erase or reset it to empty string.
Observed behavior: Page range is reset to default which is an empty string.

Case 3:
1. Preview http://www.corp.google.com/~kmadhusu/no_crawl/test_html/css_4_4.html.
2. Notice that the orientation option is displayed to the user.

Expected behavior: When the source html has print media css, we do not display
the orientation option because the user has specified the page size therefore we
infer the orientation information from that.
Observed behavior: Orientation option is displayed to the user and if the user
changes the orientation the preview data is not updated.

Case 4:
1. Preview
http://www.corp.google.com/~kmadhusu/no_crawl/test_html/zero_margins.html
2. Notice that the header footer option is displayed to the user.

Expected behavior: When the source html specifies zero margins in print media
css, we know that there is no room for header and footer option and hence we
hide that option by default. When they change to minimum or any non-zero custom
margins, we display the option to the user.

Observed behavior: Header and footer option is displayed and when the user
changes header and footer is not added/removed from the preview data.

You also need to patch other fixes for the fit to page regressions. As I said
earlier, I will review your code tomorrow.

[Robert Toscano, 2012-05-18 18:45:01]

On 2012/05/18 02:50:21, kmadhusu wrote:
> rltoscano: I found some regressions in the refactored code. As I said in our
> chat conversation, I have specified the test case repro steps, expected and
> observed behavior for reference. 
> 
> Case 1: 
> 0. Preview
http://www.corp.google.com/%7Ekmadhusu/no_crawl/test_html/css_4_4.html
> 1. Select a physical printer.
> 2. Select custom margins.
> 3. Change the top margin to 6.50"
> 4. Select Save as PDF
> 
> Expected behavior: Margin option should be reset to Default and preview data
> should be displayed.
> Observed behavior: Loading animation screen is displayed for ever.
> 
> Case 2:
> 1. Preview
http://www.corp.google.com/%7Ekmadhusu/no_crawl/test_html/css_4_4.html.
> 2. In Page range text box, enter the page number "2"
> 3. Select "All" radio button.
> 4. Notice the text of page range textbox.
> 
> Expected behavior: We should retain the selected page range and we should not
> erase or reset it to empty string.
> Observed behavior: Page range is reset to default which is an empty string.
> 
> Case 3:
> 1. Preview
http://www.corp.google.com/%7Ekmadhusu/no_crawl/test_html/css_4_4.html.
> 2. Notice that the orientation option is displayed to the user.
> 
> Expected behavior: When the source html has print media css, we do not display
> the orientation option because the user has specified the page size therefore
we
> infer the orientation information from that.
> Observed behavior: Orientation option is displayed to the user and if the user
> changes the orientation the preview data is not updated.
> 
> Case 4:
> 1. Preview
> http://www.corp.google.com/%7Ekmadhusu/no_crawl/test_html/zero_margins.html
> 2. Notice that the header footer option is displayed to the user.
> 
> Expected behavior: When the source html specifies zero margins in print media
> css, we know that there is no room for header and footer option and hence we
> hide that option by default. When they change to minimum or any non-zero
custom
> margins, we display the option to the user.
> 
> Observed behavior: Header and footer option is displayed and when the user
> changes header and footer is not added/removed from the preview data.
> 
> You also need to patch other fixes for the fit to page regressions. As I said
> earlier, I will review your code tomorrow.

OK latest patches should fix this. PTAL

[kmadhusu, 2012-05-18 20:13:46]

rltoscano: I reviewed the code and most of my comments are just style nits. I
will test the latest patch and let you know the results.

Thanks.

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/print_preview.html (right):

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
chrome/browser/resources/print_preview/print_preview.html:55:
i18n-content="systemDialogOption"></button>
Right before committing, please do a accessibility scan on  these controls just
to make sure we are not introducing any accessibility issues.

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
chrome/browser/resources/print_preview/print_preview.js:5: // TODO Might be a
problem where might be in fetching destinations state, then
TODO(rltoscano):

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
chrome/browser/resources/print_preview/print_preview.js:9: // TODO Move data/*
into print_preview.data namespace
TODO(rltoscano): Comment...

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
chrome/browser/resources/print_preview/print_preview.js:11: // TODO Handle case
where cloud print is initial destination, but cloud print is
same here.

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
chrome/browser/resources/print_preview/print_preview.js:553: * store. Also adds
a print-to-pdf printer.
nit: Is it print-to-pdf or save-to-pdf?? Please follow the same term throughout
the code.

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
chrome/browser/resources/print_preview/print_preview.js:812: throw Error(
I see a lot of "throw Error()" statements in this file. I assume these messages
are not displayed to the user and it is only for logging purpose. Can you just
call logError(msg) function to do that?

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/color_settings.html
(right):

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/color_settings.html:12: <input
class="color-settings-bw-option" type="radio" name="color" checked/>
nit: 80 column limit exceeded.

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/copies_settings.html
(right):

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/copies_settings.html:13: <input
class="copies-settings-collate-checkbox" type="checkbox" checked/>
nit: 80 column limit exceeded.

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/destination_settings.js
(right):

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/destination_settings.js:8: //
TODO This class needs a throbber while loading the destination.
nit: TODO()

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/other_options_settings.js
(right):

http://codereview.chromium.org/10108001/diff/72101/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/other_options_settings.js:156:
this.fitToPageEl_, this.printTicketStore_.hasFitToPageCapability());
nit: First argument can fit in the first line. Please refer to the function
arguments section in
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Nested_....

http://codereview.chromium.org/10108001/diff/72101/chrome/test/data/webui/pri...
File chrome/test/data/webui/print_preview.js (right):

http://codereview.chromium.org/10108001/diff/72101/chrome/test/data/webui/pri...
chrome/test/data/webui/print_preview.js:119: expectEquals('BarName',
printerList.options[1].text, 'barIndex=1');
Is there any specific reason to use numbers rather than variables here?
Sometimes magic numbers are confusing. I thought fooIndex and barIndex added
more semantics to the context.

http://codereview.chromium.org/10108001/diff/72101/chrome/test/data/webui/pri...
chrome/test/data/webui/print_preview.js:393: 'PrintPreviewWebUITest',
nit: This argument can go to line 392.

http://codereview.chromium.org/10108001/diff/60374/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

http://codereview.chromium.org/10108001/diff/60374/chrome/browser/resources/p...
chrome/browser/resources/print_preview/cloud_print_interface.js:97: var params =
{'printerid': printerId};
nit: single quote is not required.

http://codereview.chromium.org/10108001/diff/60374/chrome/browser/resources/p...
chrome/browser/resources/print_preview/cloud_print_interface.js:110: throw
Error(
throw Error ()...  => logError(msg)?

[kmadhusu, 2012-05-18 20:58:10]

rltoscano: I found some more regressions and the test cases are as follows:


Case 1 (Source is HTML, Physical printer selected and Header footer option is
checked by default):
1. Preview http://www.corp.google.com/~kmadhusu/no_crawl/test_html/css_4_4.html.

Expected behavior: Header and footer should be displayed in the preview data.
Observed behavior: Header footer option is checked by default, but there is no
header and footer in the preview data.

Case 2:
1. Preview http://www.corp.google.com/~kmadhusu/no_crawl/test_html/css_4_4.html.
2. Select "Save As PDF" option.
3. Select "None" margins.
4. Select a physical printer.
5. Notice the margin settings.

Expected behavior: Any non-custom margin setting should be preserved between
printer changes. 
Observed behavior: Margin is reset to Default.

Case 3:
1. Preview http://en.wikipedia.org/wiki/August_2010_in_sports
2. As soon you see the preview tab, click "Print using system dialog" link.
3. Notice the cancel button state.

Expected behavior: Cancel button is disabled.
Observed behavior: Cancel button is enabled.

Case 4:
1. Preview http://en.wikipedia.org/wiki/August_2010_in_sports
2. While the preview is loading, select "Save as PDF" and click "Save" button.
3. Observe the controls state after clicking the save button and before the save
dialog is displayed.

Expected behavior: Cancel button is disabled and all other controls are enabled.
Observed behavior: All controls are disabled except cancel button.

Case 5:
1. Preview http://en.wikipedia.org/wiki/August_2010_in_sports
2. After the preview is loaded, Press Ctrl + Shift + P
3. Observe the "Print using system dialog" link

Expected behavior: Throbber should be displayed next to the link
Observed behavior: Throbber is not displayed while invoking the system dialog
using shortcut keys.

Case 6:
1. Preview http://en.wikipedia.org/wiki/August_2010_in_sports
2. After the preview loaded, Select "landscape" and while the data is loading,
change the printer.
3. After the new preview data is loaded, notice the state of header and footer
option.

Expected behavior: Header footer option should be visible and the state should
be preserved between printer changes.
Observed behavior: Header footer option is hidden.

[kmadhusu, 2012-05-19 01:44:02]

rtloscano: Found a regression with your latest test patch. 

Repro steps:
1. Preview http://www.corp.google.com/~kmadhusu/no_crawl/test_html/css_4_4.html
2. Select "None" margins and wait for the preview data to load.
3. Select "Minimium" margins.
4. Make sure header footer option is displayed and checked.

Expected behavior: Header and footer should be displayed in the preview data.
Observed behavior: Header and footer is not displayed in the preview data.

Please make sure you test your changes on Win and Mac (especially the shortcut
key stuff) before committing your code. Once you upload your fix patch, I will
take a quick look and stamp it for commit process. Thanks.

[kmadhusu, 2012-05-22 19:22:52]

rltoscano: Found 3 more regressions. Thanks for fixing the previous regressions.


Case 1:
1. Preview http://www.corp.google.com/~kmadhusu/no_crawl/test_html/css_4_4.html.
2. Select "Save As PDF"
3. Make sure you see 2 pages in the preview document.
4. Enter "3" in page range text box. Make sure you see an invalid error message.
5. Select a physical printer.

Expected: We should request for a new preview and re-validate the page range
field with new page count.
Observed: When there is an invalid page range error, we are not requesting for
new preview data.

Case 2:
1. Preview http://www.corp.google.com/~kmadhusu/no_crawl/test_html/css_4_4.html.
2. Select "Save As PDF"
3. Uncheck "Header and footer" option.
3. Click "Save" button and save the preview data.
4. Preview http://www.corp.google.com/~kmadhusu/no_crawl/test_html/css_4_4.html
again.
5. Notice the state of header and footer checkbox.

Expected: Header and footer should be sticky for the given browser session.
Based on previous print job setting, this should be unchecked.
Observed: Initial print settings are not set correctly. Header footer option is
selected by default.

Case 3:
1. Preview http://www.corp.google.com/~kmadhusu/no_crawl/test_html/css_4_4.html.
2. Select a physical printer.
3. Select Custom Margins.
4. Set the right margin to 5.75"
5. Click Print button.
6. Preview http://www.corp.google.com/~kmadhusu/no_crawl/test_html/css_4_4.html
again.
7. Notice the Margins settings.

Expected: Margins settings are sticky for the given browser session. It should
be custom with the previous print job custom margin values.
Observed: Initial print settings are not set correctly. Default margins are
selected.

Thanks.

[kmadhusu, 2012-05-22 23:17:25]

LGTM.

Thanks for refactoring print preview code.

http://codereview.chromium.org/10108001/diff/81009/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/settings/color_settings.html
(right):

http://codereview.chromium.org/10108001/diff/81009/chrome/browser/resources/p...
chrome/browser/resources/print_preview/settings/color_settings.html:13:
type="radio"
style nit: type="radio" and name="color" can fit in the previous line.

[commit-bot: I haz the power, 2012-05-23 19:42:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abodenha@chromium.org/10108001/90009

[commit-bot: I haz the power, 2012-05-23 23:01:17]

Failed to apply patch for
chrome/browser/resources/print_preview/print_preview.css:
While running patch -p1 --forward --force;
patching file chrome/browser/resources/print_preview/print_preview.css
Hunk #1 FAILED at 7.
Hunk #2 FAILED at 30.
Hunk #3 succeeded at 87 (offset 14 lines).
Hunk #4 succeeded at 117 (offset 14 lines).
Hunk #5 FAILED at 228.
Hunk #6 succeeded at 396 (offset 20 lines).
Hunk #7 succeeded at 424 (offset 20 lines).
3 out of 7 hunks FAILED -- saving rejects to file
chrome/browser/resources/print_preview/print_preview.css.rej

[commit-bot: I haz the power, 2012-05-24 02:03:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abodenha@chromium.org/10108001/109004

[commit-bot: I haz the power, 2012-05-24 04:46:27]

Change committed as 138737