Remove unnecessary null checking in NavigatorContentUtils

Remove unnecessary null checking in NavigatorContentUtils

Some functions have checked if document is null. However, document is not null when frame is existed.

BUG=none

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

[gyuyoung-inactive, 2014-04-07 09:22:33]

We need to add a null checking condition because frame()->document() can return
a null.

Document* Frame::document() const
{
    return m_domWindow ? m_domWindow->document() : 0;
}

[haraken, 2014-04-07 09:23:27]

Can you add a test for the change?

[haraken, 2014-04-07 09:25:27]

As far as I see other Navigator modules (NavigatorBattery, NavigatorGamepad,
NavigatorMediaStream etc), it seems that navigator.frame()->document() doesn't
become null.

[gyuyoung-inactive, 2014-04-07 10:04:08]

On 2014/04/07 09:25:27, haraken wrote:
> As far as I see other Navigator modules (NavigatorBattery, NavigatorGamepad,
> NavigatorMediaStream etc), it seems that navigator.frame()->document() doesn't
> become null.

I don't find why it isn't null yet. 

If it is always not null, how about removing an existing null checking in
registerProtocolHandler() ? 
It is unnecessary.

void NavigatorContentUtils::registerProtocolHandler(Navigator& navigator, const
String& scheme, const String& url, const String& title, ExceptionState&
exceptionState)
{
    Document* document = navigator.frame()->document();
    if (!document)
        return;

...

[gyuyoung-inactive, 2014-04-07 12:01:21]

On 2014/04/07 10:04:08, gyuyoung wrote:
> On 2014/04/07 09:25:27, haraken wrote:
> > As far as I see other Navigator modules (NavigatorBattery, NavigatorGamepad,
> > NavigatorMediaStream etc), it seems that navigator.frame()->document()
doesn't
> > become null.
> 
> I don't find why it isn't null yet. 
> 
> If it is always not null, how about removing an existing null checking in
> registerProtocolHandler() ? 
> It is unnecessary.
> 
> void NavigatorContentUtils::registerProtocolHandler(Navigator& navigator,
const
> String& scheme, const String& url, const String& title, ExceptionState&
> exceptionState)
> {
>     Document* document = navigator.frame()->document();
>     if (!document)
>         return;
> 
> ...

Anyway, this isn't urgent. So, I would like to find why
navigator.frame()->document() is not null further.

[gyuyoung-inactive, 2014-04-16 06:11:22]

I remove the null checking in WebKit - http://trac.webkit.org/changeset/167308.

According to Darin's comment, document can be null are very early in the frame
lifetime.
https://bugs.webkit.org/show_bug.cgi?id=131652#c3

Kentaro, how do you think about this comment ?

[haraken, 2014-04-16 07:11:09]

On 2014/04/16 06:11:22, gyuyoung wrote:
> I remove the null checking in WebKit -
http://trac.webkit.org/changeset/167308.
> 
> According to Darin's comment, document can be null are very early in the frame
> lifetime.
> https://bugs.webkit.org/show_bug.cgi?id=131652#c3
> 
> Kentaro, how do you think about this comment ?

Can you add a test case or point out any call path in which that happens?

- If the document can be null, we should add the same null check to other
supplement objects of Navigator.

- If the document cannot be null, we should add ASSERT(document).

[gyuyoung-inactive, 2014-04-17 06:26:15]

On 2014/04/16 07:11:09, haraken wrote:
> On 2014/04/16 06:11:22, gyuyoung wrote:
> > I remove the null checking in WebKit -
> http://trac.webkit.org/changeset/167308.
> > 
> > According to Darin's comment, document can be null are very early in the
frame
> > lifetime.
> > https://bugs.webkit.org/show_bug.cgi?id=131652#c3
> > 
> > Kentaro, how do you think about this comment ?
> 
> Can you add a test case or point out any call path in which that happens?

LocalFrame returns a document object when m_domWindow exists as below,

    Document* LocalFrame::document() const
    {
        return m_domWindow ? m_domWindow->document() : 0;
    }

It looks m_domWindow only can be set by setDOMWindow() as below,

    void LocalFrame::setDOMWindow(PassRefPtrWillBeRawPtr<DOMWindow> domWindow)
    {
        InspectorInstrumentation::frameWindowDiscarded(this, m_domWindow.get());
        if (domWindow)
            script().clearWindowShell();
        Frame::setDOMWindow(domWindow);
    }


When I search all places which call setDOMWindow(),
*DocumentLoader::createWriteFor()* only calls it.

    PassRefPtr<DocumentWriter> DocumentLoader::createWriterFor(LocalFrame*
frame, const Document* ownerDocument, const KURL& url, const AtomicString&
mimeType, const AtomicString& encoding, bool userChosen, bool dispatch)
    {
        // In some rare cases, we'll re-used a DOMWindow for a new Document. For
example,
        // when a script calls window.open("..."), the browser gives JavaScript
a window
        // synchronously but kicks off the load in the window asynchronously.
Web sites
        // expect that modifications that they make to the window object
synchronously
        // won't be blown away when the network load commits. To make that
happen, we
        // "securely transition" the existing DOMWindow to the Document that
results from
        // the network load. See also SecurityContext::isSecureTransitionTo.
        bool shouldReuseDefaultView =
frame->loader().stateMachine()->isDisplayingInitialEmptyDocument() &&
frame->document()->isSecureTransitionTo(url);
        ...
        if (!shouldReuseDefaultView)
            frame->setDOMWindow(DOMWindow::create(*frame));

        RefPtr<Document> document =
frame->domWindow()->installNewDocument(mimeType, init);
        ...
    }


When shouldReuseDefaultView is false, setDOMWindow() isn't called. However, the
flag means that existing DOMWindow is re-used for a new Document. Anyway, after
DocumentLoader decides which one it uses, new document is attached to the
DOMWindow by "frame->domWindow()->installNewDocument(mimeType, init)" as below,

    PassRefPtr<Document> DOMWindow::installNewDocument(const String& mimeType,
const DocumentInit& init, bool forceXHTML)
    {
        ASSERT(init.frame() == m_frame);

        clearDocument();

        m_document = createDocument(mimeType, init, forceXHTML);
        ...
    }

Because DOMWindow::installNewDocument() always creates new document, document
isn't null if DOMWindow is exist. 

And to conclude, DOMWindow can't be null as well as it has always a document.

[gyuyoung-inactive, 2014-04-18 03:30:54]

Previous my comment looks too complicated to read. I think Frame(or LocalFrame)
can't be null because a frame should has a domwindow, and domwindow should has a
document. Below functions do it. Please take a look those functions.

- LocalFrame::document()
- LocalFrame::setDOMWindow()
- DocumentLoader::createWriterFor()
- DOMWindow::installNewDocument()

[haraken, 2014-04-18 03:41:17]

LGTM.

[gyuyoung-inactive, 2014-04-18 03:51:24]

The CQ bit was checked by gyuyoung.kim@samsung.com

[commit-bot: I haz the power, 2014-04-18 03:51:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gyuyoung.kim@samsung.com/224733013/50001

[commit-bot: I haz the power, 2014-04-18 05:04:04]

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

[commit-bot: I haz the power, 2014-04-18 05:04:04]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[gyuyoung-inactive, 2014-04-18 05:47:02]

The CQ bit was checked by gyuyoung.kim@samsung.com

[commit-bot: I haz the power, 2014-04-18 05:47:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gyuyoung.kim@samsung.com/224733013/50001

[commit-bot: I haz the power, 2014-04-18 06:55:16]

Change committed as 171928