Bug 73918 – [Qt][WK2] Compute and set cache capacities using the current CacheModel

• Component: WebKit Qt
• Status: RESOLVED FIXED

[zalan]

Implement WebProcess::platformSetCacheModel() for Qt and expose CacheModel API so that host apps, such as browser, can set it properly.

[Michael Brüning]

Created attachment 124551 [details]
First patch.

First patch for implementation of the patch model. It adds the implementation for Qt port of WebKit2 for Mac and Linux systems.

Next patches to be made are the implementation for Qt/WK2 on Windows, the implementation of hybrid memory and disk cache and the implementation for the WK2/Qt Quick API.

[WebKit Review Bot]

Attachment 124551 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:85:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:95:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:100:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:131:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Brüning]

Comment on attachment 124551 [details]
First patch.

Found style issues that I'll fix first. Sorry for that.

[Michael Brüning]

Created attachment 124553 [details]
First patch (updated)

Fixed style issues.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 124553 [details]
First patch (updated)

View in context: https://bugs.webkit.org/attachment.cgi?id=124553&action=review

Hello Michael, I have some suggestions and a question. :-)

> Source/WebKit2/UIProcess/qt/WebContextQt.cpp:60
> +static QString defaultDiskCacheDirectory()

I think this could be String (from WTF) instead of QString. Both functions you call here in WebCore return String and this function is used to fill a String too.

> Source/WebKit2/UIProcess/qt/WebContextQt.cpp:65
> +    if (!s_defaulDiskCacheDirectory.isEmpty())
> +        return s_defaulDiskCacheDirectory;

In case of lazy initialization I tend to write down the initialization block here, and after the conditional, one return. Just a minor suggestion, feel free to ignore.

> Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:109
> +    // The Mac port of WebKit2 uses a fudge factor of 1000 here to account for misalignment, howver,

Typo: "however".

> Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:112
> +    qint64 freeMemory = memorySize() / 1024 / 1023;

If I understood correctly memorySize() calculates the full physical memory size, not only the free memory. I think this is confusing.

Also freeMemory must be understood in MB? If so the conversion is confusing to me: it will give us more memory than the physical, or am I reading something wrong?

[Michael Brüning]

(In reply to comment #5)
> (From update of attachment 124553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124553&action=review
> 
> Hello Michael, I have some suggestions and a question. :-)

Thanks for the suggestions :).
> 
> > Source/WebKit2/UIProcess/qt/WebContextQt.cpp:60
> > +static QString defaultDiskCacheDirectory()
> 
> I think this could be String (from WTF) instead of QString. Both functions you call here in WebCore return String and this function is used to fill a String too.

Okay, true - I just did this to stay inline with the other methods in WebContextQt.cpp, they also use QString.

> 
> > Source/WebKit2/UIProcess/qt/WebContextQt.cpp:65
> > +    if (!s_defaulDiskCacheDirectory.isEmpty())
> > +        return s_defaulDiskCacheDirectory;
> 
> In case of lazy initialization I tend to write down the initialization block here, and after the conditional, one return. Just a minor suggestion, feel free to ignore.

Yeah, actually, I would have done the same, but the other methods also do it this way and I think the WebKit style guide says that one should do early returns. 

> 
> > Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:109
> > +    // The Mac port of WebKit2 uses a fudge factor of 1000 here to account for misalignment, howver,
> 
> Typo: "however".
Nice catch, will fix :)
> 
> > Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:112
> > +    qint64 freeMemory = memorySize() / 1024 / 1023;
> 
> If I understood correctly memorySize() calculates the full physical memory size, not only the free memory. I think this is confusing.

Yeah, I forgot to rename since I misinterpreted what was needed for the calcualation. I'll rename it.
> 
> Also freeMemory must be understood in MB? If so the conversion is confusing to me: it will give us more memory than the physical, or am I reading something wrong?
The fudge factor (inspired from the Mac version) is basically there to account the fact that memory might not necessarily aligned to a full Megabyte. This is probably more important for the free disk space than for the memory, but we can also remove it completely.

[Kenneth Rohde Christiansen]

Comment on attachment 124553 [details]
First patch (updated)

View in context: https://bugs.webkit.org/attachment.cgi?id=124553&action=review

How does all of this tie in with the MemoryPressure (or what is its name) class in WebKit?

> Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:65
> +    // Most of this part was copied from the Mac port (see Source/WebKit2/WebProcess/mac/WebProcessMac.mm for details).
> +    static host_basic_info_data_t hostInfo;

Can't it be shared instead?

