Stop resource loading for WebCore::MediaDocument...

Stop resource loading for WebCore::MediaDocument

BUG=17973
When a WebCore::Document is loaded, a resource request
for the document is made. In case of MediaDocument the
request for the media file is made separately through
<video> and so the DocumentLoader should be stopped.
This change mimic what is done in WebHTMLRepresentation
(which is similar to our RenderView) and stops the
resource loading in WebFrameLoaderClient since RenderView
doesn't have access to WebCore types.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=23471

[Alpha Left Google, 2009-08-11 01:58:58]

I referenced the code in WebKit (the real WebKit mac port) to make this patch. I
feel a little strange about how the request is canceled, please correct me if
I'm wrong > darin

[Alpha Left Google, 2009-08-11 01:59:34]

The code to WebHTMLRepresentation.mm:
http://trac.webkit.org/browser/trunk/WebKit/mac/WebView/WebHTMLRepresentation.mm

[scherkus (not reviewing), 2009-08-11 17:30:28]

change appears logical

deferring to darin for final LGTM

don't forget TEST=

http://codereview.chromium.org/165286/diff/1/2
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/165286/diff/1/2#newcode980
Line 980: if (webframe_->frame()->document() &&
I have no idea what this is supposed to do, so a quick 1-line comment or so will
do :)

http://codereview.chromium.org/165286/diff/1/2#newcode983
Line 983: loader->documentLoader()->cancelMainResourceLoad(
since it's webkit style, you don't need to be <= 80 characters

pick what looks best

http://codereview.chromium.org/165286/diff/1/2#newcode985
Line 985: loader->documentLoader()->response()));
indent by another two spaces

[Alpha Left Google, 2009-08-12 00:38:51]

http://codereview.chromium.org/165286/diff/1/2
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/165286/diff/1/2#newcode980
Line 980: if (webframe_->frame()->document() &&
On 2009/08/11 17:30:28, scherkus wrote:
> I have no idea what this is supposed to do, so a quick 1-line comment or so
will
> do :)

Done.

http://codereview.chromium.org/165286/diff/1/2#newcode983
Line 983: loader->documentLoader()->cancelMainResourceLoad(
On 2009/08/11 17:30:28, scherkus wrote:
> since it's webkit style, you don't need to be <= 80 characters
> 
> pick what looks best

it's still in googleland, so I'll keep it within 80 chars.

http://codereview.chromium.org/165286/diff/1/2#newcode985
Line 985: loader->documentLoader()->response()));
On 2009/08/11 17:30:28, scherkus wrote:
> indent by another two spaces

Done.

[Alpha Left Google, 2009-08-12 00:54:11]

I'm pulling this patch because it has a bug refreshing a page of MediaDocument

[Alpha Left Google, 2009-08-12 01:15:20]

It turns out that the ordering of method calls was wrong. Now I follow strictly
that is done in WebHTMLRepresentation.mm and the problem during refresh is gone.

Good for review again.

[scherkus (not reviewing), 2009-08-12 02:15:09]

I guess LGTM but still would like darin to take a peek

[darin (slow to review), 2009-08-13 23:48:12]

