aw: Limit full url href hittest to only anchor type

aw: Limit full url href hittest to only anchor type

This limits the fix for crrev.com/r223298 to anchor type, and
re-implement it and test it properly.

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

[boliu, 2013-11-27 23:45:03]

This is what I imagine the fix should be.

To be safer this time, going to run through the test cases on classic webview to
make sure we don't break anything else.

[boliu, 2013-11-28 00:55:16]

On 2013/11/27 23:45:03, boliu wrote:
> This is what I imagine the fix should be.
> 
> To be safer this time, going to run through the test cases on classic webview
to
> make sure we don't break anything else.

Going to play with ag/393768 after thanksgiving vacation.

[boliu, 2013-12-16 19:51:27]

Anyone still here? Let's fix this.

See data:
https://docs.google.com/a/google.com/spreadsheets/d/1cOUWf9iPtA58eeVLcuWX8RFk...

[boliu, 2013-12-16 20:50:56]

+kristianm

On 2013/12/16 19:51:27, boliu wrote:
> Anyone still here? Let's fix this.
> 
> See data:
>
https://docs.google.com/a/google.com/spreadsheets/d/1cOUWf9iPtA58eeVLcuWX8RFk...

[boliu, 2013-12-17 20:59:35]

primiano: ping. What reservations about this do you have?

[Primiano Tucci (use gerrit), 2013-12-18 11:47:03]

On 2013/12/17 20:59:35, boliu wrote:
> primiano: ping. What reservations about this do you have?

I don't have reservations on the change (I don't think I have ever expressed any
negative opinion in this regard).
The change itself LGTM. I am not sure that I am the most appropriate person to
review this code, though.
I just made a one line change in the past (i.e. crrev.com/23454036) and despite
the several hours spent trying to figure out all the cases, I got it wrong (i.e.
fixed one bug but caused at least another one). I just don't think I can be
considered authoritative on this piece of code, especially considered the actual
test coverage.
The only point that scares me, in fact, is testing:
WebKitHitTestTest.java claims, in its header: "Test for getHitTestResult,
requestFocusNodeHref, and requestImageRef methods", which is not really true. 
If I am reading the code correctly, WebKitHitTestTest.java is testing the
getLastHitTestResult and not any of the three mentioned in the header. There is
extra code, however, between getLastHitTestResult and
AwContents.java::requestFocusNodeHref, which is the API ultimately exposed to
WebView clients.
Our history shows that we have had a non negligible number of bugs on the
requestFocusNodeHref API, mostly regarding violated assumptions on the values of
tuple  {url, title, src} in corner cases (i.e. invalid URLs, javascript schemes,
etc.). The regression I caused in crrev.com/23454036, in fact, happened touching
the code in that extra layer.
I trust the fact that you have manually tested all such cases and this CL is not
regressing any of them, as you showed in our offline discussion.
But, as far as I can see, there is no code* actually covering those cases, in
the same way there weren't when I introduced the regression.

* There is some marginal coverage in AOSP/CTS:
https://android.googlesource.com/platform/cts/+/865be9bae4261ba1be5b3075656a6...

[boliu, 2013-12-18 15:42:41]

On 2013/12/18 11:47:03, Primiano Tucci wrote:
> On 2013/12/17 20:59:35, boliu wrote:
> > primiano: ping. What reservations about this do you have?
> 
> I don't have reservations on the change (I don't think I have ever expressed
any
> negative opinion in this regard).
> The change itself LGTM. I am not sure that I am the most appropriate person to
> review this code, though.

No one is really appropriate...you touched this code, so you are it!

> I just made a one line change in the past (i.e. crrev.com/23454036) and
despite
> the several hours spent trying to figure out all the cases, I got it wrong
(i.e.
> fixed one bug but caused at least another one). I just don't think I can be
> considered authoritative on this piece of code, especially considered the
actual
> test coverage.
> The only point that scares me, in fact, is testing:
> WebKitHitTestTest.java claims, in its header: "Test for getHitTestResult,
> requestFocusNodeHref, and requestImageRef methods", which is not really true. 
> If I am reading the code correctly, WebKitHitTestTest.java is testing the
> getLastHitTestResult and not any of the three mentioned in the header. There
is
> extra code, however, between getLastHitTestResult and
> AwContents.java::requestFocusNodeHref, which is the API ultimately exposed to
> WebView clients.

The extra logic is the mistake imo.
AwContents.getHitTestResult/requestFocusNodeHref/requestImageRef are not
supposed to contain any logic, only forwarding values. The tests does check all
the value in HitTestData, which is used to populate return values from all 3
methods. But agree that test can all requestFocusNodeHref and requestImageRef
directly. Will do this in a follow up.

> Our history shows that we have had a non negligible number of bugs on the
> requestFocusNodeHref API, mostly regarding violated assumptions on the values
of
> tuple  {url, title, src} in corner cases (i.e. invalid URLs, javascript
schemes,
> etc.). The regression I caused in crrev.com/23454036, in fact, happened
touching
> the code in that extra layer.
> I trust the fact that you have manually tested all such cases and this CL is
not
> regressing any of them, as you showed in our offline discussion.
> But, as far as I can see, there is no code* actually covering those cases, in
> the same way there weren't when I introduced the regression.
> 
> * There is some marginal coverage in AOSP/CTS:
>
https://android.googlesource.com/platform/cts/+/865be9bae4261ba1be5b3075656a6...

Our instrumentation tests is more extensive than cts tests. Although
instrumentation tests can still be improved.

[commit-bot: I haz the power, 2013-12-18 15:44:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/92903003/40001

[commit-bot: I haz the power, 2013-12-18 18:18:46]

Change committed as 241601