[Michael Brüning]

(In reply to comment #7)
> (From update of attachment 124553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124553&action=review
> 
> How does all of this tie in with the MemoryPressure (or what is its name) class in WebKit?
It calculates the limits that the MemoryPressureHandler and the MemoryCache will have for operation.
> 
> > Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:65
> > +    // Most of this part was copied from the Mac port (see Source/WebKit2/WebProcess/mac/WebProcessMac.mm for details).
> > +    static host_basic_info_data_t hostInfo;
> 
> Can't it be shared instead?
Sure, we could make the Mac part a shared private method in the WebProcess if that's what you meant.

[Michael Brüning]

Created attachment 124699 [details]
First patch (updated with suggestions).

Updated the patch with suggestions from Caio Marcelo and Kenneth.

[WebKit Review Bot]

Attachment 124699 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Brüning]

(In reply to comment #10)
> Attachment 124699 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9
> 
> Updating OpenSource
> First, rewinding head to replay your work on top of it...
> Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
> Using index info to reconstruct a base tree...
> Falling back to patching base and 3-way merge...
> Auto-merging LayoutTests/ChangeLog
> CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
> Auto-merging LayoutTests/platform/qt/Skipped
> CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
> Auto-merging Source/WebCore/ChangeLog
> CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
> Failed to merge in the changes.
> Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.
> 
> When you have resolved this problem run "git rebase --continue".
> If you would prefer to skip this patch, instead run "git rebase --skip".
> To restore the original branch and stop rebasing run "git rebase --abort".
> 
> rebase refs/remotes/origin/master: command returned error: 1
> 
> Died at Tools/Scripts/update-webkit line 164.
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Seems as if either the style-commit-queue is hosed or it has problems with git patches. Anyway, none of the changes I made touched any if these files...

[Kenneth Rohde Christiansen]

Comment on attachment 124699 [details]
First patch (updated with suggestions).

View in context: https://bugs.webkit.org/attachment.cgi?id=124699&action=review

> Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:69
> +        long pageSize = sysconf(_SC_PAGESIZE);
> +        long numberOfPages = sysconf(_SC_PHYS_PAGES);
> +
> +        if (pageSize > 0 && numberOfPages > 0)
> +            s_physicalMemorySize = static_cast<qint64>(pageSize) * static_cast<qint64>(numberOfPages);

why not turn this into a memorySize method for UNIX platforms?

Would be great to have a class for all of these, so they can be shared better.

like PlatformInfoMac, PlatformInfoUNIX etc

> Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:73
> +    // FIXME: Find a correct implementation of this for QtWebKit on Windows.

Currently this is not just windows... could be QNX for instance.

[Michael Brüning]

Created attachment 125140 [details]
Updated patch.

Added the Platform class plus implementation for unix/posix, windows and mac.

[Michael Brüning]

Created attachment 125310 [details]
Updated patch.

Fixed build problem for Qt port on Mac.

[Kenneth Rohde Christiansen]

Comment on attachment 125310 [details]
Updated patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=125310&action=review

> Source/WebCore/platform/PlatformInfo.cpp:31
> +PlatformInfo* platformInfo()

I would prefer something like PlatformInfo::instance() but i am not sure of the pattern used in webkit

> Source/WebCore/platform/PlatformInfo.cpp:33
> +    static PlatformInfo* staticInfo = new PlatformInfo;

I would prefer the name 'instance'

> Source/WebCore/platform/mac/PlatformInfoMac.cpp:36
> +uint64_t PlatformInfo::memorySize()

so is this available memory, total memory, is it in mega bytes, bytes etc? I think the name could be better

> Source/WebCore/platform/qt/FileSystemQt.cpp:194
> +uint64_t getVolumeFreeSizeForPath(const char* path)

Size? in bytes? mega bytes? 

availableBytesOnVolumeAtPath ? :-)

[Michael Brüning]

(In reply to comment #15)
> (From update of attachment 125310 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125310&action=review
> 
> > Source/WebCore/platform/PlatformInfo.cpp:31
> > +PlatformInfo* platformInfo()
> 
> I would prefer something like PlatformInfo::instance() but i am not sure of the pattern used in webkit
> 
> > Source/WebCore/platform/PlatformInfo.cpp:33
> > +    static PlatformInfo* staticInfo = new PlatformInfo;
> 
> I would prefer the name 'instance'
> 
> > Source/WebCore/platform/mac/PlatformInfoMac.cpp:36
> > +uint64_t PlatformInfo::memorySize()
> 
> so is this available memory, total memory, is it in mega bytes, bytes etc? I think the name could be better

I agree - I just copied the name from the Mac and GTK WebProcess implementation for consistency / for possible use by those ports in the future. Should we stay consistent with those or should we just change the name?

> 
> > Source/WebCore/platform/qt/FileSystemQt.cpp:194
> > +uint64_t getVolumeFreeSizeForPath(const char* path)
> 
> Size? in bytes? mega bytes? 
> 
> availableBytesOnVolumeAtPath ? :-)

This is a method that was already implemented on the GTK port. Should we change the name and also refactor the GTK code?

[Michael Brüning]

Created attachment 125626 [details]
Patch

[Michael Brüning]

Created attachment 125627 [details]
Patch

[Michael Brüning]

Created attachment 125629 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 125629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125629&action=review

> Source/WebKit2/UIProcess/qt/WebContextQt.cpp:62
> +    static String s_defaulDiskCacheDirectory;

spelling error

> Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:74
> +    unsigned cacheTotalCapacity = 0;
> +    unsigned cacheMinDeadCapacity = 0;
> +    unsigned cacheMaxDeadCapacity = 0;
> +    double deadDecodedDataDeletionInterval = 0;
> +    unsigned pageCacheCapacity = 0;
> +    unsigned long urlCacheMemoryCapacity = 0;
> +    unsigned long urlCacheDiskCapacity = 0;

why do you need to initialize these to 0? Doesn't the calculateCacheSizes aways set the values?

> Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:104
> +    QNetworkDiskCache* diskCache = new QNetworkDiskCache();

does the network access manager take over ownership?

[Michael Brüning]

(In reply to comment #20)
> (From update of attachment 125629 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125629&action=review
> 
> > Source/WebKit2/UIProcess/qt/WebContextQt.cpp:62
> > +    static String s_defaulDiskCacheDirectory;
> 
> spelling error
> 
> > Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:74
> > +    unsigned cacheTotalCapacity = 0;
> > +    unsigned cacheMinDeadCapacity = 0;
> > +    unsigned cacheMaxDeadCapacity = 0;
> > +    double deadDecodedDataDeletionInterval = 0;
> > +    unsigned pageCacheCapacity = 0;
> > +    unsigned long urlCacheMemoryCapacity = 0;
> > +    unsigned long urlCacheDiskCapacity = 0;
> 
> why do you need to initialize these to 0? Doesn't the calculateCacheSizes aways set the values?
> 
> > Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:104
> > +    QNetworkDiskCache* diskCache = new QNetworkDiskCache();
> 
> does the network access manager take over ownership?

(In reply to comment #20)
> (From update of attachment 125629 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125629&action=review
> 
> > Source/WebKit2/UIProcess/qt/WebContextQt.cpp:62
> > +    static String s_defaulDiskCacheDirectory;
> 
> spelling error
Good catch :)
> 
> > Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:74
> > +    unsigned cacheTotalCapacity = 0;
> > +    unsigned cacheMinDeadCapacity = 0;
> > +    unsigned cacheMaxDeadCapacity = 0;
> > +    double deadDecodedDataDeletionInterval = 0;
> > +    unsigned pageCacheCapacity = 0;
> > +    unsigned long urlCacheMemoryCapacity = 0;
> > +    unsigned long urlCacheDiskCapacity = 0;
> 
> why do you need to initialize these to 0? Doesn't the calculateCacheSizes aways set the values?

It's basically what all the other ports do. Anyway, I checked and it seems there is one case where cacheTotalCapacity does not get set (that happens if the physical memory of the device is smaller than 512 MB) and subsequently, cacheMinDeadCapacity and CacheMaxDeadCapacity might not be set correctly.

We could fix this and move the initialization to 0 to WebProcess::calculateCacheSizes, but I don't think the initialization's here hurt as they will probably get optimised out anyway. I agree that initializing here is unnecessary if everything gets reset anyway, but is it against some Webkit guideline to do this? 

> 
> > Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:104
> > +    QNetworkDiskCache* diskCache = new QNetworkDiskCache();
> 
> does the network access manager take over ownership?
Yes, it does.

[Michael Brüning]

Created attachment 125640 [details]
Patch

[Michael Brüning]

Created attachment 125660 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 125660 [details]
Patch

OK, lets go with this for now.

[Michael Brüning]

Comment on attachment 125660 [details]
Patch

I'll follow up with the PlatformInfo patch and cc Anders or Kling.

[WebKit Review Bot]

Comment on attachment 125660 [details]
Patch

Clearing flags on attachment: 125660

Committed r106920: <http://trac.webkit.org/changeset/106920>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #26)
> (From update of attachment 125660 [details])
> Clearing flags on attachment: 125660
> 
> Committed r106920: <http://trac.webkit.org/changeset/106920>

Reopen, because it broke the Qt Windows build:
../../../../Source/WebCore/platform/qt/FileSystemQt.cpp:42:25: fatal error: sys/statvfs.h: No such file or directory

Could you fix it?

[Michael Brüning]

Created attachment 125809 [details]
Patch

[Michael Brüning]

Comment on attachment 125809 [details]
Patch

Invalid.

[Michael Brüning]

Created attachment 125810 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 125810 [details]
Patch

rs=me, I'll land it manually.

[Csaba Osztrogonác]

Comment on attachment 125810 [details]
Patch

Clearing flags on attachment: 125810

Committed r106924: <http://trac.webkit.org/changeset/106924>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because win build is still broken:

../../../../Source/WebCore/platform/qt/FileSystemQt.cpp: In function 'uint64_t WebCore::getVolumeFreeSizeForPath(const char*)':
../../../../Source/WebCore/platform/qt/FileSystemQt.cpp:203:33: error: expected '(' before 'freeBytesToCaller'
../../../../Source/WebCore/platform/qt/FileSystemQt.cpp:203:59: error: expected ')' before ';' token

[Csaba Osztrogonác]

And it broke many tests on WK2: http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r106920%20%2819804%29/results.html
(fails before your change: http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r106919%20%2819803%29/results.html)

[Michael Brüning]

Created attachment 125821 [details]
Patch

[Csaba Osztrogonác]

Could you guys land buildfixes manually instad of waiting hours for commit queue?

[WebKit Review Bot]

Comment on attachment 125821 [details]
Patch

Rejecting attachment 125821 [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:
ebKit/chromium/third_party/yasm/source/patched-yasm --revision 73761 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
46>At revision 73761.

________ 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/11436305

[Csaba Osztrogonác]

Comment on attachment 125821 [details]
Patch

Landed in http://trac.webkit.org/changeset/106933

[Csaba Osztrogonác]

Leave it open, because WK2 tests are still broken.

[Michael Brüning]

Created attachment 125843 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 125843 [details]
Patch

bbandix was the faster - http://trac.webkit.org/changeset/106939

[Csaba Osztrogonác]

I skipped the failing tests until fix - http://trac.webkit.org/changeset/106954

[Csaba Osztrogonác]

(In reply to comment #44)
> I skipped the failing tests until fix - http://trac.webkit.org/changeset/106954

WK2 fails can be found here: http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r106951%20%2819821%29/results.html

[Caio Marcelo de Oliveira Filho]

(In reply to comment #45)
> (In reply to comment #44)
> > I skipped the failing tests until fix - http://trac.webkit.org/changeset/106954
> 
> WK2 fails can be found here: http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r106951%20%2819821%29/results.html

Michael, the fact that fast/harness/use-page-cache.html is not passing maybe indicates that the page cache capacity is not enough. One idea is to check which value those calculations are giving you for page capacity. Also, enforce page capacity 20 to see if the test pass.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #46)
> Michael, the fact that fast/harness/use-page-cache.html is not passing maybe indicates that the page cache capacity is not enough. One idea is to check which value those calculations are giving you for page capacity. Also, enforce page capacity 20 to see if the test pass.

Magic number 20 was the old default :-)

[Michael Brüning]

(In reply to comment #46)
> (In reply to comment #45)
> > (In reply to comment #44)
> > > I skipped the failing tests until fix - http://trac.webkit.org/changeset/106954
> > 
> > WK2 fails can be found here: http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r106951%20%2819821%29/results.html
> 
> Michael, the fact that fast/harness/use-page-cache.html is not passing maybe indicates that the page cache capacity is not enough. One idea is to check which value those calculations are giving you for page capacity. Also, enforce page capacity 20 to see if the test pass.

Yeah, I also found that this was the reason - the default CacheModel results in pageCacheCapacity being 0. The gtk port sets this to another value, while the Mac port does not seem to reset it from the default value. 

I have a patch built & running the test at the office, but had to leave before it finished. I'll post the patch tomorrow if it fixes the issues...

[Michael Brüning]

Created attachment 126072 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 126072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126072&action=review

> Source/WebKit2/ChangeLog:9
> +        (WebKit::WebContext::WebContext): Set default cacheModel for QT platform to

Qt

> Source/WebKit2/UIProcess/WebContext.cpp:129
> +#if PLATFORM(QT)
> +    , m_cacheModel(CacheModelPrimaryWebBrowser)
> +#else
>      , m_cacheModel(CacheModelDocumentViewer)
> +#endif

We might want an experimental setting for this with the document viewer being the default

[Michael Brüning]

(In reply to comment #50)
> (From update of attachment 126072 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126072&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        (WebKit::WebContext::WebContext): Set default cacheModel for QT platform to
> 
> Qt
Will fix. So "Qt" is the preferred way of writing? I'm asking because I've seen mixes of Qt, QT and qt being used in logs...

> 
> > Source/WebKit2/UIProcess/WebContext.cpp:129
> > +#if PLATFORM(QT)
> > +    , m_cacheModel(CacheModelPrimaryWebBrowser)
> > +#else
> >      , m_cacheModel(CacheModelDocumentViewer)
> > +#endif
> 
> We might want an experimental setting for this with the document viewer being the default

As mentioned in #qtwebkit, the experimental setting will be part of the cacheModel API implementation for Qt-WK2, which I'll post soon. However, I think it's important that we unskip those layout tests for the time being.

[Kenneth Rohde Christiansen]

It has always been Qt. QT == QuickTime

[Michael Brüning]

Created attachment 126079 [details]
Patch

[Michael Brüning]

Comment on attachment 126079 [details]
Patch

Corrected ChangeLogs

[WebKit Review Bot]

Comment on attachment 126079 [details]
Patch

Clearing flags on attachment: 126079

Committed r107090: <http://trac.webkit.org/changeset/107090>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because the bug is still valid. You broke 19 tests now:
http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r107090%20%2819898%29/results.html

Please run layout tests before commiting and _watch_ the bots after landing.

[Csaba Osztrogonác]

I think it's time to rollout the original patch and the wrong fix.
And then you can reland the whole fixed patch when it is finished.

[Csaba Osztrogonác]

All patches related to this bug were rolled out - http://trac.webkit.org/changeset/107101 (original patch, buildfixes, gardening, fix, ...)

[Michael Brüning]

Created attachment 146254 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 146254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146254&action=review

> Source/WebCore/platform/qt/FileSystemQt.cpp:222
> +#else
> +    struct statvfs volumeInfo;
> +    if (statvfs(path, &volumeInfo))
> +        return 0;
> +
> +    return static_cast<uint64_t>(volumeInfo.f_bavail) * static_cast<uint64_t>(volumeInfo.f_frsize);
> +#endif

IS this ensure to work on all non windows platforms (that we support)? :)

> Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:104
> +    // The following variables are initialised to 0 because WebProcess::calculateCacheSizes might not
> +    // set them in some rare cases.

Bug number for that?

[Michael Brüning]

(In reply to comment #61)
> (From update of attachment 146254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146254&action=review
> 
> > Source/WebCore/platform/qt/FileSystemQt.cpp:222
> > +#else
> > +    struct statvfs volumeInfo;
> > +    if (statvfs(path, &volumeInfo))
> > +        return 0;
> > +
> > +    return static_cast<uint64_t>(volumeInfo.f_bavail) * static_cast<uint64_t>(volumeInfo.f_frsize);
> > +#endif
> 
> IS this ensure to work on all non windows platforms (that we support)? :)
At least on all the POSIX systems, it should be supported.

> 
> > Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:104
> > +    // The following variables are initialised to 0 because WebProcess::calculateCacheSizes might not
> > +    // set them in some rare cases.
> 
> Bug number for that?

Not sure if this is a bug, actually. All the other ports simply initialize to 0 and some code paths in calculateCacheSizes do not reset the values. It might be possible that someone would like to override this and still have a non-zero default if calculateCacheSizes does not set any value.

[Michael Brüning]

Comment on attachment 146254 [details]
Patch

I have tested this on two different machines (one using sedkit, one using resworb environment) and gotten the same results with or without the patch apart from one test that got flaky with the patch (http/tests/xmlhttprequest/zero-length-response.html). 

Do you think we could commit this and watch the bots afterwards?

[WebKit Review Bot]

Comment on attachment 146254 [details]
Patch

Clearing flags on attachment: 146254

Committed r119965: <http://trac.webkit.org/changeset/119965>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 88771

[Michael Brüning]

Created attachment 146819 [details]
Patch

[Michael Brüning]

Comment on attachment 146819 [details]
Patch

Build fix landed manually by Simon Hausmann in r119968.

[Michael Brüning]

All reviewed patches and the build fix have been landed, so closing the bug.