Bug 125879 – [iOS] Upstream WebCore/loader changes

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Daniel Bates]

Upstream the iOS related changes to WebCore/loader.

[Daniel Bates]

Created attachment 219459 [details]
Patch

[EFL EWS Bot]

Comment on attachment 219459 [details]
Patch

Attachment 219459 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/49628249

[Daniel Bates]

Created attachment 219475 [details]
Patch

[EFL EWS Bot]

Comment on attachment 219475 [details]
Patch

Attachment 219475 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/45748145

[Daniel Bates]

Created attachment 219478 [details]
Patch

[Darin Adler]

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?

[Benjamin Poulain]

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."

?

[Darin Adler]

(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!

[Daniel Bates]

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.

[Daniel Bates]

Committed r160841: <http://trac.webkit.org/changeset/160841>

[Daniel Bates]

Committed Windows build fix in <http://trac.webkit.org/changeset/160845>.

[Darin Adler]

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!