|
Description
Bo Liu
2012-08-30 10:41:29 PDT
Created attachment 161518 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Hi Nate and reviewers, I have not added new tests or verified that this does not break existing tests, so feel free to ignore this for now. This is for implementing Android WebView powered by Chromium stack. The specific method is this one: http://developer.android.com/reference/android/webkit/WebSettings.html#setBlockNetworkImage(boolean) Comment on attachment 161518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161518&action=review LGTM with a nit, though I'd like to give others a chance to comment before approving. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:656 > + KURL kurl = m_document->completeURL(url); > + I believe callers of CachedResourceLoader are required to ensure the url they put into a ResourceRequest is completed, so this line probably isn't necessary. Can you explain what you are really trying to accomplish? BlockNetworkImages sounds like it should not block cached image loads, even for http:// URLs. But, what is implemented here would prevent loading images out of the cache. If this is your desired behavior, then perhaps the feature should be named BlockNonLocalImages? Using the term "Local" seems helpful since it matches up with the corresponding SchemeRegistry call. (In reply to comment #5) > Can you explain what you are really trying to accomplish? BlockNetworkImages sounds like it should not block cached image loads, even for http:// URLs. But, what is implemented here would prevent loading images out of the cache. If this is your desired behavior, then perhaps the feature should be named BlockNonLocalImages? Using the term "Local" seems helpful since it matches up with the corresponding SchemeRegistry call. Oh, I see. This patch does allow loading from the WebCore cache. Nevermind. Created attachment 161547 [details]
Patch
Removed completeURL call in CachedResourceLoader::shouldDeferImageLoad in Patch 2. Can I ask for some advice for testing this? This seems difficult to test with layout tests since the setting cannot be changed from a webpage. I also tried to look for tests for the existing AutoLoadImages but didn't find anything. I just learned about the chromium webkit unit tests and see if this can be a right fit there. Created attachment 161596 [details]
Patch
Added a webkit chromium unit tests in Patch 3 to test the shouldDeferImageLoad logic. This is a unit test on CachedResourceLoader only beacuse WebCore::Settings propagate the setting to CachedResourceLoader through a Timer. Not sure if this test is really worthwhile though. Comment on attachment 161596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161596&action=review > Source/WebCore/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). nit: Please delete this line. > Source/WebCore/ChangeLog:21 > + No new tests (OOPS!). I think it would be great to have a layout test for this setting to ensure end-to-end functionality. You can add this setting into DRTTestRunner and then use it in html/js layout tests similar to 'disableImageLoading' setting. > Source/WebCore/page/Settings.cpp:49 > +static void setloadImagesSettingsInAllFrames(Page* page) Perhaps, should be "setImageLoadingSettings...". > Source/WebCore/page/Settings.cpp:293 > + , m_loadImagesSettingsTimer(this, &Settings::loadImagesSettingsTimerFired) same here Thanks for the review and advice Mikhail, but a question: (In reply to comment #11) > > Source/WebCore/ChangeLog:21 > > + No new tests (OOPS!). > > I think it would be great to have a layout test for this setting to ensure end-to-end functionality. > > You can add this setting into DRTTestRunner and then use it in html/js layout tests similar to 'disableImageLoading' setting. > I did look at DRTTestRunner and was considering adding the setting to overridePreference. But I was under the impression that I would have to add it to TestRunner of all the ports and thought that was too hard. Is there a way limit the test scope to the chromium port only or otherwise a central place to add the preference to test runner of all ports? (In reply to comment #12) > Thanks for the review and advice Mikhail, but a question: > > (In reply to comment #11) > > > Source/WebCore/ChangeLog:21 > > > + No new tests (OOPS!). > > > > I think it would be great to have a layout test for this setting to ensure end-to-end functionality. > > > > You can add this setting into DRTTestRunner and then use it in html/js layout tests similar to 'disableImageLoading' setting. > > > > I did look at DRTTestRunner and was considering adding the setting to overridePreference. But I was under the impression that I would have to add it to TestRunner of all the ports and thought that was too hard. Is there a way limit the test scope to the chromium port only or otherwise a central place to add the preference to test runner of all ports? It seems that adding for mac (this is the "main" WebKit) and chromium ports should be enough, not sure about WebKit2. This is an example of adding a new preference for DRT: https://bugs.webkit.org/show_bug.cgi?id=78525 It sounds like this setting only affects behavior of images in local documents, is that accurate? Perhaps that should be more explicit in comments and naming. Also, what about network loader cache? We even have a willLoadFromCache method that predicts (with a possibility of mistake) whether network will be hit for a particular resource load. We previously discussed that our multiple subtly different settings for image loading should be simplified, so it's sad to see another one being added, especially with a somewhat misleading name. (In reply to comment #14) > It sounds like this setting only affects behavior of images in local documents, is that accurate? Perhaps that should be more explicit in comments and naming. > No, this setting affects images with network schemes, regardless of the source of the document. > Also, what about network loader cache? We even have a willLoadFromCache method that predicts (with a possibility of mistake) whether network will be hit for a particular resource load. I'm sorry, I am not familiar with this. I just checked the Chromium ResourceHandle::willLoadFromCache and it simply returns true. Is this still relevant regarding to the answer to your first question? > > We previously discussed that our multiple subtly different settings for image loading should be simplified, so it's sad to see another one being added, especially with a somewhat misleading name. I am happy to do the work if you could point me to which settings need to be simplified. One of the requirements for this change is that the blocked images are reloaded in place when the setting changes, so that's why CachedResourceLoader::canRequest is not used. (In reply to comment #15) > (In reply to comment #14) > > It sounds like this setting only affects behavior of images in local documents, is that accurate? Perhaps that should be more explicit in comments and naming. > > > > No, this setting affects images with network schemes, regardless of the source of the document. I misspoke a bit. What I was referring to was that non-local documents can only load non-local images, so the new preference is not different from what we already have for those. > I'm sorry, I am not familiar with this. I just checked the Chromium ResourceHandle::willLoadFromCache and it simply returns true. Is this still relevant regarding to the answer to your first question? Yes - other ports implement willLoadFromCache accurately. (In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > It sounds like this setting only affects behavior of images in local documents, is that accurate? Perhaps that should be more explicit in comments and naming. > > > > > > > No, this setting affects images with network schemes, regardless of the source of the document. > > I misspoke a bit. What I was referring to was that non-local documents can only load non-local images, so the new preference is not different from what we already have for those. This change is used in chromium port for android. In Android, there is the additional content: scheme that should be considered local, which I have to double check. If that is the case, yes, this will only affect local documents. I can update the comments in the next patch. Do you have a suggestion for a better name? Also see below. > > > I'm sorry, I am not familiar with this. I just checked the Chromium ResourceHandle::willLoadFromCache and it simply returns true. Is this still relevant regarding to the answer to your first question? > > Yes - other ports implement willLoadFromCache accurately. If I understand what willLoadFromCache does: it checks if the load will come from the network stack cache of the different ports? So are you suggesting using willLoadFromCache instead of SchemeRegistry::shouldTreatURLSchemeAsLocal? As Darin has pointed out, the new setting should not affect loads that do not need revalidate and is retrieved straight from WebCore::MemoryCache. This change is to support an android public java api used by apps. The implementation in the current android webkit fork only checks schemes and does not check the network cache. To maintain compatibility, this change should not need to check willLoadFromCache. So I see why this variable is badly named. Perhaps name something to: BlockNonLocalNonWebCoreCachedImages? Or change to positive: AllowOnlyWebCoreCachedOrLocalImages? (In reply to comment #14) > It sounds like this setting only affects behavior of images in local documents, is that accurate? Perhaps that should be more explicit in comments and naming. > > Also, what about network loader cache? We even have a willLoadFromCache method that predicts (with a possibility of mistake) whether network will be hit for a particular resource load. willLoadFromCache is not implemented for Chromium. It would be too expensive to implement properly. There would also be a race condition if we were to attempt to implement it. Chromium implementation is here: http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/network/chromium/ResourceHandle.cpp&exact_package=chromium&q=willLoadFromCache&type=cs&l=274 The only caller is in FrameLoader, which has a comment above it explaining the hackiness of the approach: // FIXME: Slight hack to test if the NSURL cache contains the page we're going to. // We want to know this before talking to the policy delegate, since it affects whether // we show the DoYouReallyWantToRepost nag. // // This trick has a small bug (3123893) where we might find a cache hit, but then // have the item vanish when we try to use it in the ensuing nav. This should be // extremely rare, but in that case the user will get an error on the navigation. Chromium has a more robust solution to this problem that is based on the FormData::identifier() field. Created attachment 161794 [details]
Added layout tests. Changed timer name in WebCore::Settings. Added description about documents. Have not renamed the setting yet
Comment on attachment 161794 [details] Added layout tests. Changed timer name in WebCore::Settings. Added description about documents. Have not renamed the setting yet View in context: https://bugs.webkit.org/attachment.cgi?id=161794&action=review > Source/WebCore/ChangeLog:11 > + documents since non-local documents cannot access local resources due Not sure the "since" part really matters, as it talks about local resources which are not affected by the switch. Perhaps, remove it? I guess, your intention is to say that this feature is primarily intended for local documents. As for network documents, I think it actually has the same effect as disabling images loading at all, since network documents can only access network images, not local ones. > Source/WebCore/ChangeLog:21 > + No new tests (OOPS!). I'm surprised you are still having this line. Perhaps, you should re-run prepare-ChangeLog? > LayoutTests/fast/loader/file-image-allowed-with-network-image-blocked.html:18 > + Use resource load callbacks to make sure the image is not loaded.<br> Why do you need this sentence? Page text is intended for a person who may not know much about the test itself (e.g. this can be a person who does a test results update), but needs to verify that the test has passed. I would suggest to write something like "The test passes if the image is loaded". Bonus points if you can do this yourself programmatically and display "PASS" or "FAIL" as a result. > Chromium has a more robust solution to this problem that is based on the FormData::identifier() field.
That's good to know, but I don't see how this helps to move forward with this patch. It still adds a setting saying something about network loads while making no attempt to relate that to actual network loads.
(In reply to comment #21) > > Chromium has a more robust solution to this problem that is based on the FormData::identifier() field. > > That's good to know, but I don't see how this helps to move forward with this patch. It still adds a setting saying something about network loads while making no attempt to relate that to actual network loads. (In reply to comment #21) > > Chromium has a more robust solution to this problem that is based on the FormData::identifier() field. > > That's good to know, but I don't see how this helps to move forward with this patch. It still adds a setting saying something about network loads while making no attempt to relate that to actual network loads. I think as this is a naming issue. As I stated previously, this patch should not check the network cache. Would "BlockNonLocalImages" be better? Created attachment 162048 [details]
Renamed to BlockNonLocalImages. Add overridePreference to mac DumpRenderTree runner, still need to fix expect.txt files for mac. Updated ChangeLog. Removed chromium unit tests.
(In reply to comment #20) > > LayoutTests/fast/loader/file-image-allowed-with-network-image-blocked.html:18 > > + Use resource load callbacks to make sure the image is not loaded.<br> > > Why do you need this sentence? > > Page text is intended for a person who may not know much about the test itself (e.g. this can be a person who does a test results update), but needs to verify that the test has passed. I would suggest to write something like "The test passes if the image is loaded". Bonus points if you can do this yourself programmatically and display "PASS" or "FAIL" as a result. I have not found a way to test whether the images are loaded from the html/js itself, so the test uses the resource load callbacks output to verify if the images in the page are loaded or not. I have tried the img onload attribute and it is fired regardless of if the image is blocked. Ideally it should be able to detect whether the image loaded since right now the expect.txt files do not match on mac. Will try to get this, any pointers welcome. (In reply to comment #24) > (In reply to comment #20) > > > LayoutTests/fast/loader/file-image-allowed-with-network-image-blocked.html:18 > > > + Use resource load callbacks to make sure the image is not loaded.<br> > > > > Why do you need this sentence? > > > > Page text is intended for a person who may not know much about the test itself (e.g. this can be a person who does a test results update), but needs to verify that the test has passed. I would suggest to write something like "The test passes if the image is loaded". Bonus points if you can do this yourself programmatically and display "PASS" or "FAIL" as a result. > > I have not found a way to test whether the images are loaded from the html/js itself, so the test uses the resource load callbacks output to verify if the images in the page are loaded or not. Ah, so by "resource load callbacks" you mean test server output. I was imaging "onload" event handlers. Sorry for misunderstanding. I guess, you should write something like "use test server log messages" to remove this uncertainty. > I have tried the img onload attribute and it is fired regardless of if the image is blocked. Ideally it should be able to detect whether the image loaded since right now the expect.txt files do not match on mac. Will try to get this, any pointers welcome. I use image element's 'naturalHeight' property. If the image wasn't loaded, the height is 0. There is a bug https://bugs.webkit.org/show_bug.cgi?id=71661 about the image's onload handler behaviour. Comment on attachment 162048 [details] Renamed to BlockNonLocalImages. Add overridePreference to mac DumpRenderTree runner, still need to fix expect.txt files for mac. Updated ChangeLog. Removed chromium unit tests. Attachment 162048 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13744564 New failing tests: http/tests/misc/blocked-image-reloaded-when-image-reenabled.html http/tests/misc/image-load-blocked-with-images-disabled.html Created attachment 162113 [details]
Rewrite layout tests to use naturalHeight/Width to detech image load. Fix mac WebPreferences.
Comment on attachment 162113 [details] Rewrite layout tests to use naturalHeight/Width to detech image load. Fix mac WebPreferences. View in context: https://bugs.webkit.org/attachment.cgi?id=162113&action=review > LayoutTests/fast/loader/display-image-unset-can-block-image-and-can-reload-in-place.html:21 > + window.setTimeout(runTestRest, 1000); Setting a 1 second delay in a layout test isn't a good approach. I realize that you can't use image's onload because of the bug, but why not using a setInterval for periodic checking and finishing the test as early as possible? > LayoutTests/fast/loader/display-image-unset-can-block-image-and-can-reload-in-place.html:36 > + <div id="result">FAILURE</div> I suggest putting "FAILURE: script did't run" here, and setting "FAILURE: <cause>" from the script in the case of failure. Created attachment 162310 [details]
Improve layout tests as suggested by Mikhail
Comment on attachment 162310 [details]
Improve layout tests as suggested by Mikhail
Looks good to me, need a reviewer to approve.
Comment on attachment 162310 [details]
Improve layout tests as suggested by Mikhail
LGTM, will let a chromium api reviewer actually r+
Perhaps this has already been discussed, but is there a reason WebPermissionClient::allowImage is insufficient for your use case? http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/public/WebPermissionClient.h#L50 (In reply to comment #32) > Perhaps this has already been discussed, but is there a reason WebPermissionClient::allowImage is insufficient for your use case? > > http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/public/WebPermissionClient.h#L50 The reason allowImage is not used here is because for this android webview api, the blocked images needs to be reloaded in-place when the setting is flipped to unblock. Example usage is in the android gmail app that hides the images in an email by default and then reloads them when user clicks show images button. > the blocked images needs to be reloaded in-place when the setting is flipped to unblock.
Perhaps we should add that feature to allowImage rather than inventing yet-another-way of blocking images?
Created attachment 163163 [details]
Add in-place reload to ImagesEnabled instead. No new settings now.
(In reply to comment #35) > Created an attachment (id=163163) [details] > Add in-place reload to ImagesEnabled instead. No new settings now. Needed to modify CachedResourceLoader::determineRevalidationPolicy to make sure that when ImagesEnabled is false, images are not loaded from the memory cache. This solution is not ideal since an existing image is evicted from the cache. But presumably this setting is not toggled frequently in existing ports so it might be okay? Comment on attachment 163163 [details] Add in-place reload to ImagesEnabled instead. No new settings now. Attachment 163163 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13802860 (In reply to comment #37) > (From update of attachment 163163 [details]) > Attachment 163163 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/13802860 I couldn't reproduce these errors with on my local linux chromium debug build, but it seems like all failed tests are image related. I can't see the whole command, but here is the command I used: time xvfb-run third_party/WebKit/Tools/Scripts/new-run-webkit-tests --chromium --skip-failing-tests --no-show-results --test-list=/tmp/list2 --debug Is there anything else I should try to try to reproduce these errors? Comment on attachment 163163 [details] Add in-place reload to ImagesEnabled instead. No new settings now. View in context: https://bugs.webkit.org/attachment.cgi?id=163163&action=review > Source/WebCore/loader/FrameLoader.cpp:649 > + m_frame->document()->cachedResourceLoader()->setImagesEnabled(settings ? settings->areImagesEnabled() : true); !settings || settings->areImagesEnabled() is a slightly cleaner idiom here. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:658 > + if (!m_autoLoadImages) > + return true; > + > + return false; return !m_autoLoadImages is a slightly cleaner idiom here. Comment on attachment 163163 [details] Add in-place reload to ImagesEnabled instead. No new settings now. View in context: https://bugs.webkit.org/attachment.cgi?id=163163&action=review > Source/WebKit/mac/WebView/WebPreferences.mm:774 > +- (void)setImagesEnabled: (BOOL)flag > +{ > + [self _setBoolValue: flag forKey: WebKitImagesEnabledKey]; > +} > + > +- (BOOL)imagesEnabled > +{ > + return [self _boolValueForKey: WebKitImagesEnabledKey]; > +} You can actually skip all this apple-mac specific API work by using InternalSettings.cpp for the tests. It's generally better to avoid changing public APIs for other ports. That way you can also avoid making any Chromium-specific changes to DumpRenderTree. Comment on attachment 163163 [details]
Add in-place reload to ImagesEnabled instead. No new settings now.
Does this patch work properly with redirects? We make sure to call CachedResourceLoader::canRequest for each hop in the redirect chain, but I'm not sure if the place you moved the allowImage call to gets called on redirects. Can you add a test for that case?
Created attachment 163741 [details]
Changes suggested by Adam Barth: Condition idioms, use InternalSetting for tests, add simple test for redirect
(In reply to comment #41) > (From update of attachment 163163 [details]) > Does this patch work properly with redirects? We make sure to call CachedResourceLoader::canRequest for each hop in the redirect chain, but I'm not sure if the place you moved the allowImage call to gets called on redirects. Can you add a test for that case? Adam: I could not find a *separate* CachedResourceLoader::canRequest call for redirects, so I assume a redirect happens in the same path as the normal load. I added a test for blocking/unblocking a redirected image, but I'm not sure if this is enough since it does not test the case where the allowImage decision differs for the original and redirected requests. I'll look into testing that case if you think it is important. Comment on attachment 163741 [details] Changes suggested by Adam Barth: Condition idioms, use InternalSetting for tests, add simple test for redirect Attachment 163741 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13846042 Comment on attachment 163741 [details] Changes suggested by Adam Barth: Condition idioms, use InternalSetting for tests, add simple test for redirect Attachment 163741 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13849158 New failing tests: platform/chromium/http/tests/permissionclient/image-permissions.html platform/chromium/permissionclient/image-permissions.html Created attachment 163979 [details]
Do not trigger unload when image load is deferred. Fix redirect allowImage call. Fix image-permission tests. Check onload behavior in new tests. Fix gtk build. (Maybe?) fix win build.
(In reply to comment #46) > Created an attachment (id=163979) [details] > Do not trigger unload when image load is deferred. Fix redirect allowImage call. Fix image-permission tests. Check onload behavior in new tests. Fix gtk build. (Maybe?) fix win build. Stole a few lines from https://bugs.webkit.org/show_bug.cgi?id=71661 to fix onload being fired even when image load is deferred. Right now the behavior is neither onload or onerror fires if image load is deferred. Comment on attachment 163979 [details] Do not trigger unload when image load is deferred. Fix redirect allowImage call. Fix image-permission tests. Check onload behavior in new tests. Fix gtk build. (Maybe?) fix win build. View in context: https://bugs.webkit.org/attachment.cgi?id=163979&action=review > Source/WebCore/loader/SubresourceLoader.cpp:170 > + if (m_document->cachedResourceLoader()->canRequest(m_resource->type(), newRequest.url())) Multi-line if branches must be bracketed > Source/WebCore/loader/cache/CachedResourceLoader.cpp:651 > + if (frame()) Why not to merge these two ifs? > > Does this patch work properly with redirects? We make sure to call CachedResourceLoader::canRequest for each hop in the redirect chain, but I'm not sure if the place you moved the allowImage call to gets called on redirects. Can you add a test for that case?
>
> Adam: I could not find a *separate* CachedResourceLoader::canRequest call for redirects, so I assume a redirect happens in the same path as the normal load. I added a test for blocking/unblocking a redirected image, but I'm not sure if this is enough since it does not test the case where the allowImage decision differs for the original and redirected requests. I'll look into testing that case if you think it is important.
jochen might recall off-hand if we already have a test for allowImage and redirects. If not we, should probably add one.
(In reply to comment #49) > > > Does this patch work properly with redirects? We make sure to call CachedResourceLoader::canRequest for each hop in the redirect chain, but I'm not sure if the place you moved the allowImage call to gets called on redirects. Can you add a test for that case? > > > > Adam: I could not find a *separate* CachedResourceLoader::canRequest call for redirects, so I assume a redirect happens in the same path as the normal load. I added a test for blocking/unblocking a redirected image, but I'm not sure if this is enough since it does not test the case where the allowImage decision differs for the original and redirected requests. I'll look into testing that case if you think it is important. > > jochen might recall off-hand if we already have a test for allowImage and redirects. If not we, should probably add one. Sorry, forgot to update this. The test is image-permissions needed updates in the latest patch. And it did fail legitimately and point out an error in the previous patch. Comment on attachment 163979 [details] Do not trigger unload when image load is deferred. Fix redirect allowImage call. Fix image-permission tests. Check onload behavior in new tests. Fix gtk build. (Maybe?) fix win build. View in context: https://bugs.webkit.org/attachment.cgi?id=163979&action=review > Source/WebCore/loader/SubresourceLoader.cpp:173 > + if (m_resource->type() != CachedResource::ImageResource || !m_document->cachedResourceLoader()->shouldDeferImageLoad(newRequest.url())) { > + m_resource->willSendRequest(newRequest, redirectResponse); > + return; Yeah, this is the line that's causing us to handle redirects properly. willSendRequest is called for each redirect. I missed this in the patch before. I bet we already have a test for allowImage and redirects. If we realize that we don't, we can add one in a followup patch. > Source/WebCore/testing/InternalSettings.cpp:650 > + settings()->setImagesEnabled(enabled); You also need some code to restore the original value of the setting otherwise it will stick from one test to another. See InternalSettings::Backup::restoreTo. > LayoutTests/fast/loader/display-image-unset-can-block-image-and-can-reload-in-place.html:24 > + testRunner.overridePreference('WebKitDisplayImagesKey', 1); Don't you want to change these to use internals? > LayoutTests/fast/loader/display-image-unset-can-block-image-and-can-reload-in-place.html:27 > + intervalId = window.setInterval(runTestRest, 10); We need to poll for the load? Does that mean the image load events don't fire when re-enabling images? > LayoutTests/fast/loader/images-enabled-unset-can-block-image-and-can-reload-in-place.html:7 > +var image_onload_fired = 0; image_onload_fired -> weUsuallyUseCamelCase > LayoutTests/fast/loader/images-enabled-unset-can-block-image-and-can-reload-in-place.html:11 > + testRunner.overridePreference('WebKitDisplayImagesKey', 1); Presumably this isn't needed anymore. Comment on attachment 163979 [details]
Do not trigger unload when image load is deferred. Fix redirect allowImage call. Fix image-permission tests. Check onload behavior in new tests. Fix gtk build. (Maybe?) fix win build.
This is very close. We just need to clean up the testing side slightly.
Comment on attachment 163979 [details] Do not trigger unload when image load is deferred. Fix redirect allowImage call. Fix image-permission tests. Check onload behavior in new tests. Fix gtk build. (Maybe?) fix win build. View in context: https://bugs.webkit.org/attachment.cgi?id=163979&action=review > Source/WebKit2/ChangeLog:1 > +2012-09-13 Bo Liu <boliu@chromium.org> Looks like you don't need this changelog entry anymore. Comment on attachment 163979 [details] Do not trigger unload when image load is deferred. Fix redirect allowImage call. Fix image-permission tests. Check onload behavior in new tests. Fix gtk build. (Maybe?) fix win build. View in context: https://bugs.webkit.org/attachment.cgi?id=163979&action=review >> Source/WebKit2/ChangeLog:1 >> +2012-09-13 Bo Liu <boliu@chromium.org> > > Looks like you don't need this changelog entry anymore. Actually you do. Nevermind. ;) Created attachment 164190 [details]
Restore InternalSettings. Style fixes.
Comment on attachment 164190 [details]
Restore InternalSettings. Style fixes.
Thanks!
Comment on attachment 164190 [details] Restore InternalSettings. Style fixes. Rejecting attachment 164190 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: commit-queue/Source/WebKit/chromium/third_party/v8-i18n --revision 117 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 48>At revision 117. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/13854517 Created attachment 164197 [details]
Rebase. Had conflict in InternalSettings
Comment on attachment 164197 [details] Rebase. Had conflict in InternalSettings Clearing flags on attachment: 164197 Committed r128645: <http://trac.webkit.org/changeset/128645> All reviewed patches have been landed. Closing bug. *** Bug 71661 has been marked as a duplicate of this bug. *** |