698275 - Various cleanup in HTML element implementations

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Various stuff that has been sitting in my queue for a while. Hope you like it.

Comment 1

[Jean-Yves Perrier [:teoli]]

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

[Mounir Lamouri (:mounir)]

(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

[Mounir Lamouri (:mounir)]

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 :(

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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

[Mounir Lamouri (:mounir)]

(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.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/ba55b19797a9