518711 - break up nsHTMLDocument::StartDocumentLoad

Status: NEW

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

Description

[Robert Sayre]

This method is difficult to refactor because it's really long and complicated.

Comment 1

[Robert Sayre]

Attachment: break it up

Comment 2

[Robert Sayre]

Comment on attachment 402702 [details] [diff] [review]
break it up

actually, this is going to be full of nits -- need to use content/ style. fix in a bit.

Comment 4

[Robert Sayre]

Attachment: break it up

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 402868 [details] [diff] [review]
break it up

>diff --git a/content/html/document/src/nsHTMLDocument.cpp b/content/html/document/src/nsHTMLDocument.cpp
>   if (!(contentType.EqualsLiteral("text/html") && aCommand && !nsCRT::strcmp(aCommand, "view"))) {
>     loadAsHtml5 = PR_FALSE;
>   }
>-#ifdef DEBUG
>-  else {
>-    NS_ASSERTION(mIsRegularHTML,
>-                 "Hey, someone forgot to reset mIsRegularHTML!!!");
>-  }
>-#endif
>-
>+
>+  #ifdef DEBUG
>+    else {
>+      NS_ASSERTION(mIsRegularHTML,
>+                   "Hey, someone forgot to reset mIsRegularHTML!!!");
>+    }
>+  #endif

The old location and indentation seemed better here. In general, separating the |else| from the |if| is a bad idea.

>   CSSLoader()->SetCaseSensitive(!IsHTML());
>   CSSLoader()->SetCompatibilityMode(mCompatMode);
>   
>-  PRBool needsParser = PR_TRUE;
>-  if (aCommand)
>-  {
>-    if (!nsCRT::strcmp(aCommand, "view delayedContentLoad")) {
>-      needsParser = PR_FALSE;
>-    }
>-  }
>-
>+  return loadAsHtml5;
>+}
>+
>+already_AddRefed<nsICacheEntryDescriptor>
>+nsHTMLDocument::InitCacheDescriptor(nsIChannel* aChannel)

Can you make this a static helper function, not living on the nsHTMLDocument class?

r/sr=me

Comment 6

[Robert Sayre]

Attachment: add a null check, still asserts

this asserts that the content sink isn't consuming data somewhere, looking into it

Comment 7

[Robert Sayre]

Attachment: works, addresses comments

The problem was that I needed to invert the check for parser creation, after removing the "needsParser" variable.

Comment 8

[Robert Sayre]

review ping

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 404525 [details] [diff] [review]
works, addresses comments

>+nsHTMLDocument::SetupParseMode(const char* aCommand, nsIChannel* aChannel,
>   if (!(contentType.EqualsLiteral("text/html") && aCommand && !nsCRT::strcmp(aCommand, "view"))) {
>     loadAsHtml5 = PR_FALSE;
>   }
>-#ifdef DEBUG
>-  else {
>-    NS_ASSERTION(mIsRegularHTML,
>-                 "Hey, someone forgot to reset mIsRegularHTML!!!");
>-  }
>-#endif
>-
>+
>+  #ifdef DEBUG
>+    else {
>+      NS_ASSERTION(mIsRegularHTML,
>+                   "Hey, someone forgot to reset mIsRegularHTML!!!");
>+    }
>+  #endif
>+  

I'd still prefer not to make this change. It seems bad for all sorts of reasons.

r/sr=me with that reverted.

Comment 10

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

Was this ever checked in, and if not, why not?

Comment 11

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

Yearly ping