Cleanup, added a helper to FrameView, isMainFrame(). No behavior change

Cleanup, added a helper to FrameView, isMainFrame(). No behavior change

BUG=

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

[eseidel, 2013-09-10 16:15:44]

lgtm

Seems reasonable.  Adam probably remembers these lifetimes better than I do of
the top of his head.

https://codereview.chromium.org/23654013/diff/1/Source/core/page/FrameView.cpp
File Source/core/page/FrameView.cpp (left):

https://codereview.chromium.org/23654013/diff/1/Source/core/page/FrameView.cp...
Source/core/page/FrameView.cpp:188: if (!page)
Is it OK to lose this early return?

https://codereview.chromium.org/23654013/diff/1/Source/core/page/FrameView.cp...
Source/core/page/FrameView.cpp:2577: if (!page)
OK to remove this?  I think the Frame loses its Page during teardown.  I'm not
sure when else page() can be null.

[bokan, 2013-09-10 16:19:22]

https://codereview.chromium.org/23654013/diff/1/Source/core/page/FrameView.cpp
File Source/core/page/FrameView.cpp (left):

https://codereview.chromium.org/23654013/diff/1/Source/core/page/FrameView.cp...
Source/core/page/FrameView.cpp:188: if (!page)
On 2013/09/10 16:15:44, eseidel wrote:
> Is it OK to lose this early return?
isMainFrame checks if page == NULL so this early return becomes implicit.

https://codereview.chromium.org/23654013/diff/1/Source/core/page/FrameView.cp...
Source/core/page/FrameView.cpp:2577: if (!page)
On 2013/09/10 16:15:44, eseidel wrote:
> OK to remove this?  I think the Frame loses its Page during teardown.  I'm not
> sure when else page() can be null.
Ditto here

[eseidel, 2013-09-10 16:22:17]

https://codereview.chromium.org/23654013/diff/1/Source/core/page/FrameView.cpp
File Source/core/page/FrameView.cpp (left):

https://codereview.chromium.org/23654013/diff/1/Source/core/page/FrameView.cp...
Source/core/page/FrameView.cpp:188: if (!page)
On 2013/09/10 16:19:22, bokan wrote:
> On 2013/09/10 16:15:44, eseidel wrote:
> > Is it OK to lose this early return?
> isMainFrame checks if page == NULL so this early return becomes implicit.

Yes, but it doesn't exit. I don't have full context in this diff to see if
control flow continues after the ScrollableArea:: block.  There could be later
code which assumes page was already checked, no?  I should just fine a larger
context... :)

https://codereview.chromium.org/23654013/diff/1/Source/core/page/FrameView.cp...
Source/core/page/FrameView.cpp:2577: if (!page)
On 2013/09/10 16:19:22, bokan wrote:
> On 2013/09/10 16:15:44, eseidel wrote:
> > OK to remove this?  I think the Frame loses its Page during teardown.  I'm
not
> > sure when else page() can be null.
> Ditto here

You're right.  This one does return still when !page.  my bad.

[eseidel, 2013-09-10 16:23:23]

https://codereview.chromium.org/23654013/diff/1/Source/core/page/FrameView.cpp
File Source/core/page/FrameView.cpp (left):

https://codereview.chromium.org/23654013/diff/1/Source/core/page/FrameView.cp...
Source/core/page/FrameView.cpp:188: if (!page)
On 2013/09/10 16:22:17, eseidel wrote:
> On 2013/09/10 16:19:22, bokan wrote:
> > On 2013/09/10 16:15:44, eseidel wrote:
> > > Is it OK to lose this early return?
> > isMainFrame checks if page == NULL so this early return becomes implicit.
> 
> Yes, but it doesn't exit. I don't have full context in this diff to see if
> control flow continues after the ScrollableArea:: block.  There could be later
> code which assumes page was already checked, no?  I should just fine a larger
> context... :)

Yeah, looking at larger context, this is wrong.  The autosizing code can below
can now be hit with a null page.  We won't do the scrollalblearea updates, but
we will continue on to the autosizing code.

not lgtm.

[eseidel, 2013-09-10 16:25:02]

lgtm

Bah.  I expanded the wrong context.  This function does return immediately after
the ScrollableArea block.

I would still change it to early return, as that's generally preferred:

if (!mainFrame())
    return;

ScrolableArea::...

But yes, you're right, the code is still the same.  Sorry for the confusion.

[bokan, 2013-09-10 16:27:33]

On 2013/09/10 16:25:02, eseidel wrote:
> lgtm
> 
> Bah.  I expanded the wrong context.  This function does return immediately
after
> the ScrollableArea block.
> 
> I would still change it to early return, as that's generally preferred:
> 
> if (!mainFrame())
>     return;
> 
> ScrolableArea::...
> 
> But yes, you're right, the code is still the same.  Sorry for the confusion.

Had me doubting myself :P. Changed to an early return.

[eseidel, 2013-09-10 16:37:45]

lgtm

Thanks for the very nice cleanup!

[commit-bot: I haz the power, 2013-09-10 16:38:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/23654013/9001

[commit-bot: I haz the power, 2013-09-10 18:17:00]

Change committed as 157533