Bug 27613 - WebCore/page/ContextMenuController.cpp does not conform to style standards
Summary: WebCore/page/ContextMenuController.cpp does not conform to style standards
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-23 12:03 PDT by Mike Fenton
Modified: 2009-07-29 09:33 PDT (History)
2 users (show)

See Also:


Attachments
Style patch for ContextMenuController.cpp (18.30 KB, patch)
2009-07-23 12:07 PDT, Mike Fenton
levin: review-
Details | Formatted Diff | Diff
Replacement patch for ContextMenuController.cpp style fixes. (18.76 KB, patch)
2009-07-29 07:36 PDT, Mike Fenton
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Fenton 2009-07-23 12:03:09 PDT
cpplint reports a number of valid style fixes that should be made to
WebCore/page/ContextMenuController.cpp.
Comment 1 Mike Fenton 2009-07-23 12:07:59 PDT
Created attachment 33350 [details]
Style patch for ContextMenuController.cpp
Comment 2 Eric Seidel (no email) 2009-07-23 23:14:17 PDT
Comment on attachment 33350 [details]
Style patch for ContextMenuController.cpp

This would be much easier to read if we had a white-space ignoring diff.  It's difficult for me to tell that you haven' actually changed any code here. :(
Comment 3 Mike Fenton 2009-07-24 05:06:11 PDT
The main change in the patch is to fix the indentation of the case statements.  It also fixes the include order and a few whitespace at end of line fixes.
Comment 4 David Levin 2009-07-28 11:21:59 PDT
Comment on attachment 33350 [details]
Style patch for ContextMenuController.cpp

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2009-07-23  Mike Fenton  <mike.fenton@torchmobile.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Update WebCore/page/ContextMenuController.cpp to conform to WebKit
> +        Style Guidelines as identified by cpplint.py.

Now it is cpp_style.py :)


> diff --git a/WebCore/page/ContextMenuController.cpp b/WebCore/page/ContextMenuController.cpp


Since this is about fixing styling in ContextMenuController.cpp,


Why not fix the indentation in this block of code?  Personally, I would unwrap this line but you could just indent to the createWindow(.


107	static void openNewWindow(const KURL& urlToLoad, Frame* frame)
108	{
109	    if (Page* oldPage = frame->page()) {
110	        WindowFeatures features;
111	        if (Page* newPage = oldPage->chrome()->createWindow(frame,
112	                FrameLoadRequest(ResourceRequest(urlToLoad, frame->loader()->outgoingReferrer())), features))
113	            newPage->chrome()->show();
114	    }
115	}


> +    case ContextMenuItemTagOpenFrameInNewWindow: {
> +        DocumentLoader* loader = frame->loader()->documentLoader();
> +        if (!loader->unreachableURL().isEmpty())
> +            openNewWindow(loader->unreachableURL(), frame);
> +        else
> +            openNewWindow(loader->url(), frame);
> +        break;
>          }
The indentation for the "}" needs fixing.

> +    case ContextMenuItemTagSpellingGuess:
> +        ASSERT(frame->selectedText().length());
> +        if (frame->editor()->shouldInsertText(item->title(), frame->selection()->toNormalizedRange().get(),
> +            EditorInsertActionPasted)) {
Either indent to the level of the nested paren or unwrap.

> +            Document* document = frame->document();
> +            RefPtr<ReplaceSelectionCommand> command =
> +                ReplaceSelectionCommand::create(document, createFragmentFromMarkup(document, item->title(), ""),
> +                                                                               true, false, true);

Either indent to the level of the nested paren or unwrap.


> +    case ContextMenuItemTagOpenLink:
> +        if (Frame* targetFrame = result.targetFrame()) {
> +            targetFrame->loader()->loadFrameRequest(FrameLoadRequest(ResourceRequest(result.absoluteLinkURL(),
> +                frame->loader()->outgoingReferrer())), false, false, 0, 0);

Either indent to the level of the nested paren or unwrap.
Comment 5 Mike Fenton 2009-07-29 07:36:08 PDT
Created attachment 33714 [details]
Replacement patch for ContextMenuController.cpp style fixes.

Thanks for the review.

This updated patch has all the noted changes corrected.  Functions have been unwrapped and the indentation of the brace corrected.
Comment 6 David Levin 2009-07-29 09:33:08 PDT
Committed as http://trac.webkit.org/changeset/46544