Last Comment Bug 578564 - deCOMtaminate nsIFrameSetElement
: deCOMtaminate nsIFrameSetElement
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: x86 Windows 7
: -- normal with 1 vote (vote)
: mozilla7
Assigned To: Sebastian Kromp
:
Mentors:
Depends on: 578570
Blocks: deCOM
  Show dependency treegraph
 
Reported: 2010-07-13 18:45 PDT by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2011-08-31 08:57 PDT (History)
9 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it. (83.40 KB, patch)
2011-03-22 12:02 PDT, Sebastian Kromp
khuey: feedback+
Details | Diff | Splinter Review
Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it.(fixed) (65.07 KB, patch)
2011-03-24 08:52 PDT, Sebastian Kromp
no flags Details | Diff | Splinter Review
Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it.(fixed nsGkAtoms::frameset) (65.07 KB, patch)
2011-03-24 12:35 PDT, Sebastian Kromp
no flags Details | Diff | Splinter Review
Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it.(fixed whitespaces) (20.25 KB, patch)
2011-03-30 07:36 PDT, Sebastian Kromp
bzbarsky: review-
Details | Diff | Splinter Review
Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it. (18.91 KB, patch)
2011-04-22 14:16 PDT, Sebastian Kromp
bzbarsky: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-07-13 18:45:45 PDT
There are a number of interfaces here that could be removed.
Comment 1 Sebastian Kromp 2011-03-22 12:02:32 PDT
Created attachment 520984 [details] [diff] [review]
Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-03-22 12:15:24 PDT
Comment on attachment 520984 [details] [diff] [review]
Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it.

This is a little hard to make sense of.  It looks like your editor is converting to Windows line endings.  If you can attach a diff with the comments addressed and Unix line endings this will be much better.

>+ * Sebastian Kromp <46b@gulli.com>

Don't add your name to nsHTMLFrameSetElement.cpp, add it to the .h.

>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Mozilla Communicator client code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either of the GNU General Public License Version 2 or later (the "GPL"),
>+ * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+

It looks like you have two license blocks inside nsHTMLFrameSetElement.cpp.  Remove the second one.

>diff -r 290712e55ade content/html/content/src/nsHTMLFrameSetElement.h
>+ * ***** END LICENSE BLOCK ***** */
>+#ifndef nsHTMLFrameSetElement_h__
>+#define nsHTMLFrameSetElement_h__

Newline between the comments and the macros please.

>+#endif nsHTMLFrameSetElement_h__
>+
>+

Only one newline at the end of the file.

Other than it looks good.  Please fix the newlines and the comments and attach a new diff.
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2011-03-22 13:57:41 PDT
Please use nsHTMLFrameSetElement_h instead of nsHTMLFrameSetElement_h__ for the header guards. Identifiers with two consecutive underscores are reserved.
Comment 4 Sebastian Kromp 2011-03-24 08:52:45 PDT
Created attachment 521508 [details] [diff] [review]
Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it.(fixed)
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-03-24 09:14:25 PDT
Comment on attachment 521508 [details] [diff] [review]
Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it.(fixed)

>+  static nsHTMLFrameSetElement* FromContent(nsIContent *aContent)
>+  {
>+    if (aContent->NodeInfo()->Equals(nsGkAtoms::option, kNameSpaceID_XHTML))
>+      return static_cast<nsHTMLFrameSetElement*>(aContent);
>+    return nsnull;
>+  }

Ah, now that I can see, there's one problem here.  This should be nsGkAtoms::frameset, since this is a frameset element.

Other than that it looks good.  Fix that, attach a new patch, and request review from ":bz".
Comment 6 Sebastian Kromp 2011-03-24 12:35:04 PDT
Created attachment 521600 [details] [diff] [review]
Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it.(fixed nsGkAtoms::frameset)
Comment 7 Boris Zbarsky [:bz] 2011-03-25 14:27:39 PDT
Can you please not make all those whitespace changes?  That will make this patch much simpler to review and not mess up the blame....

If we do want to make the whitespace changes, that should be done in a separate patch with no substantive changes in it.
Comment 8 Sebastian Kromp 2011-03-30 07:36:25 PDT
Created attachment 523008 [details] [diff] [review]
Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it.(fixed whitespaces)

Sorry for that, didn´t mentioned that it was that messed up. I hope it is okay now.
Comment 9 Boris Zbarsky [:bz] 2011-04-05 19:06:24 PDT
Comment on attachment 523008 [details] [diff] [review]
Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it.(fixed whitespaces)

>+++ b/content/html/content/src/nsHTMLFrameSetElement.h	Wed Mar 30 16:32:52 2011 +0200

It looks like you removed the old header and added the new one.  You should probably change things so that you hg move the old header to the new name and then edit it.  That would keep revision history better.

>+    if (aContent->NodeInfo()->Equals(nsGkAtoms::frameset, kNameSpaceID_XHTML))

  if (aContent->IsHTML(nsGkAtoms::frameset))

>+++ b/layout/generic/nsFrameSetFrame.h	Wed Mar 30 16:32:52 2011 +0200
>+#include "nsHTMLFrameSetElement.h"

Why is this include needed at all?  Just including that header in nsFrameSetFrame.cpp should be enough.
Comment 10 Sebastian Kromp 2011-04-22 14:16:29 PDT
Created attachment 527859 [details] [diff] [review]
Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it.

Okay, had fixed all that you mentioned. Just out of curiosity: why is it better to include it in the *.cpp instead of the header?
Comment 11 Boris Zbarsky [:bz] 2011-05-25 13:44:51 PDT
Comment on attachment 527859 [details] [diff] [review]
Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it.

> why is it better to include it in the *.cpp instead of the header?

Fewer chances of include hell if someone else includes the header.

Please restore the comments about GetRowSpec and GetColSpec that the interface used to have and remove the "// nsIFramesetElement" comment.  r=me with those changes, and sorry for the terrible lag.  :(

If you want me to just make those updates and push this, please let me know, ok?  Otherwise, please set the checkin-needed keyword once you post an updated patch.
Comment 12 Boris Zbarsky [:bz] 2011-06-08 14:35:21 PDT
http://hg.mozilla.org/projects/cedar/rev/f9b7d8909b78
Comment 14 Daniel Holbert [:dholbert] 2011-06-11 17:14:53 PDT
This patch added a build warning due to the text after #endif here:
> #endif nsHTMLFrameSetElement_h
"nsHTMLFrameSetElement_h" wants to be a comment there. I pushed a trivial followup to fix that: http://hg.mozilla.org/mozilla-central/rev/79394c301944
Comment 15 Ioana (away) 2011-08-31 08:57:03 PDT
Is there a way this fix can be verified by QA? If so, please help me with some test case/STR/guidelines.

Thank you

Note You need to log in before you can comment on or make changes to this bug.