Page range comparisons should use document size.

Page range comparisons should use document size.

Preview can update page count, so PageNumberSet may represent page ranges incorrectly. Now PageNumberSet is used as temporarily objects which never stored.

PreviewGenerator stores PageRange which can be compared with other independently of current document size.

Removed code duplication in page ranges parsing code and parse directly to page ranges.

BUG=173822
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182005

[Toscano, 2013-02-11 21:25:12]

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
chrome/browser/resources/print_preview/data/print_ticket_store.js:593:
getPageRange: function() {
This is not consistent. None of the ticket_items are exposed from the
ticket_store. Plz revert this part.

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
File chrome/browser/resources/print_preview/data/ticket_items/page_range.js
(right):

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
chrome/browser/resources/print_preview/data/ticket_items/page_range.js:41:
this.documentInfo_.pageCount);
if this.documentInfo_.pageCount is 0, then this condition is always false.

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
chrome/browser/resources/print_preview/data/ticket_items/page_range.js:95: if
(other == null)
Plz always use braces.

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
File chrome/browser/resources/print_preview/print_preview_utils.js (right):

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
chrome/browser/resources/print_preview/print_preview_utils.js:76: * Returns a
list of ranges in |pagesRangeText|. The ranges are
|pagesRangeText| -> |pageRangeText|

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
chrome/browser/resources/print_preview/print_preview_utils.js:96: return null;
This is true for initial case right? Where DocumentInfo.pageCount == 0.

[Vitaly Buka (NO REVIEWS), 2013-02-12 00:46:44]

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
chrome/browser/resources/print_preview/data/print_ticket_store.js:593:
getPageRange: function() {
On 2013/02/11 21:25:13, Toscano wrote:
> This is not consistent. None of the ticket_items are exposed from the
> ticket_store. Plz revert this part.

Done.

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
File chrome/browser/resources/print_preview/data/ticket_items/page_range.js
(right):

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
chrome/browser/resources/print_preview/data/ticket_items/page_range.js:41:
this.documentInfo_.pageCount);
On 2013/02/11 21:25:13, Toscano wrote:
> if this.documentInfo_.pageCount is 0, then this condition is always false.

Done.

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
chrome/browser/resources/print_preview/data/ticket_items/page_range.js:95: if
(other == null)
On 2013/02/11 21:25:13, Toscano wrote:
> Plz always use braces.

Done.

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
File chrome/browser/resources/print_preview/print_preview_utils.js (right):

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
chrome/browser/resources/print_preview/print_preview_utils.js:76: * Returns a
list of ranges in |pagesRangeText|. The ranges are
On 2013/02/11 21:25:13, Toscano wrote:
> |pagesRangeText| -> |pageRangeText|

Done.

https://codereview.chromium.org/12209086/diff/12002/chrome/browser/resources/...
chrome/browser/resources/print_preview/print_preview_utils.js:96: return null;
On 2013/02/11 21:25:13, Toscano wrote:
> This is true for initial case right? Where DocumentInfo.pageCount == 0.

Done.

[Toscano, 2013-02-12 01:39:05]

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/page_number_set.js (right):

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/page_number_set.js:14: function
PageNumberSet(pageNumberList) {
Let's add a new data structure to hold a PageIntervalSet (in addition to this
class). Add a parse method that takes in a string. This new class would have a
backing store of Array.<{from: number, to:number}>. Also add an equals method.
Also it should have a method that can generate a PageNumberSet for use by the
PreviewGenerator.

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:596: * @return
{!Array.<object.<{from: number, to: number}>>} Page ranges
Let's have this return a print_preview.data.PageIntervalSet.

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:596: * @return
{!Array.<object.<{from: number, to: number}>>} Page ranges
You don't need "object" in the data type. This is sufficient:

{!Array.<{from: number, to:number}>}

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:608:
getDocumentPageRanges: function() {
What is this one used for?

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/ticket_items/page_range.js
(right):

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/ticket_items/page_range.js:48:
getPageNumberSet: function() {
Let's get rid of this method and expose only getPageRanges (which should get
renamed to getPageIntervalSet).

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/ticket_items/page_range.js:84:
getDocumentPageRanges: function() {
We shouldn't need this one. A PageNumberSet produced from a PageIntervalSet
should be all we need.

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/native_layer.js (right):

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:144: 'pageRange':
printTicketStore.getDocumentPageRanges(),
Can we just pass in printTicketStore.getPageRanges() (which should be renamed to
getPageIntervalSet)? What happens if the backend gets a PageIntervalSet that is
not constrained to the document page count?

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:215: 'pageCount':
printTicketStore.getPageNumberSet().size,
Why are we submitting the pageCount now?

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/preview_generator.js (right):

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
chrome/browser/resources/print_preview/preview_generator.js:81: * @type
{!Array.<object.<{from: number, to: number}>>}
Plz use type PageIntervalSet.

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
chrome/browser/resources/print_preview/preview_generator.js:263:
!areRangesEqual(ticketStore.getPageRanges(), this.pageRanges_) ||
Here you would use PageIntervalSet's equals method.

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
chrome/browser/resources/print_preview/preview_generator.js:327:
this.pageRanges_ = this.printTicketStore_.getPageRanges();
In the event that page count changes, this shouldn't affect the value of a
PageIntervalSet right? So we don't need to update this. We can get rid of this
handler.

[Vitaly Buka (NO REVIEWS), 2013-02-12 01:45:30]

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/print_ticket_store.js (right):

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/print_ticket_store.js:608:
getDocumentPageRanges: function() {
Now native layer convert this into array of pages, before calculating page
number.
So if we  have 1-100000000, it's just waist of memory.

On 2013/02/12 01:39:05, Toscano wrote:
> What is this one used for?

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/data/ticket_items/page_range.js
(right):

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
chrome/browser/resources/print_preview/data/ticket_items/page_range.js:84:
getDocumentPageRanges: function() {
we still pass intervals to native code


On 2013/02/12 01:39:05, Toscano wrote:
> We shouldn't need this one. A PageNumberSet produced from a PageIntervalSet
> should be all we need.

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/native_layer.js (right):

https://codereview.chromium.org/12209086/diff/7015/chrome/browser/resources/p...
chrome/browser/resources/print_preview/native_layer.js:144: 'pageRange':
printTicketStore.getDocumentPageRanges(),
Huge arrays of pages. 


On 2013/02/12 01:39:05, Toscano wrote:
> Can we just pass in printTicketStore.getPageRanges() (which should be renamed
to
> getPageIntervalSet)? What happens if the backend gets a PageIntervalSet that
is
> not constrained to the document page count?

[Toscano, 2013-02-12 01:49:31]

LGTM

Agreed to address remaining comments in next CL.

[commit-bot: I haz the power, 2013-02-12 01:55:32]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[gene, 2013-02-12 02:19:52]

lgtm

[commit-bot: I haz the power, 2013-02-12 07:36:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/12209086/18

[commit-bot: I haz the power, 2013-02-12 08:41:26]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-12 08:58:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/12209086/18

[commit-bot: I haz the power, 2013-02-12 10:36:03]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-12 10:51:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/12209086/18

[commit-bot: I haz the power, 2013-02-12 13:31:05]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-12 19:32:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/12209086/18

[commit-bot: I haz the power, 2013-02-12 21:02:48]

Change committed as 182005