Summary: | Add GTK+ API to set a WebKitWebView in view source mode to WebKit2 | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | andersca, darin, eric, gustavo, gyuyoung.kim, mario, mrobinson, rakuco, sam, xan.lopez | ||||
Priority: | P2 | Keywords: | Gtk | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | Linux | ||||||
Attachments: |
|
Description
Carlos Garcia Campos
2012-10-15 05:58:02 PDT
Created attachment 168696 [details]
Patch
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. 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 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.
(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. :/ (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 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?
Committed r138593: <http://trac.webkit.org/changeset/138593> |