Add loadedNonEmptyDocument flag in FrameLoader for Resource Timing

Add loadedNonEmptyDocument flag in iframe for Resource Timing

Sometimes iframe src is blank in markup, and then will be set a real
URL via script, Resource Timing would like to populate resource fetched from 
the real URL, but avoid the self navigation after that, so this change add
a flag loadedNonEmptyDocument to achieve that.

BUG=236180

[Pan, 2013-04-28 05:08:53]

Hi japhet,

could you please review it?

thanks!
Pan

[eseidel, 2013-04-28 08:00:59]

I don't like this.  But it's not so much your fault as it is that this is part
of the general trend in the loader to use a zillion bools to represent a state
machine. :)  We need one giant loader state machine enum.  This is just
recording if the loader ever passed a specific state it seems.

[eseidel, 2013-04-28 08:01:48]

If nate thinks this is OK, that's all that matters.  He is the man with the plan
for the loader, and I certainly will not stand in his (or this patches) way if
this aligns with his vision! :)

[Nate Chapin, 2013-04-29 13:16:05]

On 2013/04/28 08:01:48, Eric Seidel (Google) wrote:
> If nate thinks this is OK, that's all that matters.  He is the man with the
plan
> for the loader, and I certainly will not stand in his (or this patches) way if
> this aligns with his vision! :)

I've actually just started looking again at how to make FrameLoader's state
machine sane, so I'm hesitant to add this bool at the moment. If you have a test
case that demonstrates why you feel this bool is necessary, please send it my
way so I can try to make sure I considered it in my cleanup efforts.

[James Simonsen, 2013-04-29 21:05:16]

On 2013/04/29 13:16:05, Nate Chapin wrote:
> On 2013/04/28 08:01:48, Eric Seidel (Google) wrote:
> > If nate thinks this is OK, that's all that matters.  He is the man with the
> plan
> > for the loader, and I certainly will not stand in his (or this patches) way
if
> > this aligns with his vision! :)
> 
> I've actually just started looking again at how to make FrameLoader's state
> machine sane, so I'm hesitant to add this bool at the moment. If you have a
test
> case that demonstrates why you feel this bool is necessary, please send it my
> way so I can try to make sure I considered it in my cleanup efforts.

It's for Resource Timing. If an iframe starts out with no src attribute, then
one is added later, we want to record the latter. In terms of FrameLoader, that
means the initial load of about:blank shouldn't count, but the first non-blank
load should. The state machine can't express that as is.

[Nate Chapin, 2013-04-29 21:11:13]

On 2013/04/29 21:05:16, James Simonsen wrote:
> On 2013/04/29 13:16:05, Nate Chapin wrote:
> > On 2013/04/28 08:01:48, Eric Seidel (Google) wrote:
> > > If nate thinks this is OK, that's all that matters.  He is the man with
the
> > plan
> > > for the loader, and I certainly will not stand in his (or this patches)
way
> if
> > > this aligns with his vision! :)
> > 
> > I've actually just started looking again at how to make FrameLoader's state
> > machine sane, so I'm hesitant to add this bool at the moment. If you have a
> test
> > case that demonstrates why you feel this bool is necessary, please send it
my
> > way so I can try to make sure I considered it in my cleanup efforts.
> 
> It's for Resource Timing. If an iframe starts out with no src attribute, then
> one is added later, we want to record the latter. In terms of FrameLoader,
that
> means the initial load of about:blank shouldn't count, but the first non-blank
> load should. The state machine can't express that as is.

Unless Eric objects, I would suggest storing this state on the parent
HTMLIFrameElement, rather than on FrameLoader. This contains the information to
the case where it's actually required, and spares you our collective wrath for
increasing the scope of FrameLoader :)

[eseidel, 2013-04-29 21:14:50]

Seems fine to add this bool to the iframe.  Assuming it has the callbacks to
know when this occurs.

[Pan, 2013-04-30 10:06:12]

thanks everybody.
The flag now is stored in iframe, it won't add the FrameLoader state complexity
as before.
@japhet, I check/set the state at all the place FrameState Machine advance to
"DisplayingInitialEmptyDocumentPostCommit", I'm not sure it is enough or some
may useless.

Once we make this change, two resource timing cases(diff in TestExpections) will
pass.

thanks!
Pan

[Nate Chapin, 2013-04-30 19:55:58]

https://codereview.chromium.org/14054023/diff/9001/Source/core/html/HTMLFrame...
File Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/14054023/diff/9001/Source/core/html/HTMLFrame...
Source/core/html/HTMLFrameOwnerElement.h:64: virtual void
didLoadNonEmptyDocument() {}
Hmm...I'm not sure whether it makes more sense to have this be virtual or just
have the boolean live down here on HTMLFrameOwnerElement.

https://codereview.chromium.org/14054023/diff/9001/Source/core/loader/FrameLo...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/14054023/diff/9001/Source/core/loader/FrameLo...
Source/core/loader/FrameLoader.cpp:508:
m_frame->ownerElement()->didLoadNonEmptyDocument();
Is there a case where we would load the first non-empty document by a means
other than setting the HTMLIFrameElement's src attribute? If not, could we just
set this boolean when we set the src to something non-empty?

https://codereview.chromium.org/14054023/diff/9001/Source/core/loader/cache/C...
File Source/core/loader/cache/CachedResourceLoader.cpp (left):

https://codereview.chromium.org/14054023/diff/9001/Source/core/loader/cache/C...
Source/core/loader/cache/CachedResourceLoader.cpp:490: if
(frame()->ownerElement() &&
m_documentLoader->frameLoader()->stateMachine()->committingFirstRealLoad()) {
I think this is the only use of committingFirstRealLoad(). Would you mind
checking and deleting it if that is the case?

[Pan, 2013-05-07 08:45:52]

https://codereview.chromium.org/14054023/diff/9001/Source/core/html/HTMLFrame...
File Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/14054023/diff/9001/Source/core/html/HTMLFrame...
Source/core/html/HTMLFrameOwnerElement.h:64: virtual void
didLoadNonEmptyDocument() {}
On 2013/04/30 19:55:58, Nate Chapin wrote:
> Hmm...I'm not sure whether it makes more sense to have this be virtual or just
> have the boolean live down here on HTMLFrameOwnerElement.

HTMLFrameOwnerElement has derived and derived derived class other
HTMLIframeElement, so I suggest keep the interface here, and make the Boolean
live in HTMLIframeElement only， to less affect other element.

https://codereview.chromium.org/14054023/diff/9001/Source/core/loader/FrameLo...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/14054023/diff/9001/Source/core/loader/FrameLo...
Source/core/loader/FrameLoader.cpp:508:
m_frame->ownerElement()->didLoadNonEmptyDocument();
On 2013/04/30 19:55:58, Nate Chapin wrote:
> Is there a case where we would load the first non-empty document by a means
> other than setting the HTMLIFrameElement's src attribute? If not, could we
just
> set this boolean when we set the src to something non-empty?
Yep, for example in iframe, it use "window.location = ..." which can initialize
a navigation, but this action can not be caught in HTMLIFrameElement AFAIK.

https://codereview.chromium.org/14054023/diff/9001/Source/core/loader/cache/C...
File Source/core/loader/cache/CachedResourceLoader.cpp (left):

https://codereview.chromium.org/14054023/diff/9001/Source/core/loader/cache/C...
Source/core/loader/cache/CachedResourceLoader.cpp:490: if
(frame()->ownerElement() &&
m_documentLoader->frameLoader()->stateMachine()->committingFirstRealLoad()) {
On 2013/04/30 19:55:58, Nate Chapin wrote:
> I think this is the only use of committingFirstRealLoad(). Would you mind
> checking and deleting it if that is the case?

yes, right, glad to remove :)

[Pan, 2013-05-07 09:57:15]

japhet,

This time I set the Boolean when a non-empty m_provisionalDocumentLoader loader
is back (I guess it will be back to m_documentLoader no matter provisional
loader fail or success, right?). 
Please review. :)

Thanks
Pan

[Nate Chapin, 2013-05-07 16:40:55]

https://codereview.chromium.org/14054023/diff/20001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/14054023/diff/20001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:574: if
(frame()->ownerElement() && !frame()->ownerElement()->loadedNonEmptyDocument())
{
Could we set loadedNonEmptyDocument here, rather than in FrameLoader? Or maybe
in CachedResourceLoader::loadDone()? The 5-case if statement in otherwise
unrelated code in FrameLoader worries me.

[Pan, 2013-05-07 23:39:27]

https://codereview.chromium.org/14054023/diff/20001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/14054023/diff/20001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:574: if
(frame()->ownerElement() && !frame()->ownerElement()->loadedNonEmptyDocument())
{
On 2013/05/07 16:40:55, Nate Chapin wrote:
> Could we set loadedNonEmptyDocument here, rather than in FrameLoader? Or maybe
> in CachedResourceLoader::loadDone()? The 5-case if statement in otherwise
> unrelated code in FrameLoader worries me.

great, why not? I was in trouble to find a place in FrameLoader yesterday, make
them them together is really good. thanks! :)

[Pan, 2013-05-07 23:47:14]

japhet,

ready to review, thanks! :)

Pan

[Nate Chapin, 2013-05-08 16:06:55]

Almost there :)

https://codereview.chromium.org/14054023/diff/26001/Source/core/html/HTMLIFra...
File Source/core/html/HTMLIFrameElement.h (right):

https://codereview.chromium.org/14054023/diff/26001/Source/core/html/HTMLIFra...
Source/core/html/HTMLIFrameElement.h:53: virtual void didLoadNonEmptyDocument()
OVERRIDE{ m_didLoadNonEmptyDocument = true; }
Space after OVERRIDE I believe.

https://codereview.chromium.org/14054023/diff/26001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/14054023/diff/26001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:578:
if(!frame()->ownerElement()->loadedNonEmptyDocument() &&
!m_documentLoader->requestURL().isBlankURL() && !m_documentLoader->
requestURL().isEmpty())
I think this if isn't necessary. DocumentLoader should handle empty and blank
loads without consulting CachedResourceLoader. See
DocumentLoader::shouldLoadEmpty(). Also, loadedNonEmptyDocument() was just
checked above.

[Pan, 2013-05-08 23:19:41]

> OVERRIDE{ m_didLoadNonEmptyDocument = true; }
> Space after OVERRIDE I believe.
> 
right, done.

>
https://codereview.chromium.org/14054023/diff/26001/Source/core/loader/cache/...
> File Source/core/loader/cache/CachedResourceLoader.cpp (right):
> 
>
https://codereview.chromium.org/14054023/diff/26001/Source/core/loader/cache/...
> Source/core/loader/cache/CachedResourceLoader.cpp:578:
> if(!frame()->ownerElement()->loadedNonEmptyDocument() &&
> !m_documentLoader->requestURL().isBlankURL() && !m_documentLoader->
> requestURL().isEmpty())
> I think this if isn't necessary. DocumentLoader should handle empty and blank
> loads without consulting CachedResourceLoader. See
> DocumentLoader::shouldLoadEmpty(). Also, loadedNonEmptyDocument() was just
> checked above.

yep, thanks! :)

Pan

[Nate Chapin, 2013-05-08 23:20:48]

lgtm

[Pan, 2013-05-15 05:11:24]

landed in https://chromiumcodereview.appspot.com/14632007/.