Bug 39636 – Move functions out of Frame class that were marked "move to Chrome"

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Darin Adler]

Move functions out of Frame class that were marked "move to Chrome"

[Darin Adler]

Created attachment 56954 [details]
Patch

[WebKit Review Bot]

Attachment 56954 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 56954 [details]
Patch

WebCore/ChangeLog:8
 +  	Refactoring that does not require new tests.
Evil tab

WebCore/loader/FrameLoader.cpp:692
 +              // FIXME: This is only needed if we reuse the same DOMWindow for multiple URLs.
We do that in the case of fragments but not for URLs that are "really" different.

WebCore/page/DOMWindow.h:418
 +      inline String DOMWindow::status() const
Maybe this should be on one line to save space?

[Darin Adler]

Adam, did you review the patch? Could you either set it to review+, set it to review-, or state that you think someone else should review?

[Darin Adler]

Created attachment 58693 [details]
Patch

[Chris Jerdonek]

A couple comments while waiting for Adam to respond to comment 4.

> +        Move functions out of Frame class that were marked "move to Chrome"
> +        https://bugs.webkit.org/show_bug.cgi?id=39636

Might it be worth adding a comment saying why no functions are actually getting
added to Chrome (or anywhere for that matter)?

> @@ -681,8 +680,10 @@ bool FrameLoader::didOpenURL(const KURL&
>      // its frame is not in a consistent state for rendering, so avoid setJSStatusBarText

Looks like you need to update this comment text.

>      // since it may cause clients to attempt to render the frame.
>      if (!m_creatingInitialEmptyDocument) {
> -        m_frame->setJSStatusBarText(String());
> -        m_frame->setJSDefaultStatusBarText(String());
> +        if (DOMWindow* window = m_frame->existingDOMWindow()) {

Is it possible there is a behavior change here since it doesn't look like
the current code needs the DOMWindow to exist for it to do something.

> +            window->setStatus(String());
> +            window->setDefaultStatus(String());

>      // Delete old status bar messages (if it _was_ activated on last URL).
>      if (m_frame->script()->canExecuteScripts(NotAboutToExecuteScript)) {
> -        m_frame->setJSStatusBarText(String());
> -        m_frame->setJSDefaultStatusBarText(String());
> +        if (DOMWindow* window = m_frame->existingDOMWindow()) {

Same question as above.

> +            window->setStatus(String());
> +            window->setDefaultStatus(String());

[Adam Barth]

Sorry, my comments were a drive-by.  I'll review it this afternoon.

[Adam Barth]

Comment on attachment 58693 [details]
Patch

WebCore/loader/FrameLoader.cpp:683
 +          if (DOMWindow* window = m_frame->existingDOMWindow()) {
I guess we don't care about the cases when the frame doesn't have a DOMWindow?

WebCore/page/DOMWindow.cpp:767
 +      if (!m_frame)
Don't we test for this above?

WebCore/page/DOMWindow.cpp:803
 +      if (!(page->openedByDOM() || page->getHistoryLength() <= 1 || allowScriptsToCloseWindows))
I think this would be clearer if you pushed the ! inside the ||

WebCore/page/DOMWindow.h:418
 +      inline String DOMWindow::status() const
I suspect the "inline" here is redundant.  Also, we commonly put these trivial accessors on one line.

WebCore/page/DOMWindow.h:423
 +      inline String DOMWindow::defaultStatus() const
ditto

WebKit/win/WebView.cpp: 
 +          *result = frame->shouldClose() ? TRUE : FALSE;
Do we not need this type conversion?  I don't really understand what Windows needs here..

[Darin Adler]

(In reply to comment #8)
> (From update of attachment 58693 [details])
> WebCore/loader/FrameLoader.cpp:683
>  +          if (DOMWindow* window = m_frame->existingDOMWindow()) {
> I guess we don't care about the cases when the frame doesn't have a DOMWindow?

Yes. This code resets the status string. When there is no DOM window we’re guaranteed the status string is not set.

> WebCore/page/DOMWindow.cpp:767
>  +      if (!m_frame)
> Don't we test for this above?

Yes, but the code calls focus and that can trigger events and the JavaScript event handlers can do almost anything. By the time we return, m_frame may be 0.

> WebCore/page/DOMWindow.cpp:803
>  +      if (!(page->openedByDOM() || page->getHistoryLength() <= 1 || allowScriptsToCloseWindows))
> I think this would be clearer if you pushed the ! inside the ||

That’s funny. I thought it was clearer not to do that! Let me think about other ways to make it clear.

> WebCore/page/DOMWindow.h:418
>  +      inline String DOMWindow::status() const
> I suspect the "inline" here is redundant.  Also, we commonly put these trivial accessors on one line.

The inline is not redundant because:

    1) This function definition is outside the class definition, so the rule that makes all functions defined in the class definition inline does not apply.

    2) And, the function is not marked inline in its declaration inside the class definition.

While it’s true we often put trivial accessors on one line inside the class definition, we normally do not do that for functions outside the class definition. I think in this case it’s good to leave the function outside, making the class definition even clearer. I hope it’s OK if I leave it like this.

> WebKit/win/WebView.cpp: 
>  +          *result = frame->shouldClose() ? TRUE : FALSE;
> Do we not need this type conversion?  I don't really understand what Windows needs here..

We definitely do not need it. TRUE and FALSE are constants that are equal to 1 and 0. Assigning from a bool works fine.

The only case where "? TRUE : FALSE" or the line is needed is when all three of these are true:

    1) We are converting from a boolean type that can take on values other than 1 or 0, such as Boolean or BOOL (both of which are typedefs).
    2) We are converting to a boolean type that can take on values other than 1 or 0.
    3) The code path is one where the value might not be 1 or 0.

If either type is bool then that idiom is not needed. In this case, condition (1) is not true, so "? TRUE : FALSE" is not needed.

[Darin Adler]

Comment on attachment 58693 [details]
Patch

Clearing flags on attachment: 58693

Committed r61217: <http://trac.webkit.org/changeset/61217>

[Darin Adler]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #6)
> A couple comments while waiting for Adam to respond to comment 4.

Sorry, I saw your comments, but forgot to deal with them.

> > +        Move functions out of Frame class that were marked "move to Chrome"
> > +        https://bugs.webkit.org/show_bug.cgi?id=39636
> 
> Might it be worth adding a comment saying why no functions are actually getting
> added to Chrome (or anywhere for that matter)?

You mean to the change log? Frame.h has long had comments saying that various functions need to move out to the Frame subobjects. I could mention that again here. I would be happy to add something, but not sure what it should be.

> >      // its frame is not in a consistent state for rendering, so avoid setJSStatusBarText
> 
> Looks like you need to update this comment text.

Oops, I'll take care of that.

> >      // since it may cause clients to attempt to render the frame.
> >      if (!m_creatingInitialEmptyDocument) {
> > -        m_frame->setJSStatusBarText(String());
> > -        m_frame->setJSDefaultStatusBarText(String());
> > +        if (DOMWindow* window = m_frame->existingDOMWindow()) {
> 
> Is it possible there is a behavior change here since it doesn't look like
> the current code needs the DOMWindow to exist for it to do something.

It's not. The code clears out the text, setting it to the null string. But the text is stored in the DOM window. If it's non-null the DOM window is guaranteed to exist.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61217 might have broken Qt Linux ARMv5 Release and Qt Linux ARMv7 Release