[webkit-reviews] review granted: [Bug 125879] [iOS] Upstream WebCore/loader changes : [Attachment 219478] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 125879: [iOS] Upstream WebCore/loader changes
https://bugs.webkit.org/show_bug.cgi?id=125879

Attachment 219478: Patch
https://bugs.webkit.org/attachment.cgi?id=219478&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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?