| Summary: | [WK2][GTK] ExceededDatabaseQuota method support in WebKitUIClient | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Danilo de Paula <danilo.eu> | ||||||||||||||||
| Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | UNCONFIRMED --- | ||||||||||||||||||
| Severity: | Normal | CC: | andersca, andrunko, bugs-noreply, cgarcia, commit-queue, gustavo, gyuyoung.kim, mcatanzaro, mrobinson, pnormand, rakuco, rego+ews, sam, xan.lopez | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
(In reply to comment #0) > Created an attachment (id=198727) [details] > patch > > it's a draft implementation of exceededDatabaseQuota > > When the securityOrigin reaches it's max quota, a signal will be emited and the return value of this signal should be the new quota. > When there's no one connected to that signal, the default value (5MB) will be returned. My concern now is that if the WebContext is the right place to put the set/get quota methods. Created attachment 198898 [details]
Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Attachment 198898 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/webkit2marshal.list']" exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1344: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1345: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1346: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1347: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1348: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1349: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1350: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1351: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1352: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1353: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1354: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1355: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1356: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h"
Total errors found: 13 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #3) > Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API That's not the final version. I would like to discuss the API property. Comment on attachment 198898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198898&action=review The signal is per-WebView, but the setting you are adding is per WebContext. I think this might be a better candidate for a WebSetting (applies to all WebViews in a WebViewGroup) or something more fine-grained. Having a setting on the WebContext does not allow you to create WebViews with different quotas. > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:200 > + guint64 *defaultQuota); You should use default_quota here, defaultQuota in the C++ implementation file, and default_quota for the documentation. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1356 >> + G_TYPE_UINT64); > > Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] You can ensure your code passes the style bot by uploading your patches with webkit-patch. Created attachment 199326 [details]
Patch
Comment on attachment 199326 [details]
Patch
Perhaps I'm misreading your patch, but it seems you set can adjust the default database quota in WebKitSettings, but that it isn't used at all.
(In reply to comment #8) > (From update of attachment 199326 [details]) > Perhaps I'm misreading your patch, but it seems you set can adjust the default database quota in WebKitSettings, but that it isn't used at all. It's used on webkitWebViewExceededDatabaseQuotaPermissionRequest. But forget about this patch for now, I'm assuming that the default value starts with 0. What's the best way to initiate a value on WebkitSettingsPrivate now that it's being constructed automatically? (In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 199326 [details] [details]) > > Perhaps I'm misreading your patch, but it seems you set can adjust the default database quota in WebKitSettings, but that it isn't used at all. > > It's used on webkitWebViewExceededDatabaseQuotaPermissionRequest. Ah, thanks! > But forget about this patch for now, I'm assuming that the default value starts with 0. > What's the best way to initiate a value on WebkitSettingsPrivate now that it's being constructed automatically? I think a good first patch for this API is to skip the signal for now and simply use the value that the API consumer sets on WebKitSettings. Perhaps Carlos has a different opinion, but we are trying to avoid the overwhelming amount of signals that we had in WebKit1. (In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > (From update of attachment 199326 [details] [details] [details]) > > > Perhaps I'm misreading your patch, but it seems you set can adjust the default database quota in WebKitSettings, but that it isn't used at all. > > > > It's used on webkitWebViewExceededDatabaseQuotaPermissionRequest. > > Ah, thanks! > > > But forget about this patch for now, I'm assuming that the default value starts with 0. > > What's the best way to initiate a value on WebkitSettingsPrivate now that it's being constructed automatically? > > I think a good first patch for this API is to skip the signal for now and simply use the value that the API consumer sets on WebKitSettings. Perhaps Carlos has a different opinion, but we are trying to avoid the overwhelming amount of signals that we had in WebKit1. Is there an alternative way to warn the user that an origin is over-using its database quota without using the webview signal? I agree with your concerns about the overwhelming amount of signals on webkit1, but does it pay the cost of removing a feature to keep the signal list clear? Carlos, would you mind to give your opinion/suggestion in this case, please? Anyway, since the WebkitSettings setter/getter is enough to allow the sqldatabase usage, I'm adapting the patch to include/use only that part and we can keep the signal discussion for later. Are you OK with that? (In reply to comment #11) > Is there an alternative way to warn the user that an origin is over-using its database quota without using the webview signal? I agree with your concerns about the overwhelming amount of signals on webkit1, but does it pay the cost of removing a feature to keep the signal list clear? This seems like a useful feature, but I think it should be added in tandem with consumers in downstream projects. We'll have a good idea when it's necessary because WebKitGTK+ browsers will need it. Created attachment 200073 [details]
Patch
Attachment 200073 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c', u'Tools/MiniBrowser/gtk/BrowserSettingsDialog.c', u'Tools/MiniBrowser/gtk/main.c']" exit_code: 1
Tools/MiniBrowser/gtk/main.c:155: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Tools/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h"
Tools/MiniBrowser/gtk/BrowserSettingsDialog.c:151: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1089: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1091: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Total errors found: 5 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 200074 [details]
Patch
Attachment 200074 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c', u'Tools/MiniBrowser/gtk/BrowserSettingsDialog.c', u'Tools/MiniBrowser/gtk/main.c']" exit_code: 1
Tools/MiniBrowser/gtk/main.c:155: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h"
Tools/MiniBrowser/gtk/BrowserSettingsDialog.c:151: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1089: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1091: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Total errors found: 4 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #16) > Attachment 200074 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c', u'Tools/MiniBrowser/gtk/BrowserSettingsDialog.c', u'Tools/MiniBrowser/gtk/main.c']" exit_code: 1 > Tools/MiniBrowser/gtk/main.c:155: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] > WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h" > Tools/MiniBrowser/gtk/BrowserSettingsDialog.c:151: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1089: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1091: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] > Total errors found: 4 in 12 files > If this churn bothers you you should upload your patch with webkit-patch or run check-webkit-style first. Everyone CC'd gets an email when this happens. Obviously it isn't a big deal (and I do it sometimes too), but it's less work just to use webkit-patch, I think. Created attachment 200075 [details]
Patch
There's no signal anymore Attachment 200075 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c', u'Tools/MiniBrowser/gtk/BrowserSettingsDialog.c', u'Tools/MiniBrowser/gtk/main.c']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h"
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1091: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 200076 [details]
Patch
Comment on attachment 200076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200076&action=review Looks good to me. Since this adds new API it needs review from two GTK+ reviewers and also a WebKit2 owner. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:68 > + guint64 defaultWebDatabaseQuota; You shoult use int64_t here, if possible. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1086 > + * The default quota for Web SQL Databases You might want to talk briefly about what happens when the quota is reached. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1173 > +void webkit_settings_set_default_web_database_quota(WebKitSettings* settings, guint64 default_quota) Here, but not in the doc or the header, you should rename default_quota to defaultQuota. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1568 > + guint64 quota = webkit_settings_get_default_web_database_quota(settings); > + > + return quota; You could just do return webkit_settings_get_default_web_database_quota(settings); here. > Tools/MiniBrowser/gtk/BrowserSettingsDialog.c:146 > + uIntProperty->maximum, 1, 1, 1); It's okay to make this one line. > Tools/MiniBrowser/gtk/BrowserSettingsDialog.c:151 > + adjustment = gtk_adjustment_new(uInt64Property->default_value, uInt64Property->minimum, > + uInt64Property->maximum, 1, 1, 1); Ditto. > Tools/MiniBrowser/gtk/main.c:155 > - || gParamType == G_TYPE_FLOAT); > + || gParamType == G_TYPE_UINT64 || gParamType == G_TYPE_FLOAT); Ditto. > Tools/MiniBrowser/gtk/main.c:253 > + Looks like this snuck in by accident. Carlos, can you review this patch as well? Comment on attachment 200076 [details]
Patch
I think the API is OK in general, I think there's value in letting the user agent decide to increase the quota once it's reached, but I'm in favour of not doing before we get a better feeling of how that should work UI-wise, maybe we can even add a request for increasing the quote by a fixed amount that is presented by webkit2 itself rather than by the browser, like for http basic auth…
Comment on attachment 200076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200076&action=review I don't know much about web database, but I wonder if it would make more sense to expose web database API and expose methods to get/set the web database quota per security origin, using a default value like webkit1 did. >> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:68 >> + guint64 defaultWebDatabaseQuota; > > You shoult use int64_t here, if possible. Actually uint64_t > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1087 > + */ Since: 2.2 > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1093 > + 0, G_MAXUINT64, 5 * 1024 * 1024, We could probably use a global static var or a macro for the default value. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1159 > + **/ Use single * here, and add Since: 2.2 tag > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1172 > + **/ Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:199 > - 0, // exceededDatabaseQuota > + decidePolicyForExceededDatabaseQuotaPermissionRequest, We don't usually rename the callbacks, it makes easier to match callback name and its implementation, Adding wk2 owners. (In reply to comment #19) > There's no signal anymore To avoid adding another signal to web view, we can use the generic permission request signal, adding a new permission request object for web database quota permission. (In reply to comment #27) > (In reply to comment #19) > > There's no signal anymore > > To avoid adding another signal to web view, we can use the generic permission request signal, adding a new permission request object for web database quota permission. One issue is that you don't actually respond with allow or deny, but are expected to adjust the quota setting. Should just need API for this new permission object that lets you tell it how much to increase the quota to, I guess, instead of using the allow/deny ones? I think allowing setting the quota per origin makes sense in principle, but does any current API user actually need that at the moment? (In reply to comment #29) > Should just need API for this new permission object that lets you tell it how much to increase the quota to, I guess, instead of using the allow/deny ones? I think allowing setting the quota per origin makes sense in principle, but does any current API user actually need that at the moment? That is one approach, but it opens up more questions. For instance, does the API on the permissions object proxy to the WebKitSetting property or does it only apply to the WebView in question? If it's just a proxy, isn't it redundant? If it doesn't proxy, what takes precedence when the application then adjusts the WebSettings property? (In reply to comment #29) > Should just need API for this new permission object that lets you tell it how much to increase the quota to, I guess, instead of using the allow/deny ones? Or in addition to allow/deny. set_quota just saves the value and the allow impl actually updates the quota. > I think allowing setting the quota per origin makes sense in principle, but does any current API user actually need that at the moment? Good question. (In reply to comment #30) > (In reply to comment #29) > > Should just need API for this new permission object that lets you tell it how much to increase the quota to, I guess, instead of using the allow/deny ones? I think allowing setting the quota per origin makes sense in principle, but does any current API user actually need that at the moment? > > That is one approach, but it opens up more questions. For instance, does the API on the permissions object proxy to the WebKitSetting property or does it only apply to the WebView in question? If it's just a proxy, isn't it redundant? If it doesn't proxy, what takes precedence when the application then adjusts the WebSettings property? That was my main concern, if we are going to expose a web database api like in wk1, this setting might be redundant and confusing. Well, the setting is for the default value, the permission would be to increase the quota for a specific origin (which would require API for managing the quotas per-origin, too). Related: API used in wk2-efl https://bugs.webkit.org/show_bug.cgi?id=97882. They are using a callback in the view that is invoked when the quota is exceeded. If the callback is not set the default value of 5MB is used. No websettings involved. I think we should take a similar approach by using a signal in the view but using websettings to allow setting the default value though (to be used in case no one is connected to the signal). I don't like the idea of re-using the permission-request signal for this as I believe the API would be confusing: 1) allow/deny doesn't apply directly here 2) it would require a new PermissionRequest object 3) the quota-exceeded signal carries useful information (currentQuota, currentOriginUsage, currentDatabaseUsage, expectUsage) that would also have to be handled in the new PermissionRequest object ... Let me know what you think and I will try to update the patch. Comment on attachment 200076 [details]
Patch
Martin and Carlos have several requested improvements to this patch. Removing from request queue.
(In reply to comment #34) > Let me know what you think and I will try to update the patch. What I don't understand is: why would an application want to increase the quota when it is exceeded? I can understand why apps might need to change the default quota, but what use-case do you have for an API that tells you when the quota is exceeded and then allows you the option of increasing it? Surely you're not going to expose database quotas to users, so there is not going to be any user interaction here. So what factor will your application use to determine the new value of the quota, which was not available when the quota was first set? |
Created attachment 198727 [details] patch it's a draft implementation of exceededDatabaseQuota When the securityOrigin reaches it's max quota, a signal will be emited and the return value of this signal should be the new quota. When there's no one connected to that signal, the default value (5MB) will be returned.