Bug 58379 – The default backspace event handler should mark the event as handled if navigation succeeds

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Ananta Iyengar]

The EventHandler::defaultBackspaceEventHandler function should mark the event
as handled only if the back/forward navigation was successful.

This breaks ChromeFrame (which embeds Chrome->Webkit in IE)where some of the pages
are rendered in the host browser like IE.

Scenario:-
1. IE navigates to foo.com
2. A navigation to bar.com which renders in ChromeFrame.
3. Hitting backspace on bar.com should go back to foo.com which does not happen as the
   backspace event is eaten by webkit.

[Ananta Iyengar]

Created attachment 89282 [details]
Initial patch

[Alexey Proskuryakov]

Comment on attachment 89282 [details]
Initial patch

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

> Source/WebCore/ChangeLog:3
> +        https://bugs.webkit.org/show_bug.cgi?id=58379

Please add bug title before the link.

> Source/WebCore/ChangeLog:9
> +        No new tests. (OOPS!)

The patch cannot be landed with OOPS, so you need to explain why there are no new tests instead of leaving the line as is.

[Ananta Iyengar]

Created attachment 89285 [details]
Patch with review comments addressed

[Alexey Proskuryakov]

Comment on attachment 89285 [details]
Patch with review comments addressed

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

> Source/WebCore/ChangeLog:3
> +        https://bugs.webkit.org/show_bug.cgi?id=58379

Still no bug title.

> Source/WebCore/ChangeLog:9
> +        No new tests added as there is no change in functionality.

This is not accurate. The patch fixes a bug, so of course there is change in functionality.

[Ananta Iyengar]

Created attachment 89294 [details]
Explanation added as to why there are no tests. Added the title before the bug link as requested.

[Alexey Proskuryakov]

Comment on attachment 89294 [details]
Explanation added as to why there are no tests. Added the title before the bug link as requested.

Ugh, sorry that I didn't notice it before. You have removed the Not Reviewed line that was added by prepare-ChangeLog, so commit queue won't be able to put the reviewer name in. Please add it back (I suggest running prepare-ChangeLog again to see how exactly it should look).

[Ananta Iyengar]

Created attachment 89304 [details]
Created a new patch after running the prepare-changelog script. Sorry about removing the Reviewed by line

[Alexey Proskuryakov]

Comment on attachment 89304 [details]
Created a new patch after running the prepare-changelog script. Sorry about removing the Reviewed by line

In normal case, it feels weird to let the event fly further if we happen to be at the first or last page, but block it otherwise. But this is what we do elsewhere in this file, so it's perfectly fine for this patch.

[WebKit Commit Bot]

Comment on attachment 89304 [details]
Created a new patch after running the prepare-changelog script. Sorry about removing the Reviewed by line

Clearing flags on attachment: 89304

Committed r83688: <http://trac.webkit.org/changeset/83688>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.