Bug 77875 – Web Inspector: add generic support for undo-ing DOM edits.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 125652 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 125652 [details]
Patch

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

[Pavel Feldman]

(In reply to comment #2)
> (From update of attachment 125652 [details])
> 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?

Done.

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

Done.

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

Done.

> > Source/WebCore/inspector/InspectorCSSAgent.cpp:169
> > +        m_text = other->m_text;
> 
> This is not only needed for toString, right?
> 

This will also be used in redo.

> > Source/WebCore/inspector/InspectorCSSAgent.h:101
> > +    friend class SetStyleSheetTextAction;
> 
> You can make these classes nested and just forward declare them here.

Done.

> 
> > Source/WebCore/inspector/InspectorDOMAgent.cpp:135
> > +class RemoveChildAction : public DOMAction {
> 
> All actions should be marked as WTF_MAKE_NONCOPYABLE

Done.


> 
> > Source/WebCore/inspector/InspectorDOMAgent.cpp:290
> > +private:
> 
> style: blank like above private:
> 

Done.

> > 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.

This would make the code harder due to minor (non-major) style edits.

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

Done.

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

Done.


> > 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

In other places this method is defined on style, not stylesheet.

[Pavel Feldman]

Created attachment 125819 [details]
[Patch] for landing

[Pavel Feldman]

Created attachment 125829 [details]
[Patch] with tests.

[Pavel Feldman]

Committed r106932: <http://trac.webkit.org/changeset/106932>

[Caio Marcelo de Oliveira Filho]

Hi. I think there's a chance this broke mac build see in http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/38577/steps/compile-webkit/logs/stdio (look for "OwnPtr" in the file)

[Csaba Osztrogonác]

Reopen, because the build is still broken.

[Pavel Feldman]

Committed r106953: <http://trac.webkit.org/changeset/106953>