Bug 95129 – [chromium] Link highlight should clear on page navigation.

• Component: New Bugs
• Status: RESOLVED FIXED

[W. James MacLean]

[chromium] Link highlight should clear on page navigation.

[W. James MacLean]

Created attachment 160812 [details]
Patch

Up until now, a highlight generate on a page can 'survive' in the non-composited content host when navigating to a new page, thus showing up on a page where it shouldn't be. This CL clears the link highlight when navigating to a new page.

[WebKit Review Bot]

Comment on attachment 160812 [details]
Patch

Attachment 160812 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13642074

New failing tests:
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-simple-navigate.html

[WebKit Review Bot]

Created attachment 160826 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[James Robinson]

Comment on attachment 160812 [details]
Patch

The code seems quite reasonable, but could you investigate why the test is failing?

[W. James MacLean]

Created attachment 160959 [details]
Patch

This is an experiment to see if calling notifyDone() in the destination page helps resolve the timeout. I can't imagine that something this simple needs to be marked as SLOW, though I could be wrong ...

[W. James MacLean]

(In reply to comment #5)
> Created an attachment (id=160959) [details]
> Patch
> 
> This is an experiment to see if calling notifyDone() in the destination page helps resolve the timeout. I can't imagine that something this simple needs to be marked as SLOW, though I could be wrong ...

So this seems to have made the bot happier. I didn't require the extra notifyDone() to get the test to run properly under nrwt on my local workstation, so I hope this isn't just timing flake.

[James Robinson]

Comment on attachment 160959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160959&action=review

I think your test is still flaky. Otherwise, this looks good.

> LayoutTests/platform/chromium-linux/compositing/gestures/gesture-tapHighlight-simple-navigate.html:31
> +        window.setTimeout(function() { window.testRunner.notifyDone(); }, 0);

This seems wrong - if the navigation failed or didn't finish in time, we don't want to end the test. Are you hitting situations where runTest() isn't running at all on -destination? Can you figure out why not?

> LayoutTests/platform/chromium-linux/compositing/gestures/resources/gesture-tapHighlight-simple-navigate-destination.html:15
> +        window.testRunner.notifyDone();

This is the only thing that should be ending the test

[W. James MacLean]

(In reply to comment #7)
> (From update of attachment 160959 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160959&action=review
> 
> I think your test is still flaky. Otherwise, this looks good.
> 
> > LayoutTests/platform/chromium-linux/compositing/gestures/gesture-tapHighlight-simple-navigate.html:31
> > +        window.setTimeout(function() { window.testRunner.notifyDone(); }, 0);
> 
> This seems wrong - if the navigation failed or didn't finish in time, we don't want to end the test. 

I had hoped that, if the test was successful, the javascript in the first file would terminate at "window.location = ...".

> Are you hitting situations where runTest() isn't running at all on -destination? 

No, it was failing on EWS with a TIMEOUT (so I don't know what it was getting stuck on, no output). Locally  on my Z600 it ran fine.

> Can you figure out why not?

Not definitively ... I have no output from the bot to give me confidence that I know for sure what's going on.

> > LayoutTests/platform/chromium-linux/compositing/gestures/resources/gesture-tapHighlight-simple-navigate-destination.html:15
> > +        window.testRunner.notifyDone();
> 
> This is the only thing that should be ending the test

I wasn't sure if testRunner 'lived' across the navigation. I do know that, locally, I need the "dumpAsText(true)" in the -destination file, or else it doesn't dump the pixel results. This made me think testRunner was re-initializing.

** Is testRunner supposed to survive (maintain state) across the navigation?

If we remove the notifyDone() from the first file, that would give me some confidence that if the navigation failed the test would timeout. But, even if it doesn't timeout I would expect a IMAGE failure.

Is there some good way to test for flake before I upload a new patch?

[W. James MacLean]

Created attachment 161056 [details]
Patch

This patch only uses notifyDone() in the -destination.html file. We'll let the EWS bots chew on this and see what happens.

[James Robinson]

Comment on attachment 161056 [details]
Patch

EWS seems happy and I believe this is how the test should be structured - let's go with it and see if it causes any issues on the bots.

[W. James MacLean]

Comment on attachment 161056 [details]
Patch

Sounds good ... I will keep an eye on the bots!

[WebKit Review Bot]

Comment on attachment 161056 [details]
Patch

Clearing flags on attachment: 161056

Committed r127023: <http://trac.webkit.org/changeset/127023>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.