Autofill: Improve table row matching algorithm.

Autofill: Improve table row matching algorithm.

Instead of just taking the text of the previous row, try to look for a
cell in the previous row that matches the position of the current cell.

Committed: https://crrev.com/2f7257317c2a9f959cbf49dcd0d34ea2a7a58802
Cr-Commit-Position: refs/heads/master@{#320637}

[Lei Zhang, 2015-03-13 01:52:44]

thestig@chromium.org changed reviewers:
+ estade@chromium.org

[Lei Zhang, 2015-03-13 01:52:44]

This would have helped 17_crbug_98338.html as well, but one of the label cells
in that table is commented out.

[Evan Stade, 2015-03-13 19:49:45]

will this fix that Indian site?

https://codereview.chromium.org/1006743002/diff/1/components/autofill/content...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/1006743002/diff/1/components/autofill/content...
components/autofill/content/renderer/form_autofill_util.cc:144: if
(base::StringToSizeT(colspan, &span) && span < 1)
I'd just do

base::StringToSizeT(colspan, &span);
span = std::max(span, 1U);

and it doesn't seem like the DCHECK will be necessary.

https://codereview.chromium.org/1006743002/diff/1/components/autofill/content...
components/autofill/content/renderer/form_autofill_util.cc:420: // First fine
the <td> that contains |element|.
nit: move this to L423

https://codereview.chromium.org/1006743002/diff/1/components/autofill/content...
components/autofill/content/renderer/form_autofill_util.cc:444: while
(!cell_it.isNull()) {
this looks like a job for a for loop

https://codereview.chromium.org/1006743002/diff/1/components/autofill/content...
components/autofill/content/renderer/form_autofill_util.cc:454: while
(!cell_it.isNull()) {
ditto

[Lei Zhang, 2015-03-13 21:12:50]

On 2015/03/13 19:49:45, Evan Stade wrote:
> will this fix that Indian site?

No, that site uses divs.

[Lei Zhang, 2015-03-13 21:19:53]

https://codereview.chromium.org/1006743002/diff/1/components/autofill/content...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/1006743002/diff/1/components/autofill/content...
components/autofill/content/renderer/form_autofill_util.cc:144: if
(base::StringToSizeT(colspan, &span) && span < 1)
On 2015/03/13 19:49:45, Evan Stade wrote:
> I'd just do
> 
> base::StringToSizeT(colspan, &span);
> span = std::max(span, 1U);
> 
> and it doesn't seem like the DCHECK will be necessary.

Done.

https://codereview.chromium.org/1006743002/diff/1/components/autofill/content...
components/autofill/content/renderer/form_autofill_util.cc:420: // First fine
the <td> that contains |element|.
On 2015/03/13 19:49:45, Evan Stade wrote:
> nit: move this to L423

Done.

https://codereview.chromium.org/1006743002/diff/1/components/autofill/content...
components/autofill/content/renderer/form_autofill_util.cc:444: while
(!cell_it.isNull()) {
On 2015/03/13 19:49:45, Evan Stade wrote:
> this looks like a job for a for loop

Done.

https://codereview.chromium.org/1006743002/diff/1/components/autofill/content...
components/autofill/content/renderer/form_autofill_util.cc:454: while
(!cell_it.isNull()) {
On 2015/03/13 19:49:45, Evan Stade wrote:
> ditto

Done.

[Evan Stade, 2015-03-13 21:35:49]

lgtm

[Lei Zhang, 2015-03-13 21:37:33]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-03-13 21:38:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1006743002/20001

[Lei Zhang, 2015-03-13 21:39:01]

The CQ bit was unchecked by thestig@chromium.org

[Lei Zhang, 2015-03-13 21:57:53]

BTW, StringToInt() and friends have some special behavior when they fail, so I
added an extra check for overflows.

[Lei Zhang, 2015-03-13 21:58:09]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-03-13 21:58:09]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1006743002/#ps40001
(title: "fix build, handle overflow")

[commit-bot: I haz the power, 2015-03-13 21:58:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1006743002/40001

[commit-bot: I haz the power, 2015-03-14 01:58:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-14 01:58:03]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Lei Zhang, 2015-03-14 02:36:27]

Patch set 4 fixes FormAutofillTest.LabelsInferredFromTableRow and expands the
test.

[Lei Zhang, 2015-03-14 02:36:33]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-03-14 02:36:33]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1006743002/#ps60001
(title: "fix test")

[commit-bot: I haz the power, 2015-03-14 02:36:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1006743002/60001

[commit-bot: I haz the power, 2015-03-14 03:30:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-14 03:31:29]

Patchset 4 (id:??) landed as
https://crrev.com/2f7257317c2a9f959cbf49dcd0d34ea2a7a58802
Cr-Commit-Position: refs/heads/master@{#320637}