Upstream the iOS related changes to WebCore/loader.
Created attachment 219459 [details] Patch
Comment on attachment 219459 [details] Patch Attachment 219459 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/49628249
Created attachment 219475 [details] Patch
Comment on attachment 219475 [details] Patch Attachment 219475 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/45748145
Created attachment 219478 [details] Patch
Comment on attachment 219478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=219478&action=review We really need to work to reduce these PLATFORM(IOS) once we land them. There are way too many and many look questionable. > Source/WebCore/ChangeLog:9 > + * WebCore.exp.in: Added symbols __ZN7WebCore11MemoryCache15addImageToCacheEP7CGImageRKNS_3URLERKN3WTF6StringE > + and __ZN7WebCore11MemoryCache20removeImageFromCacheERKNS_3URLERKN3WTF6StringE. I don’t think listing the mangled symbols here is helpful. > Source/WebCore/loader/DocumentLoader.cpp:106 > + Vector<RefPtr<ResourceLoader>> loadersCopy; > + copyToVector(loaders, loadersCopy); > + size_t size = loadersCopy.size(); > + for (size_t i = 0; i < size; ++i) { > + ResourceHandle* handle = loadersCopy[i]->handle(); I would write this like this: for (auto& loader : loadersCopy) { ResourceHandle* handle = loader->handle(); > Source/WebCore/loader/DocumentWriter.cpp:42 > #include "FrameView.h" > +#if PLATFORM(IOS) > +#include "PDFDocument.h" > +#endif > #include "PlaceholderDocument.h" If the include is going to be conditional like this, please put it in a separate paragraph rather than trying to sort it in the middle of the main includes paragraph. > Source/WebCore/loader/FrameLoader.cpp:156 > static const char defaultAcceptHeader[] = "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8"; > static double storedTimeOfLastCompletedLoad; > +#if PLATFORM(IOS) > +static const int memoryLevelThresholdToPrunePageCache = 20; > +#endif Should add a blank line so the iOS-specific constant is in a separate paragraph. Also, static is not needed here. > Source/WebCore/loader/FrameLoader.cpp:199 > ASSERT(!m_inProgress || m_frame.page()); > - if (m_inProgress) > + if (m_inProgress && m_frame.page()) Why? This contradicts the assertion on the line above? > Source/WebCore/loader/FrameLoader.cpp:288 > + // FIXME: <rdar://problem/9716897> Verify OpenSource r89312 was merged correctly I don’t understand this comment at all. > Source/WebCore/loader/FrameLoader.cpp:289 > + UNUSED_PARAM(url); Huh? > Source/WebCore/loader/FrameLoader.cpp:833 > + // FIXME: <rdar://problem/9716897> Verify OpenSource r89312 was merged correctly Mysterious comment again. Also, two line if body means we should have braces here. > Source/WebCore/loader/FrameLoader.cpp:3586 > FloatRect windowRect = page->chrome().windowRect(); > - FloatSize viewportSize = page->chrome().pageRect().size(); > - > if (features.xSet) > windowRect.setX(features.x); > if (features.ySet) > windowRect.setY(features.y); > + > // Zero width and height mean using default size, not minumum one. > + FloatSize viewportSize = page->chrome().pageRect().size(); Seems strange to change the !IOS code in this patch. > Source/WebCore/loader/ResourceBuffer.h:76 > + void shouldUsePurgeableMemory(bool); A setter should not be named “should”. This could be named setShouldUsePurgeableMemory. Also not clear what is iOS-specific here. > Source/WebCore/loader/SubresourceLoader.cpp:93 > + // <rdar://problem/9121719> - https://bugs.webkit.org/show_bug.cgi?id=56647. This is not a very good comment. > Source/WebCore/loader/SubresourceLoader.h:57 > + virtual const ResourceRequest& iOSOriginalRequest() const OVERRIDE { return m_iOSOriginalRequest; } What the heck is an “iOS” original request? Seems completely wrong to have this. > Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:109 > +#if PLATFORM(IOS) > + SQLiteTransactionInProgressAutoCounter transactionCounter; > +#endif The conditionals should be in the class rather than at every call site. > Source/WebCore/loader/cache/MemoryCache.cpp:68 > +#if PLATFORM(IOS) > + ASSERT(WebThreadIsLockedOrDisabled()); > +#else > ASSERT(WTF::isMainThread()); > +#endif Can we make an assertion that deals with this conditional so we don’t have to have an #if at every call site?
Comment on attachment 219478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=219478&action=review >> Source/WebCore/loader/SubresourceLoader.cpp:93 >> + // <rdar://problem/9121719> - https://bugs.webkit.org/show_bug.cgi?id=56647. > > This is not a very good comment. What about this: "On iOS, do not invoke synchronous resource load delegates while resource load scheduling is disabled to avoid re-entering style selection from a different thread (see <rdar://problem/9121719>). FIXME: This should be fixed for all ports in https://bugs.webkit.org/show_bug.cgi?id=56647." ?
(In reply to comment #7) > "On iOS, do not invoke synchronous resource load delegates while resource load scheduling is disabled to avoid re-entering style selection > from a different thread (see <rdar://problem/9121719>). FIXME: This should be fixed for all ports in https://bugs.webkit.org/show_bug.cgi?id=56647." While it’s a bit wordy, it seems really clear and answers the comment question: “Why?” Great!
Comment on attachment 219478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=219478&action=review >> Source/WebCore/ChangeLog:9 >> + and __ZN7WebCore11MemoryCache20removeImageFromCacheERKNS_3URLERKN3WTF6StringE. > > I don’t think listing the mangled symbols here is helpful. Will update this line to read: * WebCore.exp.in: Added symbols for MemoryCache::{addImageToCache, removeImageFromCache}(). >> Source/WebCore/loader/DocumentLoader.cpp:106 >> + ResourceHandle* handle = loadersCopy[i]->handle(); > > I would write this like this: > > for (auto& loader : loadersCopy) { > ResourceHandle* handle = loader->handle(); Will write using C++11 range-based for loop. >> Source/WebCore/loader/DocumentWriter.cpp:42 >> #include "PlaceholderDocument.h" > > If the include is going to be conditional like this, please put it in a separate paragraph rather than trying to sort it in the middle of the main includes paragraph. Will move conditional include to a separate paragraph. >> Source/WebCore/loader/FrameLoader.cpp:156 >> +#endif > > Should add a blank line so the iOS-specific constant is in a separate paragraph. Also, static is not needed here. Will remove keyword static and add a blank line above PLATFORM(IOS). >> Source/WebCore/loader/FrameLoader.cpp:199 >> + if (m_inProgress && m_frame.page()) > > Why? This contradicts the assertion on the line above? Will revert this change. This changed was originally a speculative fix for <rdar://problem/12798052>. As of the time of writing, we can assume Frame::page() is never null (as Frame takes a Page object as reference on construction, stores it in a pointer, and never nullifies it). On an aside, we should look to change the return type of Frame::Page() from a pointer to a reference and consider changing Frame::m_page to be a reference. >> Source/WebCore/loader/FrameLoader.cpp:288 >> + // FIXME: <rdar://problem/9716897> Verify OpenSource r89312 was merged correctly > > I don’t understand this comment at all. From reading <rdar://problem/9716897> and looking at related radars, I'll update this comment to read: FIXME: We need to initialize the document URL to the specified URL. Currently the URL is empty and hence FrameLoader::checkCompleted() will overwrite the URL of the document to be activeDocumentLoader()->documentURL(). >> Source/WebCore/loader/FrameLoader.cpp:289 >> + UNUSED_PARAM(url); > > Huh? Will remove this line. Instead, will remove the name of the parameter since the argument is unused. See my above remark for the reasoning behind the unused argument. >> Source/WebCore/loader/FrameLoader.cpp:833 >> +#if PLATFORM(IOS) >> + URL url = m_frame.document()->url(); >> + if (url.isEmpty()) >> + // FIXME: <rdar://problem/9716897> Verify OpenSource r89312 was merged correctly > > Mysterious comment again. Also, two line if body means we should have braces here. Will update the comment to read: We need to update the document URL of a PDF document to be non-empty so that both back/forward history navigation between PDF pages and fragment navigation works. See <rdar://problem/9544769> for more details. FIXME: Is there a better place for this code, say DocumentLoader? Also, we should explicitly only update the URL of the document when it's a PDFDocument object instead of assuming that a Document object with an empty URL is a PDFDocument. FIXME: This code is incorrect for a synthesized document (which also has an empty URL). The URL for a synthesized document should be the URL specified to FrameLoader::initForSynthesizedDocument(). >> Source/WebCore/loader/FrameLoader.cpp:3586 >> + FloatSize viewportSize = page->chrome().pageRect().size(); > > Seems strange to change the !IOS code in this patch. Will revert this change. >> Source/WebCore/loader/ResourceBuffer.h:76 >> + void shouldUsePurgeableMemory(bool); > > A setter should not be named “should”. This could be named setShouldUsePurgeableMemory. Also not clear what is iOS-specific here. Will rename to setShouldUsePurgeableMemory(). I will also add a FIXME comment to remove the PLATFORM(IOS)-guard once we upstream the iOS changes to SharedBuffer.{cpp, h} and SharedBufferCF.cpp. >>> Source/WebCore/loader/SubresourceLoader.cpp:93 >>> + // <rdar://problem/9121719> - https://bugs.webkit.org/show_bug.cgi?id=56647. >> >> This is not a very good comment. > > What about this: > > "On iOS, do not invoke synchronous resource load delegates while resource load scheduling is disabled to avoid re-entering style selection > from a different thread (see <rdar://problem/9121719>). FIXME: This should be fixed for all ports in https://bugs.webkit.org/show_bug.cgi?id=56647." > > ? I'll use Benjamin Poulain's suggested comment (see comment #7). >> Source/WebCore/loader/SubresourceLoader.h:57 >> + virtual const ResourceRequest& iOSOriginalRequest() const OVERRIDE { return m_iOSOriginalRequest; } > > What the heck is an “iOS” original request? Seems completely wrong to have this. I'm not sure. When I have a moment I'll look to understand its purpose. I'll add a FIXME comment for now that reads: What is an "iOS" original request? Why is it necessary? >> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:109 >> +#endif > > The conditionals should be in the class rather than at every call site. I agree. I hope you don't mind that I defer fixing this up until after we upstream the file SQLiteDatabaseTracker.h. For now, I'll add a FIXME comment. >> Source/WebCore/loader/cache/MemoryCache.cpp:68 >> +#endif > > Can we make an assertion that deals with this conditional so we don’t have to have an #if at every call site? Will revert this code and all similar code as it's sufficient to use ASSERT(WTF::isMainThread()) because WTF::isMainThread() is defined in terms of WebThreadIsLockedOrDisabled() when building WebKit for iOS. See bug #119644 for more details.
Committed r160841: <http://trac.webkit.org/changeset/160841>
Committed Windows build fix in <http://trac.webkit.org/changeset/160845>.
Comment on attachment 219478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=219478&action=review >>> Source/WebCore/loader/FrameLoader.cpp:199 >>> + if (m_inProgress && m_frame.page()) >> >> Why? This contradicts the assertion on the line above? > > Will revert this change. This changed was originally a speculative fix for <rdar://problem/12798052>. As of the time of writing, we can assume Frame::page() is never null (as Frame takes a Page object as reference on construction, stores it in a pointer, and never nullifies it). On an aside, we should look to change the return type of Frame::Page() from a pointer to a reference and consider changing Frame::m_page to be a reference. I’m worried about reverting this if it was a speculative fix; it might be helping us!