implement validations of pagination URLs

implement validations of pagination URLs

This is the 2nd part of the new pagination algorithm implementation:
various validations of potential pagination URLs.

* PageParameterDetector:
  - declares PagePattern interface that page pattern handlers must implement
  - calls PagePattern methods to validate patterns and check for paging URL
  - moves LinkInfo out to its own class PageLinkInfo, to avoid cross-access by
    PageParamInfo

* QueryParamPagePattern
  - handles page parameter in the query of potential pagination URLs
  - implements PageParameterDetector.PagePattern interface
  - add tests.

* PathComponentPagePattern
  - handles page parameter in the path of potential pagination URLs
  - implements PageParameterDetector.PagePattern interface
  - add tests.

* PageParamInfo
  - if possible, generates linear formula between text and page param of each
    pagination anchor
  - checks if page numbers of potential pagination URLs are adjacent and
    consecutive, and if they form a page sequence.
  - add tests.

BUG=464143
R=cjhopman@chromium.org

Committed: 10bc28cd4169e3b9532b5c253f620893c7650a67

[kuan, 2015-03-23 16:58:25]

kuan@chromium.org changed reviewers:
+ cjhopman@chromium.org

[cjhopman, 2015-03-27 00:16:12]

Higher level: I think this would be easier to follow if we had a PagePattern
class and a lot of the stuff here were moved to functions of that class

examples of things that could be functions of such a class:

