[webkit-reviews] review denied: [Bug 12279] Two pass loading is needed when the network has high latency and low bandwidth : [Attachment 13483] patch for ToT (rev 19972)

[bugzilla-request-daemon at macosforge.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 12279: Two pass loading is needed when the network has high latency and low
bandwidth
http://bugs.webkit.org/show_bug.cgi?id=12279

Attachment 13483: patch for ToT (rev 19972)
http://bugs.webkit.org/attachment.cgi?id=13483&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This looks great to me!

Very interesting work that will probably require more than one pass of review,
and from more than just me. Here are my comments from my first time reading
through:

Why are we turning the double tree on for the Macintosh platform? Is this
feature helpful for the Macintosh desktop browser? If it is, then what browser
would ever want this feature off? What about other WebKit clients on Macintosh?
Maybe we need a way to opt in per-WebView for compatibility with existing
programs that won't expect this behavior, like Dashboard for example.

I don't think it's clear that inFastDisplay is realted to the feature called
DOUBLE_TREE. In particular, it doesn't seem to have anything to do with the
name "double tree".Similarly, nothing about a function named needToSwitch or a
data member named m_needSwitch connects it with the fast display or double tree
names. The function name switchToFinalDocIfReady also is not clearly connected
to the double tree or to fast mode. The names are too vague given context.
There is more than one kind of switch that occurs in a frame loader, for
example the switch from provisional state to committed state.

I'd like to see the various names -- for the entire feature, the mode where you
haven't yet loaded enough to do "real" display, the transition from one mode to
the other, and the two documents -- that are clearly related. Today the feature
overall is called "double tree", the early mode is called "fast display", the
transition is called "switch" and "switch to final doc".

+#if USE(DOUBLE_TREE)
+#include "Frame.h"
+#include "FrameLoader.h"
+#endif

This is a strange change in Cache.cpp. That file already includes FrameLoader.h
so we don't need to include it again.

Also, we typically only put #if around includes in cases where we have to, like
major platform differences. This ifdef change would not require an ifdef.

+#if USE(DOUBLE_TREE)	     
+    CachedResourceClientWalker n(m_clients);
+    while (CachedResourceClient *c = n.next())
+	 c->notifyFinished(this);
+#endif        

No comment here to indicate why this is correct behavior. I guess the rule is
that CSS always pretends to be done loading immediately, but that's not obvious
so it needs some kind of comment.

This also has formatting. The * should be next to the class name,
CachedResourceClient, not the variable name, c. The surrounding code was
already incorrect; it's older code that we haven't gotten too yet. It's better
to do it our new way for new code.

+    void replaceDoc(Document* doc) { m_doc = doc; }

Please name this replaceDocument, rather than replaceDoc. We're trying to
abbreviate less. Even the class name, DocLoader, is slated to be changed
(actually, we're going to merge the two classes DocLoader and DocumentLoader).

+    if (document->isHTMLDocument() &&

Why is this check, in FrameLoader::begin, correct? Why don't we want to do this
for non-HTML documents?

+	 if (m_needSwitch)
+	     return;
+	 else
+	     switchToFinalDocIfReady();

This code shows some confusion of terminology. When I read that if I think that
is says: "if you need switch don't switch, but if you don't need a switch go
ahead and switch if ready".

Also, we don't use else statements after return in new WebKit code.

+    switch(cache->type()) {

We put space after switch keywords before te parentheses.

+	 case CachedResource::CSSStyleSheet:
+	 case CachedResource::Script:
+	     m_needSwitch = true;
+	     m_CachedRequests.add(cache);
+	     cache->ref(this);
+	     return true;
+	 default:
+	 break;
+    }

The break should be indented after default, but more importantly, we normally
list all the cases in switch statements like this one. That way the gcc
compiler can detect missing switch statements. This is very helpful when adding
a new enumeration value. It helps you spot switch statements and make a
decision about the right behavior. So please add the other cases here.

m_CachedRequests should not have a capital "C". But also I don't understand in
what sense these requests are "cached". Is the cache in "cached" a reference to
the WebCore::Cache object? I think we need a better name for these than "cached
requests"; ideally one that makes it clear how these relate to the double tree,
fast mode, and decision about when to switch.

+void FrameLoader::removeAllCachedRequests()
+{
+    if (!m_CachedRequests.isEmpty()) {
+	 HashSet<CachedResource*>::iterator end = m_CachedRequests.end();
+	 for (HashSet<CachedResource*>::iterator it = m_CachedRequests.begin();
it != end; ++it)
+	     (*it)->deref(this);
+	 m_CachedRequests.clear();	  
+    }
+}

The isEmpty() check here is unnecessary. It will be quite efficient to just
call end(), begin(), and clear() even if the set is empty.

+    if (m_CachedRequests.contains(script)) {
+	 HashSet<CachedResource*>::iterator it = m_CachedRequests.find(script);

+	 (*it)->deref(this);
+	 m_CachedRequests.remove(it);
+	 switchToFinalDocIfReady();	   
+    }

This can be written more efficiently to do only a single hash table lookup.

    HashSet<CachedResource*>::iterator it = m_CachedRequests.find(script);
    if (it != m_CachedRequests.end()) {
	(*it)->deref(this);
	m_CachedRequests.remove(it);
	switchToFinalDocIfReady();	  
    }

+#define MAX_SRC_LENGTH  128*1024

Since this is C++, we use const for things like this rather than #define. And
thus we don't use all capitals either. And we put such constants at the top of
the file rather than in between functions.

I think that FrameLoader::switchToFinalDocIfReady has too much repeated code
that's also in FrameLoader::begin. FrameLoader has some bad stuff like that in
it already, but I'd like to avoid adding more. Instead we should consider a
small bit of refactoring so we can share as much of the code as possible.

+	     oldDoc->removeAllEventListenersFromAllNodes();

Why is this helpful? Who could have added event listeners? Do we actually let
JavaScript run in the "fast display" mode? If so, how can that work? Wouldn't
at least some documents have a problem if they were loaded twice?

+#if USE(DOUBLE_TREE)
+    class FrameLoader : Noncopyable, CachedResourceClient {
+#else

I'd like this to say "private CachedResourceClient". It's correct to use
private inheritance for this, but it should be explicit. The one case where we
don't bother saying "private" explicitly is for Noncopyable, because in that
one special case it doesn't matter if it's private or public inheritance. So it
sets a bad example!

+	 // implementation for CachedResourceClient	   
+	 virtual void notifyFinished(CachedResource*);

This function should be private. We privately inherit from
CachedResourceClient.

+	 bool addCachedRequest(CachedResource*);
+	 void removeAllCachedRequests();
+	 
+	 void setUseFastDisplay(bool fastDisplay) { m_useFastDisplay =
fastDisplay; }
+	 void needToSwitch() { m_needSwitch = true; }
+	 
+	 void switchToFinalDocIfReady();	

Can some of these functions be private? We want everything private unless it
needs to be public.

+	 // whether to use fastDislay, set by client

Typo "fast dislay".

+	 String m_pendingSrc;	     

We try to avoid abbreviations where possible, and "src" here is pretty terse.

What kinds of testing have you done so far on this? Is there any way to write
tests to make sure we exercise all the code paths?

review- because at least some of these comments will need to be addressed
before we can land