[webkit-reviews] review granted: [Bug 77875] Web Inspector: add generic support for undo-ing DOM edits. : [Attachment 125652] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has granted Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 77875: Web Inspector: add generic support for undo-ing DOM edits.
https://bugs.webkit.org/show_bug.cgi?id=77875

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

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=125652&action=review


> Source/WebCore/ChangeLog:5
> +

Could you provide a high level overview of the undo approach?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:108
> +	   if (!inspectorStyleSheet->text(&m_oldText))

text -> getText?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:161
> +	   return "SetPropertyText" + m_styleSheetId + ":" +
String::number(m_propertyIndex) + ":" + (m_overwrite ? "true" : "false");

Use String::format?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:169
> +	   m_text = other->m_text;

This is not only needed for toString, right?

> Source/WebCore/inspector/InspectorCSSAgent.h:101
> +    friend class SetStyleSheetTextAction;

You can make these classes nested and just forward declare them here.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:135
> +class RemoveChildAction : public DOMAction {

All actions should be marked as WTF_MAKE_NONCOPYABLE

> Source/WebCore/inspector/InspectorDOMAgent.cpp:290
> +private:

style: blank like above private:

> Source/WebCore/inspector/InspectorHistory.cpp:106
> +    while (!m_history.isEmpty() && m_history.first()->isUndoableStateMark())


Instead of marking end of action, you could mark its beginning which would be
simpler.

> Source/WebCore/inspector/InspectorHistory.cpp:107
> +	   m_history.takeFirst();

removeFirst

> Source/WebCore/inspector/InspectorHistory.h:50
> +class InspectorHistory {

WTF_MAKE_NONCOPYABLE

> Source/WebCore/inspector/InspectorStyleSheet.h:177
> +    bool setPropertyText(ErrorString*, const InspectorCSSId&, unsigned
propertyIndex, const String& text, bool overwrite, String* oldPropertyText);

oldPropertyText -> oldText as in other places