Bug 40453 – FrameLoader cleanup: Pull subframe and plugin request functions into a separate class

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

There are a series of functions that are sort of entry points into FrameLoader that are specifically related to requesting plugin objects or subframes but that don't have an dependencies on private FrameLoader members (except m_frame and m_client).  I don't think there's any reason they can't be in a separate class.

[Nate Chapin]

Created attachment 58500 [details]
patch

[Adam Barth]

Comment on attachment 58500 [details]
patch

I like the idea.  The high-level question is whether we should move this over to the DocLoader side of things.  We can do that separately though.

WebCore/GNUmakefile.am:1423
 +      WebCore/loader/SinkDocument.h \
Bad indent here?

WebCore/loader/FrameLoader.cpp:181
 +      , m_subframeLoader(frame, client)
We usually don't give the subobjects pointers to the client.  They can get the client via m_frame->loader()->client().

WebCore/loader/FrameLoader.cpp:1268
 +  void FrameLoader::setIsComplete(bool isComplete)
This seems sort of low-level.

WebCore/loader/SubframeLoader.cpp:73
 +          || node->hasTagName(appletTag));
Should be on one line.

[Adam Barth]

Ideally, Darin would have time to review this too.

[Nate Chapin]

(In reply to comment #2)
> (From update of attachment 58500 [details])
> I like the idea.  The high-level question is whether we should move this over to the DocLoader side of things.  We can do that separately though.
> 
> WebCore/GNUmakefile.am:1423
>  +      WebCore/loader/SinkDocument.h \
> Bad indent here?

That file appears to be mostly tabs with some 4-indent spacing mixed in.  I thought I left it as it was, but I can try to fix it if you like.

> WebCore/loader/FrameLoader.cpp:1268
>  +  void FrameLoader::setIsComplete(bool isComplete)
> This seems sort of low-level.

I could make FrameLoader::started() non-private instead or move a chunk of this function back into FrameLoader.   Either of those sound more appealing?

[Adam Barth]

> I could make FrameLoader::started() non-private instead or move a chunk of this function back into FrameLoader.   Either of those sound more appealing?

I'm not sure.  I'd have to look into the details.  Use your best judgement here and I'll review your patch in detail later.

[Nate Chapin]

(In reply to comment #5)
> > I could make FrameLoader::started() non-private instead or move a chunk of this function back into FrameLoader.   Either of those sound more appealing?
> 
> I'm not sure.  I'd have to look into the details.  Use your best judgement here and I'll review your patch in detail later.

Ok, I think I'm going to experiment with making started() public.  I probably won't have time to get a new patch out this afternoon and I'm out on Monday, so I'll plan on posting a new version on Tuesday.

[Nate Chapin]

Created attachment 58810 [details]
patch #2

[Early Warning System Bot]

Attachment 58810 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3321193

[WebKit Review Bot]

Attachment 58810 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3298191

[Nate Chapin]

Created attachment 58818 [details]
Fix qt compile issue

[Adam Barth]

Comment on attachment 58818 [details]
Fix qt compile issue

I think you uploaded the wrong patch.  This one seems to be about image loads...

[Nate Chapin]

Created attachment 59117 [details]
a relevant patch

Sorry about that.  Too many clients on this machine, grabbed from the wrong one.

[Adam Barth]

Comment on attachment 59117 [details]
a relevant patch

Wow, that's amazing.  I wish I could see the diff of what you changed in each function more easily.  In the past, I've done these patches in two steps:

1) In place in FrameLoader.cpp so you can see how the code is changing.
2) Moving the next class to its own file.

In any case, it's crazy how there's practically zero communication from FrameLoader to SubframeLoader.  Nice patch.

WebCore/ChangeLog:11
 +          * Android.mk:
Darin would ask you for comments next to each of these entries to explain what you're doing.  I'm bad at writing those comments too though.

WebCore/WebCore.base.exp: 
 +  __ZNK7WebCore11FrameLoader15containsPluginsEv
I'm surprised you don't need to add the new version of this symbol as an export.  Please make sure this compiles on Mac before landing.

WebCore/GNUmakefile.am:1424
 +      WebCore/loader/SinkDocument.h \
Bad indent.  Please fix before landing.

[Nate Chapin]

http://trac.webkit.org/changeset/61584