Move markerTextForListItem from TestRunner to Internals

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jason Anderssen]

Move markerTextForListItem from TestRunner to Internals

[Jason Anderssen]

Created attachment 190435 [details]
Patch

[Benjamin Poulain]

Comment on attachment 190435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190435&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

You should remove this line.

Your changelog should also have a short explanation for the change.

> Source/WebCore/WebCore.exp.in:163
>  #if !defined(NDEBUG)
>  __ZNK7WebCore14DocumentLoader9isLoadingEv
>  #endif
> +__ZN7WebCore21markerTextForListItemEPNS_7ElementE
>  __ZN7WebCore11JSDOMWindow6s_infoE

Symbols are sorted alphabetically. Why did you move this one?

> Source/WebCore/WebCore.order:514
>  __ZN7WebCore11RenderThemeC2Ev
> +__ZN7WebCore21markerTextForListItemEPNS_7ElementE
>  __ZN7WebCore13platformThemeEv

You should not change the order file yourself. It is generated for Safari.

> Source/WebCore/testing/Internals.h:306
>  #endif
> +
>  };

No need for the blank line.

> Source/WebCore/testing/Internals.idl:249
> +    // Special Special DOM Functions from TestRunner

This comment does not add useful information for the reader, you can leave it out.

(by the way, WebKit comments must be sentences, starting with an uppercase letter, and finishing by a period)

> Source/WebKit/win/DOMCoreClasses.h:865
>  
> -    virtual HRESULT STDMETHODCALLTYPE markerTextForListItem(
> -        /* [retval][out] */ BSTR* markerText);
> -
>      virtual HRESULT STDMETHODCALLTYPE shadowPseudoId(

You cannot remove the Windows code.

We keep binary compatibility there, so removing a virtual function would break the virtual table of the class.

You can leave the WebKit/win part out.

[Benjamin Poulain]

Something I forgot: you should also remove WKBundleFrameCopyMarkerText, the related code on WebFrame, etc.

[Jason Anderssen]

Created attachment 190607 [details]
Patch

[Jason Anderssen]

Created attachment 190627 [details]
Patch

[Benjamin Poulain]

Comment on attachment 190627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190627&action=review

Looks great. Some comments still:

> Source/WebCore/ChangeLog:10
> +        as to make it more compatible with WK2

Missing period.

> Source/WebCore/testing/Internals.cpp:2013
> +String Internals::markerTextForListItem(Element* inputElement, ExceptionCode& ec)

In this case, valid input does not necessarily have to be an inputElement.
The argument name should be simply "element".

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleFrame.cpp:-138
> -{
> -    return toCopiedAPI(toImpl(frameRef)->markerText(element));
> -}

Thanks to this, you are making WebFrame::markerText useless. You should also clean that as part of the patch.

You can probably remove the include for RenderTreeAsText.h from WebFrame too.

[Jason Anderssen]

Created attachment 191058 [details]
Patch

[Benjamin Poulain]

Comment on attachment 191058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191058&action=review

> Source/WebCore/testing/Internals.cpp:2019
> +    return WebCore::markerTextForListItem(element);

Internals is in the WebCore namespace, you do not need the WebCore:: prefix here.

> Tools/DumpRenderTree/blackberry/TestRunnerBlackBerry.cpp:517
> -JSRetainPtr<JSStringRef> TestRunner::markerTextForListItem(JSContextRef context, JSValueRef nodeObject) const
> +JSRetainPtr<JSStringRef> TestRunner::layerTreeAsText() const
>  {
> -    WebCore::Element* element = toElement(toJS(toJS(context), nodeObject));
> -    if (!element)
> -        return 0;
> -
> -    JSRetainPtr<JSStringRef> markerText(Adopt, JSStringCreateWithUTF8CString(WebCore::markerTextForListItem(element).utf8().data()));
> -    return markerText;
> +    notImplemented();
> +    return 0;
>  }

Not sure why you change this to layerTreeAsText(). You can just remove all the code.

[Jason Anderssen]

Created attachment 191066 [details]
Patch

[Benjamin Poulain]

Comment on attachment 191066 [details]
Patch

LGTM.

The windows EWS is down, I will land manually so I can follow up with adding the Windows symbols.

[WebKit Review Bot]

Comment on attachment 191066 [details]
Patch

Clearing flags on attachment: 191066

Committed r144524: <http://trac.webkit.org/changeset/144524>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.