Bug 77875

Summary: Web Inspector: add generic support for undo-ing DOM edits.
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Pavel Feldman <pfeldman>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, cmarcelo, joepeck, keishi, loislo, ossy, pfeldman, pmuellr, rakuco, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 77988    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
[Patch] for landing
none
[Patch] with tests. none

Description Pavel Feldman 2012-02-06 08:13:30 PST
Patch to follow.
Comment 1 Pavel Feldman 2012-02-06 08:26:51 PST
Created attachment 125652 [details]
Patch
Comment 2 Yury Semikhatsky 2012-02-07 03:50:30 PST
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
Comment 3 Pavel Feldman 2012-02-07 04:53:38 PST
(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.
Comment 4 Pavel Feldman 2012-02-07 04:54:42 PST
Created attachment 125819 [details]
[Patch] for landing
Comment 5 Pavel Feldman 2012-02-07 05:57:36 PST
Created attachment 125829 [details]
[Patch] with tests.
Comment 6 Pavel Feldman 2012-02-07 06:00:48 PST
Committed r106932: <http://trac.webkit.org/changeset/106932>
Comment 7 Caio Marcelo de Oliveira Filho 2012-02-07 07:57:46 PST
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)
Comment 8 Csaba Osztrogon√°c 2012-02-07 07:59:54 PST
Reopen, because the build is still broken.
Comment 9 Pavel Feldman 2012-02-07 09:01:00 PST
Committed r106953: <http://trac.webkit.org/changeset/106953>