Bug 118553 - Hash navigation doesn't affect history when the page is retrieved from appcache
Summary: Hash navigation doesn't affect history when the page is retrieved from appcache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-11 01:53 PDT by Noam Rosenthal
Modified: 2013-09-04 23:39 PDT (History)
8 users (show)

See Also:


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+
zherczeg: commit-queue?
Details | Formatted Diff | Diff
patch (8.74 KB, patch)
2013-08-30 02:02 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 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.
Comment 1 Zoltan Herczeg 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.
Comment 2 Zoltan Herczeg 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?
Comment 3 Alexey Proskuryakov 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).
Comment 4 Zoltan Herczeg 2013-08-26 03:06:53 PDT
Created attachment 209634 [details]
patch
Comment 5 Brady Eidson 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"
Comment 6 Alexey Proskuryakov 2013-08-26 09:44:38 PDT
I don't have further comments beyond what Brady said.
Comment 7 Zoltan Herczeg 2013-08-27 03:53:13 PDT
Created attachment 209743 [details]
patch2

Thanks for the comments! New patch is attached.
Comment 8 Darin Adler 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.
Comment 9 Zoltan Herczeg 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?
Comment 10 Zoltan Herczeg 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.
Comment 11 Brady Eidson 2013-08-29 09:20:25 PDT
Unless I missed some change, kShouldRevealToSessionHistory is not a token that matches our coding style guidelines?
Comment 12 Zoltan Herczeg 2013-08-30 02:02:36 PDT
Created attachment 210080 [details]
patch

You are right. The attached patch will be landed.
Comment 13 WebKit Commit Bot 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.
Comment 14 Zoltan Herczeg 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.