[webkit-reviews] review denied: [Bug 110939] Move markerTextForListItem from TestRunner to Internals : [Attachment 190435] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Jason Anderssen
<janderssen at gmail.com>'s request for review:
Bug 110939: Move markerTextForListItem from TestRunner to Internals
https://bugs.webkit.org/show_bug.cgi?id=110939

Attachment 190435: Patch
https://bugs.webkit.org/attachment.cgi?id=190435&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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.