Bug 99315 – Add GTK+ API to set a WebKitWebView in view source mode to WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

ssia

[Carlos Garcia Campos]

Created attachment 168696 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 168696 [details]
Patch

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

> Source/WebKit2/UIProcess/WebFrameProxy.h:111
> +    bool inViewSourceMode() const { return m_inViewSourceMode; }

What about implementing this method through a (new) message to the Web process that would end up calling coreFrame->inViewSourceMode() to get the actual state of the web view from the value of WebCore::Frame's m_inViewSourceMode?

I know it's probably too much stuff for just keeping track of this property and that doing that in the UI process will probably be good enough, but that might change if in the future we decide to call WebCore::Frame::setInViewSourceMode() from somewhere else in WebCore.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Attachment 168696 [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
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:641:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:643:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:644:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:645:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:646:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:647:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 12 files


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

[Martin Robinson]

(In reply to comment #4)
> Attachment 168696 [details] did not pass style-queue:

I'm removing the [GTK] tag since this patch changes platform-independent code. This change looks good to me, but I'd feel more comfortable if a reviewer from Apple could look at the platform-indepndent WebKit2 bits.

> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:641:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:643:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:644:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:645:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:646:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:647:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Total errors found: 6 in 12 files

I don't think we should introduce new style errors, if we can help it. :/

[Mario Sanchez Prada]

(In reply to comment #5)
> > [...]
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:647:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> > Total errors found: 6 in 12 files
> 
> I don't think we should introduce new style errors, if we can help it. :/

There seems to be an (not solved yet) issue with the current implementation of check-webkit-style, as it's not 100% clear whether the current behavior (implemented as a patch for bug 97602 [1]) is the right one or not.

I've posted a comment in the bug originating the confusion [2] and we are currently waiting for Darin (added now to CC) for clarifying this before proceeding either with fixing the check-webkit-style script or to adopt the new style for GTK code too (as I agree making exceptions is probably not the best solution).

For the time being, and even if I agree it's weird to ignore these errors, I'd lean more towards ignoring them since it was the way we used to code until now, and I'm not sure it's wise to embrace the new style without a clear statement saying that it's the right one (and at the moment such a statement hasn't happened yet, there's only some confusion :))

My 2 cents to unblock these situations.

[1]https://bugs.webkit.org/show_bug.cgi?id=97602
[2]https://bugs.webkit.org/show_bug.cgi?id=95930#c22

[Martin Robinson]

Comment on attachment 168696 [details]
Patch

This looks good to me, apart from the style errors. Before landing though, do you think you could get a nod from one of the WebKit2 developers though?

[Carlos Garcia Campos]

Committed r138593: <http://trac.webkit.org/changeset/138593>