1442499 - EditorBase::IsPreformatted() should return bool

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement, P3)

Description

[Makoto Kato [:m_kato]]

To get nsStyleContext, it is unnecessary to use PresShell, so we can use bool as return type.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1442499 - EditorBase::IsPreformatted should return bool.

It is unnecessary to get PresShell for nsStyleText, so we can return bool
instead of nsresult and can change it to static method.

Review commit: https://reviewboard.mozilla.org/r/237094/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/237094/

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8968397 [details]
Bug 1442499 - EditorBase::IsPreformatted should return bool.

https://reviewboard.mozilla.org/r/237094/#review242872

::: editor/libeditor/EditorBase.cpp:4026
(Diff revision 1)
>   */
> -nsresult
> -EditorBase::IsPreformatted(nsIDOMNode* aNode,
> -                           bool* aResult)
> +// static
> +bool
> +EditorBase::IsPreformatted(nsINode* aNode)
>  {
> -  nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
> +  if (!aNode) {

NS_WARN_IF? Or, use nsINode& for aNode?

::: editor/libeditor/HTMLEditRules.cpp:5995
(Diff revision 1)
> -    bool isPRE;
> +    if (EditorBase::IsPreformatted(nextNode)) {
> -    htmlEditor->IsPreformatted(nextNode->AsDOMNode(), &isPRE);
> -    if (isPRE) {
>        if (EditorBase::IsTextNode(nextNode)) {
>          nsAutoString tempString;
>          nextNode->GetAsText()->GetData(tempString);
>          int32_t newlinePos = tempString.FindChar(nsCRT::LF);
>          if (newlinePos >= 0) {
>            if (static_cast<uint32_t>(newlinePos) + 1 == tempString.Length()) {
>              // No need for special processing if the newline is at the end.
>              break;
>            }
>            return EditorDOMPoint(nextNode, newlinePos + 1);
>          }
>        }
>      }

Could you connect those conditions with && and reduce the indent?

Comment 3

[Makoto Kato [:m_kato]]

Comment on attachment 8968397 [details]
Bug 1442499 - EditorBase::IsPreformatted should return bool.

https://reviewboard.mozilla.org/r/237094/#review242984

::: editor/libeditor/EditorBase.cpp:4026
(Diff revision 1)
>   */
> -nsresult
> -EditorBase::IsPreformatted(nsIDOMNode* aNode,
> -                           bool* aResult)
> +// static
> +bool
> +EditorBase::IsPreformatted(nsINode* aNode)
>  {
> -  nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
> +  if (!aNode) {

I will use NS_WARN_IF

Comment 4

[Makoto Kato [:m_kato]]

Comment on attachment 8968397 [details]
Bug 1442499 - EditorBase::IsPreformatted should return bool.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237094/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/767f9e9dd582
EditorBase::IsPreformatted should return bool. r=masayuki

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/767f9e9dd582