Bug 111265

Summary: [GTK] Add WebKitWebViewGroup to WebKit2 GTK+ API
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, commit-queue, gdesmott, gns, gyuyoung.kim, mrobinson, rakuco, rego+ews, rego, sam, webkit.review.bot, xan.lopez, zan
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 99081    
Attachments:
Description Flags
Patch
none
Rebased patch andersca: review+

Description Carlos Garcia Campos 2013-03-03 01:43:19 PST
In the current API the settings are always shared by all web views, because they are always created on the default page group. This makes impossible to have different settings on different web views. We need to expose the page group in the API, so that users can create web views on a group different than the default one to have their own settings.
Comment 1 Carlos Garcia Campos 2013-03-03 02:13:47 PST
Created attachment 191131 [details]
Patch

Note that this patch is compatible with current API because web views are created in the default page group by default, so apps currently using the API won't be affected.
Comment 2 Carlos Garcia Campos 2013-03-03 02:14:19 PST
Adding WebKit2 owners to the CC.
Comment 3 WebKit Review Bot 2013-03-03 02:20:25 PST
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
Comment 4 WebKit Review Bot 2013-03-03 02:20:41 PST
Attachment 191131 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettingsPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContextPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroupPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-docs.sgml', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk.types', u'Source/WebKit2/UIProcess/API/gtk/tests/GNUmakefile.am', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebViewGroup.cpp', u'Source/WebKit2/UIProcess/API/gtk/webkit2.h']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebViewGroup.cpp:74:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Carlos Garcia Campos 2013-03-03 02:30:13 PST
(In reply to comment #4)
> Attachment 191131 [details] did not pass style-queue:
> 
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebViewGroup.cpp:74:  Use 0 instead of NULL.  [readability/null] [5]

This NULL is a sentinel, we can't use 0 ins this particular case.
Comment 6 Carlos Garcia Campos 2013-03-03 03:38:23 PST
It seems to me that GTK+ EWS is failing because it builds with WebKit2 disabled, but tries to generate the API docs for WebKit2. 

Failed to run "['Tools/Scripts/build-webkit', '--release', '--gtk', '--update-gtk', '--no-webkit2', '--makeargs="-j8"']" exit_code: 1

Copying template files to output directory...
Running gtkdoc-scan
Running gtkdoc-scangobj
webkit2gtk-scan.o: In function `get_object_types':
webkit2gtk-scan.c:(.text+0x307): undefined reference to `webkit_web_view_group_get_type'
collect2: error: ld returned 1 exit status
Linking of scanner failed: 

I ran generate-gtkdoc locally and it worked.
Comment 7 Carlos Garcia Campos 2013-03-03 03:39:55 PST
Now the box is green, I don't understand what's going on with GTK EWS
Comment 8 Martin Robinson 2013-03-03 09:36:49 PST
Comment on attachment 191131 [details]
Patch

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

Very nice. Looks good to me.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:624
> +            _("Web View Group"),

Nit: WebView

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:625
> +            _("The web view group of the view"),

Maybe this should say "The WebKitWebViewGroup of the view?"

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.cpp:41
> + * To create a #WebKitWebView on a different WebKitWebViewGroup you can use

Nit: on -> in
Comment 9 Gustavo Noronha (kov) 2013-03-04 17:08:28 PST
I think the GTK+ EWS that failed to build (rego's) did not finish reporting the error for some reason, since there is no comment in the bug and the patch did not get cq-, then another EWS bot got the patch from the queue and it passed.
Comment 10 Gustavo Noronha (kov) 2013-03-04 17:59:08 PST
Comment on attachment 191131 [details]
Patch

WebCore makes a distinction of per-Page settings and PageGroup settings, so that you can still have different settings that make sense for each page, but can also have PageGroup-wide settings (such as local storage path), have you considered using that same approach at the API level?
Comment 11 Carlos Garcia Campos 2013-03-05 01:32:15 PST
(In reply to comment #10)
> (From update of attachment 191131 [details])
> WebCore makes a distinction of per-Page settings and PageGroup settings, so that you can still have different settings that make sense for each page, but can also have PageGroup-wide settings (such as local storage path), have you considered using that same approach at the API level?

There are no GroupSettings in WebKit2, I think those are per process and handled by the WebContext.
Comment 12 Manuel Rego Casasnovas 2013-03-05 01:40:29 PST
(In reply to comment #9)
> I think the GTK+ EWS that failed to build (rego's) did not finish reporting the error for some reason, since there is no comment in the bug and the patch did not get cq-, then another EWS bot got the patch from the queue and it passed.

My EWS doesn't have EditBugs permission yet, that's why even if it fails it doesn't add comments to the bug. I've requested it several times by IRC without success :-(. I'll try again.
Comment 13 Gustavo Noronha (kov) 2013-03-05 10:14:54 PST
(In reply to comment #12)
> My EWS doesn't have EditBugs permission yet, that's why even if it fails it doesn't add comments to the bug. I've requested it several times by IRC without success :-(. I'll try again.

Would be good to try to figure out why it failed, though, while the other EWS passed, perhaps it has left-overs that caused webkit2 docs to be built for some reason?
Comment 14 Manuel Rego Casasnovas 2013-03-06 00:03:08 PST
(In reply to comment #13)
> (In reply to comment #12)
> > My EWS doesn't have EditBugs permission yet, that's why even if it fails it doesn't add comments to the bug. I've requested it several times by IRC without success :-(. I'll try again.
> 
> Would be good to try to figure out why it failed, though, while the other EWS passed, perhaps it has left-overs that caused webkit2 docs to be built for some reason?

I've tried again the patch in the EWS and it failed again:
Generating WebKit2 documentation...
Copying template files to output directory...
Running gtkdoc-scan
Running gtkdoc-scangobj
webkit2gtk-scan.o: In function `get_object_types':
webkit2gtk-scan.c:(.text+0x307): undefined reference to `webkit_web_view_group_get_type'
collect2: error: ld returned 1 exit status
Linking of scanner failed: 
Traceback (most recent call last):
  File "/home/igalia/mrego/WebKit/Tools/gtk/generate-gtkdoc", line 203, in <module>
    saw_webkit2_warnings = generate_doc(generator)
  File "/home/igalia/mrego/WebKit/Tools/gtk/generate-gtkdoc", line 154, in generate_doc
    generator.generate(html='--skip-html' not in sys.argv)
  File "/home/igalia/mrego/WebKit/Tools/gtk/gtkdoc.py", line 137, in generate
    self._run_gtkdoc_scangobj()
  File "/home/igalia/mrego/WebKit/Tools/gtk/gtkdoc.py", line 314, in _run_gtkdoc_scangobj
    env=env, cwd=self.output_dir)
  File "/home/igalia/mrego/WebKit/Tools/gtk/gtkdoc.py", line 198, in _run_command
    % (args[0], process.returncode))
Exception: gtkdoc-scangobj produced a non-zero return code 1

 gtkdoc did not build without warnings


The real problem was that I maybe did a build without "--disable-webkit2" in the EWS before, and that was the reason why the file "WebKitBuild/Release/Source/WebKit2/webkit2gtk-3.0.pc" exists and it tried to generate WK2 documentation.

I did a clean build which fixed the problem. So, sorry for the noise.
Comment 15 Martin Robinson 2013-03-18 10:17:34 PDT
Other than the few nits, I think this is ready for owner review. Anders or Sam, do you mind double-checking that this is okay with you?
Comment 16 Carlos Garcia Campos 2013-04-23 08:22:23 PDT
Created attachment 199241 [details]
Rebased patch

Rebased and addressed review nits. Ping WebKit2 owners
Comment 17 WebKit Commit Bot 2013-04-23 08:24:15 PDT
Attachment 199241 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettingsPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContextPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroupPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-docs.sgml', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk.types', u'Source/WebKit2/UIProcess/API/gtk/tests/GNUmakefile.am', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebViewGroup.cpp', u'Source/WebKit2/UIProcess/API/gtk/webkit2.h']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebViewGroup.cpp:74:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Carlos Garcia Campos 2013-04-23 09:32:26 PDT
gtk-wk2 failure seems to be related to freedesktop.org git issues while updating dependencies.
Comment 19 Anders Carlsson 2013-04-25 08:16:43 PDT
Comment on attachment 199241 [details]
Rebased patch

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

> Source/WebKit2/ChangeLog:75
> +        (webkit_web_view_group_get_settings): Get the settins of a

Typo, settins.
Comment 20 Carlos Garcia Campos 2013-04-25 09:46:04 PDT
Committed r149117: <http://trac.webkit.org/changeset/149117>