Move markerTextForListItem from TestRunner to Internals
Created attachment 190435 [details] Patch
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.
Something I forgot: you should also remove WKBundleFrameCopyMarkerText, the related code on WebFrame, etc.
Created attachment 190607 [details] Patch
Created attachment 190627 [details] Patch
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.
Created attachment 191058 [details] Patch
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.
Created attachment 191066 [details] Patch
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.
Comment on attachment 191066 [details] Patch Clearing flags on attachment: 191066 Committed r144524: <http://trac.webkit.org/changeset/144524>
All reviewed patches have been landed. Closing bug.