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: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
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. 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? > Do we need these functions?
Most likely, yes (and we probably don't need most of the functions that Blink uses).
Created attachment 209634 [details]
patch
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" I don't have further comments beyond what Brady said. Created attachment 209743 [details]
patch2
Thanks for the comments! New patch is attached.
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. > 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?
If noone has any objection, I choose the option with a kShouldRevealToSessionHistory = true option, and land the patch. Unless I missed some change, kShouldRevealToSessionHistory is not a token that matches our coding style guidelines? Created attachment 210080 [details]
patch
You are right. The attached patch will be landed.
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.
There were no other comments so I landed the patch. http://trac.webkit.org/changeset/155099 Thanks again for the help. |
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.