isPagePatternValid()
isPagingURL()
isDynamic()
isPathPattern()
isQueryPattern()
whatever other sort of calculations we do on just the page pattern alone or are
done in sort of isolation from other calculations (like the bit around line 237)

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
File java/org/chromium/distiller/PageParameterDetector.java (right):

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:218: if
(urlPathComponentsLen > patternPathComponentsLen) return false;
why ">" and not "!="?

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:237: // Except for the
component containing the page param, the other components of doc URL must
Can this be extracted to a separate function.

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:241: for (int i = 0, j =
0; i < urlPathComponentsLen && j < patternPathComponentsLen; i++, j++) {
I'm not really sure I follow the logic here (and afaict, this will reject the
example in the comment).

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:255: if (paramIndex >= 2
&&
Extract this to another function

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:258: //
http://www.politico.com/story/2014/07/barack-obama-immigration-legal-question...,
why do we require that it be ordered yyyy/mm/dd for us to reject it? Is that
just particularly common?

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:402: }  //
getLongestCommonPrefixLength
Let's remove all these comments marking what function is ending.

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:508: *   pageParamValue =
coefficient * pageNum + delta (delta == -coefficient or delta == 0).
Do we really need this complicated linear formula? It feels like the coefficient
is useful for pages like search results or forums or something but not for
things that we actually expect to work on.

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:554: * - if only two
pages, they must be siblings
what's a sibling?

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:560: private static
boolean isPageNumberSeq(List<PageParamInfo.PageInfo> ascendingNumbers) {
Try to avoid abbreviations in function names: s/Seq/Sequence

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:597: private static
RegExp sSlashExtRegExp = null;  // Match either '/' or ".htm(l)".
This name needs to be more descriptive.

[cjhopman, 2015-03-27 00:18:21]

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
File java/org/chromium/distiller/PageParameterDetector.java (right):

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:241: for (int i = 0, j =
0; i < urlPathComponentsLen && j < patternPathComponentsLen; i++, j++) {
On 2015/03/27 00:16:12, cjhopman wrote:
> I'm not really sure I follow the logic here (and afaict, this will reject the
> example in the comment).

It won't reject that example actually. Still, how does it handle:

/thread/12/foo /thread/12/page/[*!]/foo
and
/thread/12/foo /thread/12/[*!]/foo

and does what it does in those two cases make sense?

[kuan, 2015-03-31 17:17:51]

i've addressed all comments in patch set 3 (set 4 is just nits).  ptal.  thx.

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
File java/org/chromium/distiller/PageParameterDetector.java (right):

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:218: if
(urlPathComponentsLen > patternPathComponentsLen) return false;
On 2015/03/27 00:16:12, cjhopman wrote:
> why ">" and not "!="?

because pattern can hv more path components than doc url e.g. the example in
PagePattern.hasSamePathComponentsAs().

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:237: // Except for the
component containing the page param, the other components of doc URL must
On 2015/03/27 00:16:12, cjhopman wrote:
> Can this be extracted to a separate function.

Done.

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:241: for (int i = 0, j =
0; i < urlPathComponentsLen && j < patternPathComponentsLen; i++, j++) {
On 2015/03/27 00:18:21, cjhopman wrote:
> On 2015/03/27 00:16:12, cjhopman wrote:
> > I'm not really sure I follow the logic here (and afaict, this will reject
the
> > example in the comment).
> 
> It won't reject that example actually. Still, how does it handle:
> 
> /thread/12/foo /thread/12/page/[*!]/foo

this would be invalid - pattern has extra "page" path component before page
param path component, which won't match with url's "foo" path component.

> and
> /thread/12/foo /thread/12/[*!]/foo

this would be valid - pattern's "foo" path component would match with url's
"foo" path component.
  
> and does what it does in those two cases make sense?

i don't know if this is too strict, but i worry about false-positives.  in any
case, i added ur 2 examples to the tests.

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:255: if (paramIndex >= 2
&&
On 2015/03/27 00:16:12, cjhopman wrote:
> Extract this to another function

Done.

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:258: //
http://www.politico.com/story/2014/07/barack-obama-immigration-legal-question...,
On 2015/03/27 00:16:12, cjhopman wrote:
> why do we require that it be ordered yyyy/mm/dd for us to reject it? Is that
> just particularly common?

i would think so.  how else do we detect calendar links?

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:402: }  //
getLongestCommonPrefixLength
On 2015/03/27 00:16:12, cjhopman wrote:
> Let's remove all these comments marking what function is ending.

Done.

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:508: *   pageParamValue =
coefficient * pageNum + delta (delta == -coefficient or delta == 0).
On 2015/03/27 00:16:11, cjhopman wrote:
> Do we really need this complicated linear formula? It feels like the
coefficient
> is useful for pages like search results or forums or something but not for
> things that we actually expect to work on.

it's true we don't really care about the actual values of coefficient and delta,
but without them, how do we know if the page parameter is an actual one that
maps to the page number?  won't this help weed out false-positives with
arbitrary numbers in the page parameter?

i could remove it if u're sure it's unnecessary.

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:554: * - if only two
pages, they must be siblings
On 2015/03/27 00:16:12, cjhopman wrote:
> what's a sibling?

Done.

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:560: private static
boolean isPageNumberSeq(List<PageParamInfo.PageInfo> ascendingNumbers) {
On 2015/03/27 00:16:12, cjhopman wrote:
> Try to avoid abbreviations in function names: s/Seq/Sequence

Done.

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:597: private static
RegExp sSlashExtRegExp = null;  // Match either '/' or ".htm(l)".
On 2015/03/27 00:16:12, cjhopman wrote:
> This name needs to be more descriptive.

Done.

[cjhopman, 2015-04-07 00:45:49]

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
File java/org/chromium/distiller/PageParameterDetector.java (right):

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:241: for (int i = 0, j =
0; i < urlPathComponentsLen && j < patternPathComponentsLen; i++, j++) {
On 2015/03/31 17:17:50, kuan wrote:
> On 2015/03/27 00:18:21, cjhopman wrote:
> > On 2015/03/27 00:16:12, cjhopman wrote:
> > > I'm not really sure I follow the logic here (and afaict, this will reject
> the
> > > example in the comment).
> > 
> > It won't reject that example actually. Still, how does it handle:
> > 
> > /thread/12/foo /thread/12/page/[*!]/foo
> 
> this would be invalid - pattern has extra "page" path component before page
> param path component, which won't match with url's "foo" path component.
> 
> > and
> > /thread/12/foo /thread/12/[*!]/foo
> 
> this would be valid - pattern's "foo" path component would match with url's
> "foo" path component.
>   
> > and does what it does in those two cases make sense?
> 
> i don't know if this is too strict, but i worry about false-positives.  in any
> case, i added ur 2 examples to the tests.

I guess that the behavior doesn't seem to match the comment. The comment says
"the other components of doc URL must be part of pattern's path.  But pattern
may have more components", which seems to imply that the following should be
valid:

/thread/12/foo /thread/12/page/[*!]/foo

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:508: *   pageParamValue =
coefficient * pageNum + delta (delta == -coefficient or delta == 0).
On 2015/03/31 17:17:50, kuan wrote:
> On 2015/03/27 00:16:11, cjhopman wrote:
> > Do we really need this complicated linear formula? It feels like the
> coefficient
> > is useful for pages like search results or forums or something but not for
> > things that we actually expect to work on.
> 
> it's true we don't really care about the actual values of coefficient and
delta,
> but without them, how do we know if the page parameter is an actual one that
> maps to the page number?  won't this help weed out false-positives with
> arbitrary numbers in the page parameter?
> 
> i could remove it if u're sure it's unnecessary.

i just want you to be sure if it's necessary or unnecessary. I think that you
want something like it, but it might not need to support non-1 coefficient.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/PagePattern.java (right):

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:18: public class PagePattern {
After commenting a bit below, I wonder if it would make sense to split this into
multiple classes, one for each type of page pattern. A lot of the behavior is
quite different for param vs component. I guess there's really four types:

param
dynamic param
component
partial component

I don't know if it makes sense to do four classes, but maybe two for
param/component, idk.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:20: private final int
mPageParamPos;
Some of this code below was hard for me to follow. I think that renaming some of
these variables would help:

s/mPageParamPos/mPlaceholderStart
s/mComponentPos/mPlaceholderComponentStart or mPlaceholderSegmentStart or
something
s/mQueryPos/mQueryStart or possibly even better, for things that care about
looking at just the query or the path, they should just use the query or the
path and not the whole url

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:29: static PagePattern
create(String url) {
How about having a create method for page pattern in query and in path
component? I.e. have this class be in charge of placing the
PAGE_PARAM_PLACEHOLDER into the URL?

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:42: *   - pattern and URL must have
same number of path components.
Personally, I'd have these function-level comments be higher level (otherwise
they are very likely to become out of date and not match the code as it
changes), but it's up to you.

Like:

- for path page parameter, pattern and url must be sufficiently similar and the
pattern's components cannot be calendar digits

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:52: boolean isValidFor(ParsedUrl
docUrl) {
Would it make sense to check validity when the page pattern is created?

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:72: return
(getLongestCommonSuffixLength(urlComponent, patternComponent, commonPrefixLen) +
nit: could you put the common suffix in a local variable like common prefix (or
put both calculations in the return expression).

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:78: if (isCalendarPage()) return
false;
Why do we check this here? it doesn't really depend on the docUrl. I think we
could check this during construction of the page pattern.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:110: // Both url and patterm must
have the same suffix, if available.
nit: s/patterm/pattern

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:122: 
Could we split these three cases into their own functions?

if (isDynamicParam) {
  return isDynamicParamPagingUrl(url);
} else if (isPartialPathComponent()) {
  return isPartialPathComponentPagingUrl(url);
}
return isPathComponentPagingUrl(url);

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:150: if (suffixPos < mComponentPos
|| !url.regionMatches(0, mUrlStr, 0, mComponentPos)) {
how about:

url.endsWith(mRequiredSuffix)

and calculate the required suffix on construction.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:158: // ".html" or ".html", it is
fine.
s/.html/.htm for one of the two

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:159: String diffPart =
url.substring(mComponentPos, suffixPos).toLowerCase();
I'm not really sure what diffpart is here. What is it for the last set of
examples in the comment above? Maybe it's just unclear what mComponentPos and
suffixPos are.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:178: //   -
www.foo.com/a/2/abc.html
The second part of this comment confused me because it applies to the code after
this if block. (right?)

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:186: if (urlLen < mComponentPos +
suffixLen ||
url.startsWith(mRequiredPrefix)?

I guess if you do this and the one above, you need to make it clear that only
certain types of page patterns have required prefix/suffix.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:199: if (firstDiffPos + 1 ==
mPageParamPos &&
Hm, if I read this correctly, the following returns false:

pattern: www.foo.com/a/abc[*!].html
url:     www.foo.com/a/abc.html

That seems odd.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:210: // Handle case where page
param is the entire path component.
This comment looks like it applies to just the next 8 lines or so, but I think
it actually applies to the whole rest of the function. Maybe just moving the
examples from above to down here would help.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:211: int prevComponentPos =
mUrlStr.lastIndexOf('/', mComponentPos - 1);
Should this only be looking at the path? If not, will it ever have
prevComponentPos == -1? (wouldn't the full url always have a leading scheme?)

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:213: // The url doesn't have page
number param and previous path component, like
Stated like this, it seems the comment applies to the current state... but
really it applies to the state inside the next if block. Either move it inside
the next if, or change it to something like:

Check if the url doesn't ...

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:271: private void
determineParamIndex() {
We should just call this in the constructor so that mParamIndex can always have
been initialized.

[kuan, 2015-04-10 22:41:28]

i've addressed all comments in patch set 5, the later sets r just nits in
comments and copyright year (don't know why i had 2016 before).  ptal.  thx.

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
File java/org/chromium/distiller/PageParameterDetector.java (right):

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:241: for (int i = 0, j =
0; i < urlPathComponentsLen && j < patternPathComponentsLen; i++, j++) {
On 2015/04/07 00:45:48, cjhopman wrote:
> On 2015/03/31 17:17:50, kuan wrote:
> > On 2015/03/27 00:18:21, cjhopman wrote:
> > > On 2015/03/27 00:16:12, cjhopman wrote:
> > > > I'm not really sure I follow the logic here (and afaict, this will
reject
> > the
> > > > example in the comment).
> > > 
> > > It won't reject that example actually. Still, how does it handle:
> > > 
> > > /thread/12/foo /thread/12/page/[*!]/foo
> > 
> > this would be invalid - pattern has extra "page" path component before page
> > param path component, which won't match with url's "foo" path component.
> > 
> > > and
> > > /thread/12/foo /thread/12/[*!]/foo
> > 
> > this would be valid - pattern's "foo" path component would match with url's
> > "foo" path component.
> >   
> > > and does what it does in those two cases make sense?
> > 
> > i don't know if this is too strict, but i worry about false-positives.  in
any
> > case, i added ur 2 examples to the tests.
> 
> I guess that the behavior doesn't seem to match the comment. The comment says
> "the other components of doc URL must be part of pattern's path.  But pattern
> may have more components", which seems to imply that the following should be
> valid:
> 
> /thread/12/foo /thread/12/page/[*!]/foo

i've added ur examples, with explanations, to the comments to illustrate in more
details.

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:508: *   pageParamValue =
coefficient * pageNum + delta (delta == -coefficient or delta == 0).
On 2015/04/07 00:45:48, cjhopman wrote:
> On 2015/03/31 17:17:50, kuan wrote:
> > On 2015/03/27 00:16:11, cjhopman wrote:
> > > Do we really need this complicated linear formula? It feels like the
> > coefficient
> > > is useful for pages like search results or forums or something but not for
> > > things that we actually expect to work on.
> > 
> > it's true we don't really care about the actual values of coefficient and
> delta,
> > but without them, how do we know if the page parameter is an actual one that
> > maps to the page number?  won't this help weed out false-positives with
> > arbitrary numbers in the page parameter?
> > 
> > i could remove it if u're sure it's unnecessary.
> 
> i just want you to be sure if it's necessary or unnecessary. I think that you
> want something like it, but it might not need to support non-1 coefficient.

i'm wary of removing it now, including the non-1 coefficient support, without
enough knowledge on how websites in the wild organize their pages.  i do think
we need to verify the linear formula exists.

if we only support 1 coefficient, the following pagination URLs (in the tests)
won't be detected, cos coefficient is 10:
- page 1: http://www.google.com/test
- page 2: http://www.google.com/test?page=10
- page 3: http://www.google.com/test?page=20.

i'd prefer to keep this as is, for now anyway, so i've added a TODO to reassess
its necessity.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/PagePattern.java (right):

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:18: public class PagePattern {
On 2015/04/07 00:45:49, cjhopman wrote:
> After commenting a bit below, I wonder if it would make sense to split this
into
> multiple classes, one for each type of page pattern. A lot of the behavior is
> quite different for param vs component. I guess there's really four types:
> 
> param
> dynamic param
> component
> partial component
> 
> I don't know if it makes sense to do four classes, but maybe two for
> param/component, idk.

Done. i did two - QueryParamPagePattern and PathComponentPagePattern.
fyi, during the split, i realized, and verified, dynamic param is not necessary,
because the page pattern is only valid if the entire query value is a valid
number.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:20: private final int
mPageParamPos;
On 2015/04/07 00:45:49, cjhopman wrote:
> Some of this code below was hard for me to follow. I think that renaming some
of
> these variables would help:
> 
> s/mPageParamPos/mPlaceholderStart
> s/mComponentPos/mPlaceholderComponentStart or mPlaceholderSegmentStart or
> something
> s/mQueryPos/mQueryStart or possibly even better, for things that care about
> looking at just the query or the path, they should just use the query or the
> path and not the whole url

Done.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:29: static PagePattern
create(String url) {
On 2015/04/07 00:45:48, cjhopman wrote:
> How about having a create method for page pattern in query and in path
> component? I.e. have this class be in charge of placing the
> PAGE_PARAM_PLACEHOLDER into the URL?

Done.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:42: *   - pattern and URL must have
same number of path components.
On 2015/04/07 00:45:49, cjhopman wrote:
> Personally, I'd have these function-level comments be higher level (otherwise
> they are very likely to become out of date and not match the code as it
> changes), but it's up to you.
> 
> Like:
> 
> - for path page parameter, pattern and url must be sufficiently similar and
the
> pattern's components cannot be calendar digits

Done.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:52: boolean isValidFor(ParsedUrl
docUrl) {
On 2015/04/07 00:45:48, cjhopman wrote:
> Would it make sense to check validity when the page pattern is created?

i don't believe so.  multiple pagination URLs could have the same page pattern,
which would result in multiple duplicated checks if page pattern is validated
during creation.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:72: return
(getLongestCommonSuffixLength(urlComponent, patternComponent, commonPrefixLen) +
On 2015/04/07 00:45:48, cjhopman wrote:
> nit: could you put the common suffix in a local variable like common prefix
(or
> put both calculations in the return expression).

Done.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:78: if (isCalendarPage()) return
false;
On 2015/04/07 00:45:48, cjhopman wrote:
> Why do we check this here? it doesn't really depend on the docUrl. I think we
> could check this during construction of the page pattern.

it's true it doesn't depend on doc url.  however, this goes back to the same
argument as for validating during construction of page pattern.  if we check
during construction, we'd be doing it multiple and duplicated times since
multiple pagination URLs could end up withe same pattern.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:110: // Both url and patterm must
have the same suffix, if available.
On 2015/04/07 00:45:48, cjhopman wrote:
> nit: s/patterm/pattern

Done.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:122: 
On 2015/04/07 00:45:48, cjhopman wrote:
> Could we split these three cases into their own functions?
> 
> if (isDynamicParam) {
>   return isDynamicParamPagingUrl(url);
> } else if (isPartialPathComponent()) {
>   return isPartialPathComponentPagingUrl(url);
> }
> return isPathComponentPagingUrl(url);

Done.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:150: if (suffixPos < mComponentPos
|| !url.regionMatches(0, mUrlStr, 0, mComponentPos)) {
On 2015/04/07 00:45:48, cjhopman wrote:
> how about:
> 
> url.endsWith(mRequiredSuffix)
> 
> and calculate the required suffix on construction.

Done.  i assume u mean for lines 110-119 (at the beginning of this fn); i did
the same for prefix here using startsWith().

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:158: // ".html" or ".html", it is
fine.
On 2015/04/07 00:45:48, cjhopman wrote:
> s/.html/.htm for one of the two

Done.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:159: String diffPart =
url.substring(mComponentPos, suffixPos).toLowerCase();
On 2015/04/07 00:45:48, cjhopman wrote:
> I'm not really sure what diffpart is here. What is it for the last set of
> examples in the comment above? Maybe it's just unclear what mComponentPos and
> suffixPos are.

yes, it's for the last set of examples.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:178: //   -
www.foo.com/a/2/abc.html
On 2015/04/07 00:45:49, cjhopman wrote:
> The second part of this comment confused me because it applies to the code
after
> this if block. (right?)

yes, i've now split up the comments since the functionalities are split into
partial and entire.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:186: if (urlLen < mComponentPos +
suffixLen ||
On 2015/04/07 00:45:49, cjhopman wrote:
> url.startsWith(mRequiredPrefix)?
> 
> I guess if you do this and the one above, you need to make it clear that only
> certain types of page patterns have required prefix/suffix.

Done.  i'm not sure wha tu mean by "make it clear", each check of prefix and
suffix is preceded by comments.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:199: if (firstDiffPos + 1 ==
mPageParamPos &&
On 2015/04/07 00:45:49, cjhopman wrote:
> Hm, if I read this correctly, the following returns false:
> 
> pattern: http://www.foo.com/a/abc[*!].html
> url:     http://www.foo.com/a/abc.html
> 
> That seems odd.

yes it returns false, i verified in original code.  i think the heuristic for
page param in partial path component expects the potential page number to be
preceded by one of -_;,.  btw, i added a test for this.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:210: // Handle case where page
param is the entire path component.
On 2015/04/07 00:45:49, cjhopman wrote:
> This comment looks like it applies to just the next 8 lines or so, but I think
> it actually applies to the whole rest of the function. Maybe just moving the
> examples from above to down here would help.

Done.  comment is no longer needed since i've extracted it into
isEntirePathComponentPagingUrl().

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:211: int prevComponentPos =
mUrlStr.lastIndexOf('/', mComponentPos - 1);
On 2015/04/07 00:45:48, cjhopman wrote:
> Should this only be looking at the path? If not, will it ever have
> prevComponentPos == -1? (wouldn't the full url always have a leading scheme?)

Done.  good catch.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:213: // The url doesn't have page
number param and previous path component, like
On 2015/04/07 00:45:49, cjhopman wrote:
> Stated like this, it seems the comment applies to the current state... but
> really it applies to the state inside the next if block. Either move it inside
> the next if, or change it to something like:
> 
> Check if the url doesn't ...

Done.

https://codereview.chromium.org/1029593003/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/PagePattern.java:271: private void
determineParamIndex() {
On 2015/04/07 00:45:48, cjhopman wrote:
> We should just call this in the constructor so that mParamIndex can always
have
> been initialized.

Done.

[kuan, 2015-04-11 14:21:08]

chris, pls hold off on the review.  let  me look more into the linear formula
stuff.

[kuan, 2015-04-13 17:21:38]

ok, this is now ready for review.  thx!

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
File java/org/chromium/distiller/PageParameterDetector.java (right):

https://codereview.chromium.org/1029593003/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/PageParameterDetector.java:508: *   pageParamValue =
coefficient * pageNum + delta (delta == -coefficient or delta == 0).
On 2015/04/10 22:41:27, kuan wrote:
> On 2015/04/07 00:45:48, cjhopman wrote:
> > On 2015/03/31 17:17:50, kuan wrote:
> > > On 2015/03/27 00:16:11, cjhopman wrote:
> > > > Do we really need this complicated linear formula? It feels like the
> > > coefficient
> > > > is useful for pages like search results or forums or something but not
for
> > > > things that we actually expect to work on.
> > > 
> > > it's true we don't really care about the actual values of coefficient and
> > delta,
> > > but without them, how do we know if the page parameter is an actual one
that
> > > maps to the page number?  won't this help weed out false-positives with
> > > arbitrary numbers in the page parameter?
> > > 
> > > i could remove it if u're sure it's unnecessary.
> > 
> > i just want you to be sure if it's necessary or unnecessary. I think that
you
> > want something like it, but it might not need to support non-1 coefficient.
> 
> i'm wary of removing it now, including the non-1 coefficient support, without
> enough knowledge on how websites in the wild organize their pages.  i do think
> we need to verify the linear formula exists.
> 
> if we only support 1 coefficient, the following pagination URLs (in the tests)
> won't be detected, cos coefficient is 10:
> - page 1: http://www.google.com/test
> - page 2: http://www.google.com/test?page=10
> - page 3: http://www.google.com/test?page=20.
> 
> i'd prefer to keep this as is, for now anyway, so i've added a TODO to
reassess
> its necessity.

to clarify the example above, the pagination URLs would be detected, initially. 
however, if there also exist other pagination URLs with page=arbitraryNumber, we
wouldn't know these do not form a linear formula.  in the end, we'd end up with
a non-decisive situation, and conclude that there's no pagination.

[cjhopman, 2015-04-16 21:58:32]

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
File java/org/chromium/distiller/PageParameterDetector.java (right):

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
java/org/chromium/distiller/PageParameterDetector.java:236: private static
PageParamInfo getPageParamInfo(PagePattern pagePattern,
I feel like we have to many SomethingInfo types here and it's really unclear to
me what they actually are from the type names and I find myself having to go
look at the comments on those classes. It would be nice if I didn't have to do
that, but it's not something that needs to be fixed/changed right now.

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
java/org/chromium/distiller/PageParameterDetector.java:236: private static
PageParamInfo getPageParamInfo(PagePattern pagePattern,
Would it make sense for this function to be in the PageParamInfo class? I'm not
really sure.

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
java/org/chromium/distiller/PageParameterDetector.java:249: PageParamInfo
pageParamInfo = new PageParamInfo();
Could we move some of the logic of creating the pageParamInfo here into a
PageParamInfo constructor? Same for the construction of one in the next block.

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
File java/org/chromium/distiller/PathComponentPagePattern.java (right):

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
java/org/chromium/distiller/PathComponentPagePattern.java:39: } catch (Exception
e) {
We shouldn't be throwing/catching the base Exception class. Instead, either find
an appropriate subclass of it or define our own.

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
java/org/chromium/distiller/PathComponentPagePattern.java:65: // If the page
param is inside of path components, both the pattern and doc URL must have
We know that the page param is inside of path components so this comment seems a
bit awkward:

How about just drop the part before the comma?

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
java/org/chromium/distiller/PathComponentPagePattern.java:143: return
mUrlStr.charAt(mPlaceholderStart - 1) != '/';
this is strange, shouldn't
 foo.com/article/2page/ be a partial path component?

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
java/org/chromium/distiller/PathComponentPagePattern.java:169:
determineParamIndex();
don't need to call this, it's done in the constructor. same below.

[kuan, 2015-04-20 23:11:13]

i've addressed all comments in patch set 10.  as per our 1-1 last wk, i've also
removed all code that handle single-page url candidates.  ptal.  thx.

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
File java/org/chromium/distiller/PageParameterDetector.java (right):

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
java/org/chromium/distiller/PageParameterDetector.java:236: private static
PageParamInfo getPageParamInfo(PagePattern pagePattern,
On 2015/04/16 21:58:32, cjhopman wrote:
> Would it make sense for this function to be in the PageParamInfo class? I'm
not
> really sure.

Done.  to avoid cross-access between PageParameterDetector.LinkInfo and
PageParamInfo, i extracted the former into its own class PageLinkInfo.

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
java/org/chromium/distiller/PageParameterDetector.java:236: private static
PageParamInfo getPageParamInfo(PagePattern pagePattern,
On 2015/04/16 21:58:32, cjhopman wrote:
> I feel like we have to many SomethingInfo types here and it's really unclear
to
> me what they actually are from the type names and I find myself having to go
> look at the comments on those classes. It would be nice if I didn't have to do
> that, but it's not something that needs to be fixed/changed right now.

i'm bad at names, and ran out of them :(

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
java/org/chromium/distiller/PageParameterDetector.java:249: PageParamInfo
pageParamInfo = new PageParamInfo();
On 2015/04/16 21:58:32, cjhopman wrote:
> Could we move some of the logic of creating the pageParamInfo here into a
> PageParamInfo constructor? Same for the construction of one in the next block.

Done.

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
File java/org/chromium/distiller/PathComponentPagePattern.java (right):

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
java/org/chromium/distiller/PathComponentPagePattern.java:39: } catch (Exception
e) {
On 2015/04/16 21:58:32, cjhopman wrote:
> We shouldn't be throwing/catching the base Exception class. Instead, either
find
> an appropriate subclass of it or define our own.

Done.  ditto for QueryParamPagePattern.

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
java/org/chromium/distiller/PathComponentPagePattern.java:65: // If the page
param is inside of path components, both the pattern and doc URL must have
On 2015/04/16 21:58:32, cjhopman wrote:
> We know that the page param is inside of path components so this comment seems
a
> bit awkward:
> 
> How about just drop the part before the comma?

Done.

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
java/org/chromium/distiller/PathComponentPagePattern.java:143: return
mUrlStr.charAt(mPlaceholderStart - 1) != '/';
On 2015/04/16 21:58:32, cjhopman wrote:
> this is strange, shouldn't
>  foo.com/article/2page/ be a partial path component?

good point, so i did more testing w/ the original code.
they actually meant at start of path component.  they called it "partial" and
didn't care about the suffix (i.e. whatever's after placeholder), because the
paging url must already have the same suffix by the time the code path executes
to this point (see beginning of isPaingUrl()).
now, the functionalities are renamed to reflect start of path component.

https://codereview.chromium.org/1029593003/diff/150001/java/org/chromium/dist...
java/org/chromium/distiller/PathComponentPagePattern.java:169:
determineParamIndex();
On 2015/04/16 21:58:32, cjhopman wrote:
> don't need to call this, it's done in the constructor. same below.

Done. i forgot :(

[cjhopman, 2015-04-22 00:35:35]

https://codereview.chromium.org/1029593003/diff/170001/java/org/chromium/dist...
File java/org/chromium/distiller/PageLinkInfo.java (right):

https://codereview.chromium.org/1029593003/diff/170001/java/org/chromium/dist...
java/org/chromium/distiller/PageLinkInfo.java:18: int mPosInAscendingList;
Does a PageLinkInfo always correspond to a PageParamInfo.PageInfo? If so, why is
the information split into two classes with just this implicit connection?

[kuan, 2015-04-22 13:39:38]

https://codereview.chromium.org/1029593003/diff/170001/java/org/chromium/dist...
File java/org/chromium/distiller/PageLinkInfo.java (right):

https://codereview.chromium.org/1029593003/diff/170001/java/org/chromium/dist...
java/org/chromium/distiller/PageLinkInfo.java:18: int mPosInAscendingList;
On 2015/04/22 00:35:35, cjhopman wrote:
> Does a PageLinkInfo always correspond to a PageParamInfo.PageInfo? If so, why
is
> the information split into two classes with just this implicit connection?

no it doesn't.
1) a PageParamInfo.PageInfo can have no mUrl, in which case, there'd be no
PageLinkInfo.
2) a PageParamInfo.PageInfo can emit multiple different PageLinkInfo's,
depending on the number of groups of consecutive digits in the url, as explained
in PageParameterDetector.java lines 127 and 157.

[cjhopman, 2015-04-24 02:38:56]

https://codereview.chromium.org/1029593003/diff/170001/java/org/chromium/dist...
File java/org/chromium/distiller/PageParamInfo.java (right):

https://codereview.chromium.org/1029593003/diff/170001/java/org/chromium/dist...
java/org/chromium/distiller/PageParamInfo.java:23: static final int
PAGE_NUM_ADJACENT_MASK = 1 << 0;
can these be private?

https://codereview.chromium.org/1029593003/diff/170001/java/org/chromium/dist...
java/org/chromium/distiller/PageParamInfo.java:84: Type mType = Type.UNSET;
All these private?

https://codereview.chromium.org/1029593003/diff/170001/java/org/chromium/dist...
java/org/chromium/distiller/PageParamInfo.java:122: if ((result &
PAGE_NUM_CONSECUTIVE_MASK) != PAGE_NUM_CONSECUTIVE_MASK) return null;
nit: can this be "== 0" like when checking a flag above?

https://codereview.chromium.org/1029593003/diff/170001/java/org/chromium/dist...
java/org/chromium/distiller/PageParamInfo.java:124: PageParamInfo pageParamInfo
= new PageParamInfo(Type.PAGE_NUMBER, linearFormula);
Can the PageParamInfo constructor take a List<PageInfo> instead of doing the
construction and then modifying the field?

[kuan, 2015-04-24 17:38:00]

i've addressed all comments in patch set 11.  ptal.  thx.

https://codereview.chromium.org/1029593003/diff/170001/java/org/chromium/dist...
File java/org/chromium/distiller/PageParamInfo.java (right):

https://codereview.chromium.org/1029593003/diff/170001/java/org/chromium/dist...
java/org/chromium/distiller/PageParamInfo.java:23: static final int
PAGE_NUM_ADJACENT_MASK = 1 << 0;
On 2015/04/24 02:38:56, cjhopman wrote:
> can these be private?

i'm using them in PageParamInfoTest.java.

https://codereview.chromium.org/1029593003/diff/170001/java/org/chromium/dist...
java/org/chromium/distiller/PageParamInfo.java:84: Type mType = Type.UNSET;
On 2015/04/24 02:38:56, cjhopman wrote:
> All these private?

i'll be using them in upcoming PageParameterDetectorTest.java.

https://codereview.chromium.org/1029593003/diff/170001/java/org/chromium/dist...
java/org/chromium/distiller/PageParamInfo.java:122: if ((result &
PAGE_NUM_CONSECUTIVE_MASK) != PAGE_NUM_CONSECUTIVE_MASK) return null;
On 2015/04/24 02:38:56, cjhopman wrote:
> nit: can this be "== 0" like when checking a flag above?

Done.  duh, what i did was weird :(

https://codereview.chromium.org/1029593003/diff/170001/java/org/chromium/dist...
java/org/chromium/distiller/PageParamInfo.java:124: PageParamInfo pageParamInfo
= new PageParamInfo(Type.PAGE_NUMBER, linearFormula);
On 2015/04/24 02:38:56, cjhopman wrote:
> Can the PageParamInfo constructor take a List<PageInfo> instead of doing the
> construction and then modifying the field?

Done.

[cjhopman, 2015-04-27 18:14:29]

lgtm

[kuan, 2015-04-27 18:23:30]

Committed patchset #11 (id:190001) manually as
10bc28cd4169e3b9532b5c253f620893c7650a67 (presubmit successful).