Bug 58379 - The default backspace event handler should mark the event as handled if navigation succeeds
Summary: The default backspace event handler should mark the event as handled if navig...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Ananta Iyengar
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-12 14:53 PDT by Ananta Iyengar
Modified: 2011-04-12 19:28 PDT (History)
2 users (show)

See Also:


Attachments
Initial patch (1.48 KB, patch)
2011-04-12 15:31 PDT, Ananta Iyengar
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff
Patch with review comments addressed (1.51 KB, patch)
2011-04-12 15:46 PDT, Ananta Iyengar
no flags Details | Formatted Diff | Diff
Explanation added as to why there are no tests. Added the title before the bug link as requested. (1.69 KB, patch)
2011-04-12 16:02 PDT, Ananta Iyengar
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff
Created a new patch after running the prepare-changelog script. Sorry about removing the Reviewed by line (1.74 KB, patch)
2011-04-12 16:35 PDT, Ananta Iyengar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ananta Iyengar 2011-04-12 14:53:50 PDT
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.
Comment 1 Ananta Iyengar 2011-04-12 15:31:05 PDT
Created attachment 89282 [details]
Initial patch
Comment 2 Alexey Proskuryakov 2011-04-12 15:36:10 PDT
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.
Comment 3 Ananta Iyengar 2011-04-12 15:46:43 PDT
Created attachment 89285 [details]
Patch with review comments addressed
Comment 4 Alexey Proskuryakov 2011-04-12 15:50:44 PDT
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.
Comment 5 Ananta Iyengar 2011-04-12 16:02:19 PDT
Created attachment 89294 [details]
Explanation added as to why there are no tests. Added the title before the bug link as requested.
Comment 6 Alexey Proskuryakov 2011-04-12 16:26:30 PDT
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).
Comment 7 Ananta Iyengar 2011-04-12 16:35:03 PDT
Created attachment 89304 [details]
Created a new patch after running the prepare-changelog script. Sorry about removing the Reviewed by line
Comment 8 Alexey Proskuryakov 2011-04-12 17:17:34 PDT
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.
Comment 9 WebKit Commit Bot 2011-04-12 19:28:01 PDT
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>
Comment 10 WebKit Commit Bot 2011-04-12 19:28:08 PDT
All reviewed patches have been landed.  Closing bug.