Closed
Bug 698275
Opened 13 years ago
Closed 13 years ago
Various cleanup in HTML element implementations
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: Ms2ger, Assigned: Ms2ger)
References
(Depends on 1 open bug)
Details
(Whiteboard: [qa-])
Attachments
(1 file)
132.61 KB,
patch
|
mounir
:
review+
|
Details | Diff | Splinter Review |
Various stuff that has been sitting in my queue for a while. Hope you like it.
Attachment #570550 -
Flags: review?(mounir)
Flags: in-testsuite-
Comment 1•13 years ago
|
||
Hi! At first glance, from the MDN Dev Doc point-of-view, these are changes that do not change behavior. No doc will need to be updated because of this : is this correct? Thx in advance.
Comment 2•13 years ago
|
||
(In reply to Jean-Yves Perrier [:teoli] from comment #1) > At first glance, from the MDN Dev Doc point-of-view, these are changes that > do not change behavior. No doc will need to be updated because of this : is > this correct? Yes, this is only various cleanups, no behavioral change should happen.
Comment 3•13 years ago
|
||
Comment on attachment 570550 [details] [diff] [review] Patch v1 Review of attachment 570550 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comments addressed. ::: content/html/content/src/nsGenericHTMLElement.cpp @@ +3594,5 @@ > > bool > nsGenericHTMLElement::IsCurrentBodyElement() > { > + // XXX Should this handle the case where GetBody returns a frameset? Please do not add XXX comment. Put a TODO and file a bug (and put the bug number here of course :)) ::: content/html/content/src/nsHTMLButtonElement.cpp @@ -668,5 @@ > } > > return state; > } > - You don't like those lines, do you? ::: content/html/content/src/nsHTMLFieldSetElement.cpp @@ +62,5 @@ > > nsHTMLFieldSetElement::~nsHTMLFieldSetElement() > { > PRUint32 length = mDependentElements.Length(); > + for (PRUint32 i = 0; i < length; ++i) { Seems like most people want that horrible style... :( ::: content/html/content/src/nsHTMLFormElement.cpp @@ +2021,4 @@ > > + // > + // Get the control / list of controls from the form using form["name"] > + // Could you remove those lines with "//" only? @@ +2028,5 @@ > + } > + > + // > + // If it's just a lone radio button, then select it. > + // Could you remove those lines with "//" only? @@ +2037,5 @@ > + } > + return NS_OK; > + } > + nsCOMPtr<nsIDOMNodeList> nodeList = do_QueryInterface(item); > + if (nodeList) { if (!nodeList) { return NS_OK; } ::: content/html/content/src/nsHTMLInputElement.cpp @@ +1004,5 @@ > > NS_IMETHODIMP > nsHTMLInputElement::GetList(nsIDOMHTMLElement** aValue) > { > + *aValue = nsnull; Add an empty line after this line. @@ +1022,5 @@ > + return NS_OK; > + } > + > + nsCOMPtr<nsIDOMHTMLElement> domElement = do_QueryInterface(element); > + domElement.forget(aValue); Do you really want to change: CallQueryInterface(elem, aValue); to this these two lines? Do we want to prevent CallQueryInterface as much as possible? @@ +1482,5 @@ > } > > + // > + // For radio button, we need to do some extra fun stuff > + // Remove the lines with only "//". @@ +1487,5 @@ > + if (aChecked) { > + return RadioSetChecked(aNotify); > + } > + > + if (nsIRadioGroupContainer* container = GetRadioGroupContainer()) { Assigning in a condition is ugly... Please do that instead: nsIRadioGroupContainer* container = GetRadioGroupContainer(); if (container) { /* foo */ } /* bar*/ @@ +1521,5 @@ > // > // Let the group know that we are now the One True Radio Button > // > + nsresult rv = NS_OK; > + if (nsIRadioGroupContainer* container = GetRadioGroupContainer()) { ditto @@ +1643,5 @@ > { > + if (mType != NS_FORM_INPUT_FILE) { > + return nsGenericHTMLElement::Focus(); > + } > + // for file inputs, focus the button instead Leave an empty line before this comment and put an uppercase at the beginning of the comment and a dot at the end. @@ +1644,5 @@ > + if (mType != NS_FORM_INPUT_FILE) { > + return nsGenericHTMLElement::Focus(); > + } > + // for file inputs, focus the button instead > + if (nsIFrame* frame = GetPrimaryFrame()) { No assignments in conditions, please. @@ +1646,5 @@ > + } > + // for file inputs, focus the button instead > + if (nsIFrame* frame = GetPrimaryFrame()) { > + nsIFrame* childFrame = frame->GetFirstPrincipalChild(); > + while (childFrame) { I believe this could be a for loop instead: for (nsIFrame* childFrame = frame->GetFirstPrincipalChild(); childFrame; childFrame = childFrame->GetNextSibling();) { } @@ +1654,5 @@ > + if (formCtrl && formCtrl->GetType() == NS_FORM_INPUT_BUTTON) { > + nsCOMPtr<nsIDOMElement> element = do_QueryInterface(formCtrl); > + nsIFocusManager* fm = nsFocusManager::GetFocusManager(); > + if (fm && element) > + fm->SetFocus(element, 0); Keep the style like this: if (foo) { bar; } ::: content/html/content/src/nsHTMLMediaElement.cpp @@ +2506,5 @@ > nsIContent* nsHTMLMediaElement::GetNextSource() > { > + nsCOMPtr<nsIDOMNode> thisDomNode = do_QueryObject(this); > + > + mSourceLoadCandidate = nsnull; I do not see the point in this change... having |thisDomNode| and |mSourceLoadCandidate| being set before or after |rv| doesn't change anything here. ::: content/html/content/src/nsHTMLSelectElement.cpp @@ +1234,5 @@ > NS_IMETHODIMP > nsHTMLSelectElement::SetValue(const nsAString& aValue) > { > + PRUint32 length; > + nsresult rv = GetLength(&length); Could you put an empty line before this line. @@ +1235,5 @@ > nsHTMLSelectElement::SetValue(const nsAString& aValue) > { > + PRUint32 length; > + nsresult rv = GetLength(&length); > + NS_ENSURE_SUCCESS(rv, rv); And an empty line after this one. @@ +1243,2 @@ > > + if (NS_SUCCEEDED(rv) && node) { if (NS_FAILED(rv) || !node) { continue; } would be better @@ +1246,2 @@ > > + if (option) { if (!option) { continue; } @@ +1251,2 @@ > > + if (optionVal.Equals(aValue)) { You could use continue here too but it would be less useful than for the other two. @@ +1919,4 @@ > { > + switch (aType) { > + case VALIDITY_STATE_VALUE_MISSING: { > + nsXPIDLString message; Keep: switch (foo) { case BAR: { } case FOOBAR: { } } ::: content/html/content/src/nsHTMLStyleElement.cpp @@ +177,2 @@ > } > + return ss->GetDisabled(aDisabled); Could you add an empty line before this line. @@ +187,2 @@ > } > + return ss->SetDisabled(aDisabled); Could you add an empty line before this line. ::: content/html/content/src/nsHTMLTableElement.cpp @@ +391,5 @@ > + if (caption) { > + return caption.forget(); > + } > + } > + return NULL; nsnull maybe? @@ +540,2 @@ > > + if (nsRefPtr<nsIDOMHTMLTableSectionElement> head = GetTHead()) { Please, don't do that... @@ +589,2 @@ > > + if (nsRefPtr<nsIDOMHTMLTableSectionElement> foot = GetTFoot()) { ditto ::: content/html/content/src/nsHTMLTextAreaElement.cpp @@ -1566,5 @@ > UpdateBarredFromConstraintValidation(); > > nsGenericHTMLFormElement::FieldSetDisabledChanged(aNotify); > } > - I liked those empty lines at the end of files :(
Attachment #570550 -
Flags: review?(mounir) → review+
Assignee | ||
Comment 4•13 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #3) > Comment on attachment 570550 [details] [diff] [review] [diff] [details] [review] > @@ +1022,5 @@ > > + return NS_OK; > > + } > > + > > + nsCOMPtr<nsIDOMHTMLElement> domElement = do_QueryInterface(element); > > + domElement.forget(aValue); > > Do you really want to change: > CallQueryInterface(elem, aValue); > to this these two lines? > > Do we want to prevent CallQueryInterface as much as possible? I prefer this style, but it doesn't matter much, I guess. Your call. > ::: content/html/content/src/nsHTMLSelectElement.cpp > @@ +1234,5 @@ > > NS_IMETHODIMP > > nsHTMLSelectElement::SetValue(const nsAString& aValue) > > { > > + PRUint32 length; > > + nsresult rv = GetLength(&length); > > Could you put an empty line before this line. Haven't done this, to keep the declaration next to its getter. Addressed your other comments.
Comment 5•13 years ago
|
||
(In reply to Ms2ger from comment #4) > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #3) > > Comment on attachment 570550 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review] > > @@ +1022,5 @@ > > > + return NS_OK; > > > + } > > > + > > > + nsCOMPtr<nsIDOMHTMLElement> domElement = do_QueryInterface(element); > > > + domElement.forget(aValue); > > > > Do you really want to change: > > CallQueryInterface(elem, aValue); > > to this these two lines? > > > > Do we want to prevent CallQueryInterface as much as possible? > > I prefer this style, but it doesn't matter much, I guess. Your call. Let's not change the blame then.
Assignee | ||
Comment 6•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ba55b19797a9
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
Target Milestone: --- → mozilla11
You need to log in
before you can comment on or make changes to this bug.
Description
•