Open Bug 518711 Opened 13 years ago Updated 3 years ago

break up nsHTMLDocument::StartDocumentLoad

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

People

(Reporter: sayrer, Unassigned)

References

Details

Attachments

(1 file, 3 obsolete files)

This method is difficult to refactor because it's really long and complicated.
Blocks: 102699
Attached patch break it up (obsolete) — Splinter Review
Assignee: nobody → sayrer
Attachment #402702 - Flags: superreview?(jst)
Attachment #402702 - Flags: review?(jst)
Blocks: 518860
Attachment #402702 - Flags: superreview?(jst)
Attachment #402702 - Flags: review?(jst)
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.
Duplicate of this bug: 518860
Attached patch break it up (obsolete) — Splinter Review
Summary: break up first bit of nsHTMLDocument::StartDocumentLoad → break up nsHTMLDocument::StartDocumentLoad
Attachment #402868 - Flags: superreview?(jonas)
Attachment #402868 - Flags: review?(jonas)
Attachment #402702 - Attachment is obsolete: true
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
Attachment #402868 - Flags: superreview?(jonas)
Attachment #402868 - Flags: superreview+
Attachment #402868 - Flags: review?(jonas)
Attachment #402868 - Flags: review+
Attached patch add a null check, still asserts (obsolete) — Splinter Review
this asserts that the content sink isn't consuming data somewhere, looking into it
The problem was that I needed to invert the check for parser creation, after removing the "needsParser" variable.
Attachment #402868 - Attachment is obsolete: true
Attachment #404511 - Attachment is obsolete: true
Attachment #404525 - Flags: superreview?(jonas)
Attachment #404525 - Flags: review?(jonas)
review ping
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.
Attachment #404525 - Flags: superreview?(jonas)
Attachment #404525 - Flags: superreview+
Attachment #404525 - Flags: review?(jonas)
Attachment #404525 - Flags: review+
Was this ever checked in, and if not, why not?
Yearly ping
Component: DOM → DOM: Core & HTML
Assignee: sayrer → nobody
You need to log in before you can comment on or make changes to this bug.