http://codereview.chromium.org/165286/diff/1008/9
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/165286/diff/1008/9#newcode990
Line 990: loader->documentLoader()->cancelMainResourceLoad(
can't you just use the "DocumentLoader* loader" argument instead of re-computing
it?

if you are stopping this resource load, does that mean that you are separately
fetching the resource?  so, we download the movie twice, cancelling the first
download if we get to this code?

[Alpha Left Google, 2009-08-13 23:59:21]

http://codereview.chromium.org/165286/diff/1008/9
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/165286/diff/1008/9#newcode990
Line 990: loader->documentLoader()->cancelMainResourceLoad(
Done about using DocumentLoad* loader instead.

DocumentLoader loads the movie the first time and soon MediaDocument is
constructed with a <video>. The document itself doesn't know how to render the
data so it has to be canceled. Video tag fetches it for the second time. This
only applies to the case when you enter the movie url in the address bar.

[darin (slow to review), 2009-08-14 00:02:16]

On Thu, Aug 13, 2009 at 4:59 PM, <hclam@chromium.org> wrote:

>
> http://codereview.chromium.org/165286/diff/1008/9
> File webkit/glue/webframeloaderclient_impl.cc (right):
>
> http://codereview.chromium.org/165286/diff/1008/9#newcode990
> Line 990: loader->documentLoader()->cancelMainResourceLoad(
> Done about using DocumentLoad* loader instead.
>
> DocumentLoader loads the movie the first time and soon MediaDocument is
> constructed with a <video>. The document itself doesn't know how to
> render the data so it has to be canceled. Video tag fetches it for the
> second time. This only applies to the case when you enter the movie url
> in the address bar.
>
>
I see.  That makes sense.  Note: This code will also be reached when an
IFRAME has its src set to point at a video.

-Darin



>
> http://codereview.chromium.org/165286
>

[Alpha Left Google, 2009-08-14 00:09:12]

On 2009/08/14 00:02:16, darin wrote:
> On Thu, Aug 13, 2009 at 4:59 PM, <hclam@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.org/165286/diff/1008/9
> > File webkit/glue/webframeloaderclient_impl.cc (right):
> >
> > http://codereview.chromium.org/165286/diff/1008/9#newcode990
> > Line 990: loader->documentLoader()->cancelMainResourceLoad(
> > Done about using DocumentLoad* loader instead.
> >
> > DocumentLoader loads the movie the first time and soon MediaDocument is
> > constructed with a <video>. The document itself doesn't know how to
> > render the data so it has to be canceled. Video tag fetches it for the
> > second time. This only applies to the case when you enter the movie url
> > in the address bar.
> >
> >
> I see.  That makes sense.  Note: This code will also be reached when an
> IFRAME has its src set to point at a video.
> 

That's correct. About where the loader should be canceled, we can't cancel it in
WebCore::MediaTokenizer, what webkit mac port does it that the frame loader
client cancels it (as in WebHTMLRepresentation.mm).


> -Darin
> 
> 
> 
> >
> > http://codereview.chromium.org/165286
> >
>

[darin (slow to review), 2009-08-14 00:15:37]

http://codereview.chromium.org/165286/diff/13/14
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/165286/diff/13/14#newcode985
Line 985: // If we are sending data to WebCore::MediaDocument, we should stop
here
One more tweak I would make:  move this code to the top of the function and
return after cancelling the main resource load.  There is no reason to call
DidReceiveDocumentData in this case, right?  Or, does that initial bit of
document data cause the isMediaDocument flag to be set?

[Alpha Left Google, 2009-08-14 00:32:13]

http://codereview.chromium.org/165286/diff/13/14
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/165286/diff/13/14#newcode985
Line 985: // If we are sending data to WebCore::MediaDocument, we should stop
here
On 2009/08/14 00:15:37, darin wrote:
> One more tweak I would make:  move this code to the top of the function and
> return after cancelling the main resource load.  There is no reason to call
> DidReceiveDocumentData in this case, right?  Or, does that initial bit of
> document data cause the isMediaDocument flag to be set?

Yes, it is the first bit of data that triggers the isMediaDocument(), if we move
this block up and return it will actually cause rendering problem (a red page is
displayed).

[darin (slow to review), 2009-08-14 07:30:44]

http://codereview.chromium.org/165286/diff/13/14
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/165286/diff/13/14#newcode985
Line 985: // If we are sending data to WebCore::MediaDocument, we should stop
here
On 2009/08/14 00:32:13, Alpha wrote:
> On 2009/08/14 00:15:37, darin wrote:
> > One more tweak I would make:  move this code to the top of the function and
> > return after cancelling the main resource load.  There is no reason to call
> > DidReceiveDocumentData in this case, right?  Or, does that initial bit of
> > document data cause the isMediaDocument flag to be set?
> 
> Yes, it is the first bit of data that triggers the isMediaDocument(), if we
move
> this block up and return it will actually cause rendering problem (a red page
is
> displayed). 

OK, that makes sense.  Thanks!