Summary: | MHTML files should be loadable from all schemes considered local, not just file: | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Greg Spencer <gspencer> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Greg Spencer <gspencer> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, ap, bashi, beidson, japhet, jberlin, sam, webkit.review.bot | ||||||||||||
Priority: | P2 | Keywords: | GoogleBug | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 86583 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Greg Spencer
2012-05-15 15:54:22 PDT
Created attachment 142088 [details]
A proposed patch
Created attachment 142089 [details]
Patch
Attachment 142089 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:10: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 142093 [details]
Patch
Adding Adam as a potential reviewer. Comment on attachment 142093 [details]
Patch
Makes sense to me.
Comment on attachment 142093 [details] Patch Clearing flags on attachment: 142093 Committed r117206: <http://trac.webkit.org/changeset/117206> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 86583 The patch also breaks chromium's browser_tests. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=browser_tests&tests=RenderViewHostTest.BaseURLParam I'm going to revert the patch. How does the change to MainResourceLoader::continueAfterContentPolicy affect WebArchives? Comment on attachment 142093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142093&action=review > Source/WebCore/loader/MainResourceLoader.cpp:270 > - && !m_substituteData.isValid() && !url.isLocalFile(); > + && !m_substituteData.isValid() && !SchemeRegistry::shouldTreatURLSchemeAsLocal(url); Sorry, I gave you a bad review. This function just take the scheme, not the whole URL: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/SchemeRegistry.cpp#L167 > How does the change to MainResourceLoader::continueAfterContentPolicy affect WebArchives?
It should make it possible to load web archives from all "local" URLs, not just file URLs. Normally, "file" is the only local URL scheme, but embedders can add other URL schemes that they want to work the same way as file URLs.
Comment on attachment 142093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142093&action=review >> Source/WebCore/loader/MainResourceLoader.cpp:270 >> + && !m_substituteData.isValid() && !SchemeRegistry::shouldTreatURLSchemeAsLocal(url); > > Sorry, I gave you a bad review. This function just take the scheme, not the whole URL: > > http://trac.webkit.org/browser/trunk/Source/WebCore/platform/SchemeRegistry.cpp#L167 Heh. No, you didn't give me a bad review, I gave you bad code. Clearly I should have run more of the test suite. I'll upload a new patch today. WebArchive behavior change makes me nervous, because these are always dangerous, and I don't understand our local schemes well enough. Perhaps Brady or Jessie could ponder whether it's safe. (In reply to comment #15) > WebArchive behavior change makes me nervous, because these are always dangerous, and I don't understand our local schemes well enough. Perhaps Brady or Jessie could ponder whether it's safe. One easy way to determine whether it's safe is to look at the list of schemes Safari registers as local. I suspect it's just "file" and possibly a scheme for error pages (which won't have any WebArchive resources). Created attachment 142540 [details]
Patch
OK, the latest patch should do the right thing, and the chromium browser tests that failed before are passing now (locally on my machine at least). (In reply to comment #18) > OK, the latest patch should do the right thing, and the chromium browser tests that failed before are passing now (locally on my machine at least). Have we resolved Alexey's concern? To follow the approach in Comment #16, you can just build a debug version of the apple-mac port and set a breakpoint in the function that registers local schemes. If you run that WebKit DLL in Safari, that will let us see what all Safari is registering (In reply to comment #19) > Have we resolved Alexey's concern? To follow the approach in Comment #16, > you can just build a debug version of the apple-mac port and set a > breakpoint in the function that registers local schemes. If you run that > WebKit DLL in Safari, that will let us see what all Safari is registering Well, but that would mean I would have to have a mac with XCode and know how to build the apple-mac version of WebKit. :-) I can learn how to do that, but it'll take me a day or two to set up. If you remind me on Monday, I'd be happy to run that experiment for you. (I'm currently traveling and don't have a build environment.) (In reply to comment #16) > (In reply to comment #15) > > WebArchive behavior change makes me nervous, because these are always dangerous, and I don't understand our local schemes well enough. Perhaps Brady or Jessie could ponder whether it's safe. > > One easy way to determine whether it's safe is to look at the list of schemes Safari registers as local. I suspect it's just "file" and possibly a scheme for error pages (which won't have any WebArchive resources). I checked, and this is not a concern for Safari. Thanks Jessie. Comment on attachment 142540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142540&action=review > Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:106 > // For security reasons we only load MHTML pages from the local file system. I'd change this comment slightly to say "from local URLs" rather than "from the local file system". Created attachment 142756 [details]
Patch
(In reply to comment #24) > I'd change this comment slightly to say "from local URLs" rather than "from the local file system". OK, done. Adam, is there anything else I need to do to commit this? (your "review+" doesn't seem to have done it, and I'm not familiar enough with WebKit submissions to know what's missing). As described in <http://www.webkit.org/coding/contributing.html>, please set "commit-queue?" flag. Comment on attachment 142756 [details]
Patch
As ap says, you can set the commit-queue? flag to ask folks to mark your patch for commit.
Comment on attachment 142756 [details] Patch Clearing flags on attachment: 142756 Committed r118402: <http://trac.webkit.org/changeset/118402> All reviewed patches have been landed. Closing bug. |