Bug 118553

Summary:

Hash navigation doesn't affect history when the page is retrieved from appcache

Product:

WebKit

Reporter:

Noam Rosenthal <noam>

Component:

History

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, ap, beidson, commit-queue, fishd, japhet, luiz, zherczeg

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Test case	none
patch	none
patch2	darin: review+
patch	none

Noam Rosenthal

Reported 2013-07-11 01:53:03 PDT

Created attachment 206437 [details] Test case This was reproduced on trunk and on shipping Safari, and is probably an old bug though search through the bug database didn't come up with anything concrete. When performing hash navigation on the attached page, the hash changes appear in history if the page was retrieved from the server, but not if it's retrieved from the cache. This is most likely related to SubstituteData, which is used both for cached resources and for errors. Specifically HistoryController::updateBackForwardListClippedAtTarget() calls urlForHistory, which returns an invalid URL if the document load has SubstituteData.

Attachments
Test case (275 bytes, application/octet-stream) 2013-07-11 01:53 PDT, Noam Rosenthal	no flags	Details
patch (8.67 KB, patch) 2013-08-26 03:06 PDT, Zoltan Herczeg	no flags	Details Formatted Diff Diff
patch2 (8.29 KB, patch) 2013-08-27 03:53 PDT, Zoltan Herczeg	darin: review+	Details Formatted Diff Diff
patch (8.74 KB, patch) 2013-08-30 02:02 PDT, Zoltan Herczeg	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Zoltan Herczeg

Comment 1 2013-07-30 00:19:27 PDT

The substitute data is set here: http://trac.webkit.org/browser/trunk/Source/WebCore/loader/DocumentLoader.cpp#L512 The history works in Chromium, and I checked the Blink repository: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/loader/DocumentLoader.cpp&l=490 As you can see that particular "if" statement was removed from DocumentLoader::willSendRequest. CC'ing Darin Fisher, he might know more, and I try to figure out when that code path was removed from Blink.

Zoltan Herczeg

Comment 2 2013-07-30 03:20:59 PDT

Here is the patch: https://chromiumcodereview.appspot.com/14785010 Title: Remove a bunch of ApplicationCacheHost functions and parameters we don't use. Do we need these functions?

Alexey Proskuryakov

Comment 3 2013-07-30 09:42:49 PDT

> Do we need these functions? Most likely, yes (and we probably don't need most of the functions that Blink uses).

Zoltan Herczeg

Comment 4 2013-08-26 03:06:53 PDT

Created attachment 209634 [details] patch

Brady Eidson

Comment 5 2013-08-26 09:27:18 PDT

Comment on attachment 209634 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=209634&action=review > LayoutTests/http/tests/appcache/resources/history-back.html:4 > +<html> > +<body onload="window.history.back()"> > +</body> > +</html> Would love to see this use http/tests/navigation/resources/go-back.html instead of adding a new "go-back" page. > Source/WebCore/loader/SubstituteData.h:55 > + bool keepInHistory() const { return m_keepInHistory; } > + void setKeepInHistory(bool value) { m_keepInHistory = value; } A few points on naming: - "keep in history" suggests that this represents something that was added to history, we're considering removing it, but we shouldn't remove it. That's not quite what is happening. Reading the users of DocumentLoader::urlForHistory() I think it would be more clear to say "shouldRevealToHistory" or something similar. - "History" is confusing. We need to continue unraveling the confusion between back/forward history and global browser history. Please call it "shouldRevealToSessionHistory"

Alexey Proskuryakov

Comment 6 2013-08-26 09:44:38 PDT

I don't have further comments beyond what Brady said.

Zoltan Herczeg

Comment 7 2013-08-27 03:53:13 PDT

Created attachment 209743 [details] patch2 Thanks for the comments! New patch is attached.

Darin Adler

Comment 8 2013-08-27 09:57:46 PDT

Comment on attachment 209743 [details] patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=209743&action=review > Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:89 > + substituteData = SubstituteData(resource->data(), > resource->response().mimeType(), > resource->response().textEncodingName(), KURL()); > + substituteData.setShouldRevealToSessionHistory(true); One further thought. Since the only caller is setting this at construction time, would be nice to just set this in the constructor, leave out setShouldRevealToSessionHistory entirely, and maybe even make m_shouldRevealToSessionHistory const.

Zoltan Herczeg

Comment 9 2013-08-27 11:27:13 PDT

> One further thought. Since the only caller is setting this at construction time, would be nice to just set this in the constructor, leave out setShouldRevealToSessionHistory entirely, and maybe even make m_shouldRevealToSessionHistory const. I was thinking about that myself, but passing a "true" there does not really say anything what is happening (there was a long discussion about passing true/false arguments), and I try to avoid it. So my question is, what would you prefer: passing a true, or passing an enum (which could be converted to a boolean inside the constructor) or passing a const value (defining a kSomething = true inside the class) or leave the code as it is now?

Zoltan Herczeg

Comment 10 2013-08-28 23:21:20 PDT

If noone has any objection, I choose the option with a kShouldRevealToSessionHistory = true option, and land the patch.

Brady Eidson

Comment 11 2013-08-29 09:20:25 PDT

Unless I missed some change, kShouldRevealToSessionHistory is not a token that matches our coding style guidelines?

Zoltan Herczeg

Comment 12 2013-08-30 02:02:36 PDT

Created attachment 210080 [details] patch You are right. The attached patch will be landed.

WebKit Commit Bot

Comment 13 2013-08-30 02:08:12 PDT

Attachment 210080 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http/tests/appcache/history-test-expected.txt', u'LayoutTests/http/tests/appcache/history-test.html', u'LayoutTests/http/tests/appcache/resources/history-test.html', u'LayoutTests/http/tests/appcache/resources/history-test.manifest', u'Source/WebCore/ChangeLog', u'Source/WebCore/loader/DocumentLoader.cpp', u'Source/WebCore/loader/SubstituteData.h', u'Source/WebCore/loader/appcache/ApplicationCacheHost.cpp']" exit_code: 1 Source/WebCore/loader/SubstituteData.h:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/loader/SubstituteData.h:45: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Zoltan Herczeg

Comment 14 2013-09-04 23:39:41 PDT

There were no other comments so I landed the patch. http://trac.webkit.org/changeset/155099 Thanks again for the help.

Note You need to log in before you can comment on or make changes to this bug.