Bug 73918

Summary: [Qt][WK2] Compute and set cache capacities using the current CacheModel
Product: WebKit Reporter: zalan <zalan>
Component: WebKit QtAssignee: Michael Brüning <michael.bruning>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, hausmann, kenneth, menard, michael.bruning, ossy, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 78124, 88771    
Bug Blocks:    
Attachments:
Description Flags
First patch.
none
First patch (updated)
none
First patch (updated with suggestions).
none
Updated patch.
none
Updated patch.
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description zalan 2011-12-06 06:14:30 PST
Implement WebProcess::platformSetCacheModel() for Qt and expose CacheModel API so that host apps, such as browser, can set it properly.
Comment 1 Michael Brüning 2012-01-30 07:55:05 PST
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.
Comment 2 WebKit Review Bot 2012-01-30 07:56:58 PST
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.
Comment 3 Michael Brüning 2012-01-30 08:02:49 PST
Comment on attachment 124551 [details]
First patch.

Found style issues that I'll fix first. Sorry for that.
Comment 4 Michael Brüning 2012-01-30 08:04:52 PST
Created attachment 124553 [details]
First patch (updated)

Fixed style issues.
Comment 5 Caio Marcelo de Oliveira Filho 2012-01-30 08:38:40 PST
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?
Comment 6 Michael Brüning 2012-01-30 08:51:30 PST
(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.
Comment 7 Kenneth Rohde Christiansen 2012-01-30 12:12:02 PST
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?
Comment 8 Michael Brüning 2012-01-30 12:53:33 PST
(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.
Comment 9 Michael Brüning 2012-01-31 03:25:51 PST
Created attachment 124699 [details]
First patch (updated with suggestions).

Updated the patch with suggestions from Caio Marcelo and Kenneth.
Comment 10 WebKit Review Bot 2012-01-31 03:27:58 PST
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.
Comment 11 Michael Brüning 2012-01-31 03:33:58 PST
(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...
Comment 12 Kenneth Rohde Christiansen 2012-01-31 06:00:26 PST
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.
Comment 13 Michael Brüning 2012-02-02 08:58:44 PST
Created attachment 125140 [details]
Updated patch.

Added the Platform class plus implementation for unix/posix, windows and mac.
Comment 14 Michael Brüning 2012-02-03 05:13:33 PST
Created attachment 125310 [details]
Updated patch.

Fixed build problem for Qt port on Mac.
Comment 15 Kenneth Rohde Christiansen 2012-02-06 05:14:44 PST
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 ? :-)
Comment 16 Michael Brüning 2012-02-06 05:35:41 PST
(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?
Comment 17 Michael Brüning 2012-02-06 06:02:54 PST
Created attachment 125626 [details]
Patch
Comment 18 Michael Brüning 2012-02-06 06:07:03 PST
Created attachment 125627 [details]
Patch
Comment 19 Michael Brüning 2012-02-06 06:30:43 PST
Created attachment 125629 [details]
Patch
Comment 20 Kenneth Rohde Christiansen 2012-02-06 06:36:03 PST
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?
Comment 21 Michael Brüning 2012-02-06 07:09:53 PST
(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.
Comment 22 Michael Brüning 2012-02-06 07:21:22 PST
Created attachment 125640 [details]
Patch
Comment 23 Michael Brüning 2012-02-06 09:05:06 PST
Created attachment 125660 [details]
Patch
Comment 24 Kenneth Rohde Christiansen 2012-02-06 11:53:07 PST
Comment on attachment 125660 [details]
Patch

OK, lets go with this for now.
Comment 25 Michael Brüning 2012-02-07 02:17:37 PST
Comment on attachment 125660 [details]
Patch

I'll follow up with the PlatformInfo patch and cc Anders or Kling.
Comment 26 WebKit Review Bot 2012-02-07 03:42:54 PST
Comment on attachment 125660 [details]
Patch

Clearing flags on attachment: 125660

Committed r106920: <http://trac.webkit.org/changeset/106920>
Comment 27 WebKit Review Bot 2012-02-07 03:43:00 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Csaba Osztrogonác 2012-02-07 03:59:18 PST
(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?
Comment 29 Michael Brüning 2012-02-07 04:10:20 PST
Created attachment 125809 [details]
Patch
Comment 30 Michael Brüning 2012-02-07 04:11:26 PST
Comment on attachment 125809 [details]
Patch

Invalid.
Comment 31 Michael Brüning 2012-02-07 04:15:30 PST
Created attachment 125810 [details]
Patch
Comment 32 Csaba Osztrogonác 2012-02-07 04:17:58 PST
Comment on attachment 125810 [details]
Patch

rs=me, I'll land it manually.
Comment 33 Csaba Osztrogonác 2012-02-07 04:23:23 PST
Comment on attachment 125810 [details]
Patch

Clearing flags on attachment: 125810

Committed r106924: <http://trac.webkit.org/changeset/106924>
Comment 34 Csaba Osztrogonác 2012-02-07 04:23:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 35 Csaba Osztrogonác 2012-02-07 04:48:06 PST
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
Comment 37 Michael Brüning 2012-02-07 05:11:51 PST
Created attachment 125821 [details]
Patch
Comment 38 Csaba Osztrogonác 2012-02-07 06:02:47 PST
Could you guys land buildfixes manually instad of waiting hours for commit queue?
Comment 39 WebKit Review Bot 2012-02-07 06:24:15 PST
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
Comment 40 Csaba Osztrogonác 2012-02-07 06:33:08 PST
Comment on attachment 125821 [details]
Patch

Landed in http://trac.webkit.org/changeset/106933
Comment 41 Csaba Osztrogonác 2012-02-07 06:33:33 PST
Leave it open, because WK2 tests are still broken.
Comment 42 Michael Brüning 2012-02-07 07:33:59 PST
Created attachment 125843 [details]
Patch
Comment 43 Csaba Osztrogonác 2012-02-07 08:17:39 PST
Comment on attachment 125843 [details]
Patch

bbandix was the faster - http://trac.webkit.org/changeset/106939
Comment 44 Csaba Osztrogonác 2012-02-07 09:20:30 PST
I skipped the failing tests until fix - http://trac.webkit.org/changeset/106954
Comment 45 Csaba Osztrogonác 2012-02-07 09:24:36 PST
(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
Comment 46 Caio Marcelo de Oliveira Filho 2012-02-07 10:16:29 PST
(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.
Comment 47 Caio Marcelo de Oliveira Filho 2012-02-07 10:24:24 PST
(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 :-)
Comment 48 Michael Brüning 2012-02-07 11:46:10 PST
(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...
Comment 49 Michael Brüning 2012-02-08 06:23:36 PST
Created attachment 126072 [details]
Patch
Comment 50 Kenneth Rohde Christiansen 2012-02-08 06:38:03 PST
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
Comment 51 Michael Brüning 2012-02-08 06:54:24 PST
(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.
Comment 52 Kenneth Rohde Christiansen 2012-02-08 07:03:02 PST
It has always been Qt. QT == QuickTime
Comment 53 Michael Brüning 2012-02-08 07:17:15 PST
Created attachment 126079 [details]
Patch
Comment 54 Michael Brüning 2012-02-08 07:18:16 PST
Comment on attachment 126079 [details]
Patch

Corrected ChangeLogs
Comment 55 WebKit Review Bot 2012-02-08 08:02:47 PST
Comment on attachment 126079 [details]
Patch

Clearing flags on attachment: 126079

Committed r107090: <http://trac.webkit.org/changeset/107090>
Comment 56 WebKit Review Bot 2012-02-08 08:02:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 57 Csaba Osztrogonác 2012-02-08 08:57:43 PST
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.
Comment 58 Csaba Osztrogonác 2012-02-08 09:01:33 PST
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.
Comment 59 Csaba Osztrogonác 2012-02-08 09:57:55 PST
All patches related to this bug were rolled out - http://trac.webkit.org/changeset/107101 (original patch, buildfixes, gardening, fix, ...)
Comment 60 Michael Brüning 2012-06-07 04:46:15 PDT
Created attachment 146254 [details]
Patch
Comment 61 Kenneth Rohde Christiansen 2012-06-07 04:56:41 PDT
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?
Comment 62 Michael Brüning 2012-06-07 05:08:17 PDT
(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.
Comment 63 Michael Brüning 2012-06-08 02:37:06 PDT
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?
Comment 64 WebKit Review Bot 2012-06-11 02:17:25 PDT
Comment on attachment 146254 [details]
Patch

Clearing flags on attachment: 146254

Committed r119965: <http://trac.webkit.org/changeset/119965>
Comment 65 WebKit Review Bot 2012-06-11 02:17:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 66 WebKit Review Bot 2012-06-11 02:25:52 PDT
Re-opened since this is blocked by 88771
Comment 67 Michael Brüning 2012-06-11 02:41:14 PDT
Created attachment 146819 [details]
Patch
Comment 68 Michael Brüning 2012-06-11 03:09:16 PDT
Comment on attachment 146819 [details]
Patch

Build fix landed manually by Simon Hausmann in r119968.
Comment 69 Michael Brüning 2012-06-11 03:10:08 PDT
All reviewed patches and the build fix have been landed, so closing the bug.