Refactor FrameLoader loading interface

Refactor FrameLoader loading interface

This CLs removes the reload and loadHistoryItem functions of FrameLoader in
favor of a unified load function. It also updates the public interface of
WebFrame to generate WebURLRequests for history navigations and reloads. The
end goal is to be able to only have a single function for loading in WebFrame
as well.

BUG=490713
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196753

[clamy, 2015-05-27 14:42:54]

clamy@chromium.org changed reviewers:
+ avi@chromium.org, carlosk@chromium.org, creis@chromium.org,
fdegans@chromium.org, japhet@chromium.org, nasko@chromium.org

[clamy, 2015-05-27 14:44:04]

@avi, creis, japhet: PTAL. This follows the design doc I sent you a few weeks
ago
(https://docs.google.com/document/d/1GkhqwLY56ZOvQPnsWLfIEbenl1Xxqbr2RCX8hyCsg...).
@nasko, carlosk, fdegans: FYI

[dcheng, 2015-05-27 16:26:30]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2015-05-27 16:26:30]

https://codereview.chromium.org/1156473002/diff/80001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/1156473002/diff/80001/public/web/WebFrame.h#n...
public/web/WebFrame.h:355: enum WebFrameLoadType {
enum class for new enums please. Consider just moving this enum into a
standalone file as well.

https://codereview.chromium.org/1156473002/diff/80001/public/web/WebFrame.h#n...
public/web/WebFrame.h:356: WebFrameLoadTypeStandard,
Then these can just be "Standard", "Forward", etc.

https://codereview.chromium.org/1156473002/diff/80001/public/web/WebFrame.h#n...
public/web/WebFrame.h:363: WebFrameLoadTypeInitialHistoryLoad,
Since we're adding comments anyway... what's the "initial history load"?

https://codereview.chromium.org/1156473002/diff/80001/public/web/WebFrame.h#n...
public/web/WebFrame.h:364: WebFrameLoadTypeReloadFromOrigin,
How is this different from WebFrameLoadTypeReload?

https://codereview.chromium.org/1156473002/diff/80001/public/web/WebFrame.h#n...
public/web/WebFrame.h:367: virtual WebURLRequest RequestFromHistoryItem(const
WebHistoryItem&, WebURLRequest::CachePolicy)
Blink naming convention is lowerCamelCase for method names.

https://codereview.chromium.org/1156473002/diff/80001/public/web/WebFrame.h#n...
public/web/WebFrame.h:384: virtual void loadRequest(const WebURLRequest&,
WebFrameLoadType = WebFrameLoadTypeStandard,
It seems like all the newly added methods should only be on WebLocalFrame.

[Nate Chapin, 2015-05-27 16:54:34]

https://codereview.chromium.org/1156473002/diff/80001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1156473002/diff/80001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:261: FrameLoadType loadType = reloadPolicy ==
NormalReload ?
We should probably just get rid of the ReloadPolicy enum?

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:847:
request.resourceRequest().setHasUserGesture(UserGestureIndicator::processingUserGesture());
Why move this?

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:861: FrameLoadType newLoadType =
frameLoadType;
FrameLoadType newLoadType = (frameLoadType == FrameLoadTypeStandard) ?
determineFrameLoadType(request) : frameLoadType;

?

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:886: if ((policy ==
NavigationPolicyCurrentTab && newLoadType != FrameLoadTypeReloadFromOrigin
Add the FrameLoadTypeReloadFromOrigin case to shouldPerformFragmentNavigation()
and remove the ASSERT(loadType != FrameLoadTypeReloadFromOrigin).

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:903: && isBackForwardLoadType(frameLoadType)
Why this change?

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.h:70: static ResourceRequest
resourceRequestForReload(HistoryItem*, const LocalFrame&, FrameLoadType,
I'm not sure I like this as a static method, given it's dependence on a Frame
and the HistoryItem (which is always(?) m_currentItem).

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
File Source/core/loader/FrameLoaderTypes.h (right):

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
Source/core/loader/FrameLoaderTypes.h:37: FrameLoadTypeHistorySameDocument,
If we're going to add a new type here instead of returning the HistoryLoadType
enum, FrameLoadTypeBackForward should get renamed to
FrameLoadTypeHistoryDifferentDocument. Given how similarly they're handled,
though, I think it might be better to not add a FrameLoadType, keep the
HistoryLoadType enum and store it on FrameLoadRequest.

[Avi (use Gerrit), 2015-05-27 18:13:40]

FYI, I don't have knowledge of Blink, so while this all looks plausible to me
I'm not able to intelligently comment.

[Charlie Reis, 2015-05-27 23:36:37]

I'm concerned about the Chrome side of this patch
(https://codereview.chromium.org/1157863005/), since it depends on things that
are changing significantly for OOPIF session history.

I haven't looked closely at the Blink side, but we may want to discuss/clarify
the overall plan further before landing this half.

[clamy, 2015-05-29 11:16:19]

Patchset #7 (id:120001) has been deleted

[clamy, 2015-05-29 14:41:51]

Thanks!

https://codereview.chromium.org/1156473002/diff/80001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1156473002/diff/80001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:261: FrameLoadType loadType = reloadPolicy ==
NormalReload ?
On 2015/05/27 16:54:33, Nate Chapin wrote:
> We should probably just get rid of the ReloadPolicy enum?

Should we change it to a FrameLoadType instead?

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:847:
request.resourceRequest().setHasUserGesture(UserGestureIndicator::processingUserGesture());
On 2015/05/27 16:54:34, Nate Chapin wrote:
> Why move this?

Done.

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:861: FrameLoadType newLoadType =
frameLoadType;
On 2015/05/27 16:54:34, Nate Chapin wrote:
> FrameLoadType newLoadType = (frameLoadType == FrameLoadTypeStandard) ?
> determineFrameLoadType(request) : frameLoadType;
> 
> ?

Done.

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:886: if ((policy ==
NavigationPolicyCurrentTab && newLoadType != FrameLoadTypeReloadFromOrigin
On 2015/05/27 16:54:34, Nate Chapin wrote:
> Add the FrameLoadTypeReloadFromOrigin case to
shouldPerformFragmentNavigation()
> and remove the ASSERT(loadType != FrameLoadTypeReloadFromOrigin).

Done.

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:903: && isBackForwardLoadType(frameLoadType)
On 2015/05/27 16:54:34, Nate Chapin wrote:
> Why this change?

Actually I meant to write !isBackForwardLoadType(frameLoadType). It's weird no
test caught that.

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.h:70: static ResourceRequest
resourceRequestForReload(HistoryItem*, const LocalFrame&, FrameLoadType,
On 2015/05/27 16:54:34, Nate Chapin wrote:
> I'm not sure I like this as a static method, given it's dependence on a Frame
> and the HistoryItem (which is always(?) m_currentItem).

Switched to a non-static function.

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
File Source/core/loader/FrameLoaderTypes.h (right):

https://codereview.chromium.org/1156473002/diff/80001/Source/core/loader/Fram...
Source/core/loader/FrameLoaderTypes.h:37: FrameLoadTypeHistorySameDocument,
On 2015/05/27 16:54:34, Nate Chapin wrote:
> If we're going to add a new type here instead of returning the HistoryLoadType
> enum, FrameLoadTypeBackForward should get renamed to
> FrameLoadTypeHistoryDifferentDocument. Given how similarly they're handled,
> though, I think it might be better to not add a FrameLoadType, keep the
> HistoryLoadType enum and store it on FrameLoadRequest.

I went with providing the HistoryLoadType to the load method.

https://codereview.chromium.org/1156473002/diff/80001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/1156473002/diff/80001/public/web/WebFrame.h#n...
public/web/WebFrame.h:355: enum WebFrameLoadType {
On 2015/05/27 16:26:30, dcheng wrote:
> enum class for new enums please. Consider just moving this enum into a
> standalone file as well.

Done.

https://codereview.chromium.org/1156473002/diff/80001/public/web/WebFrame.h#n...
public/web/WebFrame.h:356: WebFrameLoadTypeStandard,
On 2015/05/27 16:26:30, dcheng wrote:
> Then these can just be "Standard", "Forward", etc.

Done.

https://codereview.chromium.org/1156473002/diff/80001/public/web/WebFrame.h#n...
public/web/WebFrame.h:363: WebFrameLoadTypeInitialHistoryLoad,
On 2015/05/27 16:26:30, dcheng wrote:
> Since we're adding comments anyway... what's the "initial history load"?

Done.

https://codereview.chromium.org/1156473002/diff/80001/public/web/WebFrame.h#n...
public/web/WebFrame.h:364: WebFrameLoadTypeReloadFromOrigin,
On 2015/05/27 16:26:30, dcheng wrote:
> How is this different from WebFrameLoadTypeReload?

Done.

https://codereview.chromium.org/1156473002/diff/80001/public/web/WebFrame.h#n...
public/web/WebFrame.h:367: virtual WebURLRequest RequestFromHistoryItem(const
WebHistoryItem&, WebURLRequest::CachePolicy)
On 2015/05/27 16:26:30, dcheng wrote:
> Blink naming convention is lowerCamelCase for method names.

Done.

https://codereview.chromium.org/1156473002/diff/80001/public/web/WebFrame.h#n...
public/web/WebFrame.h:384: virtual void loadRequest(const WebURLRequest&,
WebFrameLoadType = WebFrameLoadTypeStandard,
On 2015/05/27 16:26:30, dcheng wrote:
> It seems like all the newly added methods should only be on WebLocalFrame.

Done.

[clamy, 2015-06-02 16:48:24]

@dcheng, creis, japhet: PTAL now that we agree on the chromium side patch.

[Charlie Reis, 2015-06-02 17:10:10]

On 2015/06/02 16:48:24, clamy wrote:
> @dcheng, creis, japhet: PTAL now that we agree on the chromium side patch.

I'm ok with the chromium side of this now, so I'll defer to dcheng@ and japhet@
for this one.

[Nate Chapin, 2015-06-02 17:19:35]

https://codereview.chromium.org/1156473002/diff/80001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1156473002/diff/80001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:261: FrameLoadType loadType = reloadPolicy ==
NormalReload ?
On 2015/05/29 14:41:50, clamy wrote:
> On 2015/05/27 16:54:33, Nate Chapin wrote:
> > We should probably just get rid of the ReloadPolicy enum?
> 
> Should we change it to a FrameLoadType instead?

Yeah, that was my thought.

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:455: if (m_provisionalItem &&
isBackForwardLoadType(m_loadType)) {
Nit: No {} needed

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:875: if (isBackForwardLoadType(newLoadType)
&& historyLoadType == HistorySameDocumentLoad) {
Is it possible to collapse this block into the same-document block below?

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.h:78: ResourceRequest
resourceRequestForReload(const LocalFrame&, FrameLoadType,
No need to take the LocalFrame& now, we can just use m_frame?

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Nav...
File Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Nav...
Source/core/loader/NavigationScheduler.cpp:180: if
(!frame->loader().currentItem())
Null check the ResourceRequest instead of the current item here? It looks
mysterious if you don't know that reloads are initialized based on data from the
current history item.

https://codereview.chromium.org/1156473002/diff/140001/Source/web/WebLocalFra...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1156473002/diff/140001/Source/web/WebLocalFra...
Source/web/WebLocalFrameImpl.cpp:969: if
(resourceRequest.url().protocolIs("javascript") && webFrameLoadType == Standard)
{
Are there any situations where this will get called with
RedirectWithLockedBackForwardList?

[dcheng, 2015-06-02 18:42:03]

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.h (left):

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.h:240: visitor->trace(m_item);
Why was this removed? This probably breaks Oilpan.

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.h:255: bool m_isValid;
Rather than having a m_isValid member, I'd rather see m_deferredHistoryLoad
wrapped in an OwnPtr<>.

https://codereview.chromium.org/1156473002/diff/140001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/1156473002/diff/140001/public/web/WebFrame.h#...
public/web/WebFrame.h:367: WebHistoryLoadType = WebHistoryDifferentDocumentLoad)
= 0;
Since we're changing this interface anyway, any chance we can just define the
new interface on WebLocalFrame and have callers switch to that, so we can remove
WebFrame::loadRequest completely?

That way, we won't need the default parameters for compile compatibility across
Blink rolls either.

https://codereview.chromium.org/1156473002/diff/140001/public/web/WebFrameLoa...
File public/web/WebFrameLoadType.h (right):

https://codereview.chromium.org/1156473002/diff/140001/public/web/WebFrameLoa...
public/web/WebFrameLoadType.h:14: enum WebFrameLoadType {
enum class

[clamy, 2015-06-03 14:39:15]

Thanks!

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:455: if (m_provisionalItem &&
isBackForwardLoadType(m_loadType)) {
On 2015/06/02 17:19:35, Nate Chapin wrote:
> Nit: No {} needed

Done.

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:875: if (isBackForwardLoadType(newLoadType)
&& historyLoadType == HistorySameDocumentLoad) {
On 2015/06/02 17:19:35, Nate Chapin wrote:
> Is it possible to collapse this block into the same-document block below?

Done.

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.h (left):

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.h:240: visitor->trace(m_item);
On 2015/06/02 18:42:02, dcheng wrote:
> Why was this removed? This probably breaks Oilpan.

Done.

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.h:78: ResourceRequest
resourceRequestForReload(const LocalFrame&, FrameLoadType,
On 2015/06/02 17:19:35, Nate Chapin wrote:
> No need to take the LocalFrame& now, we can just use m_frame?

Done.

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.h:255: bool m_isValid;
On 2015/06/02 18:42:02, dcheng wrote:
> Rather than having a m_isValid member, I'd rather see m_deferredHistoryLoad
> wrapped in an OwnPtr<>.

Done.

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Nav...
File Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/1156473002/diff/140001/Source/core/loader/Nav...
Source/core/loader/NavigationScheduler.cpp:180: if
(!frame->loader().currentItem())
On 2015/06/02 17:19:35, Nate Chapin wrote:
> Null check the ResourceRequest instead of the current item here? It looks
> mysterious if you don't know that reloads are initialized based on data from
the
> current history item.

Done.

https://codereview.chromium.org/1156473002/diff/140001/Source/web/WebLocalFra...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1156473002/diff/140001/Source/web/WebLocalFra...
Source/web/WebLocalFrameImpl.cpp:969: if
(resourceRequest.url().protocolIs("javascript") && webFrameLoadType == Standard)
{
On 2015/06/02 17:19:35, Nate Chapin wrote:
> Are there any situations where this will get called with
> RedirectWithLockedBackForwardList?

I don't think so. Added an ASSERT to check that it never happens.

https://codereview.chromium.org/1156473002/diff/140001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/1156473002/diff/140001/public/web/WebFrame.h#...
public/web/WebFrame.h:367: WebHistoryLoadType = WebHistoryDifferentDocumentLoad)
= 0;
On 2015/06/02 18:42:02, dcheng wrote:
> Since we're changing this interface anyway, any chance we can just define the
> new interface on WebLocalFrame and have callers switch to that, so we can
remove
> WebFrame::loadRequest completely?
> 
> That way, we won't need the default parameters for compile compatibility
across
> Blink rolls either.

Done. I've also reordered the new methods in WeblocalFrameImpl so that they fall
under the WebLocalFrame implementation.

https://codereview.chromium.org/1156473002/diff/140001/public/web/WebFrameLoa...
File public/web/WebFrameLoadType.h (right):

https://codereview.chromium.org/1156473002/diff/140001/public/web/WebFrameLoa...
public/web/WebFrameLoadType.h:14: enum WebFrameLoadType {
On 2015/06/02 18:42:02, dcheng wrote:
> enum class

Done.

[Nate Chapin, 2015-06-04 23:23:10]

Almost there, just a couple of nitpick. Sorry for the slow turnaround.

https://codereview.chromium.org/1156473002/diff/180001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1156473002/diff/180001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:260: FrameLoadType loadType = reloadPolicy ==
NormalReload ?
ReloadPolicy is still used here, is that an oversight or an intentional
decision?

https://codereview.chromium.org/1156473002/diff/180001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1156473002/diff/180001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:883: ClientRedirectPolicy clientRedirect =
sameDocumentHistoryNavigation ?
I think (though I'm not absolutely sure) that a history navigation will never be
flagged as a ClientRedirect, in which case this conditional is unnecessary.

https://codereview.chromium.org/1156473002/diff/180001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/1156473002/diff/180001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.h:70: static FrameLoadRequest
frameRequestForReload(
I'm not convinced this helper does enough to be worthwhile, but I don't feel
strongly.

https://codereview.chromium.org/1156473002/diff/180001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/1156473002/diff/180001/public/web/WebFrame.h#...
public/web/WebFrame.h:46: #include "public/web/WebFrameLoadType.h"
Why these #includes?

[clamy, 2015-06-05 14:11:17]

clamy@chromium.org changed reviewers:
+ mkwst@chromium.org

[clamy, 2015-06-05 14:11:48]

Thanks!
@mkwst: PTAL at the changes in Source/modules

https://codereview.chromium.org/1156473002/diff/180001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1156473002/diff/180001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:260: FrameLoadType loadType = reloadPolicy ==
NormalReload ?
On 2015/06/04 23:23:09, Nate Chapin wrote:
> ReloadPolicy is still used here, is that an oversight or an intentional
> decision?

Done. This was an oversight :).

https://codereview.chromium.org/1156473002/diff/180001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1156473002/diff/180001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:883: ClientRedirectPolicy clientRedirect =
sameDocumentHistoryNavigation ?
On 2015/06/04 23:23:09, Nate Chapin wrote:
> I think (though I'm not absolutely sure) that a history navigation will never
be
> flagged as a ClientRedirect, in which case this conditional is unnecessary.

Done (if the tests appear to fail I will bring it back).

https://codereview.chromium.org/1156473002/diff/180001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/1156473002/diff/180001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.h:70: static FrameLoadRequest
frameRequestForReload(
On 2015/06/04 23:23:09, Nate Chapin wrote:
> I'm not convinced this helper does enough to be worthwhile, but I don't feel
> strongly.

I removed it.

https://codereview.chromium.org/1156473002/diff/180001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/1156473002/diff/180001/public/web/WebFrame.h#...
public/web/WebFrame.h:46: #include "public/web/WebFrameLoadType.h"
On 2015/06/04 23:23:09, Nate Chapin wrote:
> Why these #includes?

There were left-overs. Removed them.

[Nate Chapin, 2015-06-05 18:16:30]

LGTM

https://codereview.chromium.org/1156473002/diff/220001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/1156473002/diff/220001/Source/core/frame/Fram...
Source/core/frame/Frame.h:73: virtual void reload(FrameLoadType,
ClientRedirectPolicy) = 0;
if FrameLoadType is used here, it should probably move to FrameTypes.h. That can
happen in a different change, though.

[clamy, 2015-06-08 15:30:25]

@mkwst: friendly ping to look at Source/modules/plugins/DOMPluginArray.cpp :).

[Mike West, 2015-06-08 18:46:57]

Source/modules LGTM. Sorry for the slow turnaround!

[clamy, 2015-06-08 19:06:52]

Thanks!

[clamy, 2015-06-08 19:07:01]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-06-08 19:07:02]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1156473002/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-08 19:07:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1156473002/240001

[commit-bot: I haz the power, 2015-06-08 19:11:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-08 19:11:04]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/58074)

[clamy, 2015-06-09 09:46:48]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-06-09 09:46:49]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1156473002/#ps260001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-09 09:46:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1156473002/260001

[commit-bot: I haz the power, 2015-06-09 11:01:05]

Committed patchset #13 (id:260001